Skip to content

Commit

Permalink
gh-118486: Support mkdir(mode=0o700) on Windows (GH-118488)
Browse files Browse the repository at this point in the history
  • Loading branch information
zooba authored May 9, 2024
1 parent c0d257c commit eb29e2f
Show file tree
Hide file tree
Showing 6 changed files with 106 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 @@ -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
<dir_fd>`.

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

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

Expand Down Expand Up @@ -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
---------
Expand Down
19 changes: 19 additions & 0 deletions Lib/test/test_os.py
Original file line number Diff line number Diff line change
Expand Up @@ -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, "<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')
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 @@ -13,6 +13,7 @@
import weakref
import gc
import shutil
import subprocess
from unittest import mock

import unittest
Expand Down Expand Up @@ -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
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.
39 changes: 37 additions & 2 deletions Modules/posixmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@
# include <winioctl.h>
# include <lmcons.h> // UNLEN
# include "osdefs.h" // SEP
# include <aclapi.h> // SetEntriesInAcl
# include <sddl.h> // SDDL_REVISION_1
# if defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM)
# define HAVE_SYMLINK
# endif /* MS_WINDOWS_DESKTOP | MS_WINDOWS_SYSTEM */
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down

0 comments on commit eb29e2f

Please sign in to comment.