Hi,
The target is failing to handle list of CHAP_A key-value pair form
initiator.
The target is expecting CHAP_A=5 always. In other cases, where (for
example)
CHAP_A=6,5 target is failing the security negotiation. Which is incorrect.
This patch handles the case (RFC 3720 section 11.1.4).
where in the initiator may send list of CHAP_A values and target replies
with appropriate CHAP_A value in response.
The patch follows -
From b6b92b426540160b8a0cd6d81fc12b2728b05681 Mon Sep 17 00:00:00 2001
From: Tejas Vaykole <tejas.vaykole@xxxxxxxxxxxxxx>
Date: Wed, 28 May 2014 11:32:11 +0530
Subject: [PATCH] target: Target Error in handling CHAP_A in a List.
The target is failing to handle list of CHAP_A key-value pair form
initiator.
The target is expecting CHAP_A=5 always. In other cases, where (for
example)
CHAP_A=6,5 target is failing the security negotiation. Which is incorrect.
This patch handles the case (RFC 3720 section 11.1.4).
where in the initiator may send list of CHAP_A values and target replies
with appropriate CHAP_A value in response.
---
drivers/target/iscsi/iscsi_target_auth.c | 81
++++++++++++++++++++++----------
drivers/target/iscsi/iscsi_target_auth.h | 1 +
2 files changed, 56 insertions(+), 26 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_auth.c
b/drivers/target/iscsi/iscsi_target_auth.c
index de77d9a..df9da15 100644
--- a/drivers/target/iscsi/iscsi_target_auth.c
+++ b/drivers/target/iscsi/iscsi_target_auth.c
@@ -72,6 +72,33 @@ static void chap_gen_challenge(
}
+static int check_algorithm(const char *a_str)
+{
+ char *tmp = NULL;
+ char *token = NULL;
+ tmp = kstrdup(a_str, GFP_KERNEL);
+ if (!tmp) {
+ pr_err("Memory allocation failed for CHAP_A temperory buffer\n");
+ return CHAP_DIGEST_UNKNOWN;
+ }
+ token = strsep(&tmp , "=");
+ while (token) {
+ token = strsep(&tmp , ",");
+ if (!token) {
+ kfree(tmp);
+ return CHAP_DIGEST_UNKNOWN;
+ }
+ if (!strncmp(token, "5", 1)) {
+ pr_debug("Selected MD5 Algorithm\n");
+ kfree(tmp);
+ return CHAP_DIGEST_MD5;
+ }
+ }
+ kfree(tmp);
+ return CHAP_DIGEST_UNKNOWN;
+}
+
+
static struct iscsi_chap *chap_server_open(
struct iscsi_conn *conn,
struct iscsi_node_auth *auth,
@@ -79,6 +106,7 @@ static struct iscsi_chap *chap_server_open(
char *aic_str,
unsigned int *aic_len)
{
+ int ret;
struct iscsi_chap *chap;
if (!(auth->naf_flags & NAF_USERID_SET) ||
@@ -93,34 +121,35 @@ static struct iscsi_chap *chap_server_open(
return NULL;
chap = conn->auth_protocol;
- /*
- * We only support MD5 MDA presently.
- */
- if (strncmp(a_str, "CHAP_A=5", 8)) {
- pr_err("CHAP_A is not MD5.\n");
+ ret = check_algorithm(a_str);
+ switch (ret) {
+ case CHAP_DIGEST_MD5:
+ pr_debug("[server] Got CHAP_A=5\n");
+ /*
+ * Send back CHAP_A set to MD5.
+ */
+ *aic_len = sprintf(aic_str, "CHAP_A=5");
+ *aic_len += 1;
+ chap->digest_type = CHAP_DIGEST_MD5;
+ pr_debug("[server] Sending CHAP_A=%d\n", chap->digest_type);
+ /*
+ * Set Identifier.
+ */
+ chap->id = conn->tpg->tpg_chap_id++;
+ *aic_len += sprintf(aic_str + *aic_len, "CHAP_I=%d", chap->id);
+ *aic_len += 1;
+ pr_debug("[server] Sending CHAP_I=%d\n", chap->id);
+ /*
+ * Generate Challenge.
+ */
+ chap_gen_challenge(conn, 1, aic_str, aic_len);
+ return chap;
+ break;
+ case CHAP_DIGEST_UNKNOWN:
+ default:
+ pr_err("Unknown CHAP_A.\n");
return NULL;
}
- pr_debug("[server] Got CHAP_A=5\n");
- /*
- * Send back CHAP_A set to MD5.
- */
- *aic_len = sprintf(aic_str, "CHAP_A=5");
- *aic_len += 1;
- chap->digest_type = CHAP_DIGEST_MD5;
- pr_debug("[server] Sending CHAP_A=%d\n", chap->digest_type);
- /*
- * Set Identifier.
- */
- chap->id = conn->tpg->tpg_chap_id++;
- *aic_len += sprintf(aic_str + *aic_len, "CHAP_I=%d", chap->id);
- *aic_len += 1;
- pr_debug("[server] Sending CHAP_I=%d\n", chap->id);
- /*
- * Generate Challenge.
- */
- chap_gen_challenge(conn, 1, aic_str, aic_len);
-
- return chap;
}
static void chap_close(struct iscsi_conn *conn)
diff --git a/drivers/target/iscsi/iscsi_target_auth.h
b/drivers/target/iscsi/iscsi_target_auth.h
index 2f463c0..d22f7b96 100644
--- a/drivers/target/iscsi/iscsi_target_auth.h
+++ b/drivers/target/iscsi/iscsi_target_auth.h
@@ -1,6 +1,7 @@
#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_
+#define CHAP_DIGEST_UNKNOWN 0
#define CHAP_DIGEST_MD5 5
#define CHAP_DIGEST_SHA 6
--
1.7.11.7
On 5/28/2014 3:02 AM, Nicholas A. Bellinger wrote:
Hi Tejus,
Replying to Andy's email in order to inline comment on the patch..
On Tue, 2014-05-27 at 10:55 -0700, Andy Grover wrote:
On 05/27/2014 12:02 AM, Tejas Vaykole wrote:
Hi,
Initially the LIO target was not able to handle CHAP_A in list
correctly.This patch handles it correctly.
Let me know if any changes are required.
Hi Tejas,
Please include patch inline next time is possible, instead of as an
attachment.
Yes, please.
+/*
+ * check_algorithm - Verifies CHAP_A string contains correct
+ * algorithm value. Presently only MD5 is
+ * supported.
+ *
+ * return values - CHAP_DIGEST_UNKNOWN for unknown value and
+ * CHAP_DIGEST_MD5 for MD5.
+ *
+ */
Please use the docbook style comments. See:
Documentation/kernel-doc-nano-HOWTO.txt
+static int check_algorithm(const char *a_str)
+{
+ char *tmp = NULL;
+ char *token = NULL;
+ tmp = kmalloc(strlen(a_str)+1, GFP_KERNEL);
Use kstrdup here..
+ if (tmp == NULL) {
+ pr_err("Memory allocation Failed\n");
+ return CHAP_DIGEST_UNKNOWN;
+ }
Use if (!foo) style conditionals..
+ strcpy(tmp, a_str);
+ token = strsep(&tmp , "=");
+ while (token != NULL) {
Drop unnecessary "!= NULL' for while (foo) {}
+ token = strsep(&tmp , ",");
+ if (token == NULL)
+ return CHAP_DIGEST_UNKNOWN;
Ditto here for if (!foo) style conditionals
+ if (!strcmp(token, "5")) {
+ pr_err("MD5 Algorithm\n");
+ return CHAP_DIGEST_MD5;
+ }
+ }
+ return CHAP_DIGEST_UNKNOWN;
+}
Use strncpy over strcpy.
Free alloced memory, or else memory leak.
+
static struct iscsi_chap *chap_server_open(
- struct iscsi_conn *conn,
- struct iscsi_node_auth *auth,
- const char *a_str,
- char *aic_str,
- unsigned int *aic_len)
+ struct iscsi_conn *conn,
+ struct iscsi_node_auth *auth,
+ const char *a_str,
+ char *aic_str,
+ unsigned int *aic_len)
{
+ int ret;
struct iscsi_chap *chap;
-
if (!(auth->naf_flags & NAF_USERID_SET) ||
- !(auth->naf_flags & NAF_PASSWORD_SET)) {
Your editor has modified some whitespace. Don't include whitespace fixes
in substantive patches please.
Also, I'm sure I'm missing something, but how exactly does this fix the
issue you described in your previous email? The current code in
chap_server_open returns NULL if not "CHAP_A=5", which results in
"security negotiation failed", doesn't it? And the new code doesn't
change this behavior as far as I can see.
Yes, always include a detailed commit log of what bug the patch
addresses, along with an explanation of why it's correct..
(Adding more of the original patch that did not appear in Andy's
response)
@@ -93,34 +126,35 @@ static struct iscsi_chap *chap_server_open(
return NULL;
chap = conn->auth_protocol;
- /*
- * We only support MD5 MDA presently.
- */
- if (strncmp(a_str, "CHAP_A=5", 8)) {
- pr_err("CHAP_A is not MD5.\n");
+ ret = check_algorithm(a_str);
+ switch (ret) {
+ case CHAP_DIGEST_MD5:
+ pr_debug("[server] Got CHAP_A=5\n");
+ /*
+ * Send back CHAP_A set to MD5.
+ */
+ *aic_len = sprintf(aic_str, "CHAP_A=5");
+ *aic_len += 1;
+ chap->digest_type = CHAP_DIGEST_MD5;
+ pr_debug("[server] Sending CHAP_A=%d\n", chap->digest_type);
+ /*
+ * Set Identifier.
+ */
+ chap->id = conn->tpg->tpg_chap_id++;
+ *aic_len += sprintf(aic_str + *aic_len, "CHAP_I=%d", chap->id);
+ *aic_len += 1;
+ pr_debug("[server] Sending CHAP_I=%d\n", chap->id);
+ /*
+ * Generate Challenge.
+ */
+ chap_gen_challenge(conn, 1, aic_str, aic_len);
+ return chap;
+ break;
+ case CHAP_DIGEST_UNKNOWN:
+ default:
+ pr_err("Unknown CHAP_A.\n");
return NULL;
}
- pr_debug("[server] Got CHAP_A=5\n");
- /*
- * Send back CHAP_A set to MD5.
- */
- *aic_len = sprintf(aic_str, "CHAP_A=5");
- *aic_len += 1;
- chap->digest_type = CHAP_DIGEST_MD5;
- pr_debug("[server] Sending CHAP_A=%d\n", chap->digest_type);
- /*
- * Set Identifier.
- */
- chap->id = conn->tpg->tpg_chap_id++;
- *aic_len += sprintf(aic_str + *aic_len, "CHAP_I=%d", chap->id);
- *aic_len += 1;
- pr_debug("[server] Sending CHAP_I=%d\n", chap->id);
- /*
- * Generate Challenge.
- */
- chap_gen_challenge(conn, 1, aic_str, aic_len);
-
- return chap;
}
The generation of the CHAP_A + CHAP_I + CHAP_C values for the login
response are independent of the actual algorithm selected. That said,
this code should be common to all algorithms, and not specific to
CHAP_DIGEST_MD5 to avoid duplication if/when another algorithm is ever
supported.
diff --git a/drivers/target/iscsi/iscsi_target_auth.h b/drivers/target/iscsi/iscsi_target_auth.h
index 2f463c0..97b4527 100644
--- a/drivers/target/iscsi/iscsi_target_auth.h
+++ b/drivers/target/iscsi/iscsi_target_auth.h
@@ -1,10 +1,11 @@
#ifndef _ISCSI_CHAP_H_
#define _ISCSI_CHAP_H_
+#define CHAP_DIGEST_UNKNOWN 0
#define CHAP_DIGEST_MD5 5
#define CHAP_DIGEST_SHA 6
-#define CHAP_CHALLENGE_LENGTH 16
+#define CHAP_CHALLENGE_LENGTH 16
Ditto here, avoid random whitespace changes in patches that fix bugs.
--nab
--
Thanks and regards.
Tejas Vaykole
Development Engineer.
Calsoft Inc.|Ext- 3169
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
