Closed Eclips4 closed 4 months ago
This looks like it'll be specific to your configuration (at least somewhat) - it doesn't trivially reproduce.
If you run test_tempfile
alone, does it pass?
This looks like it'll be specific to your configuration (at least somewhat) - it doesn't trivially reproduce.
If you run
test_tempfile
alone, does it pass?
Result from running test_tempfile
:
Can you try running icacls $env:TEMP
(or icacls "%TEMP%"
) and post the result? It should reveal your username, but you already did that above.
Can you try running
icacls $env:TEMP
(oricacls "%TEMP%"
) and post the result? It should reveal your username, but you already did that above.
PS C:\Users\KIRILL-1\CLionProjects\cpython> icacls "%TEMP%"
%TEMP%: The system cannot find the file specified.
Successfully processed 0 files; Failed processing 1 files
PS C:\Users\KIRILL-1\CLionProjects\cpython> icacls $env:TEMP
C:\Users\KIRILL-1\AppData\Local\Temp NT AUTHORITY\СИСТЕМА:(I)(OI)(CI)(F)
BUILTIN\Администраторы:(I)(OI)(CI)(F)
HOME-PC\KIRILL-1:(I)(OI)(CI)(F)
Successfully processed 1 files; Failed processing 0 files
PS C:\Users\KIRILL-1\CLionProjects\cpython>
Okay, that looks fairly like what I expected (but not what I hoped for!). I'm going to have to do some research into what this error code is caused by, and may have to ask you to try some more things if I can't repro it.
FWIW, I just started python -m test -j0
in main with 2 hr old freshly updated debug/no-gil build and it is running normally (with failures to be reported elsewhere).
@zooba, it's probably because of the localized names of "NT AUTHORITY\SYSTEM" and "BUILTIN\Administrators". I was hoping this wouldn't be an issue, and maybe it isn't. I overlooked that you misclassified the "SYSTEM" account as an alias (i.e. TRUSTEE_IS_ALIAS
) in the EXPLICIT_ACCESS_W
record. It's actually a well-known group (i.e. TRUSTEE_IS_WELL_KNOWN_GROUP
). Try making that small change to the trustee type. The implementation could be more resilient for the English name if it's classified as the right trustee type. Otherwise the new code will have to switch to always using SIDs.
I overlooked that you misclassified the "SYSTEM" account as an alias (i.e.
TRUSTEE_IS_ALIAS
) in theEXPLICIT_ACCESS_W
record. It's actually a well-known group (i.e.TRUSTEE_IS_WELL_KNOWN_GROUP
).
Oh that would make sense. I suspected the localised names, but hadn't figured out the actual reason.
Edit: Actually, hold a minute. With some more reading, I'm betting ADMINISTRATORS is wrong too.
@Eclips4 Can you try applying this patch and running the test again? It's one line:
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c
index 9f4be98b35..3279fad786 100644
--- a/Modules/posixmodule.c
+++ b/Modules/posixmodule.c
@@ -5641,7 +5641,7 @@ initializeMkdir700SecurityAttributes(
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.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;
data->ea[1].Trustee.ptstrName = L"SYSTEM";
data->ea[2].grfAccessPermissions = GENERIC_ALL;
Actually, hold a minute. With some more reading, I'm betting ADMINISTRATORS is wrong too.
"Administrators" is a "BUILTIN" alias group implemented by LSA, not a well-known group in the "NT Authority":
>>> LookupAccountName(None, 'SYSTEM')[2] == SidTypeWellKnownGroup
True
>>> LookupAccountName(None, 'Administrators')[2] == SidTypeAlias
True
Edit: Actually, hold a minute. With some more reading, I'm betting ADMINISTRATORS is wrong too.
@Eclips4 Can you try applying this patch and running the test again? It's one line:
diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 9f4be98b35..3279fad786 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -5641,7 +5641,7 @@ initializeMkdir700SecurityAttributes( 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.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP; data->ea[1].Trustee.ptstrName = L"SYSTEM"; data->ea[2].grfAccessPermissions = GENERIC_ALL;
I've applied your patch and rebuilt the interpreter, but the error from the test suite remains the same :(.
@Eclips4 Can you try the change in #118800 instead? It's a bit more thorough, but should completely avoid localization issues.
The implementation could also switch to using an SDDL string. For example:
ConvertStringSecurityDescriptorToSecurityDescriptorW(
L"D:PAI(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;FA;;;OW)",
SDDL_REVISION_1,
&securityAttributes.lpSecurityDescriptor,
&size)
The "SY", "BA", and "OW" security principle aliases are never localized. securityAttributes.lpSecurityDescriptor
reference would be cleared using LocalFree()
. The _Py_SECURITY_ATTRIBUTE_DATA
structure wouldn't be needed.
@Eclips4 Can you try the change in #118800 instead? It's a bit more thorough, but should completely avoid localization issues.
Yes, it's fixes the error. Thank you!
The implementation could also switch to using an SDDL string
This was my first attempt and it didn't work.
I'll sleep on it before trying it again - we aren't likely to ship the fix this week (sorry, non-English users), so there's a bit of time. I think my main concern is maintainability, but it's not really any more opaque than the current arrangement.
Here's a complete implementation based on ConvertStringSecurityDescriptorToSecurityDescriptorW()
:
static PyObject *
os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd)
/*[clinic end generated code: output=a70446903abe821f input=a61722e1576fab03]*/
{
if (PySys_Audit("os.mkdir", "Oii", path->object, mode,
dir_fd == DEFAULT_DIR_FD ? -1 : dir_fd) < 0) {
return NULL;
}
#ifdef MS_WINDOWS
BOOL result;
DWORD saError = 0;
SECURITY_ATTRIBUTES sa, *pSA = NULL;
Py_BEGIN_ALLOW_THREADS
if (mode == 0700) { // 0o700
// 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).
DWORD sdSize;
if (ConvertStringSecurityDescriptorToSecurityDescriptorW(
L"D:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;FA;;;OW)",
SDDL_REVISION_1,
&sa.lpSecurityDescriptor,
&sdSize))
{
pSA = &sa;
sa.nLength = sizeof(sa);
sa.bInheritHandle = FALSE;
}
else {
saError = GetLastError();
}
}
if (!saError) {
result = CreateDirectoryW(path->wide, pSA);
}
if (pSA && LocalFree(pSA->lpSecurityDescriptor)) {
saError = GetLastError();
}
Py_END_ALLOW_THREADS
if (saError) {
return PyErr_SetFromWindowsErr(saError);
}
if (!result) {
return path_error(path);
}
#else
int result;
#ifdef HAVE_MKDIRAT
int mkdirat_unavailable = 0;
#endif
// rest of POSIX implementation
#endif /* MS_WINDOWS */
Py_RETURN_NONE;
}
Agreed. Not sure what I was getting wrong the first time around, but it's working now. Removing the infrastructure around setting up SECURITY_ATTRIBUTES
is nice, now that we can see using SDDL strings is actually viable.
(As an aside, I'm recommending to the security team that we don't sit on patches in the future while waiting for a release. There will be some nuance, of course, but for most things that we have to deal with I think we're better off having the patch out in the open ASAP and associated with the CVE so that the open-source model can do its job and we end up with better fixes. Unfortunately, the CVE system has "rules" that don't allow this, so we'll likely have to fight with them before we can do it officially...)
I've updated #118800 to use the SDDL string rather than the far more complicated manual code. It's adapted from my own version of the code rather than Eryk's message (because I didn't see the full one until after I did it!), but I greatly appreciate the confirmation.
Looking for a review on the PR before I merge and backport (and merge into my manual backports for 3.8-3.12).
Just updating the title to hopefully help people find this before they report again.
Bug report
Bug description:
CPython versions tested on:
CPython main branch
Operating systems tested on:
Windows
Linked PRs