From bf596c14c2462b9a15ea738ef4f32b3abb8b63d1 Mon Sep 17 00:00:00 2001 From: Aaron Haslett Date: Tue, 23 Oct 2018 17:25:51 +1300 Subject: [PATCH 01/17] CVE-2018-14629 dns: CNAME loop prevention using counter Count number of answers generated by internal DNS query routine and stop at 20 to match Microsoft's loop prevention mechanism. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13600 Signed-off-by: Aaron Haslett Reviewed-by: Andrew Bartlett Reviewed-by: Garming Sam --- python/samba/tests/dns.py | 22 ++++++++++++++++++++++ selftest/knownfail.d/dns | 6 ++++++ source4/dns_server/dns_query.c | 6 ++++++ 3 files changed, 34 insertions(+) diff --git a/python/samba/tests/dns.py b/python/samba/tests/dns.py index 6771e3bb8c4..3e6306e2be8 100644 --- a/python/samba/tests/dns.py +++ b/python/samba/tests/dns.py @@ -844,6 +844,28 @@ class TestComplexQueries(DNSTest): self.assertEquals(response.answers[1].name, name2) self.assertEquals(response.answers[1].rdata, name0) + def test_cname_loop(self): + cname1 = "cnamelooptestrec." + self.get_dns_domain() + cname2 = "cnamelooptestrec2." + self.get_dns_domain() + cname3 = "cnamelooptestrec3." + self.get_dns_domain() + self.make_dns_update(cname1, cname2, dnsp.DNS_TYPE_CNAME) + self.make_dns_update(cname2, cname3, dnsp.DNS_TYPE_CNAME) + self.make_dns_update(cname3, cname1, dnsp.DNS_TYPE_CNAME) + + p = self.make_name_packet(dns.DNS_OPCODE_QUERY) + questions = [] + + q = self.make_name_question(cname1, + dns.DNS_QTYPE_A, + dns.DNS_QCLASS_IN) + questions.append(q) + self.finish_name_packet(p, questions) + + (response, response_packet) =\ + self.dns_transaction_udp(p, host=self.server_ip) + + max_recursion_depth = 20 + self.assertEquals(len(response.answers), max_recursion_depth) class TestInvalidQueries(DNSTest): def setUp(self): diff --git a/selftest/knownfail.d/dns b/selftest/knownfail.d/dns index a5176654cc2..a248432aafa 100644 --- a/selftest/knownfail.d/dns +++ b/selftest/knownfail.d/dns @@ -69,3 +69,9 @@ samba.tests.dns.__main__.TestSimpleQueries.test_qtype_all_query\(rodc:local\) # The SOA override should not pass against the RODC, it must not overstamp samba.tests.dns.__main__.TestSimpleQueries.test_one_SOA_query\(rodc:local\) + +# +# rodc and vampire_dc require signed dns updates, so the test setup +# fails, but the test does run on fl2003dc +^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(rodc:local\) +^samba.tests.dns.__main__.TestComplexQueries.test_cname_loop\(vampire_dc:local\) diff --git a/source4/dns_server/dns_query.c b/source4/dns_server/dns_query.c index 923f7233eb9..65faeac3b6a 100644 --- a/source4/dns_server/dns_query.c +++ b/source4/dns_server/dns_query.c @@ -40,6 +40,7 @@ #undef DBGC_CLASS #define DBGC_CLASS DBGC_DNS +#define MAX_Q_RECURSION_DEPTH 20 struct forwarder_string { const char *forwarder; @@ -419,6 +420,11 @@ static struct tevent_req *handle_dnsrpcrec_send( state->answers = answers; state->nsrecs = nsrecs; + if (talloc_array_length(*answers) >= MAX_Q_RECURSION_DEPTH) { + tevent_req_done(req); + return tevent_req_post(req, ev); + } + resolve_cname = ((rec->wType == DNS_TYPE_CNAME) && ((question->question_type == DNS_QTYPE_A) || (question->question_type == DNS_QTYPE_AAAA))); -- 2.17.1 From 6e84215d4aa7ef51096db3b187adbe22cacdd921 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 23 Oct 2018 17:33:46 +1300 Subject: [PATCH 02/17] CVE-2018-16841 heimdal: Fix segfault on PKINIT with mis-matching principal In Heimdal KRB5_KDC_ERR_CLIENT_NAME_MISMATCH is an enum, so we tried to double-free mem_ctx. This was introduced in 9a0263a7c316112caf0265237bfb2cfb3a3d370d for the MIT KDC effort. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13628 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer --- source4/kdc/db-glue.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/source4/kdc/db-glue.c b/source4/kdc/db-glue.c index bb428e45cbc..f4f3ba63299 100644 --- a/source4/kdc/db-glue.c +++ b/source4/kdc/db-glue.c @@ -2610,10 +2610,10 @@ samba_kdc_check_pkinit_ms_upn_match(krb5_context context, * comparison */ if (!(orig_sid && target_sid && dom_sid_equal(orig_sid, target_sid))) { talloc_free(mem_ctx); -#ifdef KRB5_KDC_ERR_CLIENT_NAME_MISMATCH /* Heimdal */ - return KRB5_KDC_ERR_CLIENT_NAME_MISMATCH; -#elif defined(KRB5KDC_ERR_CLIENT_NAME_MISMATCH) /* MIT */ +#if defined(KRB5KDC_ERR_CLIENT_NAME_MISMATCH) /* MIT */ return KRB5KDC_ERR_CLIENT_NAME_MISMATCH; +#else /* Heimdal (where this is an enum) */ + return KRB5_KDC_ERR_CLIENT_NAME_MISMATCH; #endif } -- 2.17.1 From 4783b9d6a43287a938b18e15f146e6895b689956 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Wed, 24 Oct 2018 15:41:28 +1300 Subject: [PATCH 03/17] CVE-2018-16841 selftest: Check for mismatching principal in certficate compared with principal in AS-REQ BUG: https://bugzilla.samba.org/show_bug.cgi?id=13628 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer --- testprogs/blackbox/test_pkinit_heimdal.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/testprogs/blackbox/test_pkinit_heimdal.sh b/testprogs/blackbox/test_pkinit_heimdal.sh index 0a13aa293e7..0912e0dbfe8 100755 --- a/testprogs/blackbox/test_pkinit_heimdal.sh +++ b/testprogs/blackbox/test_pkinit_heimdal.sh @@ -75,10 +75,18 @@ testit "STEP1 kinit with pkinit (name specified) " $samba4kinit $enctype --reque testit "STEP1 kinit renew ticket (name specified)" $samba4kinit --request-pac -R || failed=`expr $failed + 1` test_smbclient "STEP1 Test login with kerberos ccache (name specified)" 'ls' "$unc" -k yes || failed=`expr $failed + 1` +testit_expect_failure "STEP1 kinit with pkinit (wrong name specified) " $samba4kinit $enctype --request-pac --renewable $PKUSER not$USERNAME@$REALM || failed=`expr $failed + 1` + +testit_expect_failure "STEP1 kinit with pkinit (wrong name specified 2) " $samba4kinit $enctype --request-pac --renewable $PKUSER $SERVER@$REALM || failed=`expr $failed + 1` + testit "STEP1 kinit with pkinit (enterprise name specified)" $samba4kinit $enctype --request-pac --renewable $PKUSER --enterprise $USERNAME@$REALM || failed=`expr $failed + 1` testit "STEP1 kinit renew ticket (enterprise name specified)" $samba4kinit --request-pac -R || failed=`expr $failed + 1` test_smbclient "STEP1 Test login with kerberos ccache (enterprise name specified)" 'ls' "$unc" -k yes || failed=`expr $failed + 1` +testit_expect_failure "STEP1 kinit with pkinit (wrong enterprise name specified) " $samba4kinit $enctype --request-pac --renewable $PKUSER --enterprise not$USERNAME@$REALM || failed=`expr $failed + 1` + +testit_expect_failure "STEP1 kinit with pkinit (wrong enterprise name specified 2) " $samba4kinit $enctype --request-pac --renewable $PKUSER --enterprise $SERVER$@$REALM || failed=`expr $failed + 1` + testit "STEP1 kinit with pkinit (enterprise name in cert)" $samba4kinit $enctype --request-pac --renewable $PKUSER --pk-enterprise || failed=`expr $failed + 1` testit "STEP1 kinit renew ticket (enterprise name in cert)" $samba4kinit --request-pac -R || failed=`expr $failed + 1` test_smbclient "STEP1 Test login with kerberos ccache (enterprise name in cert)" 'ls' "$unc" -k yes || failed=`expr $failed + 1` -- 2.17.1 From f40e1b3b42ce23b574a4c530545ff8170ddc7330 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 6 Nov 2018 12:10:07 +1300 Subject: [PATCH 04/17] CVE-2018-16852 dcerpc dnsserver: Verification tests Tests to verify Bug 13669 - (CVE-2018-16852) NULL pointer de-reference in Samba AD DC DNS management The presence of the ZONE_MASTER_SERVERS property or the ZONE_SCAVENGING_SERVERS property in a zone record causes the server to follow a null pointer and terminate. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13669 Reviewed-by: Andrew Bartlett Signed-off-by: Gary Lockyer --- selftest/knownfail.d/bug13669 | 4 + .../tests/rpc_dns_server_dnsutils_test.c | 304 ++++++++++++++++++ source4/rpc_server/wscript_build | 17 +- source4/selftest/tests.py | 2 + 4 files changed, 325 insertions(+), 2 deletions(-) create mode 100644 selftest/knownfail.d/bug13669 create mode 100644 source4/rpc_server/tests/rpc_dns_server_dnsutils_test.c diff --git a/selftest/knownfail.d/bug13669 b/selftest/knownfail.d/bug13669 new file mode 100644 index 00000000000..74c8c130674 --- /dev/null +++ b/selftest/knownfail.d/bug13669 @@ -0,0 +1,4 @@ +^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_master_servers_empty +^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_master_servers +^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_scavenging_servers_empty +^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_scavenging_servers diff --git a/source4/rpc_server/tests/rpc_dns_server_dnsutils_test.c b/source4/rpc_server/tests/rpc_dns_server_dnsutils_test.c new file mode 100644 index 00000000000..89721135658 --- /dev/null +++ b/source4/rpc_server/tests/rpc_dns_server_dnsutils_test.c @@ -0,0 +1,304 @@ +/* + * Unit tests for source4/rpc_server/dnsserver/dnsutils.c + * + * Copyright (C) Catalyst.NET Ltd 2018 + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +/* + * from cmocka.c: + * These headers or their equivalents should be included prior to + * including + * this header file. + * + * #include + * #include + * #include + * + * This allows test applications to use custom definitions of C standard + * library functions and types. + * + */ + +#include +#include +#include +#include + + +#include "../dnsserver/dnsutils.c" + + +/* + * Test setting of an empty ZONE_MASTER_SERVERS property + */ +static void test_dnsserver_init_zoneinfo_master_servers_empty(void **state) +{ + struct dnsserver_zone *zone = NULL; + struct dnsserver_serverinfo *serverinfo = NULL; + struct dnsserver_zoneinfo *zoneinfo = NULL; + struct dnsp_DnsProperty *property = NULL; + + TALLOC_CTX *ctx = talloc_new(NULL); + + /* + * Setup the zone data + */ + zone = talloc_zero(ctx, struct dnsserver_zone); + assert_non_null(zone); + zone->name = "test"; + + /* + * Set up an empty ZONE_MASTER_SERVERS property + */ + property = talloc_zero_array(ctx, struct dnsp_DnsProperty, 1); + assert_non_null(property); + property->id = DSPROPERTY_ZONE_MASTER_SERVERS; + property->data.master_servers.addrCount = 0; + property->data.master_servers.addr = NULL; + + zone->tmp_props = property; + zone->num_props = 1; + + + /* + * Setup the server info + */ + serverinfo = talloc_zero(ctx, struct dnsserver_serverinfo); + assert_non_null(serverinfo); + + /* + * call dnsserver_init_zoneinfo + */ + zoneinfo = dnsserver_init_zoneinfo(zone, serverinfo); + + /* + * Check results + */ + assert_non_null(zoneinfo); + assert_non_null(zoneinfo->aipLocalMasters); + assert_int_equal(zoneinfo->aipLocalMasters->AddrCount, 0); + assert_null(zoneinfo->aipLocalMasters->AddrArray); + + TALLOC_FREE(ctx); +} + +/* + * Test setting of a non empty ZONE_MASTER_SERVERS property + */ +static void test_dnsserver_init_zoneinfo_master_servers(void **state) +{ + struct dnsserver_zone *zone = NULL; + struct dnsserver_serverinfo *serverinfo = NULL; + struct dnsserver_zoneinfo *zoneinfo = NULL; + struct dnsp_DnsProperty *property = NULL; + + TALLOC_CTX *ctx = talloc_new(NULL); + + /* + * Setup the zone data + */ + zone = talloc_zero(ctx, struct dnsserver_zone); + assert_non_null(zone); + zone->name = "test"; + + /* + * Set up an empty ZONE_MASTER_SERVERS property + */ + property = talloc_zero_array(ctx, struct dnsp_DnsProperty, 1); + assert_non_null(property); + property->id = DSPROPERTY_ZONE_MASTER_SERVERS; + property->data.master_servers.addrCount = 4; + property->data.master_servers.addr = + talloc_zero_array(ctx, uint32_t, 4); + property->data.master_servers.addr[0] = 1000; + property->data.master_servers.addr[1] = 1001; + property->data.master_servers.addr[2] = 1002; + property->data.master_servers.addr[3] = 1003; + + zone->tmp_props = property; + zone->num_props = 1; + + + /* + * Setup the server info + */ + serverinfo = talloc_zero(ctx, struct dnsserver_serverinfo); + assert_non_null(serverinfo); + + /* + * call dnsserver_init_zoneinfo + */ + zoneinfo = dnsserver_init_zoneinfo(zone, serverinfo); + + /* + * Check results + */ + assert_non_null(zoneinfo); + assert_non_null(zoneinfo->aipLocalMasters); + assert_int_equal(zoneinfo->aipLocalMasters->AddrCount, 4); + assert_non_null(zoneinfo->aipLocalMasters->AddrArray); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[0], 1000); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[1], 1001); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[2], 1002); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[3], 1003); + + /* + * Ensure that we're working with a copy of the property data + * and not a reference. + * The pointers should be different, and we should be able to change + * the values in the property without affecting the zoneinfo data + */ + assert_true(zoneinfo->aipLocalMasters->AddrArray != + property->data.master_servers.addr); + property->data.master_servers.addr[0] = 0; + property->data.master_servers.addr[1] = 1; + property->data.master_servers.addr[2] = 2; + property->data.master_servers.addr[3] = 3; + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[0], 1000); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[1], 1001); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[2], 1002); + assert_int_equal(zoneinfo->aipLocalMasters->AddrArray[3], 1003); + + TALLOC_FREE(ctx); +} + +/* + * Test setting of an empty ZONE_SCAVENGING_SERVERS property + */ +static void test_dnsserver_init_zoneinfo_scavenging_servers_empty(void **state) +{ + struct dnsserver_zone *zone = NULL; + struct dnsserver_serverinfo *serverinfo = NULL; + struct dnsserver_zoneinfo *zoneinfo = NULL; + struct dnsp_DnsProperty *property = NULL; + + TALLOC_CTX *ctx = talloc_new(NULL); + + /* + * Setup the zone data + */ + zone = talloc_zero(ctx, struct dnsserver_zone); + assert_non_null(zone); + zone->name = "test"; + + property = talloc_zero_array(ctx, struct dnsp_DnsProperty, 1); + assert_non_null(property); + property->id = DSPROPERTY_ZONE_SCAVENGING_SERVERS; + property->data.servers.addrCount = 0; + property->data.servers.addr = NULL; + + zone->tmp_props = property; + zone->num_props = 1; + + + serverinfo = talloc_zero(ctx, struct dnsserver_serverinfo); + assert_non_null(serverinfo); + + zoneinfo = dnsserver_init_zoneinfo(zone, serverinfo); + + assert_non_null(zoneinfo); + assert_non_null(zoneinfo->aipScavengeServers); + assert_int_equal(zoneinfo->aipScavengeServers->AddrCount, 0); + assert_null(zoneinfo->aipScavengeServers->AddrArray); + + TALLOC_FREE(ctx); +} + +/* + * Test setting of a non empty ZONE_SCAVENGING_SERVERS property + */ +static void test_dnsserver_init_zoneinfo_scavenging_servers(void **state) +{ + struct dnsserver_zone *zone = NULL; + struct dnsserver_serverinfo *serverinfo = NULL; + struct dnsserver_zoneinfo *zoneinfo = NULL; + struct dnsp_DnsProperty *property = NULL; + + TALLOC_CTX *ctx = talloc_new(NULL); + + /* + * Setup the zone data + */ + zone = talloc_zero(ctx, struct dnsserver_zone); + assert_non_null(zone); + zone->name = "test"; + + property = talloc_zero_array(ctx, struct dnsp_DnsProperty, 1); + assert_non_null(property); + property->id = DSPROPERTY_ZONE_SCAVENGING_SERVERS; + property->data.servers.addrCount = 4; + property->data.servers.addr = talloc_zero_array(ctx, uint32_t, 4); + property->data.servers.addr[0] = 1000; + property->data.servers.addr[1] = 1001; + property->data.servers.addr[2] = 1002; + property->data.servers.addr[3] = 1003; + + zone->tmp_props = property; + zone->num_props = 1; + + + serverinfo = talloc_zero(ctx, struct dnsserver_serverinfo); + assert_non_null(serverinfo); + + zoneinfo = dnsserver_init_zoneinfo(zone, serverinfo); + + assert_non_null(zoneinfo); + assert_non_null(zoneinfo->aipScavengeServers); + assert_int_equal(zoneinfo->aipScavengeServers->AddrCount, 4); + assert_non_null(zoneinfo->aipScavengeServers->AddrArray); + assert_non_null(zoneinfo->aipScavengeServers->AddrArray); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[0], 1000); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[1], 1001); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[2], 1002); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[3], 1003); + + /* + * Ensure that we're working with a copy of the property data + * and not a reference. + * The pointers should be different, and we should be able to change + * the values in the property without affecting the zoneinfo data + */ + assert_true(zoneinfo->aipScavengeServers->AddrArray != + property->data.servers.addr); + property->data.servers.addr[0] = 0; + property->data.servers.addr[1] = 1; + property->data.servers.addr[2] = 2; + property->data.servers.addr[3] = 3; + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[0], 1000); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[1], 1001); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[2], 1002); + assert_int_equal(zoneinfo->aipScavengeServers->AddrArray[3], 1003); + + + TALLOC_FREE(ctx); +} +int main(int argc, const char **argv) +{ + const struct CMUnitTest tests[] = { + cmocka_unit_test( + test_dnsserver_init_zoneinfo_master_servers_empty), + cmocka_unit_test( + test_dnsserver_init_zoneinfo_master_servers), + cmocka_unit_test( + test_dnsserver_init_zoneinfo_scavenging_servers_empty), + cmocka_unit_test( + test_dnsserver_init_zoneinfo_scavenging_servers), + }; + + cmocka_set_message_output(CM_OUTPUT_SUBUNIT); + return cmocka_run_group_tests(tests, NULL, NULL); +} diff --git a/source4/rpc_server/wscript_build b/source4/rpc_server/wscript_build index 8e05eb8a0c3..14b68c7ce0f 100644 --- a/source4/rpc_server/wscript_build +++ b/source4/rpc_server/wscript_build @@ -3,7 +3,7 @@ bld.SAMBA_SUBSYSTEM('DCERPC_SHARE', source='common/share_info.c', autoproto='common/share.h', - deps='ldb', + deps='ldb share', enabled=bld.CONFIG_SET('WITH_NTVFS_FILESERVER'), ) @@ -163,7 +163,7 @@ bld.SAMBA_MODULE('dcerpc_dnsserver', source='dnsserver/dcerpc_dnsserver.c dnsserver/dnsutils.c dnsserver/dnsdata.c dnsserver/dnsdb.c', subsystem='dcerpc_server', init_function='dcerpc_server_dnsserver_init', - deps='DCERPC_COMMON dnsserver_common' + deps='DCERPC_COMMON dnsserver_common netif' ) @@ -176,3 +176,16 @@ bld.SAMBA_MODULE('service_dcerpc', deps='dcerpc_server' ) +if bld.CONFIG_GET('ENABLE_SELFTEST'): + bld.SAMBA_BINARY( + 'test_rpc_dns_server_dnsutils', + source='tests/rpc_dns_server_dnsutils_test.c', + deps=''' + dnsserver_common + DCERPC_COMMON + cmocka + talloc + ''', + install=False, + enabled=bld.AD_DC_BUILD_IS_ENABLED() + ) diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index 9dec0adb05c..18b2c1162b0 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -1164,3 +1164,5 @@ plantestsuite("samba4.dsdb.samdb.ldb_modules.audit_util", "none", [os.path.join(bindir(), "test_audit_util")]) plantestsuite("samba4.dsdb.samdb.ldb_modules.audit_log", "none", [os.path.join(bindir(), "test_audit_log")]) +plantestsuite("samba4.dcerpc.dnsserver.dnsutils", "none", + [os.path.join(bindir(), "test_rpc_dns_server_dnsutils")]) -- 2.17.1 From 05f867db81f118215445f2c49eda4b9c3451d14a Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Tue, 6 Nov 2018 12:16:30 +1300 Subject: [PATCH 05/17] CVE-2018-16852 dcerpc dnsserver: Ensure properties are handled correctly Fixes for Bug 13669 - (CVE-2018-16852) NULL pointer de-reference in Samba AD DC DNS management The presence of the ZONE_MASTER_SERVERS property or the ZONE_SCAVENGING_SERVERS property in a zone record causes the server to follow a null pointer and terminate. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13669 Signed-off-by: Gary Lockyer Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/bug13669 | 4 -- source4/rpc_server/dnsserver/dnsutils.c | 64 +++++++++++++++++++++---- 2 files changed, 56 insertions(+), 12 deletions(-) delete mode 100644 selftest/knownfail.d/bug13669 diff --git a/selftest/knownfail.d/bug13669 b/selftest/knownfail.d/bug13669 deleted file mode 100644 index 74c8c130674..00000000000 --- a/selftest/knownfail.d/bug13669 +++ /dev/null @@ -1,4 +0,0 @@ -^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_master_servers_empty -^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_master_servers -^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_scavenging_servers_empty -^samba4.dcerpc.dnsserver.dnsutils.test_dnsserver_init_zoneinfo_scavenging_servers diff --git a/source4/rpc_server/dnsserver/dnsutils.c b/source4/rpc_server/dnsserver/dnsutils.c index a1c749074af..e4055c99e74 100644 --- a/source4/rpc_server/dnsserver/dnsutils.c +++ b/source4/rpc_server/dnsserver/dnsutils.c @@ -209,6 +209,46 @@ struct dnsserver_serverinfo *dnsserver_init_serverinfo(TALLOC_CTX *mem_ctx, } +/* + * Helper function to copy a dnsp_ip4_array struct to an IP4_ARRAY struct. + * The new structure and it's data are allocated on the supplied talloc context + */ +static struct IP4_ARRAY *copy_ip4_array( + TALLOC_CTX *ctx, + const char *name, + struct dnsp_ip4_array array) { + + struct IP4_ARRAY *ip4_array = NULL; + unsigned int i; + + ip4_array = talloc_zero(ctx, struct IP4_ARRAY); + if (ip4_array == NULL) { + DBG_ERR("Out of memory copying property [%s]\n", + name); + return NULL; + } + + ip4_array->AddrCount = array.addrCount; + if (ip4_array->AddrCount == 0) { + return ip4_array; + } + + ip4_array->AddrArray = talloc_array(ip4_array, uint32_t, + ip4_array->AddrCount); + if (ip4_array->AddrArray == NULL) { + TALLOC_FREE(ip4_array); + DBG_ERR("Out of memory copying property [%s] values\n", + name); + return NULL; + } + + for (i = 0; i < ip4_array->AddrCount; i++) { + ip4_array->AddrArray[i] = array.addr[i]; + } + + return ip4_array; +} + struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, struct dnsserver_serverinfo *serverinfo) { @@ -309,20 +349,28 @@ struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, prop->aging_enabled; break; case DSPROPERTY_ZONE_SCAVENGING_SERVERS: - zoneinfo->aipScavengeServers->AddrCount = - prop->servers.addrCount; - zoneinfo->aipScavengeServers->AddrArray = - prop->servers.addr; + zoneinfo->aipScavengeServers = + copy_ip4_array(zoneinfo, + "ZONE_SCAVENGING_SERVERS", + prop->servers); + if (zoneinfo->aipScavengeServers == NULL) { + TALLOC_FREE(zoneinfo); + return NULL; + } break; case DSPROPERTY_ZONE_AGING_ENABLED_TIME: zoneinfo->dwAvailForScavengeTime = prop->next_scavenging_cycle_hours; break; case DSPROPERTY_ZONE_MASTER_SERVERS: - zoneinfo->aipLocalMasters->AddrCount = - prop->master_servers.addrCount; - zoneinfo->aipLocalMasters->AddrArray = - prop->master_servers.addr; + zoneinfo->aipLocalMasters = + copy_ip4_array(zoneinfo, + "ZONE_MASTER_SERVERS", + prop->master_servers); + if (zoneinfo->aipLocalMasters == NULL) { + TALLOC_FREE(zoneinfo); + return NULL; + } break; case DSPROPERTY_ZONE_EMPTY: case DSPROPERTY_ZONE_SECURE_TIME: -- 2.17.1 From c78ca8b9b48a19e71f4d6ddd2e300f282fb0b247 Mon Sep 17 00:00:00 2001 From: Gary Lockyer Date: Wed, 7 Nov 2018 15:08:04 +1300 Subject: [PATCH 06/17] CVE-2018-16852 dcerpc dnsserver: refactor common properties handling dnsserver_common.c and dnsutils.c both share similar code to process zone properties. This patch extracts the common code and moves it to dnsserver_common.c. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13669 Signed-off-by: Gary Lockyer Reviewed-by: Andrew Bartlett --- source4/dns_server/dnsserver_common.c | 129 +++++++++++++++++------- source4/dns_server/dnsserver_common.h | 3 + source4/rpc_server/dnsserver/dnsutils.c | 107 ++------------------ 3 files changed, 104 insertions(+), 135 deletions(-) diff --git a/source4/dns_server/dnsserver_common.c b/source4/dns_server/dnsserver_common.c index bbbfe920f4e..cc24a6c1b52 100644 --- a/source4/dns_server/dnsserver_common.c +++ b/source4/dns_server/dnsserver_common.c @@ -742,6 +742,94 @@ bool dns_name_is_static(struct dnsp_DnssrvRpcRecord *records, return false; } +/* + * Helper function to copy a dnsp_ip4_array struct to an IP4_ARRAY struct. + * The new structure and it's data are allocated on the supplied talloc context + */ +static struct IP4_ARRAY *copy_ip4_array(TALLOC_CTX *ctx, + const char *name, + struct dnsp_ip4_array array) +{ + + struct IP4_ARRAY *ip4_array = NULL; + unsigned int i; + + ip4_array = talloc_zero(ctx, struct IP4_ARRAY); + if (ip4_array == NULL) { + DBG_ERR("Out of memory copying property [%s]\n", name); + return NULL; + } + + ip4_array->AddrCount = array.addrCount; + if (ip4_array->AddrCount == 0) { + return ip4_array; + } + + ip4_array->AddrArray = + talloc_array(ip4_array, uint32_t, ip4_array->AddrCount); + if (ip4_array->AddrArray == NULL) { + TALLOC_FREE(ip4_array); + DBG_ERR("Out of memory copying property [%s] values\n", name); + return NULL; + } + + for (i = 0; i < ip4_array->AddrCount; i++) { + ip4_array->AddrArray[i] = array.addr[i]; + } + + return ip4_array; +} + +bool dns_zoneinfo_load_zone_property(struct dnsserver_zoneinfo *zoneinfo, + struct dnsp_DnsProperty *prop) +{ + switch (prop->id) { + case DSPROPERTY_ZONE_TYPE: + zoneinfo->dwZoneType = prop->data.zone_type; + break; + case DSPROPERTY_ZONE_ALLOW_UPDATE: + zoneinfo->fAllowUpdate = prop->data.allow_update_flag; + break; + case DSPROPERTY_ZONE_NOREFRESH_INTERVAL: + zoneinfo->dwNoRefreshInterval = prop->data.norefresh_hours; + break; + case DSPROPERTY_ZONE_REFRESH_INTERVAL: + zoneinfo->dwRefreshInterval = prop->data.refresh_hours; + break; + case DSPROPERTY_ZONE_AGING_STATE: + zoneinfo->fAging = prop->data.aging_enabled; + break; + case DSPROPERTY_ZONE_SCAVENGING_SERVERS: + zoneinfo->aipScavengeServers = copy_ip4_array( + zoneinfo, "ZONE_SCAVENGING_SERVERS", prop->data.servers); + if (zoneinfo->aipScavengeServers == NULL) { + return false; + } + break; + case DSPROPERTY_ZONE_AGING_ENABLED_TIME: + zoneinfo->dwAvailForScavengeTime = + prop->data.next_scavenging_cycle_hours; + break; + case DSPROPERTY_ZONE_MASTER_SERVERS: + zoneinfo->aipLocalMasters = copy_ip4_array( + zoneinfo, "ZONE_MASTER_SERVERS", prop->data.master_servers); + if (zoneinfo->aipLocalMasters == NULL) { + return false; + } + break; + case DSPROPERTY_ZONE_EMPTY: + case DSPROPERTY_ZONE_SECURE_TIME: + case DSPROPERTY_ZONE_DELETED_FROM_HOSTNAME: + case DSPROPERTY_ZONE_AUTO_NS_SERVERS: + case DSPROPERTY_ZONE_DCPROMO_CONVERT: + case DSPROPERTY_ZONE_SCAVENGING_SERVERS_DA: + case DSPROPERTY_ZONE_MASTER_SERVERS_DA: + case DSPROPERTY_ZONE_NS_SERVERS_DA: + case DSPROPERTY_ZONE_NODE_DBFLAGS: + break; + } + return true; +} WERROR dns_get_zone_properties(struct ldb_context *samdb, TALLOC_CTX *mem_ctx, struct ldb_dn *zone_dn, @@ -774,6 +862,7 @@ WERROR dns_get_zone_properties(struct ldb_context *samdb, } for (i = 0; i < element->num_values; i++) { + bool valid_property; prop = talloc_zero(mem_ctx, struct dnsp_DnsProperty); if (prop == NULL) { return WERR_NOT_ENOUGH_MEMORY; @@ -787,42 +876,10 @@ WERROR dns_get_zone_properties(struct ldb_context *samdb, return DNS_ERR(SERVER_FAILURE); } - switch (prop->id) { - case DSPROPERTY_ZONE_AGING_STATE: - zoneinfo->fAging = prop->data.aging_enabled; - break; - case DSPROPERTY_ZONE_NOREFRESH_INTERVAL: - zoneinfo->dwNoRefreshInterval = - prop->data.norefresh_hours; - break; - case DSPROPERTY_ZONE_REFRESH_INTERVAL: - zoneinfo->dwRefreshInterval = prop->data.refresh_hours; - break; - case DSPROPERTY_ZONE_ALLOW_UPDATE: - zoneinfo->fAllowUpdate = prop->data.allow_update_flag; - break; - case DSPROPERTY_ZONE_AGING_ENABLED_TIME: - zoneinfo->dwAvailForScavengeTime = - prop->data.next_scavenging_cycle_hours; - break; - case DSPROPERTY_ZONE_SCAVENGING_SERVERS: - zoneinfo->aipScavengeServers->AddrCount = - prop->data.servers.addrCount; - zoneinfo->aipScavengeServers->AddrArray = - prop->data.servers.addr; - break; - case DSPROPERTY_ZONE_EMPTY: - case DSPROPERTY_ZONE_TYPE: - case DSPROPERTY_ZONE_SECURE_TIME: - case DSPROPERTY_ZONE_DELETED_FROM_HOSTNAME: - case DSPROPERTY_ZONE_MASTER_SERVERS: - case DSPROPERTY_ZONE_AUTO_NS_SERVERS: - case DSPROPERTY_ZONE_DCPROMO_CONVERT: - case DSPROPERTY_ZONE_SCAVENGING_SERVERS_DA: - case DSPROPERTY_ZONE_MASTER_SERVERS_DA: - case DSPROPERTY_ZONE_NS_SERVERS_DA: - case DSPROPERTY_ZONE_NODE_DBFLAGS: - break; + valid_property = + dns_zoneinfo_load_zone_property(zoneinfo, prop); + if (!valid_property) { + return DNS_ERR(SERVER_FAILURE); } } diff --git a/source4/dns_server/dnsserver_common.h b/source4/dns_server/dnsserver_common.h index 380f61b8dbc..60ecde4fa91 100644 --- a/source4/dns_server/dnsserver_common.h +++ b/source4/dns_server/dnsserver_common.h @@ -87,4 +87,7 @@ NTSTATUS dns_common_zones(struct ldb_context *samdb, TALLOC_CTX *mem_ctx, struct ldb_dn *base_dn, struct dns_server_zone **zones_ret); + +bool dns_zoneinfo_load_zone_property(struct dnsserver_zoneinfo *zoneinfo, + struct dnsp_DnsProperty *prop); #endif /* __DNSSERVER_COMMON_H__ */ diff --git a/source4/rpc_server/dnsserver/dnsutils.c b/source4/rpc_server/dnsserver/dnsutils.c index e4055c99e74..bb52a1797a9 100644 --- a/source4/rpc_server/dnsserver/dnsutils.c +++ b/source4/rpc_server/dnsserver/dnsutils.c @@ -25,6 +25,7 @@ #include "dsdb/samdb/samdb.h" #include "lib/socket/netif.h" #include "lib/util/util_net.h" +#include "dnsserver_common.h" static struct DNS_ADDR_ARRAY *fill_dns_addr_array(TALLOC_CTX *mem_ctx, struct loadparm_context *lp_ctx, @@ -208,47 +209,6 @@ struct dnsserver_serverinfo *dnsserver_init_serverinfo(TALLOC_CTX *mem_ctx, return serverinfo; } - -/* - * Helper function to copy a dnsp_ip4_array struct to an IP4_ARRAY struct. - * The new structure and it's data are allocated on the supplied talloc context - */ -static struct IP4_ARRAY *copy_ip4_array( - TALLOC_CTX *ctx, - const char *name, - struct dnsp_ip4_array array) { - - struct IP4_ARRAY *ip4_array = NULL; - unsigned int i; - - ip4_array = talloc_zero(ctx, struct IP4_ARRAY); - if (ip4_array == NULL) { - DBG_ERR("Out of memory copying property [%s]\n", - name); - return NULL; - } - - ip4_array->AddrCount = array.addrCount; - if (ip4_array->AddrCount == 0) { - return ip4_array; - } - - ip4_array->AddrArray = talloc_array(ip4_array, uint32_t, - ip4_array->AddrCount); - if (ip4_array->AddrArray == NULL) { - TALLOC_FREE(ip4_array); - DBG_ERR("Out of memory copying property [%s] values\n", - name); - return NULL; - } - - for (i = 0; i < ip4_array->AddrCount; i++) { - ip4_array->AddrArray[i] = array.addr[i]; - } - - return ip4_array; -} - struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, struct dnsserver_serverinfo *serverinfo) { @@ -257,8 +217,7 @@ struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, const char *revzone = "in-addr.arpa"; const char *revzone6 = "ip6.arpa"; int len1, len2; - union dnsPropertyData *prop = NULL; - int i=0; + unsigned int i = 0; zoneinfo = talloc_zero(zone, struct dnsserver_zoneinfo); if (zoneinfo == NULL) { @@ -326,62 +285,12 @@ struct dnsserver_zoneinfo *dnsserver_init_zoneinfo(struct dnsserver_zone *zone, zoneinfo->dwLastXfrResult = 0; for(i=0; inum_props; i++){ - prop=&(zone->tmp_props[i].data); - switch (zone->tmp_props[i].id) { - case DSPROPERTY_ZONE_TYPE: - zoneinfo->dwZoneType = - prop->zone_type; - break; - case DSPROPERTY_ZONE_ALLOW_UPDATE: - zoneinfo->fAllowUpdate = - prop->allow_update_flag; - break; - case DSPROPERTY_ZONE_NOREFRESH_INTERVAL: - zoneinfo->dwNoRefreshInterval = - prop->norefresh_hours; - break; - case DSPROPERTY_ZONE_REFRESH_INTERVAL: - zoneinfo->dwRefreshInterval = - prop->refresh_hours; - break; - case DSPROPERTY_ZONE_AGING_STATE: - zoneinfo->fAging = - prop->aging_enabled; - break; - case DSPROPERTY_ZONE_SCAVENGING_SERVERS: - zoneinfo->aipScavengeServers = - copy_ip4_array(zoneinfo, - "ZONE_SCAVENGING_SERVERS", - prop->servers); - if (zoneinfo->aipScavengeServers == NULL) { - TALLOC_FREE(zoneinfo); - return NULL; - } - break; - case DSPROPERTY_ZONE_AGING_ENABLED_TIME: - zoneinfo->dwAvailForScavengeTime = - prop->next_scavenging_cycle_hours; - break; - case DSPROPERTY_ZONE_MASTER_SERVERS: - zoneinfo->aipLocalMasters = - copy_ip4_array(zoneinfo, - "ZONE_MASTER_SERVERS", - prop->master_servers); - if (zoneinfo->aipLocalMasters == NULL) { - TALLOC_FREE(zoneinfo); - return NULL; - } - break; - case DSPROPERTY_ZONE_EMPTY: - case DSPROPERTY_ZONE_SECURE_TIME: - case DSPROPERTY_ZONE_DELETED_FROM_HOSTNAME: - case DSPROPERTY_ZONE_AUTO_NS_SERVERS: - case DSPROPERTY_ZONE_DCPROMO_CONVERT: - case DSPROPERTY_ZONE_SCAVENGING_SERVERS_DA: - case DSPROPERTY_ZONE_MASTER_SERVERS_DA: - case DSPROPERTY_ZONE_NS_SERVERS_DA: - case DSPROPERTY_ZONE_NODE_DBFLAGS: - break; + bool valid_property; + valid_property = dns_zoneinfo_load_zone_property( + zoneinfo, &zone->tmp_props[i]); + if (!valid_property) { + TALLOC_FREE(zoneinfo); + return NULL; } } -- 2.17.1 From f33f52c366f7cf140f470de44579dcb7eb832629 Mon Sep 17 00:00:00 2001 From: Garming Sam Date: Mon, 5 Nov 2018 16:18:18 +1300 Subject: [PATCH 07/17] CVE-2018-16851 ldap_server: Check ret before manipulating blob In the case of hitting the talloc ~256MB limit, this causes a crash in the server. Note that you would actually need to load >256MB of data into the LDAP. Although there is some generated/hidden data which would help you reach that limit (descriptors and RMD blobs). BUG: https://bugzilla.samba.org/show_bug.cgi?id=13674 Signed-off-by: Garming Sam Reviewed-by: Andrew Bartlett --- source4/ldap_server/ldap_server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source4/ldap_server/ldap_server.c b/source4/ldap_server/ldap_server.c index b5251e3623e..bc2f54bc146 100644 --- a/source4/ldap_server/ldap_server.c +++ b/source4/ldap_server/ldap_server.c @@ -690,13 +690,13 @@ static void ldapsrv_call_writev_start(struct ldapsrv_call *call) ret = data_blob_append(call, &blob, b.data, b.length); data_blob_free(&b); - talloc_set_name_const(blob.data, "Outgoing, encoded LDAP packet"); - if (!ret) { ldapsrv_terminate_connection(conn, "data_blob_append failed"); return; } + talloc_set_name_const(blob.data, "Outgoing, encoded LDAP packet"); + DLIST_REMOVE(call->replies, call->replies); } -- 2.17.1 From 4aabfecd290cd2769376abf7f170e832becc4112 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Tue, 6 Nov 2018 13:32:05 +1300 Subject: [PATCH 08/17] CVE-2018-16853 build: The Samba AD DC, when build with MIT Kerberos is experimental This matches https://wiki.samba.org/index.php/Running_a_Samba_AD_DC_with_MIT_Kerberos_KDC BUG: https://bugzilla.samba.org/show_bug.cgi?id=13678 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer --- wscript | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/wscript b/wscript index 19fc6d12118..7c265e7befb 100644 --- a/wscript +++ b/wscript @@ -56,6 +56,14 @@ def set_options(opt): help='build Samba with system MIT Kerberos. ' + 'You may specify list of paths where Kerberos is installed (e.g. /usr/local /usr/kerberos) to search krb5-config', action='callback', callback=system_mitkrb5_callback, dest='with_system_mitkrb5', default=False) + + opt.add_option('--with-experimental-mit-ad-dc', + help='Enable the experimental MIT Kerberos-backed AD DC. ' + + 'Note that security patches are not issued for this configuration', + action='store_true', + dest='with_experimental_mit_ad_dc', + default=False) + opt.add_option('--with-system-mitkdc', help=('Specify the path to the krb5kdc binary from MIT Kerberos'), type="string", @@ -210,7 +218,16 @@ def configure(conf): conf.DEFINE('AD_DC_BUILD_IS_ENABLED', 1) if Options.options.with_system_mitkrb5: + if not Options.options.with_experimental_mit_ad_dc and \ + not Options.options.without_ad_dc: + raise Utils.WafError('The MIT Kerberos build of Samba as an AD DC ' + + 'is experimental. Therefore ' + '--with-system-mitkrb5 requires either ' + + '--with-experimental-mit-ad-dc or ' + + '--without-ad-dc') + conf.PROCESS_SEPARATE_RULE('system_mitkrb5') + if not (Options.options.without_ad_dc or Options.options.with_system_mitkrb5): conf.DEFINE('AD_DC_BUILD_IS_ENABLED', 1) -- 2.17.1 From 862d4909eccd18942e3de8e8b0dc6e1594ec27f1 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Sun, 2 Sep 2018 17:34:03 +1200 Subject: [PATCH 09/17] CVE-2018-16857 selftest: Prepare to allow override of lockout duration in password_lockout tests This will make it easier to avoid flapping tests. Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer (cherry picked from commit a740a6131c967f9640b19a6964fd5d6f85ce853a) Backported as a dependency for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 --- source4/dsdb/tests/python/password_lockout.py | 9 ++++----- source4/dsdb/tests/python/password_lockout_base.py | 11 +++++++++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/source4/dsdb/tests/python/password_lockout.py b/source4/dsdb/tests/python/password_lockout.py index ec6cf13fe66..e817e656a2a 100755 --- a/source4/dsdb/tests/python/password_lockout.py +++ b/source4/dsdb/tests/python/password_lockout.py @@ -616,15 +616,14 @@ userPassword: thatsAcomplPASS2XYZ initial_lastlogon_relation='greater') def use_pso_lockout_settings(self, creds): + # create a PSO with the lockout settings the test cases normally expect + # + # Some test cases sleep() for self.account_lockout_duration pso = PasswordSettings("lockout-PSO", self.ldb, lockout_attempts=3, - lockout_duration=3) + lockout_duration=self.account_lockout_duration) self.addCleanup(self.ldb.delete, pso.dn) - # the test cases should sleep() for the PSO's lockoutDuration/obsvWindow - self.account_lockout_duration = 3 - self.lockout_observation_window = 3 - userdn = "cn=%s,cn=users,%s" % (creds.get_username(), self.base_dn) pso.apply_to(userdn) diff --git a/source4/dsdb/tests/python/password_lockout_base.py b/source4/dsdb/tests/python/password_lockout_base.py index 4a320684702..9d82e088bb8 100644 --- a/source4/dsdb/tests/python/password_lockout_base.py +++ b/source4/dsdb/tests/python/password_lockout_base.py @@ -323,8 +323,15 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ """) self.base_dn = self.ldb.domain_dn() - self.account_lockout_duration = 3 - self.lockout_observation_window = 3 + + # + # Some test cases sleep() for self.account_lockout_duration + # so allow it to be controlled via the subclass + # + if not hasattr(self, 'account_lockout_duration'): + self.account_lockout_duration = 3 + if not hasattr(self, 'lockout_observation_window'): + self.lockout_observation_window = 3 self.update_lockout_settings(threshold=3, duration=self.account_lockout_duration, observation_window=self.lockout_observation_window) -- 2.17.1 From 31198d39a76474d55c3d391e04d76758ee115d8e Mon Sep 17 00:00:00 2001 From: Joe Guo Date: Mon, 30 Jul 2018 18:21:29 +1200 Subject: [PATCH 10/17] CVE-2018-16857 PEP8: fix E305: expected 2 blank lines after class or function definition, found 1 Signed-off-by: Joe Guo Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall Partial backport of commit 115f2a71b88 (only password_lockout.py change) as a dependency for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 --- source4/dsdb/tests/python/password_lockout.py | 1 + 1 file changed, 1 insertion(+) diff --git a/source4/dsdb/tests/python/password_lockout.py b/source4/dsdb/tests/python/password_lockout.py index e817e656a2a..d8710866f39 100755 --- a/source4/dsdb/tests/python/password_lockout.py +++ b/source4/dsdb/tests/python/password_lockout.py @@ -1400,6 +1400,7 @@ userPassword: """ + userpass + """ self._test_samr_password_change(self.lockout1ntlm_creds, other_creds=self.lockout2ntlm_creds) + host_url = "ldap://%s" % host TestProgram(module=__name__, opts=subunitopts) -- 2.17.1 From 4d0fd1a421ad4a3ca19ed954ee91fcc36413b017 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Sun, 2 Sep 2018 18:03:06 +1200 Subject: [PATCH 11/17] CVE-2018-16857 selftest: Split up password_lockout into tests with and without a call to sleep() This means we can have a long observation window for many of the tests and so make them much more reliable. Many of these cause frustrating flapping failures in our CI systems. Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer Autobuild-User(master): Andrew Bartlett Autobuild-Date(master): Mon Sep 3 06:14:55 CEST 2018 on sn-devel-144 (cherry picked from commit 74357bf347348d3a8b7483c58e5250e98f7e8810) Backported as a dependency for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 --- source4/dsdb/tests/python/password_lockout.py | 299 +++++++++--------- 1 file changed, 157 insertions(+), 142 deletions(-) diff --git a/source4/dsdb/tests/python/password_lockout.py b/source4/dsdb/tests/python/password_lockout.py index d8710866f39..0022dee21ba 100755 --- a/source4/dsdb/tests/python/password_lockout.py +++ b/source4/dsdb/tests/python/password_lockout.py @@ -88,6 +88,42 @@ class PasswordTests(password_lockout_base.BasePasswordTestCase): self.lockout2ntlm_ldb = self._readd_user(self.lockout2ntlm_creds, lockOutObservationWindow=self.lockout_observation_window) + + def use_pso_lockout_settings(self, creds): + + # create a PSO with the lockout settings the test cases normally expect + # + # Some test cases sleep() for self.account_lockout_duration + pso = PasswordSettings("lockout-PSO", self.ldb, lockout_attempts=3, + lockout_duration=self.account_lockout_duration) + self.addCleanup(self.ldb.delete, pso.dn) + + userdn = "cn=%s,cn=users,%s" % (creds.get_username(), self.base_dn) + pso.apply_to(userdn) + + # update the global lockout settings to be wildly different to what + # the test cases normally expect + self.update_lockout_settings(threshold=10, duration=600, + observation_window=600) + + def _reset_samr(self, res): + + # Now reset the lockout, by removing ACB_AUTOLOCK (which removes the lock, despite being a generated attribute) + samr_user = self._open_samr_user(res) + acb_info = self.samr.QueryUserInfo(samr_user, 16) + acb_info.acct_flags &= ~samr.ACB_AUTOLOCK + self.samr.SetUserInfo(samr_user, 16, acb_info) + self.samr.Close(samr_user) + + +class PasswordTestsWithoutSleep(PasswordTests): + def setUp(self): + # The tests in this class do not sleep, so we can have a + # longer window and not flap on slower hosts + self.account_lockout_duration = 30 + self.lockout_observation_window = 30 + super(PasswordTestsWithoutSleep, self).setUp() + def _reset_ldap_lockoutTime(self, res): self.ldb.modify_ldif(""" dn: """ + str(res[0].dn) + """ @@ -615,22 +651,130 @@ userPassword: thatsAcomplPASS2XYZ "samr", initial_lastlogon_relation='greater') - def use_pso_lockout_settings(self, creds): + def test_multiple_logon_krb5(self): + self._test_multiple_logon(self.lockout1krb5_creds) - # create a PSO with the lockout settings the test cases normally expect - # - # Some test cases sleep() for self.account_lockout_duration - pso = PasswordSettings("lockout-PSO", self.ldb, lockout_attempts=3, - lockout_duration=self.account_lockout_duration) - self.addCleanup(self.ldb.delete, pso.dn) + def test_multiple_logon_ntlm(self): + self._test_multiple_logon(self.lockout1ntlm_creds) - userdn = "cn=%s,cn=users,%s" % (creds.get_username(), self.base_dn) - pso.apply_to(userdn) + def _test_samr_password_change(self, creds, other_creds, lockout_threshold=3): + """Tests user lockout by using bad password in SAMR password_change""" - # update the global lockout settings to be wildly different to what - # the test cases normally expect - self.update_lockout_settings(threshold=10, duration=600, - observation_window=600) + # create a connection for SAMR using another user's credentials + lp = self.get_loadparm() + net = Net(other_creds, lp, server=self.host) + + # work out the initial account values for this user + username = creds.get_username() + userdn = "cn=%s,cn=users,%s" % (username, self.base_dn) + res = self._check_account(userdn, + badPwdCount=0, + badPasswordTime=("greater", 0), + badPwdCountOnly=True) + badPasswordTime = int(res[0]["badPasswordTime"][0]) + logonCount = int(res[0]["logonCount"][0]) + lastLogon = int(res[0]["lastLogon"][0]) + lastLogonTimestamp = int(res[0]["lastLogonTimestamp"][0]) + + # prove we can change the user password (using the correct password) + new_password = "thatsAcomplPASS2" + net.change_password(newpassword=new_password.encode('utf-8'), + username=username, + oldpassword=creds.get_password()) + creds.set_password(new_password) + + # try entering 'x' many bad passwords in a row to lock the user out + new_password = "thatsAcomplPASS3" + for i in range(lockout_threshold): + badPwdCount = i + 1 + try: + print("Trying bad password, attempt #%u" % badPwdCount) + net.change_password(newpassword=new_password.encode('utf-8'), + username=creds.get_username(), + oldpassword="bad-password") + self.fail("Invalid SAMR change_password accepted") + except NTSTATUSError as e: + enum = ctypes.c_uint32(e[0]).value + self.assertEquals(enum, ntstatus.NT_STATUS_WRONG_PASSWORD) + + # check the status of the account is updated after each bad attempt + account_flags = 0 + lockoutTime = None + if badPwdCount >= lockout_threshold: + account_flags = dsdb.UF_LOCKOUT + lockoutTime = ("greater", badPasswordTime) + + res = self._check_account(userdn, + badPwdCount=badPwdCount, + badPasswordTime=("greater", badPasswordTime), + logonCount=logonCount, + lastLogon=lastLogon, + lastLogonTimestamp=lastLogonTimestamp, + lockoutTime=lockoutTime, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, + msDSUserAccountControlComputed=account_flags) + badPasswordTime = int(res[0]["badPasswordTime"][0]) + + # the user is now locked out + lockoutTime = int(res[0]["lockoutTime"][0]) + + # check the user remains locked out regardless of whether they use a + # good or a bad password now + for password in (creds.get_password(), "bad-password"): + try: + print("Trying password %s" % password) + net.change_password(newpassword=new_password.encode('utf-8'), + username=creds.get_username(), + oldpassword=password) + self.fail("Invalid SAMR change_password accepted") + except NTSTATUSError as e: + enum = ctypes.c_uint32(e[0]).value + self.assertEquals(enum, ntstatus.NT_STATUS_ACCOUNT_LOCKED_OUT) + + res = self._check_account(userdn, + badPwdCount=lockout_threshold, + badPasswordTime=badPasswordTime, + logonCount=logonCount, + lastLogon=lastLogon, + lastLogonTimestamp=lastLogonTimestamp, + lockoutTime=lockoutTime, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, + msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) + + # reset the user account lockout + self._reset_samr(res) + + # check bad password counts are reset + res = self._check_account(userdn, + badPwdCount=0, + badPasswordTime=badPasswordTime, + logonCount=logonCount, + lockoutTime=0, + lastLogon=lastLogon, + lastLogonTimestamp=lastLogonTimestamp, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, + msDSUserAccountControlComputed=0) + + # check we can change the user password successfully now + net.change_password(newpassword=new_password.encode('utf-8'), + username=username, + oldpassword=creds.get_password()) + creds.set_password(new_password) + + def test_samr_change_password(self): + self._test_samr_password_change(self.lockout1ntlm_creds, + other_creds=self.lockout2ntlm_creds) + + # same as above, but use a PSO to enforce the lockout + def test_pso_samr_change_password(self): + self.use_pso_lockout_settings(self.lockout1ntlm_creds) + self._test_samr_password_change(self.lockout1ntlm_creds, + other_creds=self.lockout2ntlm_creds) + + +class PasswordTestsWithSleep(PasswordTests): + def setUp(self): + super(PasswordTestsWithSleep, self).setUp() def _test_unicodePwd_lockout_with_clear_change(self, creds, other_ldb, initial_logoncount_relation=None): @@ -1065,12 +1209,6 @@ unicodePwd:: """ + base64.b64encode(new_utf16).decode('utf8') + """ self.use_pso_lockout_settings(self.lockout1ntlm_creds) self._test_login_lockout(self.lockout1ntlm_creds) - def test_multiple_logon_krb5(self): - self._test_multiple_logon(self.lockout1krb5_creds) - - def test_multiple_logon_ntlm(self): - self._test_multiple_logon(self.lockout1ntlm_creds) - def _testing_add_user(self, creds, lockOutObservationWindow=0): username = creds.get_username() userpass = creds.get_password() @@ -1251,15 +1389,6 @@ userPassword: """ + userpass + """ msDSUserAccountControlComputed=0) return ldb - def _reset_samr(self, res): - - # Now reset the lockout, by removing ACB_AUTOLOCK (which removes the lock, despite being a generated attribute) - samr_user = self._open_samr_user(res) - acb_info = self.samr.QueryUserInfo(samr_user, 16) - acb_info.acct_flags &= ~samr.ACB_AUTOLOCK - self.samr.SetUserInfo(samr_user, 16, acb_info) - self.samr.Close(samr_user) - def test_lockout_observation_window(self): lockout3krb5_creds = self.insta_creds(self.template_creds, username="lockout3krb5", @@ -1286,120 +1415,6 @@ userPassword: """ + userpass + """ self._testing_add_user(lockout4ntlm_creds, lockOutObservationWindow=self.lockout_observation_window) - def _test_samr_password_change(self, creds, other_creds, lockout_threshold=3): - """Tests user lockout by using bad password in SAMR password_change""" - - # create a connection for SAMR using another user's credentials - lp = self.get_loadparm() - net = Net(other_creds, lp, server=self.host) - - # work out the initial account values for this user - username = creds.get_username() - userdn = "cn=%s,cn=users,%s" % (username, self.base_dn) - res = self._check_account(userdn, - badPwdCount=0, - badPasswordTime=("greater", 0), - badPwdCountOnly=True) - badPasswordTime = int(res[0]["badPasswordTime"][0]) - logonCount = int(res[0]["logonCount"][0]) - lastLogon = int(res[0]["lastLogon"][0]) - lastLogonTimestamp = int(res[0]["lastLogonTimestamp"][0]) - - # prove we can change the user password (using the correct password) - new_password = "thatsAcomplPASS2" - net.change_password(newpassword=new_password.encode('utf-8'), - username=username, - oldpassword=creds.get_password()) - creds.set_password(new_password) - - # try entering 'x' many bad passwords in a row to lock the user out - new_password = "thatsAcomplPASS3" - for i in range(lockout_threshold): - badPwdCount = i + 1 - try: - print("Trying bad password, attempt #%u" % badPwdCount) - net.change_password(newpassword=new_password.encode('utf-8'), - username=creds.get_username(), - oldpassword="bad-password") - self.fail("Invalid SAMR change_password accepted") - except NTSTATUSError as e: - enum = ctypes.c_uint32(e[0]).value - self.assertEquals(enum, ntstatus.NT_STATUS_WRONG_PASSWORD) - - # check the status of the account is updated after each bad attempt - account_flags = 0 - lockoutTime = None - if badPwdCount >= lockout_threshold: - account_flags = dsdb.UF_LOCKOUT - lockoutTime = ("greater", badPasswordTime) - - res = self._check_account(userdn, - badPwdCount=badPwdCount, - badPasswordTime=("greater", badPasswordTime), - logonCount=logonCount, - lastLogon=lastLogon, - lastLogonTimestamp=lastLogonTimestamp, - lockoutTime=lockoutTime, - userAccountControl=dsdb.UF_NORMAL_ACCOUNT, - msDSUserAccountControlComputed=account_flags) - badPasswordTime = int(res[0]["badPasswordTime"][0]) - - # the user is now locked out - lockoutTime = int(res[0]["lockoutTime"][0]) - - # check the user remains locked out regardless of whether they use a - # good or a bad password now - for password in (creds.get_password(), "bad-password"): - try: - print("Trying password %s" % password) - net.change_password(newpassword=new_password.encode('utf-8'), - username=creds.get_username(), - oldpassword=password) - self.fail("Invalid SAMR change_password accepted") - except NTSTATUSError as e: - enum = ctypes.c_uint32(e[0]).value - self.assertEquals(enum, ntstatus.NT_STATUS_ACCOUNT_LOCKED_OUT) - - res = self._check_account(userdn, - badPwdCount=lockout_threshold, - badPasswordTime=badPasswordTime, - logonCount=logonCount, - lastLogon=lastLogon, - lastLogonTimestamp=lastLogonTimestamp, - lockoutTime=lockoutTime, - userAccountControl=dsdb.UF_NORMAL_ACCOUNT, - msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) - - # reset the user account lockout - self._reset_samr(res) - - # check bad password counts are reset - res = self._check_account(userdn, - badPwdCount=0, - badPasswordTime=badPasswordTime, - logonCount=logonCount, - lockoutTime=0, - lastLogon=lastLogon, - lastLogonTimestamp=lastLogonTimestamp, - userAccountControl=dsdb.UF_NORMAL_ACCOUNT, - msDSUserAccountControlComputed=0) - - # check we can change the user password successfully now - net.change_password(newpassword=new_password.encode('utf-8'), - username=username, - oldpassword=creds.get_password()) - creds.set_password(new_password) - - def test_samr_change_password(self): - self._test_samr_password_change(self.lockout1ntlm_creds, - other_creds=self.lockout2ntlm_creds) - - # same as above, but use a PSO to enforce the lockout - def test_pso_samr_change_password(self): - self.use_pso_lockout_settings(self.lockout1ntlm_creds) - self._test_samr_password_change(self.lockout1ntlm_creds, - other_creds=self.lockout2ntlm_creds) - host_url = "ldap://%s" % host -- 2.17.1 From fe8e05a9ea8185325ff87ac73ef0106a85cd662a Mon Sep 17 00:00:00 2001 From: Joe Guo Date: Mon, 30 Jul 2018 18:15:34 +1200 Subject: [PATCH 12/17] CVE-2018-16857 PEP8: fix E127: continuation line over-indented for visual indent Signed-off-by: Joe Guo Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall Partial backport of commit bbb9f57603d (only password_lockout_base.py change) as a dependency for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 --- .../tests/python/password_lockout_base.py | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/source4/dsdb/tests/python/password_lockout_base.py b/source4/dsdb/tests/python/password_lockout_base.py index 9d82e088bb8..1b408c75166 100644 --- a/source4/dsdb/tests/python/password_lockout_base.py +++ b/source4/dsdb/tests/python/password_lockout_base.py @@ -390,7 +390,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=("greater", 0), lastLogonTimestamp=("greater", 0), userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) logonCount = int(res[0]["logonCount"][0]) @@ -421,7 +421,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0, msg='lastlogontimestamp with wrong password') badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -440,7 +440,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=('greater', lastLogon), lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0, msg='LLTimestamp is updated to lastlogon') @@ -461,7 +461,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -483,7 +483,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -508,7 +508,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogonTimestamp=lastLogonTimestamp, lockoutTime=("greater", badPasswordTime), userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) badPasswordTime = int(res[0]["badPasswordTime"][0]) lockoutTime = int(res[0]["lockoutTime"][0]) @@ -530,7 +530,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogonTimestamp=lastLogonTimestamp, lockoutTime=lockoutTime, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) # The wrong password @@ -550,7 +550,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogonTimestamp=lastLogonTimestamp, lockoutTime=lockoutTime, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) # The correct password, but we are locked out @@ -570,7 +570,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogonTimestamp=lastLogonTimestamp, lockoutTime=lockoutTime, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) # wait for the lockout to end @@ -585,7 +585,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) # The correct password after letting the timeout expire @@ -605,7 +605,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogonTimestamp=lastLogonTimestamp, lockoutTime=0, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0, msg="lastLogon is way off") @@ -629,7 +629,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -650,7 +650,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -664,7 +664,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) # The wrong password @@ -684,7 +684,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -700,7 +700,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=("greater", lastLogon), lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) def _test_multiple_logon(self, creds): @@ -734,7 +734,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=("greater", 0), lastLogonTimestamp=("greater", 0), userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) logonCount = int(res[0]["logonCount"][0]) @@ -774,5 +774,5 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=(lastlogon_relation, lastLogon), lastLogonTimestamp=lastLogonTimestamp, userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) -- 2.17.1 From 9cb6b4e9131afac71a39a2f6a3c142723cb6ca19 Mon Sep 17 00:00:00 2001 From: Joe Guo Date: Mon, 30 Jul 2018 18:19:21 +1200 Subject: [PATCH 13/17] CVE-2018-16857 PEP8: fix E251: unexpected spaces around keyword / parameter equals Signed-off-by: Joe Guo Reviewed-by: Andrew Bartlett Reviewed-by: Douglas Bagnall Partial backport of commit 1ccc36b4010cd63 (only password_lockout_base.py change) as a dependency for: BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 --- .../tests/python/password_lockout_base.py | 60 +++++++------------ 1 file changed, 20 insertions(+), 40 deletions(-) diff --git a/source4/dsdb/tests/python/password_lockout_base.py b/source4/dsdb/tests/python/password_lockout_base.py index 1b408c75166..48a74018624 100644 --- a/source4/dsdb/tests/python/password_lockout_base.py +++ b/source4/dsdb/tests/python/password_lockout_base.py @@ -93,8 +93,7 @@ class BasePasswordTestCase(PasswordTestCase): logonCount=0, lastLogon=0, lastLogonTimestamp=("absent", None), - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) def _check_account(self, dn, @@ -389,8 +388,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ logonCount=(logoncount_relation, 0), lastLogon=("greater", 0), lastLogonTimestamp=("greater", 0), - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) logonCount = int(res[0]["logonCount"][0]) @@ -420,8 +418,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ logonCount=logonCount, lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0, msg='lastlogontimestamp with wrong password') badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -439,8 +436,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ logonCount=(logoncount_relation, logonCount), lastLogon=('greater', lastLogon), lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0, msg='LLTimestamp is updated to lastlogon') @@ -460,8 +456,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ logonCount=logonCount, lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -482,8 +477,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ logonCount=logonCount, lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -507,8 +501,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, lockoutTime=("greater", badPasswordTime), - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) badPasswordTime = int(res[0]["badPasswordTime"][0]) lockoutTime = int(res[0]["lockoutTime"][0]) @@ -529,8 +522,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, lockoutTime=lockoutTime, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) # The wrong password @@ -549,8 +541,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, lockoutTime=lockoutTime, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) # The correct password, but we are locked out @@ -569,8 +560,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, lockoutTime=lockoutTime, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) # wait for the lockout to end @@ -584,8 +574,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lockoutTime=lockoutTime, lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) # The correct password after letting the timeout expire @@ -604,8 +593,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lastLogon=(lastlogon_relation, lastLogon), lastLogonTimestamp=lastLogonTimestamp, lockoutTime=0, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0, msg="lastLogon is way off") @@ -628,8 +616,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lockoutTime=0, lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -649,8 +636,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lockoutTime=0, lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -663,8 +649,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lockoutTime=0, lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) # The wrong password @@ -683,8 +668,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lockoutTime=0, lastLogon=lastLogon, lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) @@ -699,8 +683,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ lockoutTime=0, lastLogon=("greater", lastLogon), lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) def _test_multiple_logon(self, creds): @@ -733,8 +716,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ logonCount=(logoncount_relation, 0), lastLogon=("greater", 0), lastLogonTimestamp=("greater", 0), - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) badPasswordTime = int(res[0]["badPasswordTime"][0]) logonCount = int(res[0]["logonCount"][0]) @@ -754,8 +736,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ logonCount=(logoncount_relation, logonCount), lastLogon=(lastlogon_relation, lastLogon), lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0, msg=("second logon, firstlogon was %s" % firstLogon)) @@ -773,6 +754,5 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ logonCount=(logoncount_relation, logonCount), lastLogon=(lastlogon_relation, lastLogon), lastLogonTimestamp=lastLogonTimestamp, - userAccountControl= - dsdb.UF_NORMAL_ACCOUNT, + userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=0) -- 2.17.1 From ec9cc4ed5a05490297cde3fcaac50eeeaaca8469 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Tue, 13 Nov 2018 11:49:56 +1300 Subject: [PATCH 14/17] CVE-2018-16857 tests: Sanity-check password lockout works with default values Sanity-check that when we use the default lockOutObservationWindow that user lockout actually works. The easiest way to do this is to reuse the _test_login_lockout() test-case, but stop at the point where we wait for the lockout duration to expire (because we don't want the test to wait 30 mins). This highlights a problem currently where the default values don't work. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/password_lockout | 4 +++ source4/dsdb/tests/python/password_lockout.py | 30 +++++++++++++++++++ .../tests/python/password_lockout_base.py | 6 +++- 3 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 selftest/knownfail.d/password_lockout diff --git a/selftest/knownfail.d/password_lockout b/selftest/knownfail.d/password_lockout new file mode 100644 index 00000000000..305bcbdef25 --- /dev/null +++ b/selftest/knownfail.d/password_lockout @@ -0,0 +1,4 @@ +samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_krb5\(ad_dc_ntvfs\) +samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_ntlm\(ad_dc_ntvfs\) +samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_login_lockout_ntlm\(ad_dc_ntvfs\) +samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_login_lockout_krb5\(ad_dc_ntvfs\) diff --git a/source4/dsdb/tests/python/password_lockout.py b/source4/dsdb/tests/python/password_lockout.py index 0022dee21ba..b09a732e179 100755 --- a/source4/dsdb/tests/python/password_lockout.py +++ b/source4/dsdb/tests/python/password_lockout.py @@ -1415,6 +1415,36 @@ userPassword: """ + userpass + """ self._testing_add_user(lockout4ntlm_creds, lockOutObservationWindow=self.lockout_observation_window) +class PasswordTestsWithDefaults(PasswordTests): + def setUp(self): + # The tests in this class do not sleep, so we can use the default + # timeout windows here + self.account_lockout_duration = 30 * 60 + self.lockout_observation_window = 30 * 60 + super(PasswordTestsWithDefaults, self).setUp() + + # sanity-check that user lockout works with the default settings (we just + # check the user is locked out - we don't wait for the lockout to expire) + def test_login_lockout_krb5(self): + self._test_login_lockout(self.lockout1krb5_creds, + wait_lockout_duration=False) + + def test_login_lockout_ntlm(self): + self._test_login_lockout(self.lockout1ntlm_creds, + wait_lockout_duration=False) + + # Repeat the login lockout tests using PSOs + def test_pso_login_lockout_krb5(self): + """Check the PSO lockout settings get applied to the user correctly""" + self.use_pso_lockout_settings(self.lockout1krb5_creds) + self._test_login_lockout(self.lockout1krb5_creds, + wait_lockout_duration=False) + + def test_pso_login_lockout_ntlm(self): + """Check the PSO lockout settings get applied to the user correctly""" + self.use_pso_lockout_settings(self.lockout1ntlm_creds) + self._test_login_lockout(self.lockout1ntlm_creds, + wait_lockout_duration=False) host_url = "ldap://%s" % host diff --git a/source4/dsdb/tests/python/password_lockout_base.py b/source4/dsdb/tests/python/password_lockout_base.py index 48a74018624..e8ac46dcd97 100644 --- a/source4/dsdb/tests/python/password_lockout_base.py +++ b/source4/dsdb/tests/python/password_lockout_base.py @@ -365,7 +365,7 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ def tearDown(self): super(BasePasswordTestCase, self).tearDown() - def _test_login_lockout(self, creds): + def _test_login_lockout(self, creds, wait_lockout_duration=True): username = creds.get_username() userpass = creds.get_password() userdn = "cn=%s,cn=users,%s" % (username, self.base_dn) @@ -563,6 +563,10 @@ lockoutThreshold: """ + str(lockoutThreshold) + """ userAccountControl=dsdb.UF_NORMAL_ACCOUNT, msDSUserAccountControlComputed=dsdb.UF_LOCKOUT) + # if we're just checking the user gets locked out, we can stop here + if not wait_lockout_duration: + return + # wait for the lockout to end time.sleep(self.account_lockout_duration + 1) print(self.account_lockout_duration + 1) -- 2.17.1 From 4f86beeaf3408383385ee99a74520a805dd63c0f Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Tue, 13 Nov 2018 12:24:16 +1300 Subject: [PATCH 15/17] CVE-2018-16857 dsdb/util: Correctly treat lockOutObservationWindow as 64-bit int Commit 442a38c918ae1666b35 refactored some code into a new get_lockout_observation_window() function. However, in moving the code, an ldb_msg_find_attr_as_int64() inadvertently got converted to a ldb_msg_find_attr_as_int(). ldb_msg_find_attr_as_int() will only work for values up to -2147483648 (about 3.5 minutes in MS timestamp form). Unfortunately, the automated tests used a low enough timeout that they still worked, however, password lockout would not work with the Samba default settings. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/password_lockout | 2 -- source4/dsdb/common/util.c | 10 +++++----- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/selftest/knownfail.d/password_lockout b/selftest/knownfail.d/password_lockout index 305bcbdef25..a4e37a84c21 100644 --- a/selftest/knownfail.d/password_lockout +++ b/selftest/knownfail.d/password_lockout @@ -1,4 +1,2 @@ samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_krb5\(ad_dc_ntvfs\) samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_ntlm\(ad_dc_ntvfs\) -samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_login_lockout_ntlm\(ad_dc_ntvfs\) -samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_login_lockout_krb5\(ad_dc_ntvfs\) diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c index 193fa2ae653..438a29e1773 100644 --- a/source4/dsdb/common/util.c +++ b/source4/dsdb/common/util.c @@ -5400,12 +5400,12 @@ static int64_t get_lockout_observation_window(struct ldb_message *domain_msg, struct ldb_message *pso_msg) { if (pso_msg != NULL) { - return ldb_msg_find_attr_as_int(pso_msg, - "msDS-LockoutObservationWindow", - 0); + return ldb_msg_find_attr_as_int64(pso_msg, + "msDS-LockoutObservationWindow", + 0); } else { - return ldb_msg_find_attr_as_int(domain_msg, - "lockOutObservationWindow", 0); + return ldb_msg_find_attr_as_int64(domain_msg, + "lockOutObservationWindow", 0); } } -- 2.17.1 From d12b02c78842786969557b9be7c953e9594d90dd Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Tue, 13 Nov 2018 13:19:04 +1300 Subject: [PATCH 16/17] CVE-2018-16857 dsdb/util: Fix lockOutObservationWindow for PSOs Fix a remaining place where we were trying to read the msDS-LockoutObservationWindow as an int instead of an int64. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/password_lockout | 2 -- source4/dsdb/common/util.c | 6 +++--- 2 files changed, 3 insertions(+), 5 deletions(-) delete mode 100644 selftest/knownfail.d/password_lockout diff --git a/selftest/knownfail.d/password_lockout b/selftest/knownfail.d/password_lockout deleted file mode 100644 index a4e37a84c21..00000000000 --- a/selftest/knownfail.d/password_lockout +++ /dev/null @@ -1,2 +0,0 @@ -samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_krb5\(ad_dc_ntvfs\) -samba4.ldap.password_lockout.python\(ad_dc_ntvfs\).__main__.PasswordTestsWithDefaults.test_pso_login_lockout_ntlm\(ad_dc_ntvfs\) diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c index 438a29e1773..8d863f85a29 100644 --- a/source4/dsdb/common/util.c +++ b/source4/dsdb/common/util.c @@ -5361,9 +5361,9 @@ int samdb_result_effective_badPwdCount(struct ldb_context *sam_ldb, if (res != NULL) { lockOutObservationWindow = - ldb_msg_find_attr_as_int(res->msgs[0], - "msDS-LockoutObservationWindow", - 0); + ldb_msg_find_attr_as_int64(res->msgs[0], + "msDS-LockoutObservationWindow", + 0); talloc_free(res); } else { -- 2.17.1 From 60b2cd50f4d0554cc5ca8c53b2d1fa89e56a6d06 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Tue, 13 Nov 2018 13:22:41 +1300 Subject: [PATCH 17/17] CVE-2018-16857 dsdb/util: Add better default lockOutObservationWindow Clearly the lockOutObservationWindow value is important, and using a default value of zero doesn't work very well. This patch adds a better default value (the domain default setting of 30 minutes). BUG: https://bugzilla.samba.org/show_bug.cgi?id=13683 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett --- source4/dsdb/common/util.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/source4/dsdb/common/util.c b/source4/dsdb/common/util.c index 8d863f85a29..18f700370a3 100644 --- a/source4/dsdb/common/util.c +++ b/source4/dsdb/common/util.c @@ -56,6 +56,9 @@ */ #include "dsdb/samdb/ldb_modules/util.h" +/* default is 30 minutes: -1e7 * 30 * 60 */ +#define DEFAULT_OBSERVATION_WINDOW -18000000000 + /* search the sam for the specified attributes in a specific domain, filter on objectSid being in domain_sid. @@ -5363,7 +5366,7 @@ int samdb_result_effective_badPwdCount(struct ldb_context *sam_ldb, lockOutObservationWindow = ldb_msg_find_attr_as_int64(res->msgs[0], "msDS-LockoutObservationWindow", - 0); + DEFAULT_OBSERVATION_WINDOW); talloc_free(res); } else { @@ -5402,10 +5405,11 @@ static int64_t get_lockout_observation_window(struct ldb_message *domain_msg, if (pso_msg != NULL) { return ldb_msg_find_attr_as_int64(pso_msg, "msDS-LockoutObservationWindow", - 0); + DEFAULT_OBSERVATION_WINDOW); } else { return ldb_msg_find_attr_as_int64(domain_msg, - "lockOutObservationWindow", 0); + "lockOutObservationWindow", + DEFAULT_OBSERVATION_WINDOW); } } -- 2.17.1