Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
zooba merged 9 commits intopython:mainfrommaxbachmann:patch-3
May 14, 2025

Conversation

maxbachmann
Copy link
Contributor

@maxbachmannmaxbachmann commentedMay 7, 2025
edited by bedevere-appbot
Loading

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

@zooba
Copy link
Member

Is it theConvertStringSecurityDescriptorToSecurityDescriptorW 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-miryanovsergey-miryanovMay 7, 2025
edited
Loading

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 APIis available, and it won't be removed from those.

sergey-miryanov reacted with thumbs up emoji
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
ContributorAuthor

Yes it'sConvertStringSecurityDescriptorToSecurityDescriptorW andSDDL_REVISION_1 which are not available. Security descriptors themselves are.

zooba reacted with thumbs up emoji

@zooba
Copy link
Member

zooba commentedMay 7, 2025
edited
Loading

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
ContributorAuthor

looks like just the descriptor is available:

  • InitializeSecurityDescriptor
  • InitializeAcl
  • CreateWellKnownSid
  • AddAccessAllowedAceEx
  • SetSecurityDescriptorDacl

are all restricted toAPP | System as well

@maxbachmann
Copy link
ContributorAuthor

are all restricted to APP | System as well

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

@zooba
Copy link
Member

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)

maxbachmannand others added3 commitsMay 7, 2025 21:48
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
…qqNW1.rstCo-authored-by: Steve Dower <steve.dower@microsoft.com>
@zoobazoobaenabled auto-merge (squash)May 8, 2025 11:54
@maxbachmann
Copy link
ContributorAuthor

I assume the CI failures are unrelated

@zooba
Copy link
Member

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

@maxbachmann
Copy link
ContributorAuthor

maxbachmann commentedMay 14, 2025
edited
Loading

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 reacted with thumbs up emoji

@zooba
Copy link
Member

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

@zoobazooba merged commite528aef intopython:mainMay 14, 2025
39 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@sergey-miryanovsergey-miryanovsergey-miryanov left review comments

@zoobazoobazooba left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@maxbachmann@zooba@sergey-miryanov

[8]ページ先頭

©2009-2025 Movatter.jp