Re: [PATCH v3] scsi: target: iscsi: validate CHAP_R length before base64 decode

On Wed, 20 May 2026 18:52:59 +0200, Alexandru Hossu wrote:

> chap_server_compute_hash() allocates client_digest as
> kzalloc(chap->digest_size) and then, for BASE64-encoded responses,
> passes chap_r directly to chap_base64_decode() without checking whether
> the input length could produce more than digest_size bytes of output.
> 
> chap_base64_decode() writes to the destination unconditionally as long
> as there is input to consume. With MAX_RESPONSE_LENGTH set to 128 and
> the "0b" prefix stripped by extract_param(), up to 127 base64 characters
> can reach the decoder. 127 characters decode to 95 bytes. For SHA-256
> (digest_size=32) this overflows client_digest by 63 bytes; for MD5
> (digest_size=16) the overflow is 79 bytes.
> 
> The length check at line 344 fires after the write has already happened.
> 
> The HEX branch in the same switch statement already validates the length
> up front. Apply the same approach to the BASE64 branch: strip trailing
> base64 padding characters, then reject any input whose data length
> exceeds DIV_ROUND_UP(digest_size * 4, 3) before calling the decoder.
> 
> Stripping trailing '=' before the comparison handles both padded and
> unpadded encodings. chap_base64_decode() already returns early on '=',
> so the full original string is still passed to the decoder unchanged.
> 
> Fixes: 1e5733883421 ("scsi: target: iscsi: Support base64 in CHAP")
> Cc: [email protected]
> Signed-off-by: Alexandru Hossu <[email protected]>
> ---
> v3: strip trailing '=' before length check to handle padded encodings
>     (reported by Maurizio Lombardi)
> v2: use DIV_ROUND_UP(digest_size * 4, 3) as suggested by David Disseldorp
> 
>  drivers/target/iscsi/iscsi_target_auth.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_auth.c b/drivers/target/iscsi/iscsi_target_auth.c
> index c46c69a..00487d0 100644
> --- a/drivers/target/iscsi/iscsi_target_auth.c
> +++ b/drivers/target/iscsi/iscsi_target_auth.c
> @@ -340,13 +340,22 @@ static int chap_server_compute_hash(
>  			goto out;
>  		}
>  		break;
> -	case BASE64:
> +	case BASE64: {
> +		size_t r_len = strlen(chap_r);
> +
> +		while (r_len > 0 && chap_r[r_len - 1] == '=')
> +			r_len--;
> +		if (r_len > DIV_ROUND_UP(chap->digest_size * 4, 3)) {
> +			pr_err("Malformed CHAP_R: base64 payload too long\n");
> +			goto out;
> +		}
>  		if (chap_base64_decode(client_digest, chap_r, strlen(chap_r)) !=
>  		    chap->digest_size) {
>  			pr_err("Malformed CHAP_R: invalid BASE64\n");
>  			goto out;
>  		}
>  		break;
> +	}
>  	default:
>  		pr_err("Could not find CHAP_R\n");
>  		goto out;


This looks a bit fragile, but moving the overflow check into
chap_base64_decode() probably won't make it any cleaner. I'd like to see
a comment or build-time assert in the mutual CHAP path as to why the
length check can be skipped there. Aside from that I think it makes
sense to merge this.

FWIW, I've added some base64 CHAP coverage to the libiscsi test suite
over at https://github.com/sahlberg/libiscsi/pull/469 .

Thanks, David