diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 623f38b03ed285..a793d244de9a04 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -2356,6 +2356,10 @@ features: platform-dependent. On some platforms, they are ignored and you should call :func:`chmod` explicitly to set them. + On Windows, a *mode* of ``0o700`` is specifically handled to apply access + control to the new directory such that only the current user and + administrators have access. Other values of *mode* are ignored. + This function can also support :ref:`paths relative to directory descriptors `. @@ -2370,6 +2374,9 @@ features: .. versionchanged:: 3.6 Accepts a :term:`path-like object`. + .. versionchanged:: 3.12.4 + Windows now handles a *mode* of ``0o700``. + .. function:: makedirs(name, mode=0o777, exist_ok=False) diff --git a/Doc/whatsnew/3.12.rst b/Doc/whatsnew/3.12.rst index 71bc89240c73b0..16723964121284 100644 --- a/Doc/whatsnew/3.12.rst +++ b/Doc/whatsnew/3.12.rst @@ -778,6 +778,13 @@ os Both functions may be significantly faster on newer releases of Windows. (Contributed by Steve Dower in :gh:`99726`.) +* As of 3.12.4, :func:`os.mkdir` and :func:`os.makedirs` on Windows + now support passing a *mode* value of ``0o700`` to apply access + control to the new directory. This implicitly affects + :func:`tempfile.mkdtemp` and is a mitigation for :cve:`2024-4030`. + Other values for *mode* continue to be ignored. + (Contributed by Steve Dower in :gh:`118486`.) + os.path ------- @@ -925,6 +932,10 @@ tempfile *delete_on_close* (Contributed by Evgeny Zorin in :gh:`58451`.) * :func:`tempfile.mkdtemp` now always returns an absolute path, even if the argument provided to the *dir* parameter is a relative path. +* As of 3.12.4 on Windows, the default mode ``0o700`` used by + :func:`tempfile.mkdtemp` now limits access to the new directory due to + changes to :func:`os.mkdir`. This is a mitigation for :cve:`2024-4030`. + (Contributed by Steve Dower in :gh:`118486`.) threading --------- diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 7464aaa794941c..a64761c76d62fc 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -1797,6 +1797,25 @@ def test_exist_ok_existing_regular_file(self): self.assertRaises(OSError, os.makedirs, path, exist_ok=True) os.remove(path) + @unittest.skipUnless(os.name == 'nt', "requires Windows") + def test_win32_mkdir_700(self): + base = os_helper.TESTFN + path1 = os.path.join(os_helper.TESTFN, 'dir1') + path2 = os.path.join(os_helper.TESTFN, 'dir2') + # mode=0o700 is special-cased to override ACLs on Windows + # There's no way to know exactly how the ACLs will look, so we'll + # check that they are different from a regularly created directory. + os.mkdir(path1, mode=0o700) + os.mkdir(path2, mode=0o777) + + out1 = subprocess.check_output(["icacls.exe", path1], encoding="oem") + out2 = subprocess.check_output(["icacls.exe", path2], encoding="oem") + os.rmdir(path1) + os.rmdir(path2) + out1 = out1.replace(path1, "") + out2 = out2.replace(path2, "") + self.assertNotEqual(out1, out2) + def tearDown(self): path = os.path.join(os_helper.TESTFN, 'dir1', 'dir2', 'dir3', 'dir4', 'dir5', 'dir6') diff --git a/Lib/test/test_tempfile.py b/Lib/test/test_tempfile.py index b64b6a4f2baeb5..19ddeaa169bf93 100644 --- a/Lib/test/test_tempfile.py +++ b/Lib/test/test_tempfile.py @@ -13,6 +13,7 @@ import weakref import gc import shutil +import subprocess from unittest import mock import unittest @@ -803,6 +804,33 @@ def test_mode(self): finally: os.rmdir(dir) + @unittest.skipUnless(os.name == "nt", "Only on Windows.") + def test_mode_win32(self): + # Use icacls.exe to extract the users with some level of access + # Main thing we are testing is that the BUILTIN\Users group has + # no access. The exact ACL is going to vary based on which user + # is running the test. + dir = self.do_create() + try: + out = subprocess.check_output(["icacls.exe", dir], encoding="oem").casefold() + finally: + os.rmdir(dir) + + dir = dir.casefold() + users = set() + found_user = False + for line in out.strip().splitlines(): + acl = None + # First line of result includes our directory + if line.startswith(dir): + acl = line.removeprefix(dir).strip() + elif line and line[:1].isspace(): + acl = line.strip() + if acl: + users.add(acl.partition(":")[0]) + + self.assertNotIn(r"BUILTIN\Users".casefold(), users) + def test_collision_with_existing_file(self): # mkdtemp tries another name when a file with # the chosen name already exists diff --git a/Misc/NEWS.d/next/Security/2024-05-01-20-57-09.gh-issue-118486.K44KJG.rst b/Misc/NEWS.d/next/Security/2024-05-01-20-57-09.gh-issue-118486.K44KJG.rst new file mode 100644 index 00000000000000..8ac48aac816a60 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2024-05-01-20-57-09.gh-issue-118486.K44KJG.rst @@ -0,0 +1,4 @@ +:func:`os.mkdir` on Windows now accepts *mode* of ``0o700`` to restrict +the new directory to the current user. This fixes :cve:`2024-4030` +affecting :func:`tempfile.mkdtemp` in scenarios where the base temporary +directory is more permissive than the default. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 8fce40bb686783..ebd076b4e44214 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -32,6 +32,8 @@ # include # include // UNLEN # include "osdefs.h" // SEP +# include // SetEntriesInAcl +# include // SDDL_REVISION_1 # if defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM) # define HAVE_SYMLINK # endif /* MS_WINDOWS_DESKTOP | MS_WINDOWS_SYSTEM */ @@ -5342,6 +5344,12 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd) /*[clinic end generated code: output=a70446903abe821f input=a61722e1576fab03]*/ { int result; +#ifdef MS_WINDOWS + int error = 0; + int pathError = 0; + SECURITY_ATTRIBUTES secAttr = { sizeof(secAttr) }; + SECURITY_ATTRIBUTES *pSecAttr = NULL; +#endif #ifdef HAVE_MKDIRAT int mkdirat_unavailable = 0; #endif @@ -5353,11 +5361,38 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd) #ifdef MS_WINDOWS Py_BEGIN_ALLOW_THREADS - result = CreateDirectoryW(path->wide, NULL); + if (mode == 0700 /* 0o700 */) { + ULONG sdSize; + pSecAttr = &secAttr; + // Set a discreationary ACL (D) that is protected (P) and includes + // inheritable (OICI) entries that allow (A) full control (FA) to + // SYSTEM (SY), Administrators (BA), and the owner (OW). + if (!ConvertStringSecurityDescriptorToSecurityDescriptorW( + L"D:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;FA;;;OW)", + SDDL_REVISION_1, + &secAttr.lpSecurityDescriptor, + &sdSize + )) { + error = GetLastError(); + } + } + if (!error) { + result = CreateDirectoryW(path->wide, pSecAttr); + if (secAttr.lpSecurityDescriptor && + // uncommonly, LocalFree returns non-zero on error, but still uses + // GetLastError() to see what the error code is + LocalFree(secAttr.lpSecurityDescriptor)) { + error = GetLastError(); + } + } Py_END_ALLOW_THREADS - if (!result) + if (error) { + return PyErr_SetFromWindowsErr(error); + } + if (!result) { return path_error(path); + } #else Py_BEGIN_ALLOW_THREADS #if HAVE_MKDIRAT