Attachment 14629 Details for Bug 13661 – WIP patch for master with test

[patch] WIP patch for master with test

bug13661-wip.patch (text/plain), 11.93 KB, created by Ralph Böhme on 2018-11-07 22:26:33 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2018-11-07 22:26:33 UTC

Size: 11.93 KB

patch

obsolete

>From 5dbaf823cd5630c01e6ad21007696a7b8fc40315 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>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 <slow@samba.org>
>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 <slow@samba.org>
>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 <slow@samba.org>
>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 <slow@samba.org>
>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 <slow@samba.org>
>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 <slow@samba.org>
>Reviewed-by: Stefan Metzmacher <metze@samba.org>
>---
> 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
>

Actions: View

Attachments on bug 13661: 14603 | 14623 | 14629 | 14659