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

On Wed, May 20, 2026 at 05:56:05PM +0200, Maurizio Lombardi wrote:
> 
> 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?

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.

| diggest_type            | size | size*4/3 | ROUND_UP | encoded with padding |
| ----------------------- | ---- | -------- | -------- | -------------------- |
| MD5_SIGNATURE_SIZE      | 16   | 21,33333 | 22       | 24                   |
| SHA1_SIGNATURE_SIZE     | 20   | 26,66667 | 27       | 28                   |
| SHA256_SIGNATURE_SIZE   | 32   | 42,66667 | 43       | 44                   |
| SHA3_256_SIGNATURE_SIZE | 32   | 42,66667 | 43       | 44                   |

So, that formula is not correct and will break all iscsi authentication.

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 will not require any additional length checkings:


--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -273,7 +273,7 @@ static int chap_server_compute_hash(
				goto out;
		}
 
-		client_digest = kzalloc(chap->digest_size, GFP_KERNEL);
+		client_digest = kzalloc(MAX_RESPONSE_LENGTH, GFP_KERNEL);
		if (!client_digest) {
                pr_err("Unable to allocate the client_digest buffer\n");
                goto out;

----


BR,
 Dmitry