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

On Tue May 19, 2026 at 1:50 AM CEST, 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: reject any input
> whose maximum decoded length exceeds digest_size before calling the
> decoder.
>
> DIV_ROUND_UP(digest_size * 4, 3) is the maximum number of base64
> characters that can decode to exactly digest_size bytes, matching the
> convention used in base64.h BASE64_CHARS().
>
> Fixes: 1e5733883421 ("scsi: target: iscsi: Support base64 in CHAP")
> Cc: [email protected]
> Signed-off-by: Alexandru Hossu <[email protected]>
> ---
> v2: use DIV_ROUND_UP(digest_size * 4, 3) as suggested by David Disseldorp
>
>  drivers/target/iscsi/iscsi_target_auth.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/target/iscsi/iscsi_target_auth.c b/drivers/target/iscsi/iscsi_target_auth.c
> index c46c69a..50eeded 100644
> --- a/drivers/target/iscsi/iscsi_target_auth.c
> +++ b/drivers/target/iscsi/iscsi_target_auth.c
> @@ -341,6 +341,10 @@ static int chap_server_compute_hash(
>  		}
>  		break;
>  	case BASE64:
> +		if (strlen(chap_r) > DIV_ROUND_UP(chap->digest_size * 4, 3)) {
> +			pr_err("Malformed CHAP_R: base64 payload too long\n");
> +			goto out;
> +		}


There is something that doesn't totally convince me about this length check.
Couldn't chap_r contain those Base64 padding '=' characters that
would make strlen(chap_r) too big to pass this check?

Maurizio