From 1899e16e6f552d0ab504d19f74aa5c42ab84504a Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 14 Mar 2019 18:20:06 +1300 Subject: [PATCH] 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 Reviewed-by: Jeremy Allison --- 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.34.1