session setup bugfix and torture test
Michael Adam
obnox at samba.org
Fri Sep 20 08:13:21 CEST 2013
Hi Jeremy,
I have worked quite some more on the fix and test for the smbd
session setup bug.
The fix we found earlier was not complete in that we could
not sign the respons correctly any more. Metze and I have
improved the fix to talloc-move the session from the table
to the request so that it lives exactly as long as the request
in the error case.
Another fix is for the return code of subsequent requests
in the signing case.
I have added a test "smb2.session.reauth6" that verifies the
behaviour. You can run it manually against a server with
"smbtorture //server/share smb2.session.reauth6"
and selftest runs this under "make test TESTS=samba3.smb2.session"
against various smbd setups.
I think this is a complete patchset for now.
Please review and comment!
Cheers - Michael
-------------- next part --------------
From 359fac9305c7513ec8d59c75987dd3a217cc111a Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 19 Sep 2013 23:41:51 +0200
Subject: [PATCH 1/4] s3:smbd: fix crash when smb2 session reauth fails
Authentication error in smb2 session reauth invalidates
the session. In this case the session must in contrast
to successful session setup requests be torn down and live
no longer than the request.
The talloc move of the session from the global session
table to the request ensures that the session setup
reply can still be correctly signed, but subsequent
requests on the connection don't find a session any more.
Pair-Programmed-With: Jeremy Allison <jra at samba.org>
Pair-Programmed-With: Stefan Metzmacher <metze at samba.org>
Signed-off-by: Michael Adam <obnox at samba.org>
---
source3/smbd/smb2_sesssetup.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
diff --git a/source3/smbd/smb2_sesssetup.c b/source3/smbd/smb2_sesssetup.c
index 265f802..aee6e73 100644
--- a/source3/smbd/smb2_sesssetup.c
+++ b/source3/smbd/smb2_sesssetup.c
@@ -445,23 +445,38 @@ struct smbd_smb2_session_setup_state {
static int pp_self_ref_destructor(struct smbd_smb2_session_setup_state **pp_state)
{
- (*pp_state)->session = NULL;
/*
* To make things clearer, ensure the pp_self_ref
* pointer is nulled out. We're never going to
* access this again.
*/
(*pp_state)->pp_self_ref = NULL;
+
+ if ((*pp_state)->session == NULL) {
+ return 0;
+ }
+
+ (*pp_state)->session = NULL;
+
return 0;
}
static int smbd_smb2_session_setup_state_destructor(struct smbd_smb2_session_setup_state *state)
{
/*
- * if state->session is not NULL,
- * we remove the session on failure
+ * If state->session is not NULL,
+ * we move the session from the session table to the request on failure
+ * so that the error response can be correctly signed, but the session
+ * is then really deleted when the request is done.
*/
- TALLOC_FREE(state->session);
+
+ if (state->session == NULL) {
+ return 0;
+ }
+
+ state->session->status = NT_STATUS_USER_SESSION_DELETED;
+ state->smb2req->session = talloc_move(state->smb2req, &state->session);
+
return 0;
}
@@ -614,6 +629,7 @@ static void smbd_smb2_session_setup_gensec_done(struct tevent_req *subreq)
if (NT_STATUS_EQUAL(status, NT_STATUS_MORE_PROCESSING_REQUIRED)) {
state->out_session_id = state->session->global->session_wire_id;
/* we want to keep the session */
+ state->session = NULL;
TALLOC_FREE(state->pp_self_ref);
tevent_req_nterror(req, status);
return;
@@ -654,6 +670,7 @@ static void smbd_smb2_session_setup_gensec_done(struct tevent_req *subreq)
return;
}
/* we want to keep the session */
+ state->session = NULL;
TALLOC_FREE(state->pp_self_ref);
tevent_req_done(req);
return;
@@ -670,6 +687,7 @@ static void smbd_smb2_session_setup_gensec_done(struct tevent_req *subreq)
}
/* we want to keep the session */
+ state->session = NULL;
TALLOC_FREE(state->pp_self_ref);
tevent_req_done(req);
return;
@@ -701,6 +719,7 @@ static void smbd_smb2_session_setup_previous_done(struct tevent_req *subreq)
return;
}
/* we want to keep the session */
+ state->session = NULL;
TALLOC_FREE(state->pp_self_ref);
tevent_req_done(req);
return;
@@ -717,6 +736,7 @@ static void smbd_smb2_session_setup_previous_done(struct tevent_req *subreq)
}
/* we want to keep the session */
+ state->session = NULL;
TALLOC_FREE(state->pp_self_ref);
tevent_req_done(req);
return;
--
1.7.9.5
From 6cee815daf7810bc2235f818ea5b1924be68b977 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 20 Sep 2013 01:23:55 +0200
Subject: [PATCH 2/4] smbd:smb2: fix the error code to USER_SESSION_DELETED
when trying to operate on a deleted session in the
signing case
Signed-off-by: Michael Adam <obnox at samba.org>
---
source3/smbd/smb2_server.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c
index b031c6d..d3f3a08 100644
--- a/source3/smbd/smb2_server.c
+++ b/source3/smbd/smb2_server.c
@@ -1998,7 +1998,7 @@ NTSTATUS smbd_smb2_request_dispatch(struct smbd_smb2_request *req)
if (x == NULL) {
return smbd_smb2_request_error(
- req, NT_STATUS_ACCESS_DENIED);
+ req, NT_STATUS_USER_SESSION_DELETED);
}
signing_key = x->global->channels[0].signing_key;
--
1.7.9.5
From 304546c79988ba9969279255d17cd99026d4362f Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Fri, 20 Sep 2013 07:46:54 +0200
Subject: [PATCH 3/4] libcli/smb: add smb2cli_tcon_is_encryption_on()
Signed-off-by: Michael Adam <obnox at samba.org>
---
libcli/smb/smbXcli_base.c | 5 +++++
libcli/smb/smbXcli_base.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c
index 5089919..d8a943a 100644
--- a/libcli/smb/smbXcli_base.c
+++ b/libcli/smb/smbXcli_base.c
@@ -5095,3 +5095,8 @@ void smb2cli_tcon_set_values(struct smbXcli_tcon *tcon,
tcon->smb2.should_encrypt = true;
}
}
+
+bool smb2cli_tcon_is_encryption_on(struct smbXcli_tcon *tcon)
+{
+ return tcon->smb2.should_encrypt;
+}
diff --git a/libcli/smb/smbXcli_base.h b/libcli/smb/smbXcli_base.h
index a36bcfb..c0af3d3 100644
--- a/libcli/smb/smbXcli_base.h
+++ b/libcli/smb/smbXcli_base.h
@@ -316,6 +316,7 @@ void smb2cli_tcon_set_values(struct smbXcli_tcon *tcon,
uint32_t flags,
uint32_t capabilities,
uint32_t maximal_access);
+bool smb2cli_tcon_is_encryption_on(struct smbXcli_tcon *tcon);
struct tevent_req *smb2cli_session_setup_send(TALLOC_CTX *mem_ctx,
struct tevent_context *ev,
--
1.7.9.5
From 9a5d42dd424ac705a60d1a576b8625f9a75747c6 Mon Sep 17 00:00:00 2001
From: Michael Adam <obnox at samba.org>
Date: Thu, 19 Sep 2013 22:00:19 +0200
Subject: [PATCH 4/4] s4:torture: add smb2.session.reauth6
This attempts reauth with invalid creds, hence
triggering the error path in the reauth code.
This invalidates the session and subsequente requests
on that connection fail.
Signed-off-by: Michael Adam <obnox at samba.org>
---
source4/torture/smb2/session.c | 101 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 101 insertions(+)
diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c
index 6901f47..1e1beb0 100644
--- a/source4/torture/smb2/session.c
+++ b/source4/torture/smb2/session.c
@@ -855,6 +855,106 @@ done:
return ret;
}
+/**
+ * do reauth with wrong credentials,
+ * hence triggering the error path in reauth.
+ * The invalid reauth deletes the session.
+ */
+bool test_session_reauth6(struct torture_context *tctx, struct smb2_tree *tree)
+{
+ NTSTATUS status;
+ TALLOC_CTX *mem_ctx = talloc_new(tctx);
+ char fname[256];
+ struct smb2_handle _h1;
+ struct smb2_handle *h1 = NULL;
+ struct smb2_create io1;
+ bool ret = true;
+ union smb_fileinfo qfinfo;
+ char *substitute_user;
+ struct cli_credentials *broken_creds;
+ bool ok;
+ bool encrypted;
+ bool using_kerberos;
+ NTSTATUS expected;
+ enum credentials_use_kerberos krb_state;
+
+ krb_state = cli_credentials_get_kerberos_state(cmdline_credentials);
+ using_kerberos = (krb_state == CRED_MUST_USE_KERBEROS);
+
+ encrypted = smb2cli_tcon_is_encryption_on(tree->smbXcli);
+
+ /* Add some random component to the file name. */
+ snprintf(fname, 256, "session_reauth1_%s.dat",
+ generate_random_str(tctx, 8));
+
+ smb2_util_unlink(tree, fname);
+
+ smb2_oplock_create_share(&io1, fname,
+ smb2_util_share_access(""),
+ smb2_util_oplock_level("b"));
+ io1.in.create_options |= NTCREATEX_OPTIONS_DELETE_ON_CLOSE;
+
+ status = smb2_create(tree, mem_ctx, &io1);
+ CHECK_STATUS(status, NT_STATUS_OK);
+ _h1 = io1.out.file.handle;
+ h1 = &_h1;
+ CHECK_CREATED(&io1, CREATED, FILE_ATTRIBUTE_ARCHIVE);
+ CHECK_VAL(io1.out.oplock_level, smb2_util_oplock_level("b"));
+
+ /*
+ * reauthentication with invalid credentials:
+ */
+
+ broken_creds = cli_credentials_shallow_copy(mem_ctx,
+ cmdline_credentials);
+ torture_assert(tctx, (broken_creds != NULL), "talloc error");
+
+ substitute_user = generate_random_str_list(tctx, 8,
+ "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789");
+ ok = cli_credentials_set_username(broken_creds, substitute_user,
+ CRED_SPECIFIED);
+ CHECK_VAL(ok, true);
+
+ status = smb2_session_setup_spnego(tree->session,
+ broken_creds,
+ 0 /* previous_session_id */);
+ CHECK_STATUS(status, NT_STATUS_LOGON_FAILURE);
+
+ torture_comment(tctx, "did failed reauth\n");
+ /*
+ * now verify that the invalid session reauth has closed our session
+ */
+
+ if (encrypted) {
+ expected = NT_STATUS_CONNECTION_DISCONNECTED;
+ } else if (using_kerberos) {
+ expected = NT_STATUS_INVALID_NETWORK_RESPONSE;
+ } else {
+ expected = NT_STATUS_USER_SESSION_DELETED;
+ }
+
+ smb2_oplock_create_share(&io1, fname,
+ smb2_util_share_access(""),
+ smb2_util_oplock_level("b"));
+
+ status = smb2_create(tree, mem_ctx, &io1);
+ CHECK_STATUS(status, expected);
+
+done:
+ if (h1 != NULL) {
+ smb2_util_close(tree, *h1);
+ }
+
+ smb2_util_unlink(tree, fname);
+
+ talloc_free(tree);
+
+ talloc_free(mem_ctx);
+
+ return ret;
+}
+
+
static bool test_session_expire1(struct torture_context *tctx)
{
NTSTATUS status;
@@ -980,6 +1080,7 @@ struct torture_suite *torture_smb2_session_init(void)
torture_suite_add_1smb2_test(suite, "reauth3", test_session_reauth3);
torture_suite_add_1smb2_test(suite, "reauth4", test_session_reauth4);
torture_suite_add_1smb2_test(suite, "reauth5", test_session_reauth5);
+ torture_suite_add_1smb2_test(suite, "reauth6", test_session_reauth6);
torture_suite_add_simple_test(suite, "expire1", test_session_expire1);
suite->description = talloc_strdup(suite, "SMB2-SESSION tests");
--
1.7.9.5
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 215 bytes
Desc: Digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20130920/7bea7c23/attachment.pgp>
More information about the samba-technical
mailing list