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

[David Disseldorp <ddiss@suse.de>]

Hi Alexandru,

Thanks for the report and follow up patch...

On Mon, 18 May 2026 14:18:11 +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: reject any input
> whose maximum decoded length exceeds digest_size before calling the
> decoder.
> 
> The formula (digest_size * 4 + 2) / 3 is the ceiling of digest_size *
> 4/3, i.e. the maximum number of base64 characters that can decode to
> exactly digest_size bytes.
> 
> Fixes: 1e5733883421 ("scsi: target: iscsi: Support base64 in CHAP")
> Cc: stable@vger.kernel.org
> Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
> ---
>  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..653be1a 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) > (chap->digest_size * 4 + 2) / 3) {

nit: this could be DIV_ROUND_UP(chap->digest_size * 4, 3) to match
base64.h BASE64_CHARS(), right?

> +			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");

The above check doesn't appear to catch undersize base64 CHAP responses,
unlike the hex path. How does that affect the handshake?

Finally, don't we need a similar check for the mutual CHAP code-path?

Thanks, David