From 7884d94ba71089a648ff2e7c50574e9c1096e550 Mon Sep 17 00:00:00 2001 From: Michael Adam Date: Thu, 19 Sep 2013 23:41:51 +0200 Subject: [PATCH 1/4] smbd:smb2: fix crash when smb2 session reauth fails https://bugzilla.samba.org/show_bug.cgi?id=10208 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 Pair-Programmed-With: Stefan Metzmacher Signed-off-by: Michael Adam (cherry picked from commit 25494628a2e977568de0f634602ebe893d0a5b88) --- 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.8.3.1 From 205ec3cacd684e93a0dc052264c3b5f53d6db330 Mon Sep 17 00:00:00 2001 From: Michael Adam Date: Fri, 20 Sep 2013 07:46:54 +0200 Subject: [PATCH 2/4] libcli/smb: add smb2cli_tcon_is_encryption_on() https://bugzilla.samba.org/show_bug.cgi?id=10208 Signed-off-by: Michael Adam Reviewed-by: Jeremy Allison (cherry picked from commit f6439613432f73319ca6b855256ee44921071991) --- 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 14d4cc3..6943181 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -5088,3 +5088,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 3d93427..852fc71 100644 --- a/libcli/smb/smbXcli_base.h +++ b/libcli/smb/smbXcli_base.h @@ -315,6 +315,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.8.3.1 From 3e176b79333d067faeb021a8362d16d14f6b4380 Mon Sep 17 00:00:00 2001 From: Michael Adam Date: Thu, 19 Sep 2013 22:00:19 +0200 Subject: [PATCH 3/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. https://bugzilla.samba.org/show_bug.cgi?id=10208 Signed-off-by: Michael Adam Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Tue Oct 15 22:50:27 CEST 2013 on sn-devel-104 (cherry picked from commit f50b6da7d5862aa8d4a3ea04df9b9121b083e2a8) --- 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.8.3.1 From 63d1ab9c76d70dd67ee607960fdf598e043f6e0f Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 23 Sep 2013 14:10:27 -0700 Subject: [PATCH 4/4] smbd: Invalidate the session correctly. When a session is invalidated then we must also ensure it isn't used in any pending requests being processed. Signed-off-by: Jeremy Allison Reviewed-by: Andreas Schneider (cherry picked from commit d4a5c832f1806a9c664d52a34ea1a24eb370fa89) --- source3/smbd/smb2_sesssetup.c | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/source3/smbd/smb2_sesssetup.c b/source3/smbd/smb2_sesssetup.c index dd243c9..cb8f847 100644 --- a/source3/smbd/smb2_sesssetup.c +++ b/source3/smbd/smb2_sesssetup.c @@ -457,6 +457,8 @@ 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) { + struct smbd_smb2_request *preq; + /* * If state->session is not NULL, * we move the session from the session table to the request on failure @@ -471,6 +473,27 @@ static int smbd_smb2_session_setup_state_destructor(struct smbd_smb2_session_set state->session->status = NT_STATUS_USER_SESSION_DELETED; state->smb2req->session = talloc_move(state->smb2req, &state->session); + /* + * We've made this session owned by the current request. + * Ensure that any outstanding requests don't also refer + * to it. + */ + + for (preq = state->smb2req->sconn->smb2.requests; preq != NULL; preq = preq->next) { + if (preq == state->smb2req) { + continue; + } + if (preq->session == state->smb2req->session) { + preq->session = NULL; + /* + * If we no longer have a session we can't + * sign or encrypt replies. + */ + preq->do_signing = false; + preq->do_encryption = false; + } + } + return 0; } -- 1.8.3.1