Attachment 17931 Details for Bug 15397 – Possible WIP patch for master with test

[patch] Possible WIP patch for master with test

wip-bug15397-master.patch (text/plain), 10.56 KB, created by Ralph Böhme on 2023-06-20 17:01:32 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Ralph Böhme

Created: 2023-06-20 17:01:32 UTC

Size: 10.56 KB

patch

obsolete

>From e8ce23909ee64b7ac1b2d90b8d3f2b71c241381c 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/4] CVE-XXX: CI: add a test for server-side mandatory signing
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15397
>---
> .../samba3.smb2.session-require-signing       |  1 +
> selftest/target/Samba3.pm                     |  1 +
> source3/selftest/tests.py                     |  2 ++
> source4/torture/smb2/session.c                | 30 +++++++++++++++++++
> source4/torture/smb2/smb2.c                   |  1 +
> 5 files changed, 35 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 0bb074cf11e4..489dc835b21e 100755
>--- a/selftest/target/Samba3.pm
>+++ b/selftest/target/Samba3.pm
>@@ -1295,6 +1295,7 @@ sub setup_ad_member_idmap_rid
> 	create krb5 conf = no
>         map to guest = bad user
> 	winbind expand groups = 10
>+	server signing = required
> ";
> 
> 	my $ret = $self->provision(
>diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
>index 2bc4d3720956..a6c7e92a679f 100755
>--- a/source3/selftest/tests.py
>+++ b/source3/selftest/tests.py
>@@ -1097,6 +1097,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 51df51542d47..1aba9bb7fb65 100644
>--- a/source4/torture/smb2/session.c
>+++ b/source4/torture/smb2/session.c
>@@ -5604,3 +5604,33 @@ 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)
>+{
>+	uint8_t security_mode = smb2cli_session_security_mode(tree->session->smbXcli);
>+	bool ok = true;
>+
>+	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 c595b108ce83..5b6477e47bc3 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 82f6aff02b3a9100ddd39b5a32dffa305d1fc9e2 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 20 Jun 2023 15:33:02 +0200
>Subject: [PATCH 2/4] CVE-XXX: 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
>---
> .../samba3.smb2.session-require-signing        |  1 -
> source3/smbd/proto.h                           |  1 -
> source3/smbd/smb2_signing.c                    | 18 +++++-------------
> 3 files changed, 5 insertions(+), 15 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/proto.h b/source3/smbd/proto.h
>index 6b48d7c918db..cf61c44717f1 100644
>--- a/source3/smbd/proto.h
>+++ b/source3/smbd/proto.h
>@@ -52,7 +52,6 @@ struct dcesrv_context;
> 
> /* The following definitions come from smbd/smb2_signing.c */
> 
>-bool smb2_srv_init_signing(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 4691ef4d6130..d2654a32ce14 100644
>--- a/source3/smbd/smb2_signing.c
>+++ b/source3/smbd/smb2_signing.c
>@@ -26,7 +26,7 @@
> #include "lib/param/param.h"
> #include "smb2_signing.h"
> 
>-bool smb2_srv_init_signing(struct smbXsrv_connection *conn)
>+bool srv_init_signing(struct smbXsrv_connection *conn)
> {
> 	struct loadparm_context *lp_ctx = loadparm_init_s3(conn, loadparm_s3_helpers());
> 	if (lp_ctx == NULL) {
>@@ -39,19 +39,11 @@ bool smb2_srv_init_signing(struct smbXsrv_connection *conn)
> 	 * 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)
>-{
> #if defined(WITH_SMB1SERVER)
>-	if (conn->protocol >= PROTOCOL_SMB2_02) {
>-#endif
>-		return smb2_srv_init_signing(conn);
>-#if defined(WITH_SMB1SERVER)
>-	} else {
>-		return smb1_srv_init_signing(conn);
>-	}
>+	smb1_srv_init_signing(conn);
> #endif
>+
>+	talloc_unlink(conn, lp_ctx);
>+	return true;
> }
>-- 
>2.40.0
>
>
>From aec91551c3f7e0e9c01fc9aa8193c2c4a2cbded5 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 20 Jun 2023 18:13:23 +0200
>Subject: [PATCH 3/4] CVE-XXX: smbd: remove comment in
> smbd_smb2_request_process_negprot()
>
>This is just goint to bitrot. Anyone who's interested can just grep for
>"signing_mandatory" and look what the simple code that sets it does.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15397
>---
> 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 5ac004bb2760..d89b5e7d23a0 100644
>--- a/source3/smbd/smb2_negprot.c
>+++ b/source3/smbd/smb2_negprot.c
>@@ -368,12 +368,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 4e1875eec7ffa870c84eabf7e8ce0340d3ed4ba8 Mon Sep 17 00:00:00 2001
>From: Ralph Boehme <slow@samba.org>
>Date: Tue, 20 Jun 2023 18:11:23 +0200
>Subject: [PATCH 4/4] CVE-XXX: smbd: add lp_ctx to smb1_srv_init_signing()
>
>Avoids allocating twice. No change in behaviour.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15397
>---
> source3/smbd/smb1_signing.c | 10 ++--------
> source3/smbd/smb1_signing.h |  3 ++-
> source3/smbd/smb2_signing.c |  2 +-
> 3 files changed, 5 insertions(+), 10 deletions(-)
>
>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 d2654a32ce14..e3515790b2e1 100644
>--- a/source3/smbd/smb2_signing.c
>+++ b/source3/smbd/smb2_signing.c
>@@ -41,7 +41,7 @@ bool srv_init_signing(struct smbXsrv_connection *conn)
> 	(void)lpcfg_server_signing_allowed(lp_ctx, &conn->smb2.signing_mandatory);
> 
> #if defined(WITH_SMB1SERVER)
>-	smb1_srv_init_signing(conn);
>+	smb1_srv_init_signing(lp_ctx, conn);
> #endif
> 
> 	talloc_unlink(conn, lp_ctx);
>-- 
>2.40.0
>

Actions: View

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