From 4d5c299709f697789b39d2150a0f458951397c78 Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 --- .../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 0556efd47411..07c7442ea9f0 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 6e4ed6713f61..9580db7cedf6 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -1075,6 +1075,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 fe2beafbe9ba..813b21d57774 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 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 bd04b97293084bf4b7b9d79df3f11cb846a0c686 Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 --- 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 32e5c33896b6..13b7b3b31c93 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 fbea4a2734edee42a77bc62d7fd441980884a6ec Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 --- 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 13b7b3b31c93..26cafeba091f 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 0975e3348911944480be8ca1013208c945ede61b Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 --- 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 408faf1481d4..c0e558b2c4d0 100644 --- a/source3/smbd/smb2_negprot.c +++ b/source3/smbd/smb2_negprot.c @@ -365,12 +365,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 5be293e7d725ecbfc510462f864cfffca9cc3da4 Mon Sep 17 00:00:00 2001 From: Ralph Boehme 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 --- .../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