From cc0d1c85c36c866272605080d59fc46cb244aa2b Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Fri, 15 Mar 2019 15:20:21 +1300 Subject: [PATCH 1/5] CVE-2019-3870 tests: Extend smbd tests to check for umask being overwritten The smbd changes the umask - if the code fails to restore the umask to what it was, then this is very bad. Add an extra check to every smbd-related test that the umask at the end of the test is the same as what it was at the beginning (i.e. if the smbd code changed the umask then it correctly restored the value afterwards). As the selftest sets the umask for all tests to zero, it makes it hard to detect this problem, so the test setUp() needs to set it to something else first. This extra checking is added to the setUp()/tearDown() so that it applies to all test-cases. However, any failure that occur with this approach will not be able to be known-failed. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13834 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett --- python/samba/tests/ntacls_backup.py | 5 ++-- python/samba/tests/posixacl.py | 4 ++-- python/samba/tests/smbd_base.py | 48 +++++++++++++++++++++++++++++++++++++ selftest/knownfail.d/umask-leak | 3 +++ 4 files changed, 56 insertions(+), 4 deletions(-) create mode 100644 python/samba/tests/smbd_base.py create mode 100644 selftest/knownfail.d/umask-leak diff --git a/python/samba/tests/ntacls_backup.py b/python/samba/tests/ntacls_backup.py index 03ee821e595..b6689dcd515 100644 --- a/python/samba/tests/ntacls_backup.py +++ b/python/samba/tests/ntacls_backup.py @@ -26,10 +26,11 @@ from samba import ntacls from samba.auth import system_session from samba.dcerpc import security -from samba.tests import TestCaseInTempDir, env_loadparm +from samba.tests import env_loadparm +from samba.tests.smbd_base import SmbdBaseTests -class NtaclsBackupRestoreTests(TestCaseInTempDir): +class NtaclsBackupRestoreTests(SmbdBaseTests): """ Tests for NTACLs backup and restore. """ diff --git a/python/samba/tests/posixacl.py b/python/samba/tests/posixacl.py index a758df9b19e..b52289e63cf 100644 --- a/python/samba/tests/posixacl.py +++ b/python/samba/tests/posixacl.py @@ -20,7 +20,7 @@ from samba.ntacls import setntacl, getntacl, checkset_backend from samba.dcerpc import security, smb_acl, idmap -from samba.tests import TestCaseInTempDir +from samba.tests.smbd_base import SmbdBaseTests from samba import provision import os from samba.samba3 import smbd, passdb @@ -32,7 +32,7 @@ DOM_SID = "S-1-5-21-2212615479-2695158682-2101375467" ACL = "O:S-1-5-21-2212615479-2695158682-2101375467-512G:S-1-5-21-2212615479-2695158682-2101375467-513D:(A;OICI;0x001f01ff;;;S-1-5-21-2212615479-2695158682-2101375467-512)" -class PosixAclMappingTests(TestCaseInTempDir): +class PosixAclMappingTests(SmbdBaseTests): def setUp(self): super(PosixAclMappingTests, self).setUp() diff --git a/python/samba/tests/smbd_base.py b/python/samba/tests/smbd_base.py new file mode 100644 index 00000000000..4e5c3641e2c --- /dev/null +++ b/python/samba/tests/smbd_base.py @@ -0,0 +1,48 @@ +# Unix SMB/CIFS implementation. Common code for smbd python bindings tests +# Copyright (C) Catalyst.Net Ltd 2019 +# +# 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 samba.tests import TestCaseInTempDir +import os + +TEST_UMASK = 0o022 + +class SmbdBaseTests(TestCaseInTempDir): + + def get_umask(self): + # we can only get the umask by setting it to something + curr_umask = os.umask(0) + # restore the old setting + os.umask(curr_umask) + return curr_umask + + def setUp(self): + super(SmbdBaseTests, self).setUp() + self.orig_umask = self.get_umask() + + # set an arbitrary umask - the underlying smbd code should override + # this, but it allows us to check if umask is left unset + os.umask(TEST_UMASK) + + def tearDown(self): + # the current umask should be what we set it to earlier - if it's not, + # it indicates the code has changed it and not restored it + self.assertEqual(self.get_umask(), TEST_UMASK, + "umask unexpectedly overridden by test") + + # restore the original umask value (before we interferred with it) + os.umask(self.orig_umask) + + super(SmbdBaseTests, self).tearDown() diff --git a/selftest/knownfail.d/umask-leak b/selftest/knownfail.d/umask-leak new file mode 100644 index 00000000000..5580beb4b68 --- /dev/null +++ b/selftest/knownfail.d/umask-leak @@ -0,0 +1,3 @@ +^samba.tests.ntacls_backup.samba.tests.ntacls_backup.NtaclsBackupRestoreTests.test_smbd_create_file +^samba.tests.ntacls_backup.samba.tests.ntacls_backup.NtaclsBackupRestoreTests.test_backup_online +^samba.tests.ntacls_backup.samba.tests.ntacls_backup.NtaclsBackupRestoreTests.test_backup_offline -- 2.11.0 From bf42b9afb2a9f113e7ab5910fff827ca298a9f45 Mon Sep 17 00:00:00 2001 From: Tim Beale Date: Fri, 15 Mar 2019 13:52:50 +1300 Subject: [PATCH 2/5] CVE-2019-3870 tests: Add test to check file-permissions are correct after provision This provisions a new DC and checks there are no world-writable files in the new DC's private directory. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13834 Signed-off-by: Tim Beale Reviewed-by: Andrew Bartlett --- selftest/knownfail.d/provision_fileperms | 1 + source4/selftest/tests.py | 1 + source4/setup/tests/provision_fileperms.sh | 71 ++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+) create mode 100644 selftest/knownfail.d/provision_fileperms create mode 100755 source4/setup/tests/provision_fileperms.sh diff --git a/selftest/knownfail.d/provision_fileperms b/selftest/knownfail.d/provision_fileperms new file mode 100644 index 00000000000..88b1585fd19 --- /dev/null +++ b/selftest/knownfail.d/provision_fileperms @@ -0,0 +1 @@ +samba4.blackbox.provision_fileperms.provision-fileperms\(none\) diff --git a/source4/selftest/tests.py b/source4/selftest/tests.py index c6355f3a41f..2c226ce8260 100755 --- a/source4/selftest/tests.py +++ b/source4/selftest/tests.py @@ -959,6 +959,7 @@ plantestsuite_loadlist("samba4.deletetest.python(ad_dc_default)", "ad_dc_default plantestsuite("samba4.blackbox.samba3dump", "none", [os.path.join(samba4srcdir, "selftest/test_samba3dump.sh")]) plantestsuite("samba4.blackbox.upgrade", "none", ["PYTHON=%s" % python, os.path.join(samba4srcdir, "setup/tests/blackbox_s3upgrade.sh"), '$PREFIX/provision']) plantestsuite("samba4.blackbox.provision.py", "none", ["PYTHON=%s" % python, os.path.join(samba4srcdir, "setup/tests/blackbox_provision.sh"), '$PREFIX/provision']) +plantestsuite("samba4.blackbox.provision_fileperms", "none", ["PYTHON=%s" % python, os.path.join(samba4srcdir, "setup/tests/provision_fileperms.sh"), '$PREFIX/provision']) plantestsuite("samba4.blackbox.supported_features", "none", ["PYTHON=%s" % python, os.path.join(samba4srcdir, diff --git a/source4/setup/tests/provision_fileperms.sh b/source4/setup/tests/provision_fileperms.sh new file mode 100755 index 00000000000..0b3ef0321fb --- /dev/null +++ b/source4/setup/tests/provision_fileperms.sh @@ -0,0 +1,71 @@ +#!/bin/sh + +if [ $# -lt 1 ]; then +cat < $SMB_CONF + +# provision a basic DC +testit "basic-provision" $PYTHON $BINDIR/samba-tool domain provision --server-role="dc" --domain=FOO --realm=foo.example.com --targetdir=$TARGET_DIR --configfile=$SMB_CONF + +# check the file permissions in the 'private' directory really are private +testit "provision-fileperms" check_private_file_perms $TARGET_DIR + +rm -rf $TARGET_DIR + +umask $ORIG_UMASK + +exit $failed -- 2.11.0 From e38e2a435d1a9e1bb7e2d2c5a5714c17d5f54236 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 21 Mar 2019 17:21:58 +1300 Subject: [PATCH 3/5] CVE-2019-3870 pysmbd: Include tests to show the outside umask has no impact BUG: https://bugzilla.samba.org/show_bug.cgi?id=13834 Signed-off-by: Andrew Bartlett --- python/samba/tests/ntacls_backup.py | 13 +++++++++++++ python/samba/tests/smbd_base.py | 2 +- selftest/knownfail.d/pymkdir-umask | 1 + 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 selftest/knownfail.d/pymkdir-umask diff --git a/python/samba/tests/ntacls_backup.py b/python/samba/tests/ntacls_backup.py index b6689dcd515..d92299187f3 100644 --- a/python/samba/tests/ntacls_backup.py +++ b/python/samba/tests/ntacls_backup.py @@ -111,6 +111,12 @@ class NtaclsBackupRestoreTests(SmbdBaseTests): dirpath = os.path.join(self.service_root, 'a-dir') smbd.mkdir(dirpath, self.service) + mode = os.stat(dirpath).st_mode + + # This works in conjunction with the TEST_UMASK in smbd_base + # to ensure that permissions are not related to the umask + # but instead the smb.conf settings + self.assertEquals(mode & 0o777, 0o755) self.assertTrue(os.path.isdir(dirpath)) def test_smbd_create_file(self): @@ -122,6 +128,13 @@ class NtaclsBackupRestoreTests(SmbdBaseTests): smbd.create_file(filepath, self.service) self.assertTrue(os.path.isfile(filepath)) + mode = os.stat(filepath).st_mode + + # This works in conjunction with the TEST_UMASK in smbd_base + # to ensure that permissions are not related to the umask + # but instead the smb.conf settings + self.assertEquals(mode & 0o777, 0o644) + # As well as checking that unlink works, this removes the # fake xattrs from the dev/inode based DB smbd.unlink(filepath, self.service) diff --git a/python/samba/tests/smbd_base.py b/python/samba/tests/smbd_base.py index 4e5c3641e2c..b49bcc0828f 100644 --- a/python/samba/tests/smbd_base.py +++ b/python/samba/tests/smbd_base.py @@ -17,7 +17,7 @@ from samba.tests import TestCaseInTempDir import os -TEST_UMASK = 0o022 +TEST_UMASK = 0o042 class SmbdBaseTests(TestCaseInTempDir): diff --git a/selftest/knownfail.d/pymkdir-umask b/selftest/knownfail.d/pymkdir-umask new file mode 100644 index 00000000000..5af01be44e3 --- /dev/null +++ b/selftest/knownfail.d/pymkdir-umask @@ -0,0 +1 @@ +^samba.tests.ntacls_backup.samba.tests.ntacls_backup.NtaclsBackupRestoreTests.test_smbd_mkdir \ No newline at end of file -- 2.11.0 From e8a0215fe393d47797efb054efff4b05c77389d8 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 14 Mar 2019 18:20:06 +1300 Subject: [PATCH 4/5] CVE-2019-3870 pysmbd: Move umask manipuations as close as possible to users Umask manipulation was added to pysmbd with e146fe5ef96c1522175a8e81db15d1e8879e5652 in 2012 and init_files_struct was split out in 747c3f1fb379bb68cc7479501b85741493c05812 in 2018 for Samba 4.9. (It was added to assist the smbd.create_file() routine used in the backup and restore tools, which needed to write files with full metadata). This in turn avoids leaving init_files_struct() without resetting the umask to the original, saved, value. Per umask(2) this is required before open() and mkdir() system calls (along side other file-like things such as those for Unix domain socks and FIFOs etc). Therefore for safety and clarify the additional 'belt and braces' umask manipuations elsewhere are removed. mkdir() will be protected by a umask() bracket, for correctness, in the next patch. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13834 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/provision_fileperms | 1 - selftest/knownfail.d/umask-leak | 3 --- source3/smbd/pysmbd.c | 34 ++++++++++---------------------- 3 files changed, 10 insertions(+), 28 deletions(-) delete mode 100644 selftest/knownfail.d/provision_fileperms delete mode 100644 selftest/knownfail.d/umask-leak diff --git a/selftest/knownfail.d/provision_fileperms b/selftest/knownfail.d/provision_fileperms deleted file mode 100644 index 88b1585fd19..00000000000 --- a/selftest/knownfail.d/provision_fileperms +++ /dev/null @@ -1 +0,0 @@ -samba4.blackbox.provision_fileperms.provision-fileperms\(none\) diff --git a/selftest/knownfail.d/umask-leak b/selftest/knownfail.d/umask-leak deleted file mode 100644 index 5580beb4b68..00000000000 --- a/selftest/knownfail.d/umask-leak +++ /dev/null @@ -1,3 +0,0 @@ -^samba.tests.ntacls_backup.samba.tests.ntacls_backup.NtaclsBackupRestoreTests.test_smbd_create_file -^samba.tests.ntacls_backup.samba.tests.ntacls_backup.NtaclsBackupRestoreTests.test_backup_online -^samba.tests.ntacls_backup.samba.tests.ntacls_backup.NtaclsBackupRestoreTests.test_backup_offline diff --git a/source3/smbd/pysmbd.c b/source3/smbd/pysmbd.c index fd0c9fd46a7..52d49408906 100644 --- a/source3/smbd/pysmbd.c +++ b/source3/smbd/pysmbd.c @@ -88,27 +88,19 @@ static int set_sys_acl_conn(const char *fname, { int ret; struct smb_filename *smb_fname = NULL; - mode_t saved_umask; TALLOC_CTX *frame = talloc_stackframe(); - /* we want total control over the permissions on created files, - so set our umask to 0 */ - saved_umask = umask(0); - smb_fname = synthetic_smb_fname_split(frame, fname, lp_posix_pathnames()); if (smb_fname == NULL) { TALLOC_FREE(frame); - umask(saved_umask); return -1; } ret = SMB_VFS_SYS_ACL_SET_FILE( conn, smb_fname, acltype, theacl); - umask(saved_umask); - TALLOC_FREE(frame); return ret; } @@ -135,23 +127,27 @@ static NTSTATUS init_files_struct(TALLOC_CTX *mem_ctx, } fsp->conn = conn; - /* we want total control over the permissions on created files, - so set our umask to 0 */ - saved_umask = umask(0); - smb_fname = synthetic_smb_fname_split(fsp, fname, lp_posix_pathnames()); if (smb_fname == NULL) { - umask(saved_umask); return NT_STATUS_NO_MEMORY; } fsp->fsp_name = smb_fname; + + /* + * we want total control over the permissions on created files, + * so set our umask to 0 (this matters if flags contains O_CREAT) + */ + saved_umask = umask(0); + fsp->fh->fd = SMB_VFS_OPEN(conn, smb_fname, fsp, flags, 00644); + + umask(saved_umask); + if (fsp->fh->fd == -1) { int err = errno; - umask(saved_umask); if (err == ENOENT) { return NT_STATUS_OBJECT_NAME_NOT_FOUND; } @@ -164,7 +160,6 @@ static NTSTATUS init_files_struct(TALLOC_CTX *mem_ctx, DEBUG(0,("Error doing fstat on open file %s (%s)\n", smb_fname_str_dbg(smb_fname), strerror(errno) )); - umask(saved_umask); return map_nt_error_from_unix(errno); } @@ -454,7 +449,6 @@ static PyObject *py_smbd_chown(PyObject *self, PyObject *args, PyObject *kwargs) char *fname, *service = NULL; int uid, gid; TALLOC_CTX *frame; - mode_t saved_umask; struct smb_filename *smb_fname = NULL; if (!PyArg_ParseTupleAndKeywords(args, kwargs, "sii|z", @@ -470,10 +464,6 @@ static PyObject *py_smbd_chown(PyObject *self, PyObject *args, PyObject *kwargs) return NULL; } - /* we want total control over the permissions on created files, - so set our umask to 0 */ - saved_umask = umask(0); - smb_fname = synthetic_smb_fname(talloc_tos(), fname, NULL, @@ -481,7 +471,6 @@ static PyObject *py_smbd_chown(PyObject *self, PyObject *args, PyObject *kwargs) lp_posix_pathnames() ? SMB_FILENAME_POSIX_PATH : 0); if (smb_fname == NULL) { - umask(saved_umask); TALLOC_FREE(frame); errno = ENOMEM; return PyErr_SetFromErrno(PyExc_OSError); @@ -489,14 +478,11 @@ static PyObject *py_smbd_chown(PyObject *self, PyObject *args, PyObject *kwargs) ret = SMB_VFS_CHOWN(conn, smb_fname, uid, gid); if (ret != 0) { - umask(saved_umask); TALLOC_FREE(frame); errno = ret; return PyErr_SetFromErrno(PyExc_OSError); } - umask(saved_umask); - TALLOC_FREE(frame); Py_RETURN_NONE; -- 2.11.0 From ad2658bdd68ee4f31c112fddf53eee4188f9c9e6 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 21 Mar 2019 17:24:14 +1300 Subject: [PATCH 5/5] CVE-2019-3870 pysmbd: Ensure a zero umask is set for smbd.mkdir() mkdir() is the other call that requires a umask of 0 in Samba. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13834 Signed-off-by: Andrew Bartlett --- selftest/knownfail.d/pymkdir-umask | 1 - source3/smbd/pysmbd.c | 11 ++++++++++- 2 files changed, 10 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/pymkdir-umask diff --git a/selftest/knownfail.d/pymkdir-umask b/selftest/knownfail.d/pymkdir-umask deleted file mode 100644 index 5af01be44e3..00000000000 --- a/selftest/knownfail.d/pymkdir-umask +++ /dev/null @@ -1 +0,0 @@ -^samba.tests.ntacls_backup.samba.tests.ntacls_backup.NtaclsBackupRestoreTests.test_smbd_mkdir \ No newline at end of file diff --git a/source3/smbd/pysmbd.c b/source3/smbd/pysmbd.c index 52d49408906..29db8eb01c4 100644 --- a/source3/smbd/pysmbd.c +++ b/source3/smbd/pysmbd.c @@ -782,6 +782,8 @@ static PyObject *py_smbd_mkdir(PyObject *self, PyObject *args, PyObject *kwargs) TALLOC_CTX *frame = talloc_stackframe(); struct connection_struct *conn = NULL; struct smb_filename *smb_fname = NULL; + int ret; + mode_t saved_umask; if (!PyArg_ParseTupleAndKeywords(args, kwargs, @@ -812,8 +814,15 @@ static PyObject *py_smbd_mkdir(PyObject *self, PyObject *args, PyObject *kwargs) return NULL; } + /* we want total control over the permissions on created files, + so set our umask to 0 */ + saved_umask = umask(0); + + ret = SMB_VFS_MKDIR(conn, smb_fname, 00755); - if (SMB_VFS_MKDIR(conn, smb_fname, 00755) == -1) { + umask(saved_umask); + + if (ret == -1) { DBG_ERR("mkdir error=%d (%s)\n", errno, strerror(errno)); TALLOC_FREE(frame); return NULL; -- 2.11.0