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: Switch mkdir(mode=0o700) on Windows to use OWNER RIGHTS instead of CURRENT_USER #118515

Merged
merged 1 commit into from
May 2, 2024

Conversation

zooba
Copy link
Member

@zooba zooba commented May 2, 2024

This ensures directories created while elevated are not accessible by the same user when non-elevated.

@zooba zooba added the skip news label May 2, 2024
@zooba zooba requested a review from eryksun May 2, 2024 17:12
@zooba zooba merged commit 8ed5466 into python:main May 2, 2024
36 checks passed
@zooba zooba deleted the gh-118486 branch May 2, 2024 18:43
@@ -5616,13 +5617,25 @@ initializeMkdir700SecurityAttributes(
return GetLastError();
}

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

@eryksun eryksun May 3, 2024

Choose a reason for hiding this comment

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

If you prefer to use "CURRENT_USER", you could just move it to the end of the array. Then the SetEntriesInAclW() call would use a count of 2 or 3 depending on whether the current user is an elevated administrator. The latter is determined by querying TokenElevationType. For example:

    ULONG entry_count = 3;

    TOKEN_ELEVATION_TYPE elevation_type;
    DWORD return_length;
    if (GetTokenInformation(GetCurrentProcessToken(), TokenElevationType,
            &elevation_type, sizeof(elevation_type), &return_length) &&
        elevation_type == TokenElevationTypeFull)
    {
        entry_count = 2;
    }

GetCurrentProcessToken() returns the pseudohandle (HANDLE)-4, which has been supported since Windows 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, but I definitely don't want to try to detect anything about the user's current state. That feels brittle (e.g. if more things are added that ought to be detected).

OWNER RIGHTS is good, and it's actually what I tried to find first. The fallback is just for completeness - I don't expect CreateWellKnownSid to ever fail here, but at least if it does, we're probably not doing anything too bad.

SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants