python · ambv · May 24, 2024 · May 1, 2024 · May 7, 2024 · May 9, 2024
diff --git a/Doc/library/os.rst b/Doc/library/os.rst
@@ -2178,6 +2178,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
   <dir_fd>`.

@@ -2192,6 +2196,9 @@ features:
   .. versionchanged:: 3.6
      Accepts a :term:`path-like object`.

+   .. versionchanged:: 3.11.10
+      Windows now handles a *mode* of ``0o700``.
+

 .. function:: makedirs(name, mode=0o777, exist_ok=False)


diff --git a/Doc/whatsnew/3.11.rst b/Doc/whatsnew/3.11.rst
@@ -899,6 +899,13 @@ os
  instead of ``CryptGenRandom()`` which is deprecated.
  (Contributed by Donghee Na in :issue:`44611`.)

+* As of 3.11.10, :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`.)
+

 .. _whatsnew311-pathlib:

@@ -1059,6 +1066,11 @@ tempfile
  such as compression modules.
  (Contributed by Carey Metcalfe in :gh:`70363`.)

+* As of 3.11.10 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`.)
+

 .. _whatsnew311-threading:


diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py
@@ -1743,6 +1743,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, "<PATH>")
+        out2 = out2.replace(path2, "<PATH>")
+        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
@@ -11,6 +11,7 @@
 import stat
 import types
 import weakref
+import subprocess
 from unittest import mock

 import unittest
@@ -801,6 +802,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
@@ -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
@@ -34,6 +34,11 @@
 #include "pycore_pystate.h"       // _PyInterpreterState_GET()
 #include "pycore_signal.h"        // Py_NSIG

+#ifdef MS_WINDOWS
+#  include <aclapi.h>             // SetEntriesInAcl
+#  include <sddl.h>               // SDDL_REVISION_1
+#endif
+
 #include "structmember.h"         // PyMemberDef
 #ifndef MS_WINDOWS
 #  include "posixmodule.h"
@@ -4582,6 +4587,146 @@ os__path_normpath_impl(PyObject *module, PyObject *path)
    return result;
 }

+#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

@@ -4611,6 +4756,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
@@ -4622,11 +4773,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
-Original file line number
+Diff line change
@@ -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`
                   zooba marked this conversation as resolved.
              

          
            Show resolved
            Hide resolved
        

                
  
            
  

  
    
  
    
      

              Uh oh!

              
There was an error while loading. Please reload this page.
+    affecting :func:`tempfile.mkdtemp` in scenarios where the base temporary
+    directory is more permissive than the default.