[PATCH 0/2] scsi: target: fix CHAP_N handling

[PATCH 0/2] scsi: target: fix CHAP_N handling

[David Disseldorp]

The sashiko bot pointed out issues with CHAP_N handling recently, when
reviewing a patch for a separate issue:
https://sashiko.dev/#/patchset/20260521151121.808477-1-hossu.alexandru%40gmail.com
  Since extract_param() unconditionally strips '0x' or '0b' prefixes and
  alters the returned type, wouldn't a valid user with a name like '0xalice' or
  '0bob' have their username mutated to 'alice' or 'ob'?

I believe this behaviour is contrary to the iSCSI spec. I added some simple
libiscsi tests to cover prefix stripping:
https://github.com/sahlberg/libiscsi/pull/472

These patches attempt to fix CHAP_N handling.

----------------------------------------------------------------
David Disseldorp (2):
      scsi: target: add extract_param_str() helper
      scsi: target: fix auth when CHAP_N carries a hex/b64 prefix

 drivers/target/iscsi/iscsi_target_auth.c |  8 ++---
 drivers/target/iscsi/iscsi_target_nego.c | 37 ++++++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_nego.h |  1 +
 3 files changed, 40 insertions(+), 6 deletions(-)

[PATCH 1/2] scsi: target: add extract_param_str() helper

[David Disseldorp]

The existing extract_param() helper, detects and strips any hex (0x/0X)
or base64 (0b/0B). This makes sense for some parameters, but not strings
such as CHAP_N.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/target/iscsi/iscsi_target_nego.c | 37 ++++++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_nego.h |  1 +
 2 files changed, 38 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index b03ed154ca34e..53b17d3cc86c3 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -98,6 +98,43 @@ int extract_param(
 	return 0;
 }
 
+/* same as extract_param() above, but don't interpret any type-prefix */
+int extract_param_str(
+	const char *in_buf,
+	const char *pattern,
+	unsigned int max_length,
+	char *out_buf)
+{
+	char *ptr;
+	int len;
+
+	if (!in_buf || !pattern || !out_buf)
+		return -EINVAL;
+
+	ptr = strstr(in_buf, pattern);
+	if (!ptr)
+		return -ENOENT;
+
+	ptr = strstr(ptr, "=");
+	if (!ptr)
+		return -EINVAL;
+
+	ptr += 1;
+	len = strlen_semi(ptr);
+	if (len < 0)
+		return -EINVAL;
+
+	if (len >= max_length) {
+		pr_err("Length of input: %d exceeds max_length:"
+			" %d\n", len, max_length);
+		return -EINVAL;
+	}
+	memcpy(out_buf, ptr, len);
+	out_buf[len] = '\0';
+
+	return 0;
+}
+
 static struct iscsi_node_auth *iscsi_get_node_auth(struct iscsit_conn *conn)
 {
 	struct iscsi_portal_group *tpg;
diff --git a/drivers/target/iscsi/iscsi_target_nego.h b/drivers/target/iscsi/iscsi_target_nego.h
index e60a46d348352..6b72edd2aef2e 100644
--- a/drivers/target/iscsi/iscsi_target_nego.h
+++ b/drivers/target/iscsi/iscsi_target_nego.h
@@ -13,6 +13,7 @@ struct iscsi_np;
 extern void convert_null_to_semi(char *, int);
 extern int extract_param(const char *, const char *, unsigned int, char *,
 		unsigned char *);
+extern int extract_param_str(const char *, const char *, unsigned int, char *);
 extern int iscsi_target_check_login_request(struct iscsit_conn *,
 		struct iscsi_login *);
 extern int iscsi_target_locate_portal(struct iscsi_np *, struct iscsit_conn *,
-- 
2.51.0

Re: [PATCH 1/2] scsi: target: add extract_param_str() helper

[Lee Duncan]

On Tue, Jun 2, 2026 at 5:04 AM David Disseldorp <ddiss@suse.de> wrote:
>
> The existing extract_param() helper, detects and strips any hex (0x/0X)
> or base64 (0b/0B). This makes sense for some parameters, but not strings
> such as CHAP_N.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/iscsi/iscsi_target_nego.c | 37 ++++++++++++++++++++++++
>  drivers/target/iscsi/iscsi_target_nego.h |  1 +
>  2 files changed, 38 insertions(+)
>
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
> index b03ed154ca34e..53b17d3cc86c3 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -98,6 +98,43 @@ int extract_param(
>         return 0;
>  }
>
> +/* same as extract_param() above, but don't interpret any type-prefix */
> +int extract_param_str(
> +       const char *in_buf,
> +       const char *pattern,
> +       unsigned int max_length,
> +       char *out_buf)
> +{
> +       char *ptr;
> +       int len;
> +
> +       if (!in_buf || !pattern || !out_buf)
> +               return -EINVAL;
> +
> +       ptr = strstr(in_buf, pattern);
> +       if (!ptr)
> +               return -ENOENT;
> +
> +       ptr = strstr(ptr, "=");
> +       if (!ptr)
> +               return -EINVAL;
> +
> +       ptr += 1;
> +       len = strlen_semi(ptr);
> +       if (len < 0)
> +               return -EINVAL;
> +
> +       if (len >= max_length) {
> +               pr_err("Length of input: %d exceeds max_length:"
> +                       " %d\n", len, max_length);
> +               return -EINVAL;
> +       }
> +       memcpy(out_buf, ptr, len);
> +       out_buf[len] = '\0';
> +
> +       return 0;
> +}
> +
>  static struct iscsi_node_auth *iscsi_get_node_auth(struct iscsit_conn *conn)
>  {
>         struct iscsi_portal_group *tpg;
> diff --git a/drivers/target/iscsi/iscsi_target_nego.h b/drivers/target/iscsi/iscsi_target_nego.h
> index e60a46d348352..6b72edd2aef2e 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.h
> +++ b/drivers/target/iscsi/iscsi_target_nego.h
> @@ -13,6 +13,7 @@ struct iscsi_np;
>  extern void convert_null_to_semi(char *, int);
>  extern int extract_param(const char *, const char *, unsigned int, char *,
>                 unsigned char *);
> +extern int extract_param_str(const char *, const char *, unsigned int, char *);
>  extern int iscsi_target_check_login_request(struct iscsit_conn *,
>                 struct iscsi_login *);
>  extern int iscsi_target_locate_portal(struct iscsi_np *, struct iscsit_conn *,
> --
> 2.51.0
>
>

Reviewed-by: Lee Duncan <lduncan@suse.com>

[PATCH 2/2] scsi: target: fix auth when CHAP_N carries a hex/b64 prefix

[David Disseldorp]

Attempting to authenticate using a CHAP username with a '0x' or '0b'
prefix currently fails. This is due to extract_param()'s behaviour of
stripping these prefixes, and the subsequent (type == HEX) error-path.
I believe this behaviour is contrary to the RFC 3720 specification,
which states:

  5.1.  Text Format
  ...
  text-value: A string of zero or more characters that consist of
  letters, digits, dot, minus, plus, commercial at, underscore,
  slash, left bracket, right bracket, or colon.

  11.1.4.  Challenge Handshake Authentication Protocol (CHAP)
  ...
    CHAP_A=<A> CHAP_I=<I> CHAP_C=<C>

   Where A is one of A1,A2... that were proposed by the initiator.

   In the third step, the initiator MUST continue with:

      CHAP_N=<N> CHAP_R=<R>
   ...
   Where N, (A,A1,A2), I, C, and R are (correspondingly) the Name,
   Algorithm, Identifier, Challenge, and Response as defined in
   [RFC1994], N is a text string, A,A1,A2, and I are numbers, and C and
   R are large-binary-values ...

"N is a text string" implies that any hex or base64 encoding prefix
should not be interpreted or stripped. Fix this by using the new
extract_param_str() helper function to obtain the CHAP_N value as-is.

Reported-by: Sashiko (gemini/gemini-3.1-pro-preview)
Link: https://sashiko.dev/#/patchset/20260521151121.808477-1-hossu.alexandru%40gmail.com
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 drivers/target/iscsi/iscsi_target_auth.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target_auth.c b/drivers/target/iscsi/iscsi_target_auth.c
index a3ad2d244dbee..6f21075e58416 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -303,12 +303,8 @@ static int chap_server_compute_hash(
 	/*
 	 * Extract CHAP_N.
 	 */
-	if (extract_param(nr_in_ptr, "CHAP_N", MAX_CHAP_N_SIZE, chap_n,
-				&type) < 0) {
-		pr_err("Could not find CHAP_N.\n");
-		goto out;
-	}
-	if (type == HEX) {
+	ret = extract_param_str(nr_in_ptr, "CHAP_N", MAX_CHAP_N_SIZE, chap_n);
+	if (ret < 0) {
 		pr_err("Could not find CHAP_N.\n");
 		goto out;
 	}
-- 
2.51.0

Re: [PATCH 2/2] scsi: target: fix auth when CHAP_N carries a hex/b64 prefix

[Lee Duncan]

On Tue, Jun 2, 2026 at 5:04 AM David Disseldorp <ddiss@suse.de> wrote:
>
> Attempting to authenticate using a CHAP username with a '0x' or '0b'
> prefix currently fails. This is due to extract_param()'s behaviour of
> stripping these prefixes, and the subsequent (type == HEX) error-path.
> I believe this behaviour is contrary to the RFC 3720 specification,
> which states:
>
>   5.1.  Text Format
>   ...
>   text-value: A string of zero or more characters that consist of
>   letters, digits, dot, minus, plus, commercial at, underscore,
>   slash, left bracket, right bracket, or colon.
>
>   11.1.4.  Challenge Handshake Authentication Protocol (CHAP)
>   ...
>     CHAP_A=<A> CHAP_I=<I> CHAP_C=<C>
>
>    Where A is one of A1,A2... that were proposed by the initiator.
>
>    In the third step, the initiator MUST continue with:
>
>       CHAP_N=<N> CHAP_R=<R>
>    ...
>    Where N, (A,A1,A2), I, C, and R are (correspondingly) the Name,
>    Algorithm, Identifier, Challenge, and Response as defined in
>    [RFC1994], N is a text string, A,A1,A2, and I are numbers, and C and
>    R are large-binary-values ...
>
> "N is a text string" implies that any hex or base64 encoding prefix
> should not be interpreted or stripped. Fix this by using the new
> extract_param_str() helper function to obtain the CHAP_N value as-is.
>
> Reported-by: Sashiko (gemini/gemini-3.1-pro-preview)
> Link: https://sashiko.dev/#/patchset/20260521151121.808477-1-hossu.alexandru%40gmail.com
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  drivers/target/iscsi/iscsi_target_auth.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_auth.c b/drivers/target/iscsi/iscsi_target_auth.c
> index a3ad2d244dbee..6f21075e58416 100644
> --- a/drivers/target/iscsi/iscsi_target_auth.c
> +++ b/drivers/target/iscsi/iscsi_target_auth.c
> @@ -303,12 +303,8 @@ static int chap_server_compute_hash(
>         /*
>          * Extract CHAP_N.
>          */
> -       if (extract_param(nr_in_ptr, "CHAP_N", MAX_CHAP_N_SIZE, chap_n,
> -                               &type) < 0) {
> -               pr_err("Could not find CHAP_N.\n");
> -               goto out;
> -       }
> -       if (type == HEX) {
> +       ret = extract_param_str(nr_in_ptr, "CHAP_N", MAX_CHAP_N_SIZE, chap_n);
> +       if (ret < 0) {
>                 pr_err("Could not find CHAP_N.\n");
>                 goto out;
>         }
> --
> 2.51.0
>
>

Reviewed-by: Lee Duncan <lduncan@suse.com>

Re: [PATCH 0/2] scsi: target: fix CHAP_N handling

[John Garry]

On 02/06/2026 12:43, David Disseldorp wrote:
> The sashiko bot pointed out issues with CHAP_N handling recently, when
> reviewing a patch for a separate issue:
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260521151121.808477-1-hossu.alexandru*40gmail.com__;IyU!!ACWV5N9M2RV99hQ!MNesZJ3IsH9Mv0iZxHUcVmbC_3uwDkJgMhAX8i1TelyqqZD_dAq1cwIy6RtYI8D3boJh5iFeGhtTvfTX$
>    Since extract_param() unconditionally strips '0x' or '0b' prefixes and
>    alters the returned type, wouldn't a valid user with a name like '0xalice' or
>    '0bob' have their username mutated to 'alice' or 'ob'?

is there a real world case or vulnerability being fixed here?

> 
> I believe this behaviour is contrary to the iSCSI spec. I added some simple
> libiscsi tests to cover prefix stripping:
> https://urldefense.com/v3/__https://github.com/sahlberg/libiscsi/pull/472__;!!ACWV5N9M2RV99hQ!MNesZJ3IsH9Mv0iZxHUcVmbC_3uwDkJgMhAX8i1TelyqqZD_dAq1cwIy6RtYI8D3boJh5iFeGh1A_gMb$
> 
> These patches attempt to fix CHAP_N handling.
> 
> ----------------------------------------------------------------
> David Disseldorp (2):
>        scsi: target: add extract_param_str() helper
>        scsi: target: fix auth when CHAP_N carries a hex/b64 prefix
> 
>   drivers/target/iscsi/iscsi_target_auth.c |  8 ++---
>   drivers/target/iscsi/iscsi_target_nego.c | 37 ++++++++++++++++++++++++
>   drivers/target/iscsi/iscsi_target_nego.h |  1 +
>   3 files changed, 40 insertions(+), 6 deletions(-)
> 
> 

Re: [PATCH 0/2] scsi: target: fix CHAP_N handling

[David Disseldorp]

On Tue, 2 Jun 2026 17:42:57 +0100, John Garry wrote:

> On 02/06/2026 12:43, David Disseldorp wrote:
> > The sashiko bot pointed out issues with CHAP_N handling recently, when
> > reviewing a patch for a separate issue:
> > https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260521151121.808477-1-hossu.alexandru*40gmail.com__;IyU!!ACWV5N9M2RV99hQ!MNesZJ3IsH9Mv0iZxHUcVmbC_3uwDkJgMhAX8i1TelyqqZD_dAq1cwIy6RtYI8D3boJh5iFeGhtTvfTX$
> >    Since extract_param() unconditionally strips '0x' or '0b' prefixes and
> >    alters the returned type, wouldn't a valid user with a name like '0xalice' or
> >    '0bob' have their username mutated to 'alice' or 'ob'?  
> 
> is there a real world case or vulnerability being fixed here?

No vulnerability -  the "real world case" is as above: CHAP
authentication currently fails if the CHAP username begins with 0x, 0b
or the upper case variants. The bug is trivial to reproduce.

Thanks, David

Re: [PATCH 0/2] scsi: target: fix CHAP_N handling

[John Garry]

On 03/06/2026 00:19, David Disseldorp wrote:
> On Tue, 2 Jun 2026 17:42:57 +0100, John Garry wrote:
> 
>> On 02/06/2026 12:43, David Disseldorp wrote:
>>> The sashiko bot pointed out issues with CHAP_N handling recently, when
>>> reviewing a patch for a separate issue:
>>> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260521151121.808477-1-hossu.alexandru*40gmail.com__;IyU!!ACWV5N9M2RV99hQ!MNesZJ3IsH9Mv0iZxHUcVmbC_3uwDkJgMhAX8i1TelyqqZD_dAq1cwIy6RtYI8D3boJh5iFeGhtTvfTX$
>>>     Since extract_param() unconditionally strips '0x' or '0b' prefixes and
>>>     alters the returned type, wouldn't a valid user with a name like '0xalice' or
>>>     '0bob' have their username mutated to 'alice' or 'ob'?
>>
>> is there a real world case or vulnerability being fixed here?
> 
> No vulnerability -  the "real world case" is as above: CHAP
> authentication currently fails if the CHAP username begins with 0x, 0b
> or the upper case variants. The bug is trivial to reproduce.
> 
Understood.

To me, this is a fix for a problem which does not exist. And even if the 
spec says it's possible, it does not mean that we need to support it. If 
0x prefix is part of the username, I think that problems should be expected.

Anyway, the code itself looks ok, so:

Reviewed-by: John Garry <john.g.garry@oracle.com>

But the maintainer can make the judgement to take this obviously.