Bug in login negotiations
William Studenmund
Wasabi target, I discovered what seems to be a bug in the security
phase login handling.
Oh, this issue was observed with core-iscsi-v1.6.2.5.
I've attached a tethereal -V dump of the capture, and I have removed
the ethernet frame info & the details for the TCP and IP protocols.
The important points I see are:
Frame 4 is the initial Login Request packet. AuthMethod=CHAP,None,
the T bit is NOT set, and NSG is 0 (security phase).
The target is configured to only support "None", so it negotiates
AuthMethod=None. As the T bit was not set, we are still in Security
Phase.
The target's response is in frame 7.
The problem happens in Frame 9, the second Login Request from the
initiator. The initiator now thinks that we are in the Operational
Phase (CSG is 1). It attempts to negotiate HeaderDigest and
DataDigest. The target does not think we're in the operational phase,
so these keys get rejected.
I think the target is correct that, just because we negotiated
AuthMethod=None, we aren't automatically in Operational Phase
negotiations. You still have to have the T bit and transition.
I do think the target reported the error wrong. Because this case was
never expected, the target just copies CSG values from the passed-in
packet. Thus it reports that we are in CSG=1 even though it isn't.
Also, it should report an Initiator Error for the invalid transition.
I'll work on this.
I think the bug is around line 778 of iscsi_initiator_nego.c where we
call iscsi_initiator_post_csg_zero() if AuthMethod was negotiated to
None. That's wrong, as we should only transition if the 'T' bit is set.
I think what should have happened is that, because "None" is an
offered security method, the initial Login Request (frame 4 here)
should have had the T bit set and NSG set to 1. Then the target would
have agreed to "None" and triggered the transition to the Operational
Parameter stage.
Thoughts?
Take care,
Bill
n...@kernel.org
I think the correct logic here needs to be:
1) iscsi_initiator_parameter_negotiation():
If 'None' is present in the value list for AuthMethod, go ahead and
set:
login_req->flags |= (CSG0|NSG1|T_BIT);
2) iscsi_initiator_handle_csg_zero()
If != 'None' Authmethod is selected by the acceptor:
1) Clear NSG1 and T_BIT from login_req->flags after the first Login
Request in sent with 'None' Authmethod present, and let
iscsi_initiator_check_key_for_transit() reset when the
post-authentication strage transit occurs.
This would be the quickest solution.
Thanks!
--nab
> --Apple-Mail-9--486479100
> Content-Type: text/plain
> Content-Disposition: inline;
> filename="PacketLog.txt"
> X-Google-AttachSize: 5651
>
> Frame 1 (62 bytes on wire, 62 bytes captured)
> Internet Protocol, Src: 192.168.1.101 (192.168.1.101), Dst: 192.168.1.10 (192.168.1.10)
> Transmission Control Protocol, Src Port: 36774 (36774), Dst Port: iscsi-target (3260), Seq: 0, Ack: 0, Len: 0
>
> Frame 2 (62 bytes on wire, 62 bytes captured)
> Internet Protocol, Src: 192.168.1.10 (192.168.1.10), Dst: 192.168.1.101 (192.168.1.101)
> Transmission Control Protocol, Src Port: iscsi-target (3260), Dst Port: 36774 (36774), Seq: 0, Ack: 1, Len: 0
>
> Frame 3 (54 bytes on wire, 54 bytes captured)
> Internet Protocol, Src: 192.168.1.101 (192.168.1.101), Dst: 192.168.1.10 (192.168.1.10)
> Transmission Control Protocol, Src Port: 36774 (36774), Dst Port: iscsi-target (3260), Seq: 1, Ack: 1, Len: 0
>
> Frame 4 (226 bytes on wire, 226 bytes captured)
> Internet Protocol, Src: 192.168.1.101 (192.168.1.101), Dst: 192.168.1.10 (192.168.1.10)
> Transmission Control Protocol, Src Port: 36774 (36774), Dst Port: iscsi-target (3260), Seq: 1, Ack: 1, Len: 172
> iSCSI (Login Command)
> Opcode: Login Command (0x03)
> 0... .... = T: Stay in current login stage
> .0.. .... = C: Text is complete
> .... 00.. = CSG: Security negotiation (0x00)
> VersionMax: 0x00
> VersionMin: 0x00
> TotalAHSLength: 0x00
> DataSegmentLength: 0x0000007c
> ISID: 809908C60000
> 10.. .... = ISID_t: Random (0x02)
> ..00 0000 = ISID_a: 0x00
> ISID_b: 0x9908
> ISID_c: 0xc6
> ISID_d: 0x0000
> TSIH: 0x0000
> InitiatorTaskTag: 0x00000001
> CID: 0x0000
> CmdSN: 0x00000001
> ExpStatSN: 0x00000000
> Key/Value Pairs
> KeyValue: AuthMethod=CHAP,None
> KeyValue: InitiatorName=iqn.2005-09.com.rackable.test:host101
> KeyValue: InitiatorAlias=PyX Initiator
> KeyValue: SessionType=Discovery
>
> Frame 5 (60 bytes on wire, 60 bytes captured)
> Internet Protocol, Src: 192.168.1.10 (192.168.1.10), Dst: 192.168.1.101 (192.168.1.101)
> Transmission Control Protocol, Src Port: iscsi-target (3260), Dst Port: 36774 (36774), Seq: 1, Ack: 173, Len: 0
>
> Frame 6 (60 bytes on wire, 60 bytes captured)
> Internet Protocol, Src: 192.168.1.10 (192.168.1.10), Dst: 192.168.1.101 (192.168.1.101)
> Transmission Control Protocol, Src Port: iscsi-target (3260), Dst Port: 36774 (36774), Seq: 1, Ack: 173, Len: 0
>
> Frame 7 (118 bytes on wire, 118 bytes captured)
> Internet Protocol, Src: 192.168.1.10 (192.168.1.10), Dst: 192.168.1.101 (192.168.1.101)
> Transmission Control Protocol, Src Port: iscsi-target (3260), Dst Port: 36774 (36774), Seq: 1, Ack: 173, Len: 64
> iSCSI (Login Response)
> Opcode: Login Response (0x23)
> 0... .... = T: Stay in current login stage
> .0.. .... = C: Text is complete
> .... 00.. = CSG: Security negotiation (0x00)
> VersionMax: 0x00
> VersionActive: 0x00
> TotalAHSLength: 0x00
> DataSegmentLength: 0x00000010
> ISID: 809908C60000
> 10.. .... = ISID_t: Random (0x02)
> ..00 0000 = ISID_a: 0x00
> ISID_b: 0x9908
> ISID_c: 0xc6
> ISID_d: 0x0000
> TSIH: 0x0000
> InitiatorTaskTag: 0x00000001
> StatSN: 0x00000000
> ExpCmdSN: 0x00000001
> MaxCmdSN: 0x00000008
> Status: Success (0x0000)
> Key/Value Pairs
> KeyValue: AuthMethod=None
>
> Frame 8 (54 bytes on wire, 54 bytes captured)
> Internet Protocol, Src: 192.168.1.101 (192.168.1.101), Dst: 192.168.1.10 (192.168.1.10)
> Transmission Control Protocol, Src Port: 36774 (36774), Dst Port: iscsi-target (3260), Seq: 173, Ack: 65, Len: 0
>
> Frame 9 (182 bytes on wire, 182 bytes captured)
> Internet Protocol, Src: 192.168.1.101 (192.168.1.101), Dst: 192.168.1.10 (192.168.1.10)
> Transmission Control Protocol, Src Port: 36774 (36774), Dst Port: iscsi-target (3260), Seq: 173, Ack: 65, Len: 128
> iSCSI (Login Command)
> Opcode: Login Command (0x03)
> 0... .... = T: Stay in current login stage
> .0.. .... = C: Text is complete
> .... 01.. = CSG: Operational negotiation (0x01)
> VersionMax: 0x00
> VersionMin: 0x00
> TotalAHSLength: 0x00
> DataSegmentLength: 0x0000004e
> ISID: 809908C60000
> 10.. .... = ISID_t: Random (0x02)
> ..00 0000 = ISID_a: 0x00
> ISID_b: 0x9908
> ISID_c: 0xc6
> ISID_d: 0x0000
> TSIH: 0x0000
> InitiatorTaskTag: 0x00000001
> CID: 0x0000
> CmdSN: 0x00000001
> ExpStatSN: 0x00000001
> Key/Value Pairs
> KeyValue: HeaderDigest=CRC32C,None
> KeyValue: DataDigest=CRC32C,None
> KeyValue: MaxRecvDataSegmentLength=8192
> Padding: 6174
>
> Frame 10 (60 bytes on wire, 60 bytes captured)
> Internet Protocol, Src: 192.168.1.10 (192.168.1.10), Dst: 192.168.1.101 (192.168.1.101)
> Transmission Control Protocol, Src Port: iscsi-target (3260), Dst Port: 36774 (36774), Seq: 65, Ack: 301, Len: 0
>
> Frame 11 (142 bytes on wire, 142 bytes captured)
> Internet Protocol, Src: 192.168.1.10 (192.168.1.10), Dst: 192.168.1.101 (192.168.1.101)
> Transmission Control Protocol, Src Port: iscsi-target (3260), Dst Port: 36774 (36774), Seq: 65, Ack: 301, Len: 88
> iSCSI (Login Response)
> Opcode: Login Response (0x23)
> 0... .... = T: Stay in current login stage
> .0.. .... = C: Text is complete
> .... 01.. = CSG: Operational negotiation (0x01)
> VersionMax: 0x00
> VersionActive: 0x00
> TotalAHSLength: 0x00
> DataSegmentLength: 0x00000026
> ISID: 809908C60000
> 10.. .... = ISID_t: Random (0x02)
> ..00 0000 = ISID_a: 0x00
> ISID_b: 0x9908
> ISID_c: 0xc6
> ISID_d: 0x0000
> TSIH: 0x0000
> InitiatorTaskTag: 0x00000001
> StatSN: 0x00000001
> ExpCmdSN: 0x00000001
> MaxCmdSN: 0x00000008
> Status: Missing parameter (0x0207)
> Key/Value Pairs
> KeyValue: HeaderDigest=Reject
> KeyValue: DataDigest=Reject
> Padding: 0000
>
> --Apple-Mail-9--486479100--
n...@kernel.org
Thanks,
--nab
diff -urN core-iscsi-v1.6.2.6/iscsi_initiator_nego.c
core-iscsi-v1.6.2.7/iscsi_initiator_nego.c
--- core-iscsi-v1.6.2.6/iscsi_initiator_nego.c 2006-03-12
19:22:54.294036000 -0800
+++ core-iscsi-v1.6.2.7/iscsi_initiator_nego.c 2006-06-18
18:47:20.000000000 -0700
@@ -498,7 +498,7 @@
static int iscsi_initiator_handle_csg_zero(iscsi_conn_t *conn,
iscsi_login_t *login)
{
int ret;
- iscsi_param_t *param;
+ iscsi_param_t *param = NULL;
struct iscsi_init_login_cmnd *login_req;
struct iscsi_targ_login_rsp *login_rsp;
@@ -516,7 +516,17 @@
else if (ret > 0)
goto do_auth;
- if (login->first_response)
+ if (login->first_response) {
+ if (!(param = iscsi_find_param_from_key(AUTHMETHOD,
conn->param_list)))
+ return(-1);
+
+ if ((IS_PSTATE_PROPOSER(param) &&
IS_PSTATE_RESPONSE_GOT(param))) {
+ if (strcmp(param->value, NONE))
+ login_req->flags &= ~NSG1;
+ login_req->flags &= ~T_BIT;
+ }
+ }
+
if (iscsi_initiator_check_first_response(conn, login) <
0)
return(-1);
@@ -532,12 +542,6 @@
if (login_rsp->flags & T_BIT)
return(iscsi_initiator_post_csg_zero(conn, login));
- if (!(param = iscsi_find_param_from_key(AUTHMETHOD,
conn->param_list)))
- return(-1);
-
- if (!strcmp(param->value, NONE))
- return(iscsi_initiator_post_csg_zero(conn, login));
-
if ((IS_PSTATE_PROPOSER(param) &&
IS_PSTATE_RESPONSE_GOT(param)) ||
IS_PSTATE_ACCEPTOR(param))
goto do_auth;
@@ -639,6 +643,7 @@
*/
static int iscsi_initiator_parameter_negotiation(iscsi_conn_t *conn,
iscsi_login_t *login)
{
+ char *buf;
int ret = 0;
iscsi_param_t *param;
iscsi_session_t *sess = SESS(conn);
@@ -678,7 +683,8 @@
" skipping SecurityNegotiation phase.\n",
c->channel_id);
login->current_stage = 1;
login_req->flags |= (CSG1|NSG3|T_BIT);
- }
+ } else if ((buf = strstr(param->value, NONE)))
+ login_req->flags |= (NSG1|T_BIT);
if (iscsi_encode_text_output(
(login_req->flags & CSG1) ?
n...@kernel.org
We have two options for logic in the scenario previously mentioned:
1) Set T_BIT in first login request, if a non 'None' Authmethod
parameter is negoitated, NSG and T_BIT are cleared until the initiator
sends the final Authmethod specific key as mentioned with
iscsi_initiator_check_key_for_transit() at which point they are both
set as expected.
2) Keep NSG=1 + T_BIT set in every Login Request PDU in the
SecurityAuthentication Stage when 'None' is in the AuthMethod value
list.
Both are legal so I don't think there are any additional considerations
as far as interopt is concerned. The logic of the former is included
in the patch, if there is any additional considerations why the later
should be considered as the solution (ie: Setting NSG=1 + T_BIT in all
CSG=0 PDUs) please let me know.
Thanks again Bill,
--nab
William Studenmund
Hash: SHA1
On Jun 18, 2006, at 7:29 PM, n...@kernel.org wrote:
> A quick followup:
>
> We have two options for logic in the scenario previously mentioned:
>
> 1) Set T_BIT in first login request, if a non 'None' Authmethod
> parameter is negoitated, NSG and T_BIT are cleared until the initiator
> sends the final Authmethod specific key as mentioned with
> iscsi_initiator_check_key_for_transit() at which point they are both
> set as expected.
>
> 2) Keep NSG=1 + T_BIT set in every Login Request PDU in the
> SecurityAuthentication Stage when 'None' is in the AuthMethod value
> list.
>
> Both are legal so I don't think there are any additional
> considerations
> as far as interopt is concerned. The logic of the former is included
> in the patch, if there is any additional considerations why the later
> should be considered as the solution (ie: Setting NSG=1 + T_BIT in all
> CSG=0 PDUs) please let me know.
I haven't had a chance to test this, but I believe that (1) is best.
If we're in the middle of security negotiation, we don't want to exit
out of it before the last PDU has been sent.
The problem with (2) is that we really don't want to transition out
of the auth method until the end. To do otherwise would be opening us
up for security phase violations. Say a target impostor didn't want
to do mutual, saw us set the T_BIT at the first bit of CHAP
processing, then replies with the T_BIT set so we can't ask for mutual.
Since security negotiation is supposed to happen in distinct steps
and (2) would, at first glance, let us end w/o completing all of the
steps, I don't think (2) really is legal.
Take care,
Bill
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Darwin)
iD8DBQFEl4NADJT2Egh26K0RArZeAJ47m0jxflog3LopR+lnftOKp1tEgACeK7Zo
u6X6tJ8kyRT6EDCqZjMDLdg=
=PVXZ
-----END PGP SIGNATURE-----
Nicholas A. Bellinger
The #1 logic and previous patch sound good to me, lets get this tested
and included. I know you are a busy guy, so let us know the status on
this and we can put out another release. Also, would this be a scenario
that Albert could test w/ his Wasabi Storagebuilder? As the list has
noticed over the past year, he is very good at finding and reporting
login & authentication bugs! :-)
--nab
William Studenmund
Hash: SHA1
On Jun 19, 2006, at 11:27 PM, Nicholas A. Bellinger wrote:
> The #1 logic and previous patch sound good to me, lets get this tested
> and included. I know you are a busy guy, so let us know the status on
> this and we can put out another release. Also, would this be a
> scenario
> that Albert could test w/ his Wasabi Storagebuilder? As the list has
> noticed over the past year, he is very good at finding and reporting
> login & authentication bugs! :-)
Actually, anyone can test this! Though I haven't seen how other
targets handle the underlying problem.
To see the error, configure the target to support "None"
authentication and tell core-iscsi to offer "AuthMethod=None,CHAP".
You should go through a security phase and then select None,
triggering the issue.
Then apply the patch, and see how things are different.
Take care,
Bill
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (Darwin)
iD8DBQFEmCHdDJT2Egh26K0RAvoPAJ9/EkXB7RUjatwC55seKj2rcRd9NACgnrcb
Tj0kZSpzNG6qeZu54tO5Hhc=
=YsDW
-----END PGP SIGNATURE-----
Nicholas A. Bellinger
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On Jun 19, 2006, at 11:27 PM, Nicholas A. Bellinger wrote:
>
> > The #1 logic and previous patch sound good to me, lets get this tested
> > and included. I know you are a busy guy, so let us know the status on
> > this and we can put out another release. Also, would this be a
> > scenario
> > that Albert could test w/ his Wasabi Storagebuilder? As the list has
> > noticed over the past year, he is very good at finding and reporting
> > login & authentication bugs! :-)
>
> Actually, anyone can test this! Though I haven't seen how other
> targets handle the underlying problem.
>
> To see the error, configure the target to support "None"
> authentication and tell core-iscsi to offer "AuthMethod=None,CHAP".
> You should go through a security phase and then select None,
> triggering the issue.
>
> Then apply the patch, and see how things are different.
>
Ok, I tested this with a slightly different patch (the previous one was
broken), and the issue is resolved. I have this committed for v1.6.2.7,
and will release after we get the mutual authentication + discovery
validated that Albert has been looking at. Please feel free to take a
look at the latter as the folks in the UK are heading to bed soon.
Thanks again!
--
Nicholas A. Bellinger <n...@linux-mips.org>
Index: iscsi_initiator_nego.c
===================================================================
--- iscsi_initiator_nego.c (revision 3932)
+++ iscsi_initiator_nego.c (working copy)
@@ -498,7 +498,7 @@
static int iscsi_initiator_handle_csg_zero(iscsi_conn_t *conn,
iscsi_login_t *login)
{
int ret;
- iscsi_param_t *param;
+ iscsi_param_t *param = NULL;
struct iscsi_init_login_cmnd *login_req;
struct iscsi_targ_login_rsp *login_rsp;
@@ -516,9 +516,20 @@
else if (ret > 0)
goto do_auth;
- if (login->first_response)
+ if (login->first_response) {
+ if (!(param = iscsi_find_param_from_key(AUTHMETHOD,
conn->param_list)))
+ return(-1);
+
+ if ((IS_PSTATE_PROPOSER(param) &&
IS_PSTATE_RESPONSE_GOT(param))) {
+ if (strcmp(param->value, NONE)) {
+ login_req->flags &= ~NSG1;
+ login_req->flags &= ~T_BIT;
+ }
+ }
+
if (iscsi_initiator_check_first_response(conn, login) <
0)
return(-1);
+ }
ret = iscsi_encode_text_output(
PHASE_SECURITY|PHASE_DECLARATIVE,
@@ -532,12 +543,6 @@
if (login_rsp->flags & T_BIT)
return(iscsi_initiator_post_csg_zero(conn, login));
- if (!(param = iscsi_find_param_from_key(AUTHMETHOD,
conn->param_list)))
- return(-1);
-
- if (!strcmp(param->value, NONE))
- return(iscsi_initiator_post_csg_zero(conn, login));
-
if ((IS_PSTATE_PROPOSER(param) && IS_PSTATE_RESPONSE_GOT(param))
||
IS_PSTATE_ACCEPTOR(param))
goto do_auth;
@@ -639,6 +644,7 @@
*/
static int iscsi_initiator_parameter_negotiation(iscsi_conn_t *conn,
iscsi_login_t *login)
{
+ char *buf;
int ret = 0;
iscsi_param_t *param;
iscsi_session_t *sess = SESS(conn);
@@ -678,7 +684,8 @@