Skip to content

gh-133562: disable security descriptor handling on unsupported WinAPI partitions #133563

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

Merged
merged 9 commits into from
May 14, 2025

Conversation

maxbachmann
Copy link
Contributor

@maxbachmann maxbachmann commented May 7, 2025

@zooba you added the handling for mode 0o700. Is it fine to just ignore this mode, or would we need to error out in this case?

@zooba
Copy link
Member

zooba commented May 7, 2025

Is it the ConvertStringSecurityDescriptorToSecurityDescriptorW function that's not available? Surely we support security descriptors everywhere... they're pretty fundamental to Windows. Maybe we need to construct it directly...

This change was in response to a vulnerability report (mkdtemp not meeting the required permissions when run as a non-user account), so skipping it for contexts where it's entirely not possible would be okay. I'd prefer to keep it in if we can, though.

@@ -5726,6 +5726,7 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd)

#ifdef MS_WINDOWS
Py_BEGIN_ALLOW_THREADS
#if defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM)
Copy link
Contributor

@sergey-miryanov sergey-miryanov May 7, 2025

Choose a reason for hiding this comment

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

Maybe you should explicitly use MS_WINDOWS_GAMES or comment that it is for XBox build?

Copy link
Member

Choose a reason for hiding this comment

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

We decided last time around to use inclusive filters rather than exclusion ones - we know the platforms where the API is available, and it won't be removed from those.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explanation!

@maxbachmann
Copy link
Contributor Author

Yes it's ConvertStringSecurityDescriptorToSecurityDescriptorW and SDDL_REVISION_1 which are not available. Security descriptors themselves are.

@zooba
Copy link
Member

zooba commented May 7, 2025

We should at least try building up the SD manually before just disabling it. According to Copilot, this code should do it - could you try integrating it in and see (I'm pretty sure the memory allocations can also be simplified for our case, perhaps with an inline struct definition)?

#include <windows.h>
#include <stdio.h>

int main() {
    // Initialize a security descriptor
    SECURITY_DESCRIPTOR securityDescriptor;
    if (!InitializeSecurityDescriptor(&securityDescriptor, SECURITY_DESCRIPTOR_REVISION)) {
        fprintf(stderr, "Failed to initialize security descriptor. Error: %lu\n", GetLastError());
        return 1;
    }

    // Create a DACL
    // Allocate memory for the DACL (size must accommodate ACEs and DACL header)
    DWORD daclSize = sizeof(ACL) +
                     3 * (sizeof(ACCESS_ALLOWED_ACE) - sizeof(DWORD)) + // Size of 3 ACEs
                     3 * SECURITY_MAX_SID_SIZE; // Maximum size for 3 SIDs
    PACL dacl = (PACL)HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, daclSize);
    if (!dacl) {
        fprintf(stderr, "Failed to allocate memory for DACL. Error: %lu\n", GetLastError());
        return 1;
    }

    if (!InitializeAcl(dacl, daclSize, ACL_REVISION)) {
        fprintf(stderr, "Failed to initialize DACL. Error: %lu\n", GetLastError());
        HeapFree(GetProcessHeap(), 0, dacl);
        return 1;
    }

    // Define and create well-known SIDs
    BYTE systemSidBuffer[SECURITY_MAX_SID_SIZE];
    BYTE administratorsSidBuffer[SECURITY_MAX_SID_SIZE];
    BYTE ownerSidBuffer[SECURITY_MAX_SID_SIZE];
    DWORD sidSize = SECURITY_MAX_SID_SIZE;

    if (!CreateWellKnownSid(WinLocalSystemSid, NULL, systemSidBuffer, &sidSize) ||
        !CreateWellKnownSid(WinBuiltinAdministratorsSid, NULL, administratorsSidBuffer, &sidSize) ||
        !CreateWellKnownSid(WinCreatorOwnerSid, NULL, ownerSidBuffer, &sidSize)) {
        fprintf(stderr, "Failed to create well-known SIDs. Error: %lu\n", GetLastError());
        HeapFree(GetProcessHeap(), 0, dacl);
        return 1;
    }

    // Add ACEs to the DACL
    if (!AddAccessAllowedAceEx(dacl, ACL_REVISION, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, GENERIC_ALL, (PSID)systemSidBuffer) ||
        !AddAccessAllowedAceEx(dacl, ACL_REVISION, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, GENERIC_ALL, (PSID)administratorsSidBuffer) ||
        !AddAccessAllowedAceEx(dacl, ACL_REVISION, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, GENERIC_ALL, (PSID)ownerSidBuffer)) {
        fprintf(stderr, "Failed to add ACE to DACL. Error: %lu\n", GetLastError());
        HeapFree(GetProcessHeap(), 0, dacl);
        return 1;
    }

    // Set the DACL in the security descriptor
    if (!SetSecurityDescriptorDacl(&securityDescriptor, TRUE, dacl, FALSE)) {
        fprintf(stderr, "Failed to set DACL in security descriptor. Error: %lu\n", GetLastError());
        HeapFree(GetProcessHeap(), 0, dacl);
        return 1;
    }

    // Security descriptor successfully initialized
    printf("Security descriptor successfully initialized.\n");

    // Clean up
    HeapFree(GetProcessHeap(), 0, dacl);

    return 0;
}

@maxbachmann
Copy link
Contributor Author

looks like just the descriptor is available:

  • InitializeSecurityDescriptor
  • InitializeAcl
  • CreateWellKnownSid
  • AddAccessAllowedAceEx
  • SetSecurityDescriptorDacl

are all restricted to APP | System as well

@maxbachmann
Copy link
Contributor Author

are all restricted to APP | System as well

looking at the partition definition this means it's available for everything except WINAPI_FAMILY_GAMES

@zooba
Copy link
Member

zooba commented May 7, 2025

Okay, in that case, let's just exclude the functionality. But I'll add some clarifying comments to go with it, since this does look like a security risk (apart from the limited context in which it applies)

maxbachmann and others added 3 commits May 7, 2025 21:48
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
…qqNW1.rst

Co-authored-by: Steve Dower <steve.dower@microsoft.com>
@zooba zooba enabled auto-merge (squash) May 8, 2025 11:54
@maxbachmann
Copy link
Contributor Author

I assume the CI failures are unrelated

@zooba
Copy link
Member

zooba commented May 9, 2025

Hopefully they are, I'll merge from main in case they've been fixed somewhere eles.

@maxbachmann
Copy link
Contributor Author

maxbachmann commented May 14, 2025

ping @zooba the ci does now pass.

Also I do have three more PRs in regards to compiling on xbox:

With those three additional changes + the mimalloc change (I will try to get this merged upstream first), I have Python 3.13 running on the xbox again.

@zooba
Copy link
Member

zooba commented May 14, 2025

Looks like no actual tests were run, I'll update again and see if that helps.

@zooba zooba merged commit e528aef into python:main May 14, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants