From 7360b39346aaaf3ef54fa9214e3d6d3efe2a0cef 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 96deca54961055b7bf895780d071ee2fab5362d2 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 ca6882c225e..9bba2665663 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 0e3bf90efb23d494d7f6ae11c556f22f9abf5a79 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 70f535e1a49..75960dbc790 100755 --- a/selftest/target/Samba3.pm +++ b/selftest/target/Samba3.pm @@ -1679,6 +1679,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"; @@ -1860,6 +1861,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 @@ -2358,6 +2361,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 a71075f88b3..5b9a5e0ba08 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -721,6 +721,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 a8021d9515ecf75d52d038fe78f72da2c79731af 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 aa379f36ac5feb718c924b030308a29769657f7b 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 34ebe10cd79..d73d426ee3c 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -542,6 +542,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 994ada1d45a18ce2858e17c31e45b614eb26b3aa 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 77e284b99159feee4c5513bc1f739209d26ca9bf 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 0119cf5a2888cd3d97927cb77872fbad82362020 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