Skip to content

Commit

Permalink
[3.11] pythongh-118486: Support mkdir(mode=0o700) on Windows (pythonG…
Browse files Browse the repository at this point in the history
  • Loading branch information
zooba authored May 24, 2024
1 parent 4762b36 commit 35c799d
Show file tree
Hide file tree
Showing 6 changed files with 103 additions and 2 deletions.
7 changes: 7 additions & 0 deletions Doc/library/os.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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>`.

Expand All @@ -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)

Expand Down
12 changes: 12 additions & 0 deletions Doc/whatsnew/3.11.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down Expand Up @@ -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:

Expand Down
12 changes: 12 additions & 0 deletions Lib/test/test_os.py
Original file line number Diff line number Diff line change
Expand Up @@ -1743,6 +1743,18 @@ 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
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',
'dir4', 'dir5', 'dir6')
Expand Down
28 changes: 28 additions & 0 deletions Lib/test/test_tempfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import stat
import types
import weakref
import subprocess
from unittest import mock

import unittest
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff 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
affecting :func:`tempfile.mkdtemp` in scenarios where the base temporary
directory is more permissive than the default.
42 changes: 40 additions & 2 deletions Modules/posixmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -4611,6 +4616,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
Expand All @@ -4622,11 +4633,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 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(
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
Expand Down

0 comments on commit 35c799d

Please sign in to comment.