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

On 5/21/26 16:38, David Disseldorp wrote:
> 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.
> 
Please, no.
The length check should be part of the chap_base64_decode() function,
which should reject inputs with the wrong length. _And_ you need
to add a 'length' argument for 'client_digest' such that the function
nows the size of the output buffer and can avoid precisely these
issues.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
[email protected]                               +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich