From 60fc725feae7a62e098f424a8561535a15093d49 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 8 Nov 2018 16:24:45 +0100 Subject: [PATCH 01/11] s3:selftest: split "raw.session" and "smb2.session" The next commit is going to add a testsuite to "smb2.session". Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit d0a8899ed57c2b368c3870b3899a3422251222aa) --- source3/selftest/tests.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 2763b1bf146..810658d1338 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -458,7 +458,12 @@ tests= base + raw + smb2 + rpc + unix + local + rap + nbt + libsmbclient + idmap plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') plansmbtorture4testsuite(t, "simpleserver", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD') - elif t == "raw.session" or t == "smb2.session": + elif t == "raw.session": + plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD', 'plain') + plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmpenc -U$USERNAME%$PASSWORD', 'enc') + plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -k no -U$USERNAME%$PASSWORD', 'ntlm') + plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -k yes -U$USERNAME%$PASSWORD', 'krb5') + elif t == "smb2.session": plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD', 'plain') plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmpenc -U$USERNAME%$PASSWORD', 'enc') plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -k no -U$USERNAME%$PASSWORD', 'ntlm') -- 2.17.2 From 718a2b7d864d188aac278368e8a8cd1afaf44bd0 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 9 Nov 2018 12:33:29 +0100 Subject: [PATCH 02/11] s3:selftest: also run smb2.session torture testsuite against ad_member The next commit adds a subtest to the smb2.session testsuite that requires Kerberos (ad_dc would work), but where neither SMB2 server or client must require signing (ad_dc, being an AD DC, requires signing). The ad_member environment supports Kerberos with the SMB2 server not mandating signing, that'll do. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit b86c94f0b929f2d9e521d41396c4e1611f5a4c5b) --- source3/selftest/tests.py | 1 + 1 file changed, 1 insertion(+) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 810658d1338..09cd5159a0d 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -468,6 +468,7 @@ tests= base + raw + smb2 + rpc + unix + local + rap + nbt + libsmbclient + idmap plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmpenc -U$USERNAME%$PASSWORD', 'enc') plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -k no -U$USERNAME%$PASSWORD', 'ntlm') plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -k yes -U$USERNAME%$PASSWORD', 'krb5') + plansmbtorture4testsuite(t, "ad_member", '//$SERVER/tmp -k yes -U$DC_USERNAME@$REALM%$DC_PASSWORD', 'krb5') elif t == "rpc.lsa": plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD', 'over ncacn_np ') plansmbtorture4testsuite(t, "nt4_dc", 'ncacn_ip_tcp:$SERVER_IP -U$USERNAME%$PASSWORD', 'over ncacn_ip_tcp ') -- 2.17.2 From 26e58cc1f5c97bfa84c7c4a99217077570d0e4f6 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 9 Nov 2018 15:17:19 +0100 Subject: [PATCH 03/11] libcli/smb: add smb2cli_session_require_signed_response() Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit d407201d9bd4ee5ae5609dd107e3ab9ee7afbeb0) --- libcli/smb/smbXcli_base.c | 7 +++++++ libcli/smb/smbXcli_base.h | 2 ++ 2 files changed, 9 insertions(+) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index 9edb6292777..5a473dd91aa 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -161,6 +161,7 @@ struct smb2cli_session { uint64_t nonce_low; uint16_t channel_sequence; bool replay_active; + bool require_signed_response; }; struct smbXcli_session { @@ -5719,6 +5720,12 @@ void smb2cli_session_stop_replay(struct smbXcli_session *session) session->smb2->replay_active = false; } +void smb2cli_session_require_signed_response(struct smbXcli_session *session, + bool require_signed_response) +{ + session->smb2->require_signed_response = require_signed_response; +} + NTSTATUS smb2cli_session_update_preauth(struct smbXcli_session *session, const struct iovec *iov) { diff --git a/libcli/smb/smbXcli_base.h b/libcli/smb/smbXcli_base.h index 536c7ab60f4..42c2519c7ff 100644 --- a/libcli/smb/smbXcli_base.h +++ b/libcli/smb/smbXcli_base.h @@ -492,6 +492,8 @@ uint16_t smb2cli_session_reset_channel_sequence(struct smbXcli_session *session, uint16_t smb2cli_session_current_channel_sequence(struct smbXcli_session *session); void smb2cli_session_start_replay(struct smbXcli_session *session); void smb2cli_session_stop_replay(struct smbXcli_session *session); +void smb2cli_session_require_signed_response(struct smbXcli_session *session, + bool require_signed_response); NTSTATUS smb2cli_session_update_preauth(struct smbXcli_session *session, const struct iovec *iov); NTSTATUS smb2cli_session_set_session_key(struct smbXcli_session *session, -- 2.17.2 From ddd4312c02391d4ca8e32817be584a7f45fe80f1 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 9 Nov 2018 15:26:44 +0100 Subject: [PATCH 04/11] libcli/smb: maintain require_signed_response in smbXcli_req_state Not used for now, that comes next. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit 67cfb01611869b7590ccd836dd13a80e53545714) --- libcli/smb/smbXcli_base.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index 5a473dd91aa..a93f07c8ee6 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -290,6 +290,7 @@ struct smbXcli_req_state { uint64_t encryption_session_id; bool signing_skipped; + bool require_signed_response; bool notify_async; bool got_async; uint16_t cancel_flags; @@ -2963,6 +2964,8 @@ struct tevent_req *smb2cli_req_create(TALLOC_CTX *mem_ctx, state->smb2.should_sign = session->smb2->should_sign; state->smb2.should_encrypt = session->smb2->should_encrypt; + state->smb2.require_signed_response = + session->smb2->require_signed_response; if (cmd == SMB2_OP_SESSSETUP && session->smb2_channel.signing_key.length == 0 && -- 2.17.2 From 8264e9eec582b9333e78e6b5b5b230b9673bf97f Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 10 Nov 2018 21:56:28 +0100 Subject: [PATCH 05/11] libcli/smb: defer singing check a little bit This allows adding an additional condition to the if check where the condition state may be modified in the "if (opcode == SMB2_OP_SESSSETUP)" case directly above. No change in behaviour. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit 7abf3900218e3d27c075b405735b2c38ec0fc4ca) --- libcli/smb/smbXcli_base.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index a93f07c8ee6..ea7ca22f644 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -3753,12 +3753,6 @@ static NTSTATUS smb2cli_conn_dispatch_incoming(struct smbXcli_conn *conn, } last_session = session; - if (state->smb2.should_sign) { - if (!(flags & SMB2_HDR_FLAG_SIGNED)) { - return NT_STATUS_ACCESS_DENIED; - } - } - if (flags & SMB2_HDR_FLAG_SIGNED) { uint64_t uid = BVAL(inhdr, SMB2_HDR_SESSION_ID); @@ -3807,6 +3801,12 @@ static NTSTATUS smb2cli_conn_dispatch_incoming(struct smbXcli_conn *conn, } } + if (state->smb2.should_sign) { + if (!(flags & SMB2_HDR_FLAG_SIGNED)) { + return NT_STATUS_ACCESS_DENIED; + } + } + if (cur[0].iov_len == SMB2_TF_HDR_SIZE) { const uint8_t *tf = (const uint8_t *)cur[0].iov_base; uint64_t uid = BVAL(tf, SMB2_TF_SESSION_ID); -- 2.17.2 From 706f6c0933495b4dc22bd2e435d15693cbb485b6 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Sat, 10 Nov 2018 22:00:04 +0100 Subject: [PATCH 06/11] libcli/smb: use require_signed_response in smb2cli_conn_dispatch_incoming() This can be used by the upper layers to force checking a response is signed. It will be used to implement verification of session setup reauth responses in a torture test. That comes next. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit 53fe148476a5566b7a8204d7e44b6e75ce7d45bc) --- libcli/smb/smbXcli_base.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index ea7ca22f644..d0cc33b8b05 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -3799,14 +3799,29 @@ static NTSTATUS smb2cli_conn_dispatch_incoming(struct smbXcli_conn *conn, */ signing_key = NULL; } + + if (!NT_STATUS_IS_OK(status)) { + /* + * Only check the signature of the last response + * of a successfull session auth. This matches + * Windows behaviour for NTLM auth and reauth. + */ + state->smb2.require_signed_response = false; + } } - if (state->smb2.should_sign) { + if (state->smb2.should_sign || + state->smb2.require_signed_response) + { if (!(flags & SMB2_HDR_FLAG_SIGNED)) { return NT_STATUS_ACCESS_DENIED; } } + if (signing_key == NULL && state->smb2.require_signed_response) { + signing_key = &session->smb2_channel.signing_key; + } + if (cur[0].iov_len == SMB2_TF_HDR_SIZE) { const uint8_t *tf = (const uint8_t *)cur[0].iov_base; uint64_t uid = BVAL(tf, SMB2_TF_SESSION_ID); -- 2.17.2 From bb9cb2fed17bc546451faed1316c456181b2d965 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 8 Nov 2018 15:42:46 +0100 Subject: [PATCH 07/11] s4:torture/smb2/session: invalidate credential cache Invalidate credential cache before connecting to the server, otherwise we will reuse the credentials from the credential cache populated by the preceeding tests. Also invalidate it at the end, otherwise subsequent tests might run into problems if the credentials expire while authenticating. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit 368e1860654e737aa2fa9516cdd3668fa644009a) --- source4/torture/smb2/session.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c index 7dc9ba19ee6..65cc53ba337 100644 --- a/source4/torture/smb2/session.c +++ b/source4/torture/smb2/session.c @@ -1073,6 +1073,8 @@ static bool test_session_expire1i(struct torture_context *tctx, torture_assert_int_equal(tctx, use_kerberos, CRED_MUST_USE_KERBEROS, "please use -k yes"); + cli_credentials_invalidate_ccache(credentials, CRED_SPECIFIED); + lpcfg_set_option(tctx->lp_ctx, "gensec_gssapi:requested_life_time=4"); lpcfg_smbcli_options(tctx->lp_ctx, &options); @@ -1167,6 +1169,8 @@ static bool test_session_expire1i(struct torture_context *tctx, ret = true; done: + cli_credentials_invalidate_ccache(credentials, CRED_SPECIFIED); + if (h1 != NULL) { smb2_util_close(tree, *h1); } @@ -1236,6 +1240,8 @@ static bool test_session_expire2i(struct torture_context *tctx, torture_assert_int_equal(tctx, use_kerberos, CRED_MUST_USE_KERBEROS, "please use -k yes"); + cli_credentials_invalidate_ccache(credentials, CRED_SPECIFIED); + lpcfg_set_option(tctx->lp_ctx, "gensec_gssapi:requested_life_time=4"); lpcfg_smbcli_options(tctx->lp_ctx, &options); @@ -1547,6 +1553,8 @@ static bool test_session_expire2i(struct torture_context *tctx, ret = true; done: + cli_credentials_invalidate_ccache(credentials, CRED_SPECIFIED); + if (h1 != NULL) { smb2_util_close(tree, *h1); } -- 2.17.2 From 9e8001c0055ef76e98bcdabe25a8990129528d59 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 9 Nov 2018 15:34:24 +0100 Subject: [PATCH 08/11] s4:torture/smb2/session: require a signed session setup reauth response All existing tests using this function require signing, so currently this passes. A subsequent commit adds a test where neither client nor server require signing and that's where this trap will explode. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit ffc424ee6bedc3c208acb4c0c83da836a12d6123) --- source4/torture/smb2/session.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c index 65cc53ba337..a088754ec00 100644 --- a/source4/torture/smb2/session.c +++ b/source4/torture/smb2/session.c @@ -1154,12 +1154,20 @@ static bool test_session_expire1i(struct torture_context *tctx, */ cli_credentials_invalidate_ccache(credentials, CRED_SPECIFIED); + if (!force_encryption) { + smb2cli_session_require_signed_response( + tree->session->smbXcli, true); + } + torture_comment(tctx, "reauth => OK\n"); status = smb2_session_setup_spnego(tree->session, credentials, 0 /* previous_session_id */); torture_assert_ntstatus_ok_goto(tctx, status, ret, done, "smb2_session_setup_spnego failed"); + + smb2cli_session_require_signed_response( + tree->session->smbXcli, false); } ZERO_STRUCT(qfinfo.access_information.out); -- 2.17.2 From f10b6c228390c1e0b07af2493ea77255b9b9ceb8 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 9 Nov 2018 12:19:16 +0100 Subject: [PATCH 09/11] s4:torture/smb2/session: add force_signing to test_session_expire1i Existing callers pass true, so no change in behaviour. The next commit adds an additional test that passes force_signing=false. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit 5fdea4095ac82536192c8d91c411b22e2683a5c1) --- source4/torture/smb2/session.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c index a088754ec00..0a0b54774e2 100644 --- a/source4/torture/smb2/session.c +++ b/source4/torture/smb2/session.c @@ -1047,6 +1047,7 @@ bool test_session_reauth6(struct torture_context *tctx, struct smb2_tree *tree) static bool test_session_expire1i(struct torture_context *tctx, + bool force_signing, bool force_encryption) { NTSTATUS status; @@ -1078,7 +1079,9 @@ static bool test_session_expire1i(struct torture_context *tctx, lpcfg_set_option(tctx->lp_ctx, "gensec_gssapi:requested_life_time=4"); lpcfg_smbcli_options(tctx->lp_ctx, &options); - options.signing = SMB_SIGNING_REQUIRED; + if (force_signing) { + options.signing = SMB_SIGNING_REQUIRED; + } status = smb2_connect(tctx, host, @@ -1191,12 +1194,14 @@ static bool test_session_expire1i(struct torture_context *tctx, static bool test_session_expire1s(struct torture_context *tctx) { return test_session_expire1i(tctx, + true, /* force_signing */ false); /* force_encryption */ } static bool test_session_expire1e(struct torture_context *tctx) { return test_session_expire1i(tctx, + true, /* force_signing */ true); /* force_encryption */ } -- 2.17.2 From 804e192e6c46a2a34ce9b3dab5bf37f03330d990 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Fri, 9 Nov 2018 12:39:41 +0100 Subject: [PATCH 10/11] s4:torture/smb2/session: session reauth response must be signed This test checks that a session setup reauth is signed even when neither client nor server require signing. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Andreas Schneider (cherry picked from commit 181f18c4bf70754a6f3132375d06250baab2871b) --- selftest/knownfail.d/samba3.smb2 | 1 + source4/torture/smb2/session.c | 8 ++++++++ 2 files changed, 9 insertions(+) create mode 100644 selftest/knownfail.d/samba3.smb2 diff --git a/selftest/knownfail.d/samba3.smb2 b/selftest/knownfail.d/samba3.smb2 new file mode 100644 index 00000000000..7e96e6798e7 --- /dev/null +++ b/selftest/knownfail.d/samba3.smb2 @@ -0,0 +1 @@ +^samba3.smb2.session krb5.expire1n\(ad_member\) diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c index 0a0b54774e2..57a5addcfcc 100644 --- a/source4/torture/smb2/session.c +++ b/source4/torture/smb2/session.c @@ -1191,6 +1191,13 @@ static bool test_session_expire1i(struct torture_context *tctx, return ret; } +static bool test_session_expire1n(struct torture_context *tctx) +{ + return test_session_expire1i(tctx, + false, /* force_signing */ + false); /* force_encryption */ +} + static bool test_session_expire1s(struct torture_context *tctx) { return test_session_expire1i(tctx, @@ -1742,6 +1749,7 @@ struct torture_suite *torture_smb2_session_init(TALLOC_CTX *ctx) 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, "expire1n", test_session_expire1n); torture_suite_add_simple_test(suite, "expire1s", test_session_expire1s); torture_suite_add_simple_test(suite, "expire1e", test_session_expire1e); torture_suite_add_simple_test(suite, "expire2s", test_session_expire2s); -- 2.17.2 From f95c887de0d4d01e4289847a3207f1aeebb511cd Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Thu, 8 Nov 2018 17:31:41 +0100 Subject: [PATCH 11/11] s3:smb2_sesssetup: check session_info security level before it gets talloc_move'd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We talloc_move() session_info to session->global->auth_session_info which sets session_info to NULL. This means security_session_user_level(NULL, NULL) will always return SECURITY_ANONYMOUS so we never sign the session setup response. Bug: https://bugzilla.samba.org/show_bug.cgi?id=13661 Signed-off-by: Ralph Boehme Reviewed-by: Stefan Metzmacher Reviewed-by: Andreas Schneider Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Tue Nov 13 14:22:46 CET 2018 on sn-devel-144 (cherry picked from commit bb93e691ca9b1922bf552363a1e7d70792749d67) --- selftest/knownfail.d/samba3.smb2 | 1 - source3/smbd/smb2_sesssetup.c | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) delete mode 100644 selftest/knownfail.d/samba3.smb2 diff --git a/selftest/knownfail.d/samba3.smb2 b/selftest/knownfail.d/samba3.smb2 deleted file mode 100644 index 7e96e6798e7..00000000000 --- a/selftest/knownfail.d/samba3.smb2 +++ /dev/null @@ -1 +0,0 @@ -^samba3.smb2.session krb5.expire1n\(ad_member\) diff --git a/source3/smbd/smb2_sesssetup.c b/source3/smbd/smb2_sesssetup.c index fe5835b83f3..5420d4f09bb 100644 --- a/source3/smbd/smb2_sesssetup.c +++ b/source3/smbd/smb2_sesssetup.c @@ -525,6 +525,10 @@ static NTSTATUS smbd_smb2_reauth_generic_return(struct smbXsrv_session *session, reload_services(smb2req->sconn, conn_snum_used, true); + if (security_session_user_level(session_info, NULL) >= SECURITY_USER) { + smb2req->do_signing = true; + } + session->status = NT_STATUS_OK; TALLOC_FREE(session->global->auth_session_info); session->global->auth_session_info = talloc_move(session->global, @@ -551,10 +555,6 @@ static NTSTATUS smbd_smb2_reauth_generic_return(struct smbXsrv_session *session, conn_clear_vuid_caches(xconn->client->sconn, session->compat->vuid); - if (security_session_user_level(session_info, NULL) >= SECURITY_USER) { - smb2req->do_signing = true; - } - *out_session_id = session->global->session_wire_id; return NT_STATUS_OK; -- 2.17.2