From e8ce23909ee64b7ac1b2d90b8d3f2b71c241381c Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 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 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 baaefe32b15e900539ee676a72a97ad317e955bb Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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..50911312cc69 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(conn, lp_ctx); #endif talloc_unlink(conn, lp_ctx); -- 2.40.0