Skip to content

Commit

Permalink
[3.10] gh-118486: Support mkdir(mode=0o700) on Windows (GH-118488) (G…
Browse files Browse the repository at this point in the history
…H-118740)

Co-authored-by: Łukasz Langa <lukasz@langa.pl>
  • Loading branch information
zooba and ambv committed May 24, 2024
1 parent 333c7dc commit c8f868d
Show file tree
Hide file tree
Showing 6 changed files with 107 additions and 3 deletions.
7 changes: 7 additions & 0 deletions Doc/library/os.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
<dir_fd>`.

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

Expand Down
15 changes: 15 additions & 0 deletions Doc/whatsnew/3.10.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
-------
Expand Down Expand Up @@ -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
-------
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 @@ -1635,6 +1635,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 @@ -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
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.
44 changes: 41 additions & 3 deletions Modules/posixmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -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 <aclapi.h> // SetEntriesInAcl
# include <sddl.h> // SDDL_REVISION_1
#endif

#include "structmember.h" // PyMemberDef
#ifndef MS_WINDOWS
# include "posixmodule.h"
Expand Down Expand Up @@ -4465,7 +4471,6 @@ os__path_splitroot_impl(PyObject *module, path_t *path)

#endif /* MS_WINDOWS */


/*[clinic input]
os.mkdir
Expand Down Expand Up @@ -4495,6 +4500,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 @@ -4506,11 +4517,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 c8f868d

Please sign in to comment.