From 8af84b503d0b62a3db0d806d39f42c1e08746079 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 9 May 2024 17:43:21 +0100 Subject: [PATCH] gh-118773: Use language-invariant SDDL string instead of aliases for ACLs. (GH-118800) --- ...-05-08-21-59-38.gh-issue-118773.7dFRJY.rst | 2 + Modules/posixmodule.c | 176 +++--------------- 2 files changed, 24 insertions(+), 154 deletions(-) create mode 100644 Misc/NEWS.d/next/Security/2024-05-08-21-59-38.gh-issue-118773.7dFRJY.rst diff --git a/Misc/NEWS.d/next/Security/2024-05-08-21-59-38.gh-issue-118773.7dFRJY.rst b/Misc/NEWS.d/next/Security/2024-05-08-21-59-38.gh-issue-118773.7dFRJY.rst new file mode 100644 index 00000000000000..bfec178f6318a7 --- /dev/null +++ b/Misc/NEWS.d/next/Security/2024-05-08-21-59-38.gh-issue-118773.7dFRJY.rst @@ -0,0 +1,2 @@ +Fixes creation of ACLs in :func:`os.mkdir` on Windows to work correctly on +non-English machines. diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 9f4be98b35186e..5fe6036b3817e4 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5541,146 +5541,6 @@ 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]; - 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 @@ -5713,8 +5573,8 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd) #ifdef MS_WINDOWS int error = 0; int pathError = 0; + SECURITY_ATTRIBUTES secAttr = { sizeof(secAttr) }; SECURITY_ATTRIBUTES *pSecAttr = NULL; - struct _Py_SECURITY_ATTRIBUTE_DATA secAttrData; #endif #ifdef HAVE_MKDIRAT int mkdirat_unavailable = 0; @@ -5727,26 +5587,34 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd) #ifdef MS_WINDOWS Py_BEGIN_ALLOW_THREADS - switch (mode) { - case 0x1C0: // 0o700 - error = initializeMkdir700SecurityAttributes(&pSecAttr, &secAttrData); - break; - default: - error = initializeDefaultSecurityAttributes(&pSecAttr, &secAttrData); - break; + 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); - error = clearSecurityAttributes(&pSecAttr, &secAttrData); - } else { - // Ignore error from "clear" - we have a more interesting one already - clearSecurityAttributes(&pSecAttr, &secAttrData); + 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 (error) { - PyErr_SetFromWindowsErr(error); - return NULL; + return PyErr_SetFromWindowsErr(error); } if (!result) { return path_error(path);