Attachment 14530 Details for Bug 13653 – Patch for master.

[patch] Patch for master.

bug13653.patch.txt (text/plain), 18.37 KB, created by Gary Lockyer on 2018-10-16 02:40:27 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Gary Lockyer

Created: 2018-10-16 02:40:27 UTC

Size: 18.37 KB

patch

obsolete

>From 007d0cf8b92a811b5aa94643c59a874225370900 Mon Sep 17 00:00:00 2001
>From: Gary Lockyer <gary@catalyst.net.nz>
>Date: Mon, 15 Oct 2018 16:01:47 +1300
>Subject: [PATCH 1/2] dsdb encrypted_secrets tests: Allow "ldb://" in file path
>
>When creating a new user and specifying the local file path of the
>sam.ldb DB, it's possible to create an account that you can't actually
>login with.
>
>This commit contains tests to verify the bug.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13653
>
>Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> python/samba/tests/blackbox/bug13653.py       | 225 ++++++++++++++++++
> selftest/knownfail.d/bug13653                 |  14 ++
> selftest/knownfail.d/encrypted_secrets        |   6 +
> .../tests/test_encrypted_secrets.c            |   1 -
> source4/dsdb/samdb/ldb_modules/wscript_build  |  17 +-
> source4/selftest/tests.py                     |  21 +-
> 6 files changed, 280 insertions(+), 4 deletions(-)
> create mode 100644 python/samba/tests/blackbox/bug13653.py
> create mode 100644 selftest/knownfail.d/bug13653
>
>diff --git a/python/samba/tests/blackbox/bug13653.py b/python/samba/tests/blackbox/bug13653.py
>new file mode 100644
>index 00000000000..575afe8b006
>--- /dev/null
>+++ b/python/samba/tests/blackbox/bug13653.py
>@@ -0,0 +1,225 @@
>+# Black box tests verify bug 13653
>+#
>+# 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 <http://www.gnu.org/licenses/>.
>+
>+"""Blackbox test verifying bug 13653
>+
>+https://bugzilla.samba.org/show_bug.cgi?id=13653
>+
>+
>+When creating a new user and specifying the local filepath of the sam.ldb DB,
>+it's possible to create an account that you can't actually login with.
>+
>+This only happens if the DB is using encrypted secrets and you specify "ldb://"
>+in the sam.ldb path, e.g. "-H ldb://st/ad_dc/private/sam.ldb".
>+The user account will be created, but its secrets will not be encrypted.
>+Attempts to login as the user will then be rejected due to invalid credentials.
>+
>+We think this may also cause replication/joins to break.
>+
>+You do get a warning about "No encrypted secrets key file" when this happens,
>+although the reason behind this message is not obvious. Specifying a "tdb://"
>+prefix, or not specifying a prefix, works fine.
>+
>+Example of the problem below using the ad_dc testenv.
>+
>+addc$ bin/samba-tool user create tdb-user pass12#
>+      -H tdb://st/ad_dc/private/sam.ldb
>+User 'tdb-user' created successfully
>+
>+# HERE: using the "ldb://" prefix generates a warning, but the user is still
>+# created successfully.
>+
>+addc$ bin/samba-tool user create ldb-user pass12#
>+      -H ldb://st/ad_dc/private/sam.ldb
>+No encrypted secrets key file. Secret attributes will not be encrypted or
>+decrypted
>+
>+User 'ldb-user' created successfully
>+
>+addc$ bin/samba-tool user create noprefix-user pass12#
>+      -H st/ad_dc/private/sam.ldb
>+User 'noprefix-user' created successfully
>+
>+addc$ bin/ldbsearch -H ldap://$SERVER -Utdb-user%pass12# '(cn=tdb-user)' dn
>+# record 1
>+dn: CN=tdb-user,CN=Users,DC=addom,DC=samba,DC=example,DC=com
>+
>+# Referral
>+ref: ldap://addom.samba.example.com/CN=Configuration,DC=addom,DC=samba,
>+     DC=example,DC=com
>+
>+# Referral
>+ref: ldap://addom.samba.example.com/DC=DomainDnsZones,DC=addom,DC=samba,
>+     DC=example,DC=com
>+
>+# Referral
>+ref: ldap://addom.samba.example.com/DC=ForestDnsZones,DC=addom,DC=samba,
>+     DC=example,DC=com
>+
>+# returned 4 records
>+# 1 entries
>+# 3 referrals
>+
>+# HERE: can't login as the user created with "ldb://" prefix
>+
>+addc$ bin/ldbsearch -H ldap://$SERVER -Uldb-user%pass12# '(cn=ldb-user)' dn
>+Wrong username or password: kinit for ldb-user@ADDOM.SAMBA.EXAMPLE.COM failed
>+(Client not found in Kerberos database)
>+
>+Failed to bind - LDAP error 49 LDAP_INVALID_CREDENTIALS
>+               -  <8009030C: LdapErr: DSID-0C0904DC,
>+                    comment: AcceptSecurityContext error, data 54e, v1db1> <>
>+Failed to connect to 'ldap://addc' with backend
>+    'ldap': LDAP error 49 LDAP_INVALID_CREDENTIALS
>+            -  <8009030C: LdapErr: DSID-0C0904DC,
>+               comment: AcceptSecurityContext error, data 54e, v1db1> <>
>+Failed to connect to ldap://addc - LDAP error 49 LDAP_INVALID_CREDENTIALS
>+    -  <8009030C: LdapErr: DSID-0C0904DC,
>+       comment: AcceptSecurityContext error, data 54e, v1db1> <>
>+addc$ bin/ldbsearch -H ldap://$SERVER -Unoprefix-user%pass12#
>+      '(cn=noprefix-user)' dn
>+# record 1
>+dn: CN=noprefix-user,CN=Users,DC=addom,DC=samba,DC=example,DC=com
>+
>+# Referral
>+ref: ldap://addom.samba.example.com/CN=Configuration,DC=addom,DC=samba,
>+    DC=example,DC=com
>+
>+# Referral
>+ref: ldap://addom.samba.example.com/DC=DomainDnsZones,DC=addom,DC=samba,
>+     DC=example,DC=com
>+
>+# Referral
>+ref: ldap://addom.samba.example.com/DC=ForestDnsZones,DC=addom,DC=samba,
>+     DC=example,DC=com
>+
>+# returned 4 records
>+# 1 entries
>+# 3 referrals
>+"""
>+
>+from samba.tests import (
>+    BlackboxTestCase,
>+    BlackboxProcessError,
>+    delete_force,
>+    env_loadparm)
>+from samba.credentials import Credentials
>+from samba.samdb import SamDB
>+from samba.auth import system_session
>+import random
>+import string
>+from os import environ
>+
>+
>+class Bug13653Tests(BlackboxTestCase):
>+
>+    # Open a local connection to the SamDB
>+    # and load configuration from the OS environment.
>+    def setUp(self):
>+        super(Bug13653Tests, self).setUp()
>+        self.env = environ["TEST_ENV"]
>+        self.server = environ["SERVER"]
>+        self.prefix = environ["PREFIX_ABS"]
>+        lp = env_loadparm()
>+        creds = Credentials()
>+        session = system_session()
>+        creds.guess(lp)
>+        self.ldb = SamDB(session_info=session,
>+                         credentials=creds,
>+                         lp=lp)
>+
>+    # Delete the user account created by the test case.
>+    # The user name is in self.user
>+    def tearDown(self):
>+        super(Bug13653Tests, self).tearDown()
>+        try:
>+            dn = "CN=%s,CN=Users,%s" % (self.user, self.ldb.domain_dn())
>+            delete_force(self.ldb, dn)
>+        except Exception as e:
>+            # We ignore any exceptions deleting the user in tearDown
>+            # this allows the known fail mechanism to work for this test
>+            # so the test can be committed before the fix.
>+            # otherwise this delete fails with
>+            #   Error(11)  unpacking encrypted secret, data possibly corrupted
>+            #   or altered
>+            pass
>+
>+    # Generate a random password that can be safely  passed on the command line
>+    # i.e. it does not contain any shell meta characters.
>+    def random_password(self, count=32):
>+        password = random.choice(string.ascii_uppercase)
>+        password += random.choice(string.digits)
>+        password += random.choice(string.ascii_lowercase)
>+        password += ''.join(random.choice(string.ascii_uppercase +
>+                    string.ascii_lowercase +
>+                    string.digits) for x in range(count - 3))
>+        return password
>+
>+    # Delete the user account created by the test case.
>+    # The user name is in self.user
>+    def delete_user(self):
>+        dn = "CN=%s,CN=Users,%s" % (self.user, self.ldb.domain_dn())
>+        try:
>+            delete_force(self.ldb, dn)
>+        except Exception as e:
>+            self.fail(str(e))
>+
>+    def _test_scheme(self, scheme):
>+        """Ensure a user can be created by samba-tool with the supplied scheme
>+           and that that user can logon."""
>+
>+        self.delete_user()
>+
>+        password = self.random_password()
>+        db_path = "%s/%s/%s/private/sam.ldb" % (scheme, self.prefix, self.env)
>+        try:
>+            command =\
>+                "bin/samba-tool user create %s %s -H %s" % (
>+                    self.user, password, db_path)
>+            self.check_run(command)
>+            command =\
>+                "bin/ldbsearch -H ldap://%s/ -U%s%%%s '(cn=%s)' dn" % (
>+                    self.server, self.user, password, self.user)
>+            self.check_run(command)
>+        except BlackboxProcessError as e:
>+            self.fail(str(e))
>+
>+    def test_tdb_scheme(self):
>+        """Ensure a user can be created by samba-tool with the "tbd://" scheme
>+           and that that user can logon."""
>+
>+        self.user = "TDB_USER"
>+        self._test_scheme("tdb://")
>+
>+    def test_mdb_scheme(self):
>+        """Ensure a user can be created by samba-tool with the "mdb://" scheme
>+           and that that user can logon.
>+
>+           NOTE: this test is currently in knownfail.d/encrypted_secrets as
>+                 sam.ldb is currently a tdb even if the lmdb backend is
>+                 selected
>+        """
>+
>+        self.user = "MDB_USER"
>+        self._test_scheme("mdb://")
>+
>+    def test_ldb_scheme(self):
>+        """Ensure a user can be created by samba-tool with the "ldb://" scheme
>+           and that that user can logon."""
>+
>+        self.user = "LDB_USER"
>+        self._test_scheme("ldb://")
>diff --git a/selftest/knownfail.d/bug13653 b/selftest/knownfail.d/bug13653
>new file mode 100644
>index 00000000000..14b955de178
>--- /dev/null
>+++ b/selftest/knownfail.d/bug13653
>@@ -0,0 +1,14 @@
>+^samba.tests.blackbox.bug13653.samba.tests.blackbox.bug13653.Bug13653Tests.test_ldb_scheme
>+^samba.tests.blackbox.bug13653.python3.samba.tests.blackbox.bug13653.Bug13653Tests.test_ldb_scheme
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_key_file
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_key_file_short_key
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_key_file_long_key
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_gnutls_value_encryption
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_gnutls_altered_header
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_gnutls_altered_data
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_gnutls_altered_iv
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_samba_value_encryption
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_samba_altered_header
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_samba_altered_data
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_samba_altered_iv
>+^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_message_encryption_decryption
>diff --git a/selftest/knownfail.d/encrypted_secrets b/selftest/knownfail.d/encrypted_secrets
>index e36ff5fb2c2..7475d3faaab 100644
>--- a/selftest/knownfail.d/encrypted_secrets
>+++ b/selftest/knownfail.d/encrypted_secrets
>@@ -8,3 +8,9 @@
> ^samba.tests.encrypted_secrets.samba.tests.encrypted_secrets.EncryptedSecretsTests.test_required_features$fl2000dc:local$
> ^samba.tests.encrypted_secrets.python3.samba.tests.encrypted_secrets.EncryptedSecretsTests.test_encrypted_secrets$fl2000dc:local$
> ^samba.tests.encrypted_secrets.python3.samba.tests.encrypted_secrets.EncryptedSecretsTests.test_required_features$fl2000dc:local$
>+#
>+# The tests for bug 13563 https://bugzilla.samba.org/show_bug.cgi?id=13653
>+# should fail in the mdb case, as sam.ldb is currently a tdb file.
>+#
>+^samba.tests.blackbox.bug13653.python3.samba.tests.blackbox.bug13653.Bug13653Tests.test_mdb_scheme
>+^samba.tests.blackbox.bug13653.samba.tests.blackbox.bug13653.Bug13653Tests.test_mdb_scheme
>diff --git a/source4/dsdb/samdb/ldb_modules/tests/test_encrypted_secrets.c b/source4/dsdb/samdb/ldb_modules/tests/test_encrypted_secrets.c
>index 4aa325635ce..258e1ba829f 100644
>--- a/source4/dsdb/samdb/ldb_modules/tests/test_encrypted_secrets.c
>+++ b/source4/dsdb/samdb/ldb_modules/tests/test_encrypted_secrets.c
>@@ -27,7 +27,6 @@ int ldb_encrypted_secrets_module_init(const char *version);
> #define TEST_ENCRYPTED_SECRETS
> #include "../encrypted_secrets.c"
> 
>-#define TEST_BE "tdb"
> struct ldbtest_ctx {
> 	struct tevent_context *ev;
> 	struct ldb_context *ldb;
>diff --git a/source4/dsdb/samdb/ldb_modules/wscript_build b/source4/dsdb/samdb/ldb_modules/wscript_build
>index 9e0ac281cc6..89629ab1cb8 100644
>--- a/source4/dsdb/samdb/ldb_modules/wscript_build
>+++ b/source4/dsdb/samdb/ldb_modules/wscript_build
>@@ -28,8 +28,9 @@ bld.SAMBA_BINARY('test_unique_object_sids',
>             DSDB_MODULE_HELPERS
>         ''',
>         install=False)
>-bld.SAMBA_BINARY('test_encrypted_secrets',
>+bld.SAMBA_BINARY('test_encrypted_secrets_tdb',
>         source='tests/test_encrypted_secrets.c',
>+        cflags='-DTEST_BE=\"tdb\"',
>         deps='''
>             talloc
>             samba-util
>@@ -40,6 +41,20 @@ bld.SAMBA_BINARY('test_encrypted_secrets',
>             DSDB_MODULE_HELPERS
>         ''',
>         install=False)
>+if conf.env.HAVE_LMDB:
>+    bld.SAMBA_BINARY('test_encrypted_secrets_mdb',
>+            source='tests/test_encrypted_secrets.c',
>+            cflags='-DTEST_BE=\"mdb\"',
>+            deps='''
>+                talloc
>+                samba-util
>+                samdb-common
>+                samdb
>+                cmocka
>+                gnutls
>+                DSDB_MODULE_HELPERS
>+            ''',
>+            install=False)
> 
> if bld.AD_DC_BUILD_IS_ENABLED():
>     bld.PROCESS_SEPARATE_RULE("server")
>diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py
>index 32427954ec6..4c33350321b 100755
>--- a/source4/selftest/tests.py
>+++ b/source4/selftest/tests.py
>@@ -1186,12 +1186,29 @@ for env in ["ad_dc_ntvfs", "ad_dc", "fl2000dc", "fl2003dc", "fl2008r2dc",
>             'renamedc', 'offlinebackupdc', 'labdc']:
>     plantestsuite("samba4.blackbox.dbcheck(%s)" % env, env + ":local", ["PYTHON=%s" % python, os.path.join(bbdir, "dbcheck.sh"), '$PREFIX/provision', configuration])
> 
>+#
>+# Tests to verify bug 13653 https://bugzilla.samba.org/show_bug.cgi?id=13653
>+# ad_dc has an lmdb backend, ad_dc_ntvfs has a tdb backend.
>+#
>+planoldpythontestsuite("ad_dc_ntvfs:local",
>+                       "samba.tests.blackbox.bug13653",
>+                       extra_args=['-U"$USERNAME%$PASSWORD"'],
>+                       environ={'TEST_ENV': 'ad_dc_ntvfs'},
>+                       py3_compatible=True)
>+planoldpythontestsuite("ad_dc:local",
>+                       "samba.tests.blackbox.bug13653",
>+                       extra_args=['-U"$USERNAME%$PASSWORD"'],
>+                       environ={'TEST_ENV': 'ad_dc'},
>+                       py3_compatible=True)
> # cmocka tests not requiring a specific environment
> #
> plantestsuite("samba4.dsdb.samdb.ldb_modules.unique_object_sids", "none",
>               [os.path.join(bindir(), "test_unique_object_sids")])
>-plantestsuite("samba4.dsdb.samdb.ldb_modules.encrypted_secrets", "none",
>-              [os.path.join(bindir(), "test_encrypted_secrets")])
>+plantestsuite("samba4.dsdb.samdb.ldb_modules.encrypted_secrets.tdb", "none",
>+              [os.path.join(bindir(), "test_encrypted_secrets_tdb")])
>+if "HAVE_LMDB" in config_hash:
>+    plantestsuite("samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb", "none",
>+                  [os.path.join(bindir(), "test_encrypted_secrets_mdb")])
> plantestsuite("lib.audit_logging.audit_logging", "none",
>               [os.path.join(bindir(), "audit_logging_test")])
> plantestsuite("lib.audit_logging.audit_logging.errors", "none",
>-- 
>2.17.1
>
>
>From cb10d354dd21686c386b20d04df2404220052d78 Mon Sep 17 00:00:00 2001
>From: Gary Lockyer <gary@catalyst.net.nz>
>Date: Mon, 15 Oct 2018 16:02:40 +1300
>Subject: [PATCH 2/2] dsdb encrypted_secrets: Allow "ldb:// and "mdb://" in
> file path
>
>Correctly handle "ldb://" and "mdb://" schemes in the file path when
>determining the path for the encrypted secrets key file.
>
>When creating a new user and specifying the local file path of the
>sam.ldb DB, it was possible to create an account that you could not
>login with. The path for the key file was incorrectly calculated
>for the "ldb://" and "mdb://" schemes, the scheme was not stripped from
>the path and the subsequent open of the key file failed.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=13653
>
>Signed-off-by: Gary Lockyer <gary@catalyst.net.nz>
>---
> selftest/knownfail.d/bug13653                      | 14 --------------
> source4/dsdb/samdb/ldb_modules/encrypted_secrets.c |  6 ++++++
> 2 files changed, 6 insertions(+), 14 deletions(-)
> delete mode 100644 selftest/knownfail.d/bug13653
>
>diff --git a/selftest/knownfail.d/bug13653 b/selftest/knownfail.d/bug13653
>deleted file mode 100644
>index 14b955de178..00000000000
>--- a/selftest/knownfail.d/bug13653
>+++ /dev/null
>@@ -1,14 +0,0 @@
>-^samba.tests.blackbox.bug13653.samba.tests.blackbox.bug13653.Bug13653Tests.test_ldb_scheme
>-^samba.tests.blackbox.bug13653.python3.samba.tests.blackbox.bug13653.Bug13653Tests.test_ldb_scheme
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_key_file
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_key_file_short_key
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_key_file_long_key
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_gnutls_value_encryption
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_gnutls_altered_header
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_gnutls_altered_data
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_gnutls_altered_iv
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_samba_value_encryption
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_samba_altered_header
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_samba_altered_data
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_samba_altered_iv
>-^samba4.dsdb.samdb.ldb_modules.encrypted_secrets.mdb.test_message_encryption_decryption
>diff --git a/source4/dsdb/samdb/ldb_modules/encrypted_secrets.c b/source4/dsdb/samdb/ldb_modules/encrypted_secrets.c
>index ef69bb0831c..b2df15c08f4 100644
>--- a/source4/dsdb/samdb/ldb_modules/encrypted_secrets.c
>+++ b/source4/dsdb/samdb/ldb_modules/encrypted_secrets.c
>@@ -131,6 +131,12 @@ static const char* get_key_directory(TALLOC_CTX *ctx, struct ldb_context *ldb)
> 	if (strncmp("tdb://", sam_ldb_path, 6) == 0) {
> 		sam_ldb_path += 6;
> 	}
>+	else if (strncmp("ldb://", sam_ldb_path, 6) == 0) {
>+		sam_ldb_path += 6;
>+	}
>+	else if (strncmp("mdb://", sam_ldb_path, 6) == 0) {
>+		sam_ldb_path += 6;
>+	}
> 	private_dir = talloc_strdup(ctx, sam_ldb_path);
> 	if (private_dir == NULL) {
> 		ldb_set_errstring(ldb,
>-- 
>2.17.1
>

Actions: View

Attachments on bug 13653: 14530 | 14533 | 14547