From 1193b36d3ea4ec9fafaa81321b1ef545bb4be677 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Wed, 1 May 2024 20:57:29 +0100 Subject: [PATCH 1/4] gh-118486: Support mkdir(mode=0o700) on Windows --- Doc/library/os.rst | 7 + Doc/whatsnew/3.10.rst | 15 ++ Lib/test/test_os.py | 19 ++ Lib/test/test_tempfile.py | 28 +++ ...-05-01-20-57-09.gh-issue-118486.K44KJG.rst | 4 + Modules/posixmodule.c | 175 +++++++++++++++++- 6 files changed, 246 insertions(+), 2 deletions(-) create mode 100644 Misc/NEWS.d/next/Security/2024-05-01-20-57-09.gh-issue-118486.K44KJG.rst diff --git a/Doc/library/os.rst b/Doc/library/os.rst index 28990c216af3d1..8405fb1917e714 100644 --- a/Doc/library/os.rst +++ b/Doc/library/os.rst @@ -2065,6 +2065,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 `. @@ -2079,6 +2083,9 @@ features: .. versionchanged:: 3.6 Accepts a :term:`path-like object`. + .. versionchanged:: 3.10.15 + Windows now handles a *mode* of ``0o700``. + .. function:: makedirs(name, mode=0o777, exist_ok=False) diff --git a/Doc/whatsnew/3.10.rst b/Doc/whatsnew/3.10.rst index 43da72aece9dfd..547907c4329fd5 100644 --- a/Doc/whatsnew/3.10.rst +++ b/Doc/whatsnew/3.10.rst @@ -1247,6 +1247,13 @@ Add :data:`~os.O_EVTONLY`, :data:`~os.O_FSYNC`, :data:`~os.O_SYMLINK` and :data:`~os.O_NOFOLLOW_ANY` for macOS. (Contributed by Dong-hee Na in :issue:`43106`.) +As of 3.10.15, :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 ------- @@ -1399,6 +1406,14 @@ Add :data:`sys.stdlib_module_names`, containing the list of the standard library module names. (Contributed by Victor Stinner in :issue:`42955`.) +tempfile +-------- + +As of 3.10.15 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`.) + _thread ------- diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 6b443c48f8fcb5..ada5c939ba253f 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -1635,6 +1635,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 30d57baf977f52..6e06bfc7f2f78d 100644 --- a/Lib/test/test_tempfile.py +++ b/Lib/test/test_tempfile.py @@ -11,6 +11,7 @@ import stat import types import weakref +import subprocess from unittest import mock import unittest @@ -800,6 +801,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 c0421a94c17397..5a9802e03e5ddc 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -33,6 +33,12 @@ #include "pycore_import.h" // _PyImport_ReInitLock() #include "pycore_initconfig.h" // _PyStatus_EXCEPTION() #include "pycore_pystate.h" // _PyInterpreterState_GET() + +#ifdef MS_WINDOWS +# include // SetEntriesInAcl +# include // SDDL_REVISION_1 +#endif + #include "structmember.h" // PyMemberDef #ifndef MS_WINDOWS # include "posixmodule.h" @@ -4466,6 +4472,146 @@ os__path_splitroot_impl(PyObject *module, path_t *path) #endif /* MS_WINDOWS */ +#ifdef MS_WINDOWS + +/* We centralise SECURITY_ATTRIBUTE initialization based around +templates that will probably mostly match common POSIX mode settings. +The _Py_SECURITY_ATTRIBUTE_DATA structure contains temporary data, as +a constructed SECURITY_ATTRIBUTE structure typically refers to memory +that has to be alive while it's being used. + +Typical use will look like: + SECURITY_ATTRIBUTES *pSecAttr = NULL; + struct _Py_SECURITY_ATTRIBUTE_DATA secAttrData; + int error, error2; + + Py_BEGIN_ALLOW_THREADS + switch (mode) { + case 0x1C0: // 0o700 + error = initializeMkdir700SecurityAttributes(&pSecAttr, &secAttrData); + break; + ... + default: + error = initializeDefaultSecurityAttributes(&pSecAttr, &secAttrData); + break; + } + + if (!error) { + // do operation, passing pSecAttr + } + + // Unconditionally clear secAttrData. + error2 = clearSecurityAttributes(&pSecAttr, &secAttrData); + if (!error) { + error = error2; + } + Py_END_ALLOW_THREADS + + if (error) { + PyErr_SetFromWindowsErr(error); + return NULL; + } +*/ + +struct _Py_SECURITY_ATTRIBUTE_DATA { + SECURITY_ATTRIBUTES securityAttributes; + PACL acl; + SECURITY_DESCRIPTOR sd; + EXPLICIT_ACCESS_W ea[4]; + char sid[64]; +}; + +static int +initializeDefaultSecurityAttributes( + PSECURITY_ATTRIBUTES *securityAttributes, + struct _Py_SECURITY_ATTRIBUTE_DATA *data +) { + assert(securityAttributes); + assert(data); + *securityAttributes = NULL; + memset(data, 0, sizeof(*data)); + return 0; +} + +static int +initializeMkdir700SecurityAttributes( + PSECURITY_ATTRIBUTES *securityAttributes, + struct _Py_SECURITY_ATTRIBUTE_DATA *data +) { + assert(securityAttributes); + assert(data); + *securityAttributes = NULL; + memset(data, 0, sizeof(*data)); + + if (!InitializeSecurityDescriptor(&data->sd, SECURITY_DESCRIPTOR_REVISION) + || !SetSecurityDescriptorGroup(&data->sd, NULL, TRUE)) { + return GetLastError(); + } + + int use_alias = 0; + DWORD cbSid = sizeof(data->sid); + if (!CreateWellKnownSid(WinCreatorOwnerRightsSid, NULL, (PSID)data->sid, &cbSid)) { + use_alias = 1; + } + + data->securityAttributes.nLength = sizeof(SECURITY_ATTRIBUTES); + data->ea[0].grfAccessPermissions = GENERIC_ALL; + data->ea[0].grfAccessMode = SET_ACCESS; + data->ea[0].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; + if (use_alias) { + data->ea[0].Trustee.TrusteeForm = TRUSTEE_IS_NAME; + data->ea[0].Trustee.TrusteeType = TRUSTEE_IS_ALIAS; + data->ea[0].Trustee.ptstrName = L"CURRENT_USER"; + } else { + data->ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; + data->ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; + data->ea[0].Trustee.ptstrName = (LPWCH)(SID*)data->sid; + } + + data->ea[1].grfAccessPermissions = GENERIC_ALL; + data->ea[1].grfAccessMode = SET_ACCESS; + data->ea[1].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; + data->ea[1].Trustee.TrusteeForm = TRUSTEE_IS_NAME; + data->ea[1].Trustee.TrusteeType = TRUSTEE_IS_ALIAS; + data->ea[1].Trustee.ptstrName = L"SYSTEM"; + + data->ea[2].grfAccessPermissions = GENERIC_ALL; + data->ea[2].grfAccessMode = SET_ACCESS; + data->ea[2].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; + data->ea[2].Trustee.TrusteeForm = TRUSTEE_IS_NAME; + data->ea[2].Trustee.TrusteeType = TRUSTEE_IS_ALIAS; + data->ea[2].Trustee.ptstrName = L"ADMINISTRATORS"; + + int r = SetEntriesInAclW(3, data->ea, NULL, &data->acl); + if (r) { + return r; + } + if (!SetSecurityDescriptorDacl(&data->sd, TRUE, data->acl, FALSE)) { + return GetLastError(); + } + data->securityAttributes.lpSecurityDescriptor = &data->sd; + *securityAttributes = &data->securityAttributes; + return 0; +} + +static int +clearSecurityAttributes( + PSECURITY_ATTRIBUTES *securityAttributes, + struct _Py_SECURITY_ATTRIBUTE_DATA *data +) { + assert(securityAttributes); + assert(data); + *securityAttributes = NULL; + if (data->acl) { + if (LocalFree((void *)data->acl)) { + return GetLastError(); + } + } + return 0; +} + +#endif + /*[clinic input] os.mkdir @@ -4495,6 +4641,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 *pSecAttr = NULL; + struct _Py_SECURITY_ATTRIBUTE_DATA secAttrData; +#endif #ifdef HAVE_MKDIRAT int mkdirat_unavailable = 0; #endif @@ -4506,11 +4658,30 @@ 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); + switch (mode) { + case 0x1C0: // 0o700 + error = initializeMkdir700SecurityAttributes(&pSecAttr, &secAttrData); + break; + default: + error = initializeDefaultSecurityAttributes(&pSecAttr, &secAttrData); + break; + } + if (!error) { + result = CreateDirectoryW(path->wide, pSecAttr); + error = clearSecurityAttributes(&pSecAttr, &secAttrData); + } else { + // Ignore error from "clear" - we have a more interesting one already + clearSecurityAttributes(&pSecAttr, &secAttrData); + } Py_END_ALLOW_THREADS - if (!result) + if (error) { + PyErr_SetFromWindowsErr(error); + return NULL; + } + if (!result) { return path_error(path); + } #else Py_BEGIN_ALLOW_THREADS #if HAVE_MKDIRAT From 338717220588eb40dd042f55e1a92ef8acc60f76 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Tue, 7 May 2024 22:14:16 +0100 Subject: [PATCH 2/4] Fix CVE in docs --- Doc/whatsnew/3.10.rst | 4 ++-- .../Security/2024-05-01-20-57-09.gh-issue-118486.K44KJG.rst | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Doc/whatsnew/3.10.rst b/Doc/whatsnew/3.10.rst index 547907c4329fd5..f92e61ca47c3d8 100644 --- a/Doc/whatsnew/3.10.rst +++ b/Doc/whatsnew/3.10.rst @@ -1250,7 +1250,7 @@ and :data:`~os.O_NOFOLLOW_ANY` for macOS. As of 3.10.15, :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 +mitigation for CVE-2024-4030. Other values for *mode* continue to be ignored. (Contributed by Steve Dower in :gh:`118486`.) @@ -1411,7 +1411,7 @@ tempfile As of 3.10.15 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`. +changes to :func:`os.mkdir`. This is a mitigation for CVE-2024-4030. (Contributed by Steve Dower in :gh:`118486`.) _thread 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 index 8ac48aac816a60..a28a4e5cdb6991 100644 --- 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 @@ -1,4 +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` +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. From 8b2d7332558ba5295887675e52c8dcad994b22db Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 9 May 2024 17:46:30 +0100 Subject: [PATCH 3/4] Use language-invariant ACL --- Modules/posixmodule.c | 177 ++++++------------------------------------ 1 file changed, 22 insertions(+), 155 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 5a9802e03e5ddc..e8cf2fc1b10c8a 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4471,147 +4471,6 @@ os__path_splitroot_impl(PyObject *module, path_t *path) #endif /* MS_WINDOWS */ - -#ifdef MS_WINDOWS - -/* We centralise SECURITY_ATTRIBUTE initialization based around -templates that will probably mostly match common POSIX mode settings. -The _Py_SECURITY_ATTRIBUTE_DATA structure contains temporary data, as -a constructed SECURITY_ATTRIBUTE structure typically refers to memory -that has to be alive while it's being used. - -Typical use will look like: - SECURITY_ATTRIBUTES *pSecAttr = NULL; - struct _Py_SECURITY_ATTRIBUTE_DATA secAttrData; - int error, error2; - - Py_BEGIN_ALLOW_THREADS - switch (mode) { - case 0x1C0: // 0o700 - error = initializeMkdir700SecurityAttributes(&pSecAttr, &secAttrData); - break; - ... - default: - error = initializeDefaultSecurityAttributes(&pSecAttr, &secAttrData); - break; - } - - if (!error) { - // do operation, passing pSecAttr - } - - // Unconditionally clear secAttrData. - error2 = clearSecurityAttributes(&pSecAttr, &secAttrData); - if (!error) { - error = error2; - } - Py_END_ALLOW_THREADS - - if (error) { - PyErr_SetFromWindowsErr(error); - return NULL; - } -*/ - -struct _Py_SECURITY_ATTRIBUTE_DATA { - SECURITY_ATTRIBUTES securityAttributes; - PACL acl; - SECURITY_DESCRIPTOR sd; - EXPLICIT_ACCESS_W ea[4]; - char sid[64]; -}; - -static int -initializeDefaultSecurityAttributes( - PSECURITY_ATTRIBUTES *securityAttributes, - struct _Py_SECURITY_ATTRIBUTE_DATA *data -) { - assert(securityAttributes); - assert(data); - *securityAttributes = NULL; - memset(data, 0, sizeof(*data)); - return 0; -} - -static int -initializeMkdir700SecurityAttributes( - PSECURITY_ATTRIBUTES *securityAttributes, - struct _Py_SECURITY_ATTRIBUTE_DATA *data -) { - assert(securityAttributes); - assert(data); - *securityAttributes = NULL; - memset(data, 0, sizeof(*data)); - - if (!InitializeSecurityDescriptor(&data->sd, SECURITY_DESCRIPTOR_REVISION) - || !SetSecurityDescriptorGroup(&data->sd, NULL, TRUE)) { - return GetLastError(); - } - - int use_alias = 0; - DWORD cbSid = sizeof(data->sid); - if (!CreateWellKnownSid(WinCreatorOwnerRightsSid, NULL, (PSID)data->sid, &cbSid)) { - use_alias = 1; - } - - data->securityAttributes.nLength = sizeof(SECURITY_ATTRIBUTES); - data->ea[0].grfAccessPermissions = GENERIC_ALL; - data->ea[0].grfAccessMode = SET_ACCESS; - data->ea[0].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; - if (use_alias) { - data->ea[0].Trustee.TrusteeForm = TRUSTEE_IS_NAME; - data->ea[0].Trustee.TrusteeType = TRUSTEE_IS_ALIAS; - data->ea[0].Trustee.ptstrName = L"CURRENT_USER"; - } else { - data->ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID; - data->ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; - data->ea[0].Trustee.ptstrName = (LPWCH)(SID*)data->sid; - } - - data->ea[1].grfAccessPermissions = GENERIC_ALL; - data->ea[1].grfAccessMode = SET_ACCESS; - data->ea[1].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; - data->ea[1].Trustee.TrusteeForm = TRUSTEE_IS_NAME; - data->ea[1].Trustee.TrusteeType = TRUSTEE_IS_ALIAS; - data->ea[1].Trustee.ptstrName = L"SYSTEM"; - - data->ea[2].grfAccessPermissions = GENERIC_ALL; - data->ea[2].grfAccessMode = SET_ACCESS; - data->ea[2].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT; - data->ea[2].Trustee.TrusteeForm = TRUSTEE_IS_NAME; - data->ea[2].Trustee.TrusteeType = TRUSTEE_IS_ALIAS; - data->ea[2].Trustee.ptstrName = L"ADMINISTRATORS"; - - int r = SetEntriesInAclW(3, data->ea, NULL, &data->acl); - if (r) { - return r; - } - if (!SetSecurityDescriptorDacl(&data->sd, TRUE, data->acl, FALSE)) { - return GetLastError(); - } - data->securityAttributes.lpSecurityDescriptor = &data->sd; - *securityAttributes = &data->securityAttributes; - return 0; -} - -static int -clearSecurityAttributes( - PSECURITY_ATTRIBUTES *securityAttributes, - struct _Py_SECURITY_ATTRIBUTE_DATA *data -) { - assert(securityAttributes); - assert(data); - *securityAttributes = NULL; - if (data->acl) { - if (LocalFree((void *)data->acl)) { - return GetLastError(); - } - } - return 0; -} - -#endif - /*[clinic input] os.mkdir @@ -4644,8 +4503,8 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd) #ifdef MS_WINDOWS int error = 0; int pathError = 0; + SECURITY_ATTRIBUTES secAttr = { sizeof(secAttr) }; SECURITY_ATTRIBUTES *pSecAttr = NULL; - struct _Py_SECURITY_ATTRIBUTE_DATA secAttrData; #endif #ifdef HAVE_MKDIRAT int mkdirat_unavailable = 0; @@ -4658,26 +4517,34 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd) #ifdef MS_WINDOWS Py_BEGIN_ALLOW_THREADS - switch (mode) { - case 0x1C0: // 0o700 - error = initializeMkdir700SecurityAttributes(&pSecAttr, &secAttrData); - break; - default: - error = initializeDefaultSecurityAttributes(&pSecAttr, &secAttrData); - break; + 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); - error = clearSecurityAttributes(&pSecAttr, &secAttrData); - } else { - // Ignore error from "clear" - we have a more interesting one already - clearSecurityAttributes(&pSecAttr, &secAttrData); + 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 (error) { - PyErr_SetFromWindowsErr(error); - return NULL; + return PyErr_SetFromWindowsErr(error); } if (!result) { return path_error(path); From 77ede7c481db33bf114c72e371267f35a999b4b8 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Wed, 15 May 2024 11:59:41 +0100 Subject: [PATCH 4/4] gh-118486: Simplify test_win32_mkdir_700 to check the exact ACL (GH-119056) --- Lib/test/test_os.py | 23 ++++++++--------------- Modules/posixmodule.c | 2 +- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index ada5c939ba253f..044ce5c1a39f30 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -1638,21 +1638,14 @@ def test_exist_ok_existing_regular_file(self): @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) + path = os.path.abspath(os.path.join(os_helper.TESTFN, 'dir')) + os.mkdir(path, mode=0o700) + out = subprocess.check_output(["cacls.exe", path, "/s"], encoding="oem") + os.rmdir(path) + self.assertEqual( + out.strip(), + f'{path} "D:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;FA;;;OW)"', + ) def tearDown(self): path = os.path.join(os_helper.TESTFN, 'dir1', 'dir2', 'dir3', diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index e8cf2fc1b10c8a..43e69fc322b595 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -4520,7 +4520,7 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd) if (mode == 0700 /* 0o700 */) { ULONG sdSize; pSecAttr = &secAttr; - // Set a discreationary ACL (D) that is protected (P) and includes + // Set a discretionary 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(