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

On 5/21/26 02:43, Alexandru Hossu wrote:
> On Wed, May 20, 2026, Dmitry Bogdanov <[email protected]> wrote:
>> Yes, the length of Base64 decoded string is not deterministic.
>> Moreover, length of Base64 encoded string must be divisible by 4. Which
>> is biger that 4/3 of decoded.
>>
>> | MD5_SIGNATURE_SIZE      | 16   | 21,33333 | 22       | 24                   |
>> | SHA256_SIGNATURE_SIZE   | 32   | 42,66667 | 43       | 44                   |
>>
>> So, that formula is not correct and will break all iscsi authentication.
> 
> v3 (sent about an hour before your email) already handles this. Trailing
> '=' are stripped before the comparison, so the check is applied only to
> the data characters:
> 
> 	while (r_len > 0 && chap_r[r_len - 1] == '=')
> 		r_len--;
> 	if (r_len > DIV_ROUND_UP(chap->digest_size * 4, 3)) {
> 
> Using your table as input:
> 
>    MD5 padded:     "data==" -> r_len = 24-2 = 22, 22 <= 22 ✓
>    SHA-256 padded: "data="  -> r_len = 44-1 = 43, 43 <= 43 ✓
> 
>> Alexandru, may be better just to change size of client_diggest variable
>> to match it with chap_r like for initiatorchg and initiatorchg_binhex?
> 
> That also prevents the overflow. The length check is preferred for
> consistency with the HEX branch, which validates input length before
> calling the decoder rather than relying on a larger destination buffer.
> 
I would prefer to use the size of the original string as a buffer;
that will ensure that the buffer is always large enough and avoids
a lot of rather pointless calculation. There really is no point
in trying to be precise on the number of bytes to allocate; at
most we'll be saving 3 bytes which really is not worth the effort.

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