winapi audit events returning garbage

[zooba]

The CreateFile and CreateNamedPipe audit events from the winapi module appear to be returning garbage instead of the names. There's potential for buffer overreads and/or information leakage.

Linked PRs

• gh-119690: Adds Unicode support for named pipes in _winapi #119717
• [3.13] gh-119690: Adds Unicode support for named pipes in _winapi (GH-119717) #119732
• [3.12] gh-119690: Adds Unicode support for named pipes in _winapi (GH-119717) #119733
• [3.11] gh-119690: Fixes buffer type confusion in _winapi.CreateNamedPipe audit event #119734
• [3.10] gh-119690: Fixes buffer type confusion in _winapi.CreateFile and _winapi.CreateNamedPipe audit events #119735
• [3.9] gh-119690: Fixes buffer type confusion in _winapi.CreateFile and _winapi.CreateNamedPipe audit events (GH-119735) #123679
• [3.8] gh-119690: Fixes buffer type confusion in _winapi.CreateFile and _winapi.CreateNamedPipe audit events (GH-119735) #123680

[zooba]

At a glance, it looks like _winapi is still using MBCS paths by default. We should switch it all to Unicode. (There may already be an issue for this.)

[eryksun]

In 3.11+, the audit call looks correct in _winapi_CreateFile_impl(). The wrapper was updated to link with CreateFileW() and make file_name a wide-character string (LPCWSTR). The other audited values are of the DWORD type (i.e. unsigned long). Strictly they should be "k" values, not "I" values (i.e. unsigned int), but in practice "k" and "I" are equivalent on Windows. However, prior to 3.11 the audit call is incorrect.

At a glance, it looks like _winapi is still using MBCS paths by default. We should switch it all to Unicode. (There may already be an issue for this.)

I think it's just the CreateNamedPipe() and WaitNamedPipe() calls that need to be fixed.

• _winapi_CreateNamedPipe_impl() mistakenly links with CreateNamedPipeA(), and, in that case, the audit call mistakenly assumes that name is a wide-character string.
• _winapi_WaitNamedPipe_impl() mistakenly links with WaitNamedPipeA(). Apparently this call doesn't get audited.

Here's a couple more that aren't pressing issues:

• new_overlapped() links with CreateEventA, as does _PySignal_Init() in "Modules/signalmodule.c". They should use CreateEventW() for consistency, but it's not an encoding or auditing problem.
• _winapi_CreateJunction_impl() links with LookupPrivilegeValueA(). I don't think we should manually redefine SE_RESTORE_NAME as L"SeRestorePrivilege" just to support directly calling LookupPrivilegeValueW(). Instead, call LookupPrivilegeValueA(). More generally, there is no sufficient reason for this code to try to enable the restore privilege. It is not required to set a junction reparse point. I rewrote _winapi_CreateJunction_impl() more correctly in issue 97586.

[zooba]

In 3.11+, the audit call looks correct in _winapi_CreateFile_impl().

Hmm, I've definitely got garbage in logs from 3.11, but it might be an earlier version. Worth going back and fixing, or else I'll be going back and fixing in my own builds.

I think it's just the CreateNamedPipe() and WaitNamedPipe() calls that need to be fixed.

Thanks for confirming. I would've gone crazy trying to figure out the problem with CreateFile otherwise!

• More generally, there is no sufficient reason for this code to try to enable the restore privilege. It is not required to set a junction reparse point.

Is it not? That privilege was the cause of one of the weirdest bugs I've ever had to diagnose (it was causing an unrelated stat test to succeed when run as admin after the privilege was set but not cleared, because it allowed the backup privilege to be used on a file ACLd for no access 🤦‍♂️)

• I rewrote _winapi_CreateJunction_impl() more correctly ...

I was excited, but I think you rewrote it more correctly for Windows, not for us! As long as there's a specific API for us to use, I'd rather use it, even if technically all the structs used are supported. If a user is not supposed to be creating junctions, they'll get a relevant security error (and if an admin is auditing junction creation, they'll get those audit events).

Either way, no plans to touch anything outside of winapi in this issue.

[eryksun]

I was excited, but I think you rewrote it more correctly for Windows, not for us! As long as there's a specific API for us to use, I'd rather use it, even if technically all the structs used are supported. If a user is not supposed to be creating junctions, they'll get a relevant security error (and if an admin is auditing junction creation, they'll get those audit events).

I don't know what you mean about a user not being allowed to be create junctions. There is no special privilege required to create a junction. You just need synchronize, read-attributes, and write-data access to set the reparse point. There is no API to create a junction per se. There's SetVolumeMountPointW(), but that sets a "\??\Volume{GUID}\" volume mountpoint and registers it with the system mountpoint manager. The latter requires Administrator access.

I rewrote the code for _winapi_CreateJunction_impl() to address multiple serious flaws in the existing code (only intended as is for internal testing) in order to bring it up to a level good enough that we could support adding a function in the os module to support creating junctions. Python's inability to create junctions is a notable gap compared to other scripting languages and shells.

[zooba]

Oh my bad, there isn't an API for creating a junction. I was probably thinking of the mount point API, yeah.

Python's inability to create junctions is a notable gap compared to other scripting languages and shells.

Perhaps, but I haven't heard any requests for it. The closest I've seen is people requesting posixpath.isjunction so they don't have to test for the function.