session setup bugfix and torture test
Michael Adam
obnox at samba.org
Sat Sep 21 03:05:22 CEST 2013
Hi Metze,
On 2013-09-20 at 11:41 +0200, Stefan (metze) Metzmacher wrote:
>
> I think this changes are a good start, but I try to
> fix this more broadly similar to this change
>
> commit e6a58d370403e818bc2cfb8389751b78adcc14fd
> s4:rpc_server: make sure we don't terminate a connection with
> pending requests (bug #9820)
>
> ...
>
> The goal should be that we move any session, tcon and open to a deleted list
> on the connection and just mark them as deleted like this
>
> session->status = NT_STATUS_USER_SESSION_DELETED;
> tcon->status = NT_STATUS_NETWORK_NAME_DELETED;
> open->status = NT_STATUS_FILE_CLOSED;
>
> And do through all pending request which match the closed object
> and do tevent_req_cancel(smb2req->subreq);
>
> Before we process any new incoming request, we would scan the deleted lists
> and clean the objects up if they aren't used anymore.
Ok. Sounds good. I need to think about this..
For now find attached an update to the patchset that
addresses your comments below:
> Am 20.09.2013 08:13, schrieb Michael Adam:
> > --- 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;
> > }
>
> I think we should remove this hunk, it doesn't change anything.
Done.
> > + substitute_user = generate_random_str_list(tctx, 8,
> > + "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789");
> > + ok = cli_credentials_set_username(broken_creds, substitute_user,
> > + CRED_SPECIFIED);
> > + CHECK_VAL(ok, true);
> > +
>
> Maybe we should just set a wrong password here?
Oh yeah. That was what I wanted to do initially, but somehow
I did not see the set_password function when I first looked :)
Changed the test accordingly.
> > + 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;
>
> Is this correct? didn't we get that from smb2_session_setup_spnego()
> I can't see why the create should fail with that status.
Indeed. I changed the test to skip completely if kerberos is
used. I was deceived by the fact that smb2_session_setup_spnego()
returns the correct NT_STATUS, but since this is caught in
kerbereos preauth, the session reauth request is never sent
to the server and the session is hence not deleted on the server.
So the test is invalid in the kerberos case...
Cheers - Michael
-------------- next part --------------
From b71f36d690b9c17c8f151d170ce65fd08ede9a16 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] smbd:smb2: 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 | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/source3/smbd/smb2_sesssetup.c b/source3/smbd/smb2_sesssetup.c
index 265f802..dd243c9 100644
--- a/source3/smbd/smb2_sesssetup.c
+++ b/source3/smbd/smb2_sesssetup.c
@@ -458,10 +458,19 @@ static int pp_self_ref_destructor(struct smbd_smb2_session_setup_state **pp_stat
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 +623,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 +664,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 +681,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 +713,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 +730,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 90c960301e93b734a03709d331ddb30abf828033 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 9b25af71795dce4eacc9bea5ca2365bdb6ea3c5a 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 15f00dced6aadc24d8c70dc21bf991321c4d03e0 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 : test failing
reauth
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 | 103 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 103 insertions(+)
diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c
index 6901f47..b28b496 100644
--- a/source4/torture/smb2/session.c
+++ b/source4/torture/smb2/session.c
@@ -855,6 +855,108 @@ 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;
+ char *corrupted_password;
+ struct cli_credentials *broken_creds;
+ bool ok;
+ bool encrypted;
+ NTSTATUS expected;
+ enum credentials_use_kerberos krb_state;
+
+ krb_state = cli_credentials_get_kerberos_state(cmdline_credentials);
+ if (krb_state == CRED_MUST_USE_KERBEROS) {
+ torture_skip(tctx,
+ "Can't test failing session setup with 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");
+
+ corrupted_password = talloc_asprintf(mem_ctx, "%s%s",
+ cli_credentials_get_password(broken_creds),
+ "corrupt");
+ torture_assert(tctx, (corrupted_password != NULL), "talloc error");
+
+ ok = cli_credentials_set_password(broken_creds, corrupted_password,
+ 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 {
+ 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 +1082,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/20130921/3a7967a7/attachment.pgp>
More information about the samba-technical
mailing list