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

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

[Alexandru Hossu]

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

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

[David Disseldorp]

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

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

[Alexandru Hossu]

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: stable@vger.kernel.org
Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
---
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;
+		}
 		if (chap_base64_decode(client_digest, chap_r, strlen(chap_r)) !=
 		    chap->digest_size) {
 			pr_err("Malformed CHAP_R: invalid BASE64\n");
-- 
2.54.0

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

[Maurizio Lombardi]

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: stable@vger.kernel.org
> Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
> ---
> 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

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

[Alexandru Hossu]

On Wed, May 20, 2026, Maurizio Lombardi <mlombard@arkamax.eu> wrote:
> 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?

Correct. For SHA-256, a padded encoding of the 32-byte digest is 44
characters (43 data + one '='), but DIV_ROUND_UP(32 * 4, 3) = 43, so a
legitimate padded response would be incorrectly rejected.

v3 strips trailing '=' before the comparison:

	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;
	}

chap_base64_decode() already handles '=' by returning early, so
stripping them from the pre-check does not affect decoding.

v3 below.

Alexandru

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

[Dmitry Bogdanov]

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: stable@vger.kernel.org
> > Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
> > ---
> > 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

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

[Alexandru Hossu]

On Wed, May 20, 2026, Dmitry Bogdanov <d.bogdanov@yadro.com> 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.

Alexandru

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

[Alexandru Hossu]

On Mon, May 19, 2026, David Disseldorp <ddiss@suse.de> wrote:
> nit: this could be DIV_ROUND_UP(chap->digest_size * 4, 3) to match
> base64.h BASE64_CHARS(), right?

Yes, equivalent and will use it in v2.

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

An undersize response decodes to fewer than digest_size bytes.
chap_base64_decode() returns cp - dst, which is less than digest_size,
so the existing != digest_size check at line 345 fires and the handshake
fails. The result is the same as the hex path.

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

The mutual path decodes CHAP_C into initiatorchg_binhex, allocated as
kzalloc(CHAP_CHALLENGE_STR_LEN) = kzalloc(4096). extract_param() caps
the input at CHAP_CHALLENGE_STR_LEN characters, so at most 4095 base64
chars reach the decoder, producing at most 3071 decoded bytes. 3071 < 4096,
so the destination cannot overflow. The post-decode > 1024 check is a
semantic limit on challenge size, not a safety net against overflow.

v2 with DIV_ROUND_UP below.

Alexandru

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

[Alexandru Hossu]

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: stable@vger.kernel.org
Signed-off-by: Alexandru Hossu <hossu.alexandru@gmail.com>
---
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;
-- 
2.54.0