Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[3.8] gh-118486: Support mkdir(mode=0o700) on Windows (GH-118488) #118742

Merged
merged 6 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions Doc/library/os.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1909,6 +1909,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 @@ -1923,6 +1927,9 @@ features:
.. versionchanged:: 3.6
Accepts a :term:`path-like object`.

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


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

Expand Down
16 changes: 16 additions & 0 deletions Doc/whatsnew/3.8.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1046,6 +1046,13 @@ treat junctions as links.

(Contributed by Steve Dower in :issue:`37834`.)

As of 3.8.20, :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 @@ -1252,6 +1259,15 @@ in a standardized and extensible format, and offers several other benefits.
(Contributed by C.A.M. Gerlach in :issue:`36268`.)


tempfile
--------

As of 3.8.20 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
12 changes: 12 additions & 0 deletions Lib/test/test_os.py
Original file line number Diff line number Diff line change
Expand Up @@ -1380,6 +1380,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 = support.TESTFN
path = os.path.abspath(os.path.join(support.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(support.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 contextlib
import stat
import weakref
import subprocess
from unittest import mock

import unittest
Expand Down Expand Up @@ -760,6 +761,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[len(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 @@ -40,6 +40,12 @@
#include "pycore_pystate.h" /* _PyRuntime */
#include "pythread.h"
#include "structmember.h"

#ifdef MS_WINDOWS
# include <aclapi.h> // SetEntriesInAcl
# include <sddl.h> // SDDL_REVISION_1
#endif

#ifndef MS_WINDOWS
# include "posixmodule.h"
#else
Expand Down Expand Up @@ -4122,7 +4128,6 @@ os__path_splitroot_impl(PyObject *module, path_t *path)

#endif /* MS_WINDOWS */


/*[clinic input]
os.mkdir

Expand Down Expand Up @@ -4151,6 +4156,12 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd)
/*[clinic end generated code: output=a70446903abe821f input=e965f68377e9b1ce]*/
{
int result;
#ifdef MS_WINDOWS
int error = 0;
int pathError = 0;
SECURITY_ATTRIBUTES secAttr = { sizeof(secAttr) };
SECURITY_ATTRIBUTES *pSecAttr = NULL;
#endif

if (PySys_Audit("os.mkdir", "Oii", path->object, mode,
dir_fd == DEFAULT_DIR_FD ? -1 : dir_fd) < 0) {
Expand All @@ -4159,11 +4170,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
Loading