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.9] gh-118486: Support mkdir(mode=0o700) on Windows (GH-118488) #118741

Merged
merged 6 commits into from
May 24, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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 @@ -1929,6 +1929,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 @@ -1943,6 +1947,9 @@ features:
.. versionchanged:: 3.6
Accepts a :term:`path-like object`.

.. versionchanged:: 3.9.20
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.9.rst
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,13 @@ Added :func:`os.waitstatus_to_exitcode` function:
convert a wait status to an exit code.
(Contributed by Victor Stinner in :issue:`40094`.)

As of 3.9.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`.)

pathlib
-------

Expand Down Expand Up @@ -704,6 +711,14 @@ Previously, :attr:`sys.stderr` was block-buffered when non-interactive. Now
``stderr`` defaults to always being line-buffered.
(Contributed by Jendrik Seipp in :issue:`13601`.)

tempfile
--------

As of 3.9.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`.)

tracemalloc
-----------

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 @@ -1493,6 +1493,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')
zooba marked this conversation as resolved.
Show resolved Hide resolved
# 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(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 stat
import types
import weakref
import subprocess
from unittest import mock

import unittest
Expand Down Expand Up @@ -772,6 +773,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.
175 changes: 173 additions & 2 deletions Modules/posixmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@
#include "pycore_ceval.h" // _PyEval_ReInitThreads()
#include "pycore_import.h" // _PyImport_ReInitLock()
#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 @@ -4426,6 +4432,146 @@ os__path_splitroot_impl(PyObject *module, path_t *path)
#endif /* MS_WINDOWS */


#ifdef MS_WINDOWS

/* We centralise SECURITY_ATTRIBUTE initialization based around
templates that will probably mostly match common POSIX mode settings.
The _Py_SECURITY_ATTRIBUTE_DATA structure contains temporary data, as
a constructed SECURITY_ATTRIBUTE structure typically refers to memory
that has to be alive while it's being used.

Typical use will look like:
SECURITY_ATTRIBUTES *pSecAttr = NULL;
struct _Py_SECURITY_ATTRIBUTE_DATA secAttrData;
int error, error2;

Py_BEGIN_ALLOW_THREADS
switch (mode) {
case 0x1C0: // 0o700
error = initializeMkdir700SecurityAttributes(&pSecAttr, &secAttrData);
break;
...
default:
error = initializeDefaultSecurityAttributes(&pSecAttr, &secAttrData);
break;
}

if (!error) {
// do operation, passing pSecAttr
}

// Unconditionally clear secAttrData.
error2 = clearSecurityAttributes(&pSecAttr, &secAttrData);
if (!error) {
error = error2;
}
Py_END_ALLOW_THREADS

if (error) {
PyErr_SetFromWindowsErr(error);
return NULL;
}
*/

struct _Py_SECURITY_ATTRIBUTE_DATA {
SECURITY_ATTRIBUTES securityAttributes;
PACL acl;
SECURITY_DESCRIPTOR sd;
EXPLICIT_ACCESS_W ea[4];
char sid[64];
};

static int
initializeDefaultSecurityAttributes(
PSECURITY_ATTRIBUTES *securityAttributes,
struct _Py_SECURITY_ATTRIBUTE_DATA *data
) {
assert(securityAttributes);
assert(data);
*securityAttributes = NULL;
memset(data, 0, sizeof(*data));
return 0;
}

static int
initializeMkdir700SecurityAttributes(
PSECURITY_ATTRIBUTES *securityAttributes,
struct _Py_SECURITY_ATTRIBUTE_DATA *data
) {
assert(securityAttributes);
assert(data);
*securityAttributes = NULL;
memset(data, 0, sizeof(*data));

if (!InitializeSecurityDescriptor(&data->sd, SECURITY_DESCRIPTOR_REVISION)
|| !SetSecurityDescriptorGroup(&data->sd, NULL, TRUE)) {
return GetLastError();
}

int use_alias = 0;
DWORD cbSid = sizeof(data->sid);
if (!CreateWellKnownSid(WinCreatorOwnerRightsSid, NULL, (PSID)data->sid, &cbSid)) {
use_alias = 1;
}

data->securityAttributes.nLength = sizeof(SECURITY_ATTRIBUTES);
data->ea[0].grfAccessPermissions = GENERIC_ALL;
data->ea[0].grfAccessMode = SET_ACCESS;
data->ea[0].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT;
if (use_alias) {
data->ea[0].Trustee.TrusteeForm = TRUSTEE_IS_NAME;
data->ea[0].Trustee.TrusteeType = TRUSTEE_IS_ALIAS;
data->ea[0].Trustee.ptstrName = L"CURRENT_USER";
} else {
data->ea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;
data->ea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
data->ea[0].Trustee.ptstrName = (LPWCH)(SID*)data->sid;
}

data->ea[1].grfAccessPermissions = GENERIC_ALL;
data->ea[1].grfAccessMode = SET_ACCESS;
data->ea[1].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT;
data->ea[1].Trustee.TrusteeForm = TRUSTEE_IS_NAME;
data->ea[1].Trustee.TrusteeType = TRUSTEE_IS_ALIAS;
data->ea[1].Trustee.ptstrName = L"SYSTEM";

data->ea[2].grfAccessPermissions = GENERIC_ALL;
data->ea[2].grfAccessMode = SET_ACCESS;
data->ea[2].grfInheritance = SUB_CONTAINERS_AND_OBJECTS_INHERIT;
data->ea[2].Trustee.TrusteeForm = TRUSTEE_IS_NAME;
data->ea[2].Trustee.TrusteeType = TRUSTEE_IS_ALIAS;
data->ea[2].Trustee.ptstrName = L"ADMINISTRATORS";

int r = SetEntriesInAclW(3, data->ea, NULL, &data->acl);
if (r) {
return r;
}
if (!SetSecurityDescriptorDacl(&data->sd, TRUE, data->acl, FALSE)) {
return GetLastError();
}
data->securityAttributes.lpSecurityDescriptor = &data->sd;
*securityAttributes = &data->securityAttributes;
return 0;
}

static int
clearSecurityAttributes(
PSECURITY_ATTRIBUTES *securityAttributes,
struct _Py_SECURITY_ATTRIBUTE_DATA *data
) {
assert(securityAttributes);
assert(data);
*securityAttributes = NULL;
if (data->acl) {
if (LocalFree((void *)data->acl)) {
return GetLastError();
}
}
return 0;
}

#endif

/*[clinic input]
os.mkdir

Expand Down Expand Up @@ -4454,6 +4600,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 *pSecAttr = NULL;
struct _Py_SECURITY_ATTRIBUTE_DATA secAttrData;
#endif
#ifdef HAVE_MKDIRAT
int mkdirat_unavailable = 0;
#endif
Expand All @@ -4465,11 +4617,30 @@ 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);
switch (mode) {
case 0x1C0: // 0o700
error = initializeMkdir700SecurityAttributes(&pSecAttr, &secAttrData);
break;
default:
error = initializeDefaultSecurityAttributes(&pSecAttr, &secAttrData);
break;
}
if (!error) {
result = CreateDirectoryW(path->wide, pSecAttr);
error = clearSecurityAttributes(&pSecAttr, &secAttrData);
} else {
// Ignore error from "clear" - we have a more interesting one already
clearSecurityAttributes(&pSecAttr, &secAttrData);
}
Py_END_ALLOW_THREADS

if (!result)
if (error) {
PyErr_SetFromWindowsErr(error);
return NULL;
}
if (!result) {
return path_error(path);
}
#else
Py_BEGIN_ALLOW_THREADS
#if HAVE_MKDIRAT
Expand Down
Loading