From 2727496981d9bf7e898df5892d1a8afc57d98077 Mon Sep 17 00:00:00 2001 From: Isaac Boukris Date: Thu, 3 Oct 2019 13:09:29 +0300 Subject: [PATCH 1/8] spnego: ignore server mech_types list We should not use the mech list sent by the server in the last 'negotiate' packet in CIFS protocol, as it is not protected and may be subject to downgrade attacks. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14106 Signed-off-by: Isaac Boukris Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett Reviewed-by: Stefan Metzmacher --- auth/gensec/spnego.c | 31 ++++++++++++++++++++++++++----- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c index 0b3fbdce7ac..dc73e324d99 100644 --- a/auth/gensec/spnego.c +++ b/auth/gensec/spnego.c @@ -511,7 +511,11 @@ static NTSTATUS gensec_spnego_client_negTokenInit_start( } n->mech_idx = 0; - n->mech_types = spnego_in->negTokenInit.mechTypes; + + /* Do not use server mech list as it isn't protected. Instead, get all + * supported mechs (excluding SPNEGO). */ + n->mech_types = gensec_security_oids(gensec_security, n, + GENSEC_OID_SPNEGO); if (n->mech_types == NULL) { return NT_STATUS_INVALID_PARAMETER; } @@ -658,13 +662,30 @@ static NTSTATUS gensec_spnego_client_negTokenInit_finish( DATA_BLOB *out) { struct spnego_data spnego_out; - const char *my_mechs[] = {NULL, NULL}; + const char * const *mech_types = NULL; bool ok; - my_mechs[0] = spnego_state->neg_oid; + if (n->mech_types == NULL) { + DBG_WARNING("No mech_types list\n"); + return NT_STATUS_INVALID_PARAMETER; + } + + for (mech_types = n->mech_types; *mech_types != NULL; mech_types++) { + int cmp = strcmp(*mech_types, spnego_state->neg_oid); + + if (cmp == 0) { + break; + } + } + + if (*mech_types == NULL) { + DBG_ERR("Can't find selected sub mechanism in mech_types\n"); + return NT_STATUS_INVALID_PARAMETER; + } + /* compose reply */ spnego_out.type = SPNEGO_NEG_TOKEN_INIT; - spnego_out.negTokenInit.mechTypes = my_mechs; + spnego_out.negTokenInit.mechTypes = mech_types; spnego_out.negTokenInit.reqFlags = data_blob_null; spnego_out.negTokenInit.reqFlagsPadding = 0; spnego_out.negTokenInit.mechListMIC = data_blob_null; @@ -676,7 +697,7 @@ static NTSTATUS gensec_spnego_client_negTokenInit_finish( } ok = spnego_write_mech_types(spnego_state, - my_mechs, + mech_types, &spnego_state->mech_types); if (!ok) { DBG_ERR("failed to write mechTypes\n"); -- 2.21.0 From 3e68711b1c5fee4a17994233037b25789f44906e Mon Sep 17 00:00:00 2001 From: Andreas Schneider Date: Thu, 10 Oct 2019 16:18:21 +0200 Subject: [PATCH 2/8] s3:libsmb: Do not check the SPNEGO neg token for KRB5 The list is not protected and this could be a downgrade attack. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14106 Pair-Programmed-With: Isaac Boukris Reviewed-by: Andreas Schneider Signed-off-by: Andreas Schneider Signed-off-by: Isaac Boukris Reviewed-by: Stefan Metzmacher --- source3/libsmb/cliconnect.c | 50 ------------------------------------- 1 file changed, 50 deletions(-) diff --git a/source3/libsmb/cliconnect.c b/source3/libsmb/cliconnect.c index 94cec062881..3df35931bb6 100644 --- a/source3/libsmb/cliconnect.c +++ b/source3/libsmb/cliconnect.c @@ -232,8 +232,6 @@ NTSTATUS cli_session_creds_prepare_krb5(struct cli_state *cli, char *canon_principal = NULL; char *canon_realm = NULL; const char *target_hostname = NULL; - const DATA_BLOB *server_blob = NULL; - bool got_kerberos_mechanism = false; enum credentials_use_kerberos krb5_state; bool try_kerberos = false; bool need_kinit = false; @@ -242,48 +240,6 @@ NTSTATUS cli_session_creds_prepare_krb5(struct cli_state *cli, bool ok; target_hostname = smbXcli_conn_remote_name(cli->conn); - server_blob = smbXcli_conn_server_gss_blob(cli->conn); - - /* the server might not even do spnego */ - if (server_blob != NULL && server_blob->length != 0) { - char *OIDs[ASN1_MAX_OIDS] = { NULL, }; - size_t i; - - /* - * The server sent us the first part of the SPNEGO exchange in the - * negprot reply. It is WRONG to depend on the principal sent in the - * negprot reply, but right now we do it. If we don't receive one, - * we try to best guess, then fall back to NTLM. - */ - ok = spnego_parse_negTokenInit(frame, - *server_blob, - OIDs, - NULL, - NULL); - if (!ok) { - TALLOC_FREE(frame); - return NT_STATUS_INVALID_PARAMETER; - } - if (OIDs[0] == NULL) { - TALLOC_FREE(frame); - return NT_STATUS_INVALID_PARAMETER; - } - - /* make sure the server understands kerberos */ - for (i = 0; OIDs[i] != NULL; i++) { - if (i == 0) { - DEBUG(3,("got OID=%s\n", OIDs[i])); - } else { - DEBUGADD(3,("got OID=%s\n", OIDs[i])); - } - - if (strcmp(OIDs[i], OID_KERBEROS5_OLD) == 0 || - strcmp(OIDs[i], OID_KERBEROS5) == 0) { - got_kerberos_mechanism = true; - break; - } - } - } auth_requested = cli_credentials_authentication_requested(creds); if (auth_requested) { @@ -333,12 +289,6 @@ NTSTATUS cli_session_creds_prepare_krb5(struct cli_state *cli, need_kinit = false; } else if (krb5_state == CRED_MUST_USE_KERBEROS) { need_kinit = try_kerberos; - } else if (!got_kerberos_mechanism) { - /* - * Most likely the server doesn't support - * Kerberos, don't waste time doing a kinit - */ - need_kinit = false; } else { need_kinit = try_kerberos; } -- 2.21.0 From c46b7cd0f34356fa45e1cb97ccb1a819ddaf481a Mon Sep 17 00:00:00 2001 From: Isaac Boukris Date: Mon, 7 Oct 2019 23:51:19 +0300 Subject: [PATCH 3/8] selftest: s3: add a test for spnego downgrade from krb5 to ntlm BUG: https://bugzilla.samba.org/show_bug.cgi?id=14106 Signed-off-by: Isaac Boukris Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett Reviewed-by: Stefan Metzmacher --- selftest/knownfail.d/spnego_downgrade | 1 + selftest/target/Samba3.pm | 9 +++++ source3/script/tests/test_smbd_no_krb5.sh | 46 +++++++++++++++++++++++ source3/selftest/tests.py | 4 ++ 4 files changed, 60 insertions(+) create mode 100644 selftest/knownfail.d/spnego_downgrade create mode 100755 source3/script/tests/test_smbd_no_krb5.sh diff --git a/selftest/knownfail.d/spnego_downgrade b/selftest/knownfail.d/spnego_downgrade new file mode 100644 index 00000000000..494a55fd43d --- /dev/null +++ b/selftest/knownfail.d/spnego_downgrade @@ -0,0 +1 @@ +^samba3.blackbox.smbd_no_krb5.test_spnego_downgrade diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm index 32bd8698df2..b6bfcef824d 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -1608,6 +1608,7 @@ sub provision($$$$$$$$$) my $dfqconffile="$libdir/dfq.conf"; my $errorinjectconf="$libdir/error_inject.conf"; my $delayinjectconf="$libdir/delay_inject.conf"; + my $globalinjectconf="$libdir/global_inject.conf"; my $nss_wrapper_pl = "$ENV{PERL} $self->{srcdir}/third_party/nss_wrapper/nss_wrapper.pl"; my $nss_wrapper_passwd = "$privatedir/passwd"; @@ -1796,6 +1797,8 @@ sub provision($$$$$$$$$) #it just means we ALLOW one to be configured. allow insecure wide links = yes + include = $globalinjectconf + # Begin extra options $extra_options # End extra options @@ -2331,6 +2334,12 @@ sub provision($$$$$$$$$) } close(DFQCONF); + unless (open(DELAYCONF, ">$globalinjectconf")) { + warn("Unable to open $globalinjectconf"); + return undef; + } + close(DELAYCONF); + ## ## create a test account ## diff --git a/source3/script/tests/test_smbd_no_krb5.sh b/source3/script/tests/test_smbd_no_krb5.sh new file mode 100755 index 00000000000..e9dbb4ae80e --- /dev/null +++ b/source3/script/tests/test_smbd_no_krb5.sh @@ -0,0 +1,46 @@ +#!/bin/sh + +if [ $# -lt 1 ]; then +cat < $global_inject_conf + +# verify that kerberos fails +test_smbclient_expect_failure "smbd_no_krb5" "ls" "//$SERVER/tmp" -k $opt || failed=`expr $failed + 1` + +# verify downgrade to ntlmssp +test_smbclient "test_spnego_downgrade" "ls" "//$SERVER/tmp" $opt || failed=`expr $failed + 1` + +echo '' > $global_inject_conf + +testok $0 $failed diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 5b5a1978988..93c41ef956d 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -751,6 +751,10 @@ plantestsuite("samba3.blackbox.net_tdb", "simpleserver:local", plantestsuite("samba3.blackbox.smbd_error", "simpleserver:local", [os.path.join(samba3srcdir, "script/tests/test_smbd_error.sh")]) +plantestsuite("samba3.blackbox.smbd_no_krb5", "ad_member:local", + [os.path.join(samba3srcdir, "script/tests/test_smbd_no_krb5.sh"), + smbclient3, '$SERVER', "$DC_USERNAME", "$DC_PASSWORD", "$PREFIX"]) + plantestsuite("samba3.blackbox.durable_v2_delay", "simpleserver:local", [os.path.join(samba3srcdir, "script/tests/test_durable_handle_reconnect.sh")]) -- 2.21.0 From de1095518d87b12c56a8fc237a5563f3425c12d8 Mon Sep 17 00:00:00 2001 From: Isaac Boukris Date: Wed, 4 Sep 2019 16:31:21 +0300 Subject: [PATCH 4/8] spnego: add client option to omit sending an optimistic token BUG: https://bugzilla.samba.org/show_bug.cgi?id=14106 Signed-off-by: Isaac Boukris Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett Reviewed-by: Stefan Metzmacher --- auth/gensec/spnego.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c index dc73e324d99..97472c26837 100644 --- a/auth/gensec/spnego.c +++ b/auth/gensec/spnego.c @@ -136,6 +136,7 @@ struct spnego_state { bool done_mic_check; bool simulate_w2k; + bool no_optimistic; /* * The following is used to implement @@ -187,6 +188,10 @@ static NTSTATUS gensec_spnego_client_start(struct gensec_security *gensec_securi spnego_state->simulate_w2k = gensec_setting_bool(gensec_security->settings, "spnego", "simulate_w2k", false); + spnego_state->no_optimistic = gensec_setting_bool(gensec_security->settings, + "spnego", + "client_no_optimistic", + false); gensec_security->private_data = spnego_state; return NT_STATUS_OK; @@ -1944,6 +1949,12 @@ static void gensec_spnego_update_pre(struct tevent_req *req) * blob and NT_STATUS_OK. */ state->sub.status = NT_STATUS_OK; + } else if (spnego_state->state_position == SPNEGO_CLIENT_START && + spnego_state->no_optimistic) { + /* + * Skip optimistic token per conf. + */ + state->sub.status = NT_STATUS_MORE_PROCESSING_REQUIRED; } else { /* * MORE_PROCESSING_REQUIRED => -- 2.21.0 From 1d63cf3e11e160ba4b2134377c375c1645988e2d Mon Sep 17 00:00:00 2001 From: Isaac Boukris Date: Wed, 4 Sep 2019 16:39:43 +0300 Subject: [PATCH 5/8] selftest: add tests for no optimistic spnego exchange BUG: https://bugzilla.samba.org/show_bug.cgi?id=14106 Signed-off-by: Isaac Boukris Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett Reviewed-by: Stefan Metzmacher --- selftest/knownfail.d/spnego_no_optimistic | 1 + source4/selftest/tests.py | 4 ++++ 2 files changed, 5 insertions(+) create mode 100644 selftest/knownfail.d/spnego_no_optimistic diff --git a/selftest/knownfail.d/spnego_no_optimistic b/selftest/knownfail.d/spnego_no_optimistic new file mode 100644 index 00000000000..54f51446be0 --- /dev/null +++ b/selftest/knownfail.d/spnego_no_optimistic @@ -0,0 +1 @@ +^samba4.smb.spnego.*.no_optimistic diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 3f55649f217..1772611eb53 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -547,6 +547,10 @@ plansmbtorture4testsuite('base.xcopy', "ad_dc_ntvfs", ['//$NETBIOSNAME/xcopy_sha plansmbtorture4testsuite('base.xcopy', "ad_dc_ntvfs", ['//$NETBIOSNAME/xcopy_share', '-k', 'no', '--signing=required', '-U%'], modname="samba4.smb.signing --signing=required anon") plansmbtorture4testsuite('base.xcopy', "s4member", ['//$NETBIOSNAME/xcopy_share', '-k', 'no', '--signing=no', '-U%'], modname="samba4.smb.signing --signing=no anon") +# Test SPNEGO without issuing an optimistic token +opt='--option=spnego:client_no_optimistic=yes' +plansmbtorture4testsuite('base.xcopy', "ad_dc", ['//$NETBIOSNAME/xcopy_share', '-U$USERNAME%$PASSWORD', opt, '-k', 'no'], modname="samba4.smb.spnego.ntlmssp.no_optimistic") +plansmbtorture4testsuite('base.xcopy', "ad_dc", ['//$NETBIOSNAME/xcopy_share', '-U$USERNAME%$PASSWORD', opt, '-k', 'yes'], modname="samba4.smb.spnego.krb5.no_optimistic") wb_opts_default = ["--option=\"torture:strict mode=no\"", "--option=\"torture:timelimit=1\"", "--option=\"torture:winbindd_separator=/\"", "--option=\"torture:winbindd_netbios_name=$SERVER\"", "--option=\"torture:winbindd_netbios_domain=$DOMAIN\""] -- 2.21.0 From 7fea067f6d3710147646d882e8428eec7ba561be Mon Sep 17 00:00:00 2001 From: Stefan Metzmacher Date: Fri, 11 Oct 2019 13:23:17 +0200 Subject: [PATCH 6/8] python/tests/gensec: make it possible to add knownfail tests for gensec.update() BUG: https://bugzilla.samba.org/show_bug.cgi?id=14106 Signed-off-by: Stefan Metzmacher Reviewed-by: Andreas Schneider --- python/samba/tests/gensec.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/python/samba/tests/gensec.py b/python/samba/tests/gensec.py index b5ce51de756..c9056ef9681 100644 --- a/python/samba/tests/gensec.py +++ b/python/samba/tests/gensec.py @@ -79,10 +79,16 @@ class GensecTests(samba.tests.TestCase): while True: if not client_finished: print("running client gensec_update") - (client_finished, client_to_server) = self.gensec_client.update(server_to_client) + try: + (client_finished, client_to_server) = self.gensec_client.update(server_to_client) + except samba.NTSTATUSError as nt: + raise AssertionError(nt) if not server_finished: print("running server gensec_update") - (server_finished, server_to_client) = self.gensec_server.update(client_to_server) + try: + (server_finished, server_to_client) = self.gensec_server.update(client_to_server) + except samba.NTSTATUSError as nt: + raise AssertionError(nt) if client_finished and server_finished: break -- 2.21.0 From 2d78d30ef183292573400c267d2d51a2c53d98e8 Mon Sep 17 00:00:00 2001 From: Isaac Boukris Date: Fri, 11 Oct 2019 00:20:16 +0300 Subject: [PATCH 7/8] python/tests/gensec: add spnego downgrade python tests BUG: https://bugzilla.samba.org/show_bug.cgi?id=14106 Pair-Programmed-With: Andreas Schneider Signed-off-by: Isaac Boukris Signed-off-by: Andreas Schneider Reviewed-by: Stefan Metzmacher --- python/samba/tests/gensec.py | 24 +++++++++++++++++++++++- selftest/knownfail.d/samba.tests.gensec | 2 ++ 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 selftest/knownfail.d/samba.tests.gensec diff --git a/python/samba/tests/gensec.py b/python/samba/tests/gensec.py index c9056ef9681..47bb6c82a01 100644 --- a/python/samba/tests/gensec.py +++ b/python/samba/tests/gensec.py @@ -47,11 +47,17 @@ class GensecTests(samba.tests.TestCase): def test_info_uninitialized(self): self.assertRaises(RuntimeError, self.gensec.session_info) - def _test_update(self, mech, client_mech=None): + def _test_update(self, mech, client_mech=None, client_only_opt=None): """Test GENSEC by doing an exchange with ourselves using GSSAPI against a KDC""" """Start up a client and server GENSEC instance to test things with""" + if client_only_opt: + orig_client_opt = self.lp_ctx.get(client_only_opt) + if not orig_client_opt: + orig_client_opt = '' + self.lp_ctx.set(client_only_opt, "yes") + self.gensec_client = gensec.Security.start_client(self.settings) self.gensec_client.set_credentials(self.get_credentials()) self.gensec_client.want_feature(gensec.FEATURE_SEAL) @@ -60,6 +66,9 @@ class GensecTests(samba.tests.TestCase): else: self.gensec_client.start_mech_by_sasl_name(mech) + if client_only_opt: + self.lp_ctx.set(client_only_opt, "no") + self.gensec_server = gensec.Security.start_server(settings=self.settings, auth_context=auth.AuthContext(lp_ctx=self.lp_ctx)) creds = Credentials() @@ -78,11 +87,15 @@ class GensecTests(samba.tests.TestCase): """Run the actual call loop""" while True: if not client_finished: + if client_only_opt: + self.lp_ctx.set(client_only_opt, "yes") print("running client gensec_update") try: (client_finished, client_to_server) = self.gensec_client.update(server_to_client) except samba.NTSTATUSError as nt: raise AssertionError(nt) + if client_only_opt: + self.lp_ctx.set(client_only_opt, "no") if not server_finished: print("running server gensec_update") try: @@ -93,6 +106,9 @@ class GensecTests(samba.tests.TestCase): if client_finished and server_finished: break + if client_only_opt: + self.lp_ctx.set(client_only_opt, orig_client_opt) + self.assertTrue(server_finished) self.assertTrue(client_finished) @@ -121,6 +137,12 @@ class GensecTests(samba.tests.TestCase): def test_update_spnego(self): self._test_update("GSS-SPNEGO") + def test_update_spnego_downgrade(self): + self._test_update("GSS-SPNEGO", "spnego", "gensec:gssapi_krb5") + + def test_update_no_optimistic_spnego(self): + self._test_update("GSS-SPNEGO", "spnego", "spnego:client_no_optimistic") + def test_update_w2k_spnego_client(self): self.lp_ctx.set("spnego:simulate_w2k", "yes") diff --git a/selftest/knownfail.d/samba.tests.gensec b/selftest/knownfail.d/samba.tests.gensec new file mode 100644 index 00000000000..afc9eba9af5 --- /dev/null +++ b/selftest/knownfail.d/samba.tests.gensec @@ -0,0 +1,2 @@ +^samba.tests.gensec.samba.tests.gensec.GensecTests.test_update_no_optimistic_spnego +^samba.tests.gensec.samba.tests.gensec.GensecTests.test_update_spnego_downgrade -- 2.21.0 From c33124380253c5724899b605cd6e1bfd744f50fa Mon Sep 17 00:00:00 2001 From: Isaac Boukris Date: Wed, 4 Sep 2019 17:04:12 +0300 Subject: [PATCH 8/8] spnego: fix server handling of no optimistic exchange BUG: https://bugzilla.samba.org/show_bug.cgi?id=14106 Signed-off-by: Isaac Boukris Reviewed-by: Andreas Schneider Reviewed-by: Andrew Bartlett Reviewed-by: Stefan Metzmacher Autobuild-User(master): Andreas Schneider Autobuild-Date(master): Sat Oct 12 15:51:42 UTC 2019 on sn-devel-184 --- auth/gensec/spnego.c | 13 +++++++++++++ selftest/knownfail.d/samba.tests.gensec | 2 -- selftest/knownfail.d/spnego_downgrade | 1 - selftest/knownfail.d/spnego_no_optimistic | 1 - 4 files changed, 13 insertions(+), 4 deletions(-) delete mode 100644 selftest/knownfail.d/samba.tests.gensec delete mode 100644 selftest/knownfail.d/spnego_downgrade delete mode 100644 selftest/knownfail.d/spnego_no_optimistic diff --git a/auth/gensec/spnego.c b/auth/gensec/spnego.c index 97472c26837..ddbe03c5d6b 100644 --- a/auth/gensec/spnego.c +++ b/auth/gensec/spnego.c @@ -1321,6 +1321,10 @@ static NTSTATUS gensec_spnego_server_negTokenInit_step( spnego_state->mic_requested = true; } + if (sub_in.length == 0) { + spnego_state->no_optimistic = true; + } + /* * Note that 'cur_sec' is temporary memory, but * cur_sec->oid points to a const string in the @@ -1955,6 +1959,15 @@ static void gensec_spnego_update_pre(struct tevent_req *req) * Skip optimistic token per conf. */ state->sub.status = NT_STATUS_MORE_PROCESSING_REQUIRED; + } else if (spnego_state->state_position == SPNEGO_SERVER_START && + state->sub.in.length == 0 && spnego_state->no_optimistic) { + /* + * If we didn't like the mechanism for which the client sent us + * an optimistic token, or if he didn't send any, don't call + * the sub mechanism just yet. + */ + state->sub.status = NT_STATUS_MORE_PROCESSING_REQUIRED; + spnego_state->no_optimistic = false; } else { /* * MORE_PROCESSING_REQUIRED => diff --git a/selftest/knownfail.d/samba.tests.gensec b/selftest/knownfail.d/samba.tests.gensec deleted file mode 100644 index afc9eba9af5..00000000000 --- a/selftest/knownfail.d/samba.tests.gensec +++ /dev/null @@ -1,2 +0,0 @@ -^samba.tests.gensec.samba.tests.gensec.GensecTests.test_update_no_optimistic_spnego -^samba.tests.gensec.samba.tests.gensec.GensecTests.test_update_spnego_downgrade diff --git a/selftest/knownfail.d/spnego_downgrade b/selftest/knownfail.d/spnego_downgrade deleted file mode 100644 index 494a55fd43d..00000000000 --- a/selftest/knownfail.d/spnego_downgrade +++ /dev/null @@ -1 +0,0 @@ -^samba3.blackbox.smbd_no_krb5.test_spnego_downgrade diff --git a/selftest/knownfail.d/spnego_no_optimistic b/selftest/knownfail.d/spnego_no_optimistic deleted file mode 100644 index 54f51446be0..00000000000 --- a/selftest/knownfail.d/spnego_no_optimistic +++ /dev/null @@ -1 +0,0 @@ -^samba4.smb.spnego.*.no_optimistic -- 2.21.0