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

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

Merged
merged 3 commits into from
May 2, 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 @@ -2430,6 +2430,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 @@ -2444,6 +2448,9 @@ features:
.. versionchanged:: 3.6
Accepts a :term:`path-like object`.

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


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

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 @@ -1811,6 +1811,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
zooba marked this conversation as resolved.
Show resolved Hide resolved
# 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,2 @@
:func:`os.mkdir` now accepts *mode* of ``0o700`` to restrict the new
directory to the current user.
158 changes: 156 additions & 2 deletions Modules/posixmodule.c
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,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 @@ -5539,6 +5541,133 @@ os__path_normpath_impl(PyObject *module, PyObject *path)
return result;
}

#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];
};

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();
}

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;
data->ea[0].Trustee.TrusteeForm = TRUSTEE_IS_NAME;
data->ea[0].Trustee.TrusteeType = TRUSTEE_IS_ALIAS;
data->ea[0].Trustee.ptstrName = L"CURRENT_USER";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the security issue for an elevated administrator? Say you're an elevated administrator with the account name "SpamAdmin". If you create a directory that you should only be able to access when elevated (e.g. in another user's profile), then no permissions should be granted to the "SpamAdmin" account, which would allow access even when not elevated.

Since administrator access tokens use the "Administrators" group as the owner of new objects, a generic approach would be to assign full control to the "OWNER_RIGHTS" SID (S-1-3-4)1 instead of the current user. Otherwise the implementation has to omit the entry for the current user if the user is an elevated administrator.

Footnotes

  1. Note there are significant differences in what ownership means and how it's handled on POSIX vs Windows. On POSIX, the owner must be a user, not a group, and the owner permissions apply exactly to the owner, even if an ACL is present. On Windows, the owner can be a group. Also, access is cumulative across the mandatory label and entries in the discretionary ACL. A user that's an effective owner could match other allow or deny entries in the discretionary ACL, and thus have a different set of rights than the explicit owner-rights. This isn't a problem given the simple ACL that's being set for 0o700.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's interesting, I hadn't come across OWNER_RIGHTS before (seems it's newer than the rest). It doesn't appear to be a supported alias, unfortunately.

I think it's good to know about, but as you say in the footnote, CURRENT_USER is closer to emulating POSIX semantics (for better or worse). If we/someone produce a library with full-featured ACL support, being able to arrange that nuance would be worth supporting, but I think this sufficiently improves our "best effort" (by imitating the ACLs that would be applied to any directory in the user profile, but for directories outside of the profile as well).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a user wouldn't be allowed to create the directory without elevating, they shouldn't have access to it when not elevated. That's why the owner is set to the Administrators group in this case, for inherited "CREATOR_OWNER" permissions. Since we're bypassing inheritance, the onus is on us to make this secure. That's addressed by omitting the "CURRENT_USER" entry if the current user is an elevated administrator. Or by using "OWNER_RIGHTS".

That's interesting, I hadn't come across OWNER_RIGHTS before (seems it's newer than the rest). It doesn't appear to be a supported alias, unfortunately.

It's been around for a long time; I think since Windows 7 (2008). One would use CreateWellKnownSid(WinCreatorOwnerRightsSid, ...) and set an entry for TRUSTEE_IS_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 @@ -5568,6 +5697,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 *pSecAttr = NULL;
struct _Py_SECURITY_ATTRIBUTE_DATA secAttrData;
#endif
#ifdef HAVE_MKDIRAT
int mkdirat_unavailable = 0;
#endif
Expand All @@ -5579,11 +5714,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);
zooba marked this conversation as resolved.
Show resolved Hide resolved
}
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