From 5dbaf823cd5630c01e6ad21007696a7b8fc40315 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 7 Nov 2018 14:00:25 +0100 Subject: [PATCH 1/6] libcli: don't overwrite status code --- libcli/smb/smbXcli_base.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index 9edb6292777..93b4c18e407 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -3889,15 +3889,17 @@ static NTSTATUS smb2cli_conn_dispatch_incoming(struct smbXcli_conn *conn, } if (signing_key) { - status = smb2_signing_check_pdu(*signing_key, - state->conn->protocol, - &cur[1], 3); - if (!NT_STATUS_IS_OK(status)) { + NTSTATUS signing_status; + + signing_status = smb2_signing_check_pdu(*signing_key, + state->conn->protocol, + &cur[1], 3); + if (!NT_STATUS_IS_OK(signing_status)) { /* * If the signing check fails, we disconnect * the connection. */ - return status; + return signing_status; } } -- 2.17.2 From 36d8c70e8092a8c51cc3444f4f3041355afe9c76 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 7 Nov 2018 16:42:22 +0100 Subject: [PATCH 2/6] libcli/smb: add smbXcli_session_signing_key() --- libcli/smb/smbXcli_base.c | 22 ++++++++++++++++++++++ libcli/smb/smbXcli_base.h | 2 ++ 2 files changed, 24 insertions(+) diff --git a/libcli/smb/smbXcli_base.c b/libcli/smb/smbXcli_base.c index 93b4c18e407..2afac33b805 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -5569,6 +5569,28 @@ NTSTATUS smbXcli_session_application_key(struct smbXcli_session *session, return NT_STATUS_OK; } +NTSTATUS smbXcli_session_signing_key(struct smbXcli_session *session, + DATA_BLOB *_key) +{ + DATA_BLOB key = data_blob_null; + + if (session->conn == NULL) { + return NT_STATUS_NO_USER_SESSION_KEY; + } + + if (session->conn->protocol >= PROTOCOL_SMB2_02) { + key = session->smb2_channel.signing_key; + } + + if (key.length == 0) { + return NT_STATUS_NO_USER_SESSION_KEY; + } + + *_key = key; + + return NT_STATUS_OK; +} + void smbXcli_session_set_disconnect_expired(struct smbXcli_session *session) { session->disconnect_expired = true; diff --git a/libcli/smb/smbXcli_base.h b/libcli/smb/smbXcli_base.h index 536c7ab60f4..5667a5d2607 100644 --- a/libcli/smb/smbXcli_base.h +++ b/libcli/smb/smbXcli_base.h @@ -471,6 +471,8 @@ bool smbXcli_session_is_authenticated(struct smbXcli_session *session); NTSTATUS smbXcli_session_application_key(struct smbXcli_session *session, TALLOC_CTX *mem_ctx, DATA_BLOB *key); +NTSTATUS smbXcli_session_signing_key(struct smbXcli_session *session, + DATA_BLOB *_key); void smbXcli_session_set_disconnect_expired(struct smbXcli_session *session); uint16_t smb1cli_session_current_id(struct smbXcli_session* session); void smb1cli_session_set_id(struct smbXcli_session* session, -- 2.17.2 From 46736f35ef421ff65fd591b880950445f8fe6fc3 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 7 Nov 2018 22:20:59 +0100 Subject: [PATCH 3/6] libcli/smb: add smb2cli_session_is_encryption_on() --- 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 2afac33b805..c693f4c678c 100644 --- a/libcli/smb/smbXcli_base.c +++ b/libcli/smb/smbXcli_base.c @@ -6135,6 +6135,11 @@ NTSTATUS smb2cli_session_set_channel_key(struct smbXcli_session *session, return NT_STATUS_OK; } +bool smb2cli_session_is_encryption_on(struct smbXcli_session *session) +{ + return session->smb2->should_encrypt; +} + NTSTATUS smb2cli_session_encryption_on(struct smbXcli_session *session) { if (!session->smb2->should_sign) { diff --git a/libcli/smb/smbXcli_base.h b/libcli/smb/smbXcli_base.h index 5667a5d2607..131a5cd5109 100644 --- a/libcli/smb/smbXcli_base.h +++ b/libcli/smb/smbXcli_base.h @@ -507,6 +507,7 @@ NTSTATUS smb2cli_session_set_channel_key(struct smbXcli_session *session, const DATA_BLOB channel_key, const struct iovec *recv_iov); NTSTATUS smb2cli_session_encryption_on(struct smbXcli_session *session); +bool smb2cli_session_is_encryption_on(struct smbXcli_session *session); struct smbXcli_tcon *smbXcli_tcon_create(TALLOC_CTX *mem_ctx); struct smbXcli_tcon *smbXcli_tcon_copy(TALLOC_CTX *mem_ctx, -- 2.17.2 From 3d46d297dda3eae3dcb3bd1dd39013c2daabfebf Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 7 Nov 2018 16:42:36 +0100 Subject: [PATCH 4/6] s4:libcli/smb2: check session-setup reauth response signature --- selftest/knownfail.d/samba3.smb2 | 9 ++++++ source4/libcli/smb2/session.c | 47 ++++++++++++++++++++++++++++---- 2 files changed, 51 insertions(+), 5 deletions(-) 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..251f8f45cb9 --- /dev/null +++ b/selftest/knownfail.d/samba3.smb2 @@ -0,0 +1,9 @@ +^samba3.smb2.session plain.reauth1\(nt4_dc\) +^samba3.smb2.session plain.reauth2\(nt4_dc\) +^samba3.smb2.session plain.reauth3\(nt4_dc\) +^samba3.smb2.session plain.reauth4\(nt4_dc\) +^samba3.smb2.session enc.reauth1\(nt4_dc\) +^samba3.smb2.session enc.reauth2\(nt4_dc\) +^samba3.smb2.session enc.reauth3\(nt4_dc\) +^samba3.smb2.session enc.reauth4\(nt4_dc\) +^samba3.smb2.session krb5.expire1e\(ad_dc\) diff --git a/source4/libcli/smb2/session.c b/source4/libcli/smb2/session.c index e94512d3d33..04a13031e34 100644 --- a/source4/libcli/smb2/session.c +++ b/source4/libcli/smb2/session.c @@ -380,11 +380,6 @@ static void smb2_session_setup_spnego_both_ready(struct tevent_req *req) return; } - if (state->reauth) { - tevent_req_done(req); - return; - } - if (cli_credentials_is_anonymous(state->credentials)) { /* * Windows server does not set the @@ -399,6 +394,48 @@ static void smb2_session_setup_spnego_both_ready(struct tevent_req *req) return; } + if (state->reauth) { + DATA_BLOB key; + enum protocol_types protocol; + + if (smb2cli_session_is_encryption_on(session->smbXcli)) { + tevent_req_done(req); + return; + } + + protocol = smbXcli_conn_protocol(session->transport->conn); + if (protocol < PROTOCOL_SMB3_10) { + /* + * In theory we should always check this, Windows and + * Samba always sign session setup auth responses, but + * alas not all SMB servers might do this, so this check + * might break us with those. See also the comment in + * smb2cli_session_set_session_key(). + */ + tevent_req_done(req); + return; + } + + status = smbXcli_session_signing_key(session->smbXcli, &key); + if (NT_STATUS_EQUAL(status, NT_STATUS_NO_USER_SESSION_KEY)) { + tevent_req_done(req); + return; + } + if (tevent_req_nterror(req, status)) { + return; + } + + status = smb2_signing_check_pdu(key, + protocol, + state->recv_iov, + 3); + if (tevent_req_nterror(req, status)) { + return; + } + tevent_req_done(req); + return; + } + status = gensec_session_key(session->gensec, state, &session_key); if (tevent_req_nterror(req, status)) { -- 2.17.2 From 93d78057dcf0c4f3d8789eb841166519852eb8b0 Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Wed, 7 Nov 2018 15:28:58 +0100 Subject: [PATCH 5/6] s4:torture/smb2/session: add another session expiration test This tests session expiration without forcing signing. --- source4/torture/smb2/session.c | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c index 7dc9ba19ee6..9adbc596cef 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; @@ -1076,7 +1077,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, @@ -1176,15 +1179,24 @@ 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, + 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 */ } @@ -1721,6 +1733,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 265ffa6ed118b80f72643c5840a2433c10e8bb4a Mon Sep 17 00:00:00 2001 From: Ralph Boehme Date: Mon, 22 Oct 2018 18:21:58 +0200 Subject: [PATCH 6/6] s3:smb2_sesssetup: check session_info security level before it gets talloc_move'd 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 --- selftest/knownfail.d/samba3.smb2 | 9 --------- source3/smbd/smb2_sesssetup.c | 8 ++++---- 2 files changed, 4 insertions(+), 13 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 251f8f45cb9..00000000000 --- a/selftest/knownfail.d/samba3.smb2 +++ /dev/null @@ -1,9 +0,0 @@ -^samba3.smb2.session plain.reauth1\(nt4_dc\) -^samba3.smb2.session plain.reauth2\(nt4_dc\) -^samba3.smb2.session plain.reauth3\(nt4_dc\) -^samba3.smb2.session plain.reauth4\(nt4_dc\) -^samba3.smb2.session enc.reauth1\(nt4_dc\) -^samba3.smb2.session enc.reauth2\(nt4_dc\) -^samba3.smb2.session enc.reauth3\(nt4_dc\) -^samba3.smb2.session enc.reauth4\(nt4_dc\) -^samba3.smb2.session krb5.expire1e\(ad_dc\) 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