gh-118486: Support mkdir(mode=0o700) on Windows (GH-118488)

This commit is contained in:
Steve Dower 2024-05-09 19:18:56 +01:00 committed by GitHub
parent c0d257cc69
commit eb29e2f590
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 106 additions and 2 deletions

View file

@ -2356,6 +2356,10 @@ features:
platform-dependent. On some platforms, they are ignored and you should call platform-dependent. On some platforms, they are ignored and you should call
:func:`chmod` explicitly to set them. :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 This function can also support :ref:`paths relative to directory descriptors
<dir_fd>`. <dir_fd>`.
@ -2370,6 +2374,9 @@ features:
.. versionchanged:: 3.6 .. versionchanged:: 3.6
Accepts a :term:`path-like object`. 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) .. function:: makedirs(name, mode=0o777, exist_ok=False)

View file

@ -778,6 +778,13 @@ os
Both functions may be significantly faster on newer releases of Both functions may be significantly faster on newer releases of
Windows. (Contributed by Steve Dower in :gh:`99726`.) 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 os.path
------- -------
@ -925,6 +932,10 @@ tempfile
*delete_on_close* (Contributed by Evgeny Zorin in :gh:`58451`.) *delete_on_close* (Contributed by Evgeny Zorin in :gh:`58451`.)
* :func:`tempfile.mkdtemp` now always returns an absolute path, even if the * :func:`tempfile.mkdtemp` now always returns an absolute path, even if the
argument provided to the *dir* parameter is a relative path. 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 threading
--------- ---------

View file

@ -1797,6 +1797,25 @@ class MakedirTests(unittest.TestCase):
self.assertRaises(OSError, os.makedirs, path, exist_ok=True) self.assertRaises(OSError, os.makedirs, path, exist_ok=True)
os.remove(path) 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): def tearDown(self):
path = os.path.join(os_helper.TESTFN, 'dir1', 'dir2', 'dir3', path = os.path.join(os_helper.TESTFN, 'dir1', 'dir2', 'dir3',
'dir4', 'dir5', 'dir6') 'dir4', 'dir5', 'dir6')

View file

@ -13,6 +13,7 @@ import types
import weakref import weakref
import gc import gc
import shutil import shutil
import subprocess
from unittest import mock from unittest import mock
import unittest import unittest
@ -803,6 +804,33 @@ class TestMkdtemp(TestBadTempdir, BaseTestCase):
finally: finally:
os.rmdir(dir) 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): def test_collision_with_existing_file(self):
# mkdtemp tries another name when a file with # mkdtemp tries another name when a file with
# the chosen name already exists # the chosen name already exists

View file

@ -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.

View file

@ -32,6 +32,8 @@
# include <winioctl.h> # include <winioctl.h>
# include <lmcons.h> // UNLEN # include <lmcons.h> // UNLEN
# include "osdefs.h" // SEP # include "osdefs.h" // SEP
# include <aclapi.h> // SetEntriesInAcl
# include <sddl.h> // SDDL_REVISION_1
# if defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM) # if defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM)
# define HAVE_SYMLINK # define HAVE_SYMLINK
# endif /* MS_WINDOWS_DESKTOP | MS_WINDOWS_SYSTEM */ # endif /* MS_WINDOWS_DESKTOP | MS_WINDOWS_SYSTEM */
@ -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]*/ /*[clinic end generated code: output=a70446903abe821f input=a61722e1576fab03]*/
{ {
int result; int result;
#ifdef MS_WINDOWS
int error = 0;
int pathError = 0;
SECURITY_ATTRIBUTES secAttr = { sizeof(secAttr) };
SECURITY_ATTRIBUTES *pSecAttr = NULL;
#endif
#ifdef HAVE_MKDIRAT #ifdef HAVE_MKDIRAT
int mkdirat_unavailable = 0; int mkdirat_unavailable = 0;
#endif #endif
@ -5353,11 +5361,38 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd)
#ifdef MS_WINDOWS #ifdef MS_WINDOWS
Py_BEGIN_ALLOW_THREADS 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 Py_END_ALLOW_THREADS
if (!result) if (error) {
return PyErr_SetFromWindowsErr(error);
}
if (!result) {
return path_error(path); return path_error(path);
}
#else #else
Py_BEGIN_ALLOW_THREADS Py_BEGIN_ALLOW_THREADS
#if HAVE_MKDIRAT #if HAVE_MKDIRAT