Attachment 17948 Details for Bug 15397 – Patch for 4.17

[patch] Patch for 4.17

CVE-2023-3347-signing-4.17-01.patch (text/plain), 15.10 KB, created by Ralph Böhme on 2023-06-23 14:23:49 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2023-06-23 14:23:49 UTC

Size: 15.10 KB

patch

obsolete

>From 2c987aa203f12390c51810e4fbca6a176180a8b1 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 20 Jun 2023 12:46:31 +0200
>Subject: [PATCH 1/5] CVE-2023-3347: CI: add a test for server-side mandatory
> signing
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15397
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> .../samba3.smb2.session-require-signing       |  1 +
> selftest/target/Samba3.pm                     |  1 +
> source3/selftest/tests.py                     |  2 +
> source4/torture/smb2/session.c                | 64 +++++++++++++++++++
> source4/torture/smb2/smb2.c                   |  1 +
> 5 files changed, 69 insertions(+)
> create mode 100644 selftest/knownfail.d/samba3.smb2.session-require-signing
>
>diff --git a/selftest/knownfail.d/samba3.smb2.session-require-signing b/selftest/knownfail.d/samba3.smb2.session-require-signing
>new file mode 100644
>index 000000000000..53b7a7022a83
>--- /dev/null
>+++ b/selftest/knownfail.d/samba3.smb2.session-require-signing
>@@ -0,0 +1 @@
>+^samba3.smb2.session-require-signing.bug15397
>diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
>index 9c590547c943..3336c5b8e97c 100755
>--- a/selftest/target/Samba3.pm
>+++ b/selftest/target/Samba3.pm
>@@ -1294,6 +1294,7 @@ sub setup_ad_member_idmap_rid
> 	# values required for tests to succeed
> 	create krb5 conf = no
>         map to guest = bad user
>+	server signing = required
> ";
> 
> 	my $ret = $self->provision(
>diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
>index 04349c1f1f75..887bf6d52933 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -938,6 +938,8 @@ tests = base + raw + smb2 + rpc + unix + local + rap + nbt + idmap + vfs
>         # Certain tests fail when run against ad_member with MIT kerberos because the private krb5.conf overrides the provisioned lib/krb5.conf,
>         # ad_member_idmap_rid sets "create krb5.conf = no"
>         plansmbtorture4testsuite(t, "ad_member_idmap_rid", '//$SERVER/tmp -k yes -U$DC_USERNAME@$REALM%$DC_PASSWORD', 'krb5')
>+    elif t == "smb2.session-require-signing":
>+        plansmbtorture4testsuite(t, "ad_member_idmap_rid", '//$SERVER_IP/tmp -U$DC_USERNAME@$REALM%$DC_PASSWORD')
>     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 ')
>diff --git a/source4/torture/smb2/session.c b/source4/torture/smb2/session.c
>index 92f9e638ff47..e417008cad7a 100644
>--- a/source4/torture/smb2/session.c
>+++ b/source4/torture/smb2/session.c
>@@ -5498,3 +5498,67 @@ struct torture_suite *torture_smb2_session_init(TALLOC_CTX *ctx)
> 
> 	return suite;
> }
>+
>+static bool test_session_require_sign_bug15397(struct torture_context *tctx,
>+					       struct smb2_tree *_tree)
>+{
>+	const char *host = torture_setting_string(tctx, "host", NULL);
>+	const char *share = torture_setting_string(tctx, "share", NULL);
>+	struct cli_credentials *_creds = samba_cmdline_get_creds();
>+	struct cli_credentials *creds = NULL;
>+	struct smbcli_options options;
>+	struct smb2_tree *tree = NULL;
>+	uint8_t security_mode;
>+	NTSTATUS status;
>+	bool ok = true;
>+
>+	/*
>+	 * Setup our own connection so we can control the signing flags
>+	 */
>+
>+	creds = cli_credentials_shallow_copy(tctx, _creds);
>+	torture_assert(tctx, creds != NULL, "cli_credentials_shallow_copy");
>+
>+	options = _tree->session->transport->options;
>+	options.client_guid = GUID_random();
>+	options.signing = SMB_SIGNING_IF_REQUIRED;
>+
>+	status = smb2_connect(tctx,
>+			      host,
>+			      lpcfg_smb_ports(tctx->lp_ctx),
>+			      share,
>+			      lpcfg_resolve_context(tctx->lp_ctx),
>+			      creds,
>+			      &tree,
>+			      tctx->ev,
>+			      &options,
>+			      lpcfg_socket_options(tctx->lp_ctx),
>+			      lpcfg_gensec_settings(tctx, tctx->lp_ctx));
>+	torture_assert_ntstatus_ok_goto(tctx, status, ok, done,
>+					"smb2_connect failed");
>+
>+	security_mode = smb2cli_session_security_mode(tree->session->smbXcli);
>+
>+	torture_assert_int_equal_goto(
>+		tctx,
>+		security_mode,
>+		SMB2_NEGOTIATE_SIGNING_REQUIRED | SMB2_NEGOTIATE_SIGNING_ENABLED,
>+		ok,
>+		done,
>+		"Signing not required");
>+
>+done:
>+	return ok;
>+}
>+
>+struct torture_suite *torture_smb2_session_req_sign_init(TALLOC_CTX *ctx)
>+{
>+	struct torture_suite *suite =
>+	    torture_suite_create(ctx, "session-require-signing");
>+
>+	torture_suite_add_1smb2_test(suite, "bug15397",
>+				     test_session_require_sign_bug15397);
>+
>+	suite->description = talloc_strdup(suite, "SMB2-SESSION require signing tests");
>+	return suite;
>+}
>diff --git a/source4/torture/smb2/smb2.c b/source4/torture/smb2/smb2.c
>index c717db50b70c..8621f09d820c 100644
>--- a/source4/torture/smb2/smb2.c
>+++ b/source4/torture/smb2/smb2.c
>@@ -189,6 +189,7 @@ NTSTATUS torture_smb2_init(TALLOC_CTX *ctx)
> 	torture_suite_add_suite(suite, torture_smb2_sharemode_init(suite));
> 	torture_suite_add_1smb2_test(suite, "hold-oplock", test_smb2_hold_oplock);
> 	torture_suite_add_suite(suite, torture_smb2_session_init(suite));
>+	torture_suite_add_suite(suite, torture_smb2_session_req_sign_init(suite));
> 	torture_suite_add_suite(suite, torture_smb2_replay_init(suite));
> 	torture_suite_add_simple_test(suite, "dosmode", torture_smb2_dosmode);
> 	torture_suite_add_simple_test(suite, "async_dosmode", torture_smb2_async_dosmode);
>-- 
>2.40.0
>
>
>From dfde1691d336fe658857e576cec0debd3a2450b9 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Wed, 21 Jun 2023 15:06:12 +0200
>Subject: [PATCH 2/5] CVE-2023-3347: smbd: pass lp_ctx to
> smb[1|2]_srv_init_signing()
>
>No change in behaviour.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15397
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> source3/smbd/proto.h        |  3 ++-
> source3/smbd/smb1_signing.c | 10 ++--------
> source3/smbd/smb1_signing.h |  3 ++-
> source3/smbd/smb2_signing.c | 25 +++++++++++++++----------
> 4 files changed, 21 insertions(+), 20 deletions(-)
>
>diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
>index c4a330145151..67cc5e8a380a 100644
>--- a/source3/smbd/proto.h
>+++ b/source3/smbd/proto.h
>@@ -52,7 +52,8 @@ struct dcesrv_context;
> 
> /* The following definitions come from smbd/smb2_signing.c */
> 
>-bool smb2_srv_init_signing(struct smbXsrv_connection *conn);
>+bool smb2_srv_init_signing(struct loadparm_context *lp_ctx,
>+			   struct smbXsrv_connection *conn);
> bool srv_init_signing(struct smbXsrv_connection *conn);
> 
> /* The following definitions come from smbd/aio.c  */
>diff --git a/source3/smbd/smb1_signing.c b/source3/smbd/smb1_signing.c
>index 6bcb0629c4f0..aa3027d53182 100644
>--- a/source3/smbd/smb1_signing.c
>+++ b/source3/smbd/smb1_signing.c
>@@ -170,18 +170,13 @@ static void smbd_shm_signing_free(TALLOC_CTX *mem_ctx, void *ptr)
>  Called by server negprot when signing has been negotiated.
> ************************************************************/
> 
>-bool smb1_srv_init_signing(struct smbXsrv_connection *conn)
>+bool smb1_srv_init_signing(struct loadparm_context *lp_ctx,
>+			   struct smbXsrv_connection *conn)
> {
> 	bool allowed = true;
> 	bool desired;
> 	bool mandatory = false;
> 
>-	struct loadparm_context *lp_ctx = loadparm_init_s3(conn, loadparm_s3_helpers());
>-	if (lp_ctx == NULL) {
>-		DEBUG(10, ("loadparm_init_s3 failed\n"));
>-		return false;
>-	}
>-
> 	/*
> 	 * if the client and server allow signing,
> 	 * we desire to use it.
>@@ -195,7 +190,6 @@ bool smb1_srv_init_signing(struct smbXsrv_connection *conn)
> 	 */
> 
> 	desired = lpcfg_server_signing_allowed(lp_ctx, &mandatory);
>-	talloc_unlink(conn, lp_ctx);
> 
> 	if (lp_async_smb_echo_handler()) {
> 		struct smbd_shm_signing *s;
>diff --git a/source3/smbd/smb1_signing.h b/source3/smbd/smb1_signing.h
>index 56c59c5bbc21..26f60420dfa8 100644
>--- a/source3/smbd/smb1_signing.h
>+++ b/source3/smbd/smb1_signing.h
>@@ -33,4 +33,5 @@ bool smb1_srv_is_signing_negotiated(struct smbXsrv_connection *conn);
> void smb1_srv_set_signing(struct smbXsrv_connection *conn,
> 		     const DATA_BLOB user_session_key,
> 		     const DATA_BLOB response);
>-bool smb1_srv_init_signing(struct smbXsrv_connection *conn);
>+bool smb1_srv_init_signing(struct loadparm_context *lp_ctx,
>+			   struct smbXsrv_connection *conn);
>diff --git a/source3/smbd/smb2_signing.c b/source3/smbd/smb2_signing.c
>index 4691ef4d6130..c1f876f9cd74 100644
>--- a/source3/smbd/smb2_signing.c
>+++ b/source3/smbd/smb2_signing.c
>@@ -26,32 +26,37 @@
> #include "lib/param/param.h"
> #include "smb2_signing.h"
> 
>-bool smb2_srv_init_signing(struct smbXsrv_connection *conn)
>+bool smb2_srv_init_signing(struct loadparm_context *lp_ctx,
>+			   struct smbXsrv_connection *conn)
> {
>-	struct loadparm_context *lp_ctx = loadparm_init_s3(conn, loadparm_s3_helpers());
>-	if (lp_ctx == NULL) {
>-		DBG_DEBUG("loadparm_init_s3 failed\n");
>-		return false;
>-	}
>-
> 	/*
> 	 * For SMB2 all we need to know is if signing is mandatory.
> 	 * It is always allowed and desired, whatever the smb.conf says.
> 	 */
> 	(void)lpcfg_server_signing_allowed(lp_ctx, &conn->smb2.signing_mandatory);
>-	talloc_unlink(conn, lp_ctx);
> 	return true;
> }
> 
> bool srv_init_signing(struct smbXsrv_connection *conn)
> {
>+	struct loadparm_context *lp_ctx = NULL;
>+	bool ok;
>+
>+	lp_ctx = loadparm_init_s3(conn, loadparm_s3_helpers());
>+	if (lp_ctx == NULL) {
>+		DBG_DEBUG("loadparm_init_s3 failed\n");
>+		return false;
>+	}
>+
> #if defined(WITH_SMB1SERVER)
> 	if (conn->protocol >= PROTOCOL_SMB2_02) {
> #endif
>-		return smb2_srv_init_signing(conn);
>+		ok = smb2_srv_init_signing(lp_ctx, conn);
> #if defined(WITH_SMB1SERVER)
> 	} else {
>-		return smb1_srv_init_signing(conn);
>+		ok = smb1_srv_init_signing(lp_ctx, conn);
> 	}
> #endif
>+	talloc_unlink(conn, lp_ctx);
>+	return ok;
> }
>-- 
>2.40.0
>
>
>From fd5f7d15869b67c41f681539348f7917a8c2fc0c Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Wed, 21 Jun 2023 15:10:58 +0200
>Subject: [PATCH 3/5] CVE-2023-3347: smbd: inline smb2_srv_init_signing() code
> in srv_init_signing()
>
>It's now a one-line function, imho the overall code is simpler if that code is
>just inlined.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15397
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> source3/smbd/proto.h        |  2 --
> source3/smbd/smb2_signing.c | 19 ++++++-------------
> 2 files changed, 6 insertions(+), 15 deletions(-)
>
>diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
>index 67cc5e8a380a..8075f4e567e3 100644
>--- a/source3/smbd/proto.h
>+++ b/source3/smbd/proto.h
>@@ -52,8 +52,6 @@ struct dcesrv_context;
> 
> /* The following definitions come from smbd/smb2_signing.c */
> 
>-bool smb2_srv_init_signing(struct loadparm_context *lp_ctx,
>-			   struct smbXsrv_connection *conn);
> bool srv_init_signing(struct smbXsrv_connection *conn);
> 
> /* The following definitions come from smbd/aio.c  */
>diff --git a/source3/smbd/smb2_signing.c b/source3/smbd/smb2_signing.c
>index c1f876f9cd74..ef4a54d57107 100644
>--- a/source3/smbd/smb2_signing.c
>+++ b/source3/smbd/smb2_signing.c
>@@ -26,21 +26,10 @@
> #include "lib/param/param.h"
> #include "smb2_signing.h"
> 
>-bool smb2_srv_init_signing(struct loadparm_context *lp_ctx,
>-			   struct smbXsrv_connection *conn)
>-{
>-	/*
>-	 * For SMB2 all we need to know is if signing is mandatory.
>-	 * It is always allowed and desired, whatever the smb.conf says.
>-	 */
>-	(void)lpcfg_server_signing_allowed(lp_ctx, &conn->smb2.signing_mandatory);
>-	return true;
>-}
>-
> bool srv_init_signing(struct smbXsrv_connection *conn)
> {
> 	struct loadparm_context *lp_ctx = NULL;
>-	bool ok;
>+	bool ok = true;
> 
> 	lp_ctx = loadparm_init_s3(conn, loadparm_s3_helpers());
> 	if (lp_ctx == NULL) {
>@@ -51,7 +40,11 @@ bool srv_init_signing(struct smbXsrv_connection *conn)
> #if defined(WITH_SMB1SERVER)
> 	if (conn->protocol >= PROTOCOL_SMB2_02) {
> #endif
>-		ok = smb2_srv_init_signing(lp_ctx, conn);
>+		/*
>+		 * For SMB2 all we need to know is if signing is mandatory.
>+		 * It is always allowed and desired, whatever the smb.conf says.
>+		 */
>+		(void)lpcfg_server_signing_allowed(lp_ctx, &conn->smb2.signing_mandatory);
> #if defined(WITH_SMB1SERVER)
> 	} else {
> 		ok = smb1_srv_init_signing(lp_ctx, conn);
>-- 
>2.40.0
>
>
>From afe8b8d4f55ac330683645dad149a1691c15e2bd Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 20 Jun 2023 18:13:23 +0200
>Subject: [PATCH 4/5] CVE-2023-3347: smbd: remove comment in
> smbd_smb2_request_process_negprot()
>
>This is just going to bitrot. Anyone who's interested can just grep for
>"signing_mandatory" and look up what it does.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15397
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> source3/smbd/smb2_negprot.c | 6 ------
> 1 file changed, 6 deletions(-)
>
>diff --git a/source3/smbd/smb2_negprot.c b/source3/smbd/smb2_negprot.c
>index baddbecaade9..685a1460cef4 100644
>--- a/source3/smbd/smb2_negprot.c
>+++ b/source3/smbd/smb2_negprot.c
>@@ -361,12 +361,6 @@ NTSTATUS smbd_smb2_request_process_negprot(struct smbd_smb2_request *req)
> 	}
> 
> 	security_mode = SMB2_NEGOTIATE_SIGNING_ENABLED;
>-	/*
>-	 * We use xconn->smb2.signing_mandatory set up via
>-	 * srv_init_signing() -> smb2_srv_init_signing().
>-	 * This calls lpcfg_server_signing_allowed() to get the correct
>-	 * defaults, e.g. signing_required for an ad_dc.
>-	 */
> 	if (xconn->smb2.signing_mandatory) {
> 		security_mode |= SMB2_NEGOTIATE_SIGNING_REQUIRED;
> 	}
>-- 
>2.40.0
>
>
>From d01483dba47134ee1cc959d3e52ef2af1b1221d8 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 20 Jun 2023 15:33:02 +0200
>Subject: [PATCH 5/5] CVE-2023-3347: smbd: fix "server signing = mandatory"
>
>This was broken by commit 1f3f6e20dc086a36de52bffd0bc36e15fb19e1c6 because when
>calling srv_init_signing() very early after accepting the connection in
>smbd_add_connection(), conn->protocol is still PROTOCOL_NONE.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15397
>
>Signed-off-by: Ralph Boehme <slow@samba.org>
>---
> .../samba3.smb2.session-require-signing       |  1 -
> source3/smbd/smb2_signing.c                   | 19 ++++++++-----------
> 2 files changed, 8 insertions(+), 12 deletions(-)
> delete mode 100644 selftest/knownfail.d/samba3.smb2.session-require-signing
>
>diff --git a/selftest/knownfail.d/samba3.smb2.session-require-signing b/selftest/knownfail.d/samba3.smb2.session-require-signing
>deleted file mode 100644
>index 53b7a7022a83..000000000000
>--- a/selftest/knownfail.d/samba3.smb2.session-require-signing
>+++ /dev/null
>@@ -1 +0,0 @@
>-^samba3.smb2.session-require-signing.bug15397
>diff --git a/source3/smbd/smb2_signing.c b/source3/smbd/smb2_signing.c
>index ef4a54d57107..73d07380dfa1 100644
>--- a/source3/smbd/smb2_signing.c
>+++ b/source3/smbd/smb2_signing.c
>@@ -37,19 +37,16 @@ bool srv_init_signing(struct smbXsrv_connection *conn)
> 		return false;
> 	}
> 
>+	/*
>+	 * For SMB2 all we need to know is if signing is mandatory.
>+	 * It is always allowed and desired, whatever the smb.conf says.
>+	 */
>+	(void)lpcfg_server_signing_allowed(lp_ctx, &conn->smb2.signing_mandatory);
>+
> #if defined(WITH_SMB1SERVER)
>-	if (conn->protocol >= PROTOCOL_SMB2_02) {
>-#endif
>-		/*
>-		 * For SMB2 all we need to know is if signing is mandatory.
>-		 * It is always allowed and desired, whatever the smb.conf says.
>-		 */
>-		(void)lpcfg_server_signing_allowed(lp_ctx, &conn->smb2.signing_mandatory);
>-#if defined(WITH_SMB1SERVER)
>-	} else {
>-		ok = smb1_srv_init_signing(lp_ctx, conn);
>-	}
>+	ok = smb1_srv_init_signing(lp_ctx, conn);
> #endif
>+
> 	talloc_unlink(conn, lp_ctx);
> 	return ok;
> }
>-- 
>2.40.0
>

Flags:

metze: review+
jra: review+
slow: ci-passed+

Actions: View

Attachments on bug 15397: 17925 | 17929 | 17930 | 17931 | 17934 | 17938 | 17947 | 17948 | 17964