bpo-31512: Add non-elevated symlink support for dev mode Windows 10

[vidartf]

• Adds the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE = 0x2 flag when calling CreateSymbolicLinkW on Windows. It also defines this flag if it is not already defined.
• Removes symlink checks that are unneeded with Windows XP not being supported, c.f. discussion on issue tracker.

This is my first time submitting a PR to cpython, so any help with procedural errors are welcome.

https://bugs.python.org/issue31512

[pfmoore]

I'm not particularly familiar with the details of what was added in Creators update, but wouldn't it be better to leave enable_symlink, for systems that are not in developer mode (where presumably you still need to add the privilege to be allowed to create symbolic links)?

[vidartf]

@pfmoore The enable_symlink functionality was removed based on the feedback by eryksun on the issue. Citing the relevant part:

Py_CreateSymbolicLinkW can be removed in 3.5+ because Windows XP is no longer supported and calling enable_symlink() is pointless.

The Windows API uses some privileges simply to determine which security principals can access a capability. Whether the privilege is currently enabled or disabled in the current access token doesn't matter because functions automatically enable it for the current thread (in an impersonation token).

In this case, CreateSymbolicLink calls RtlAcquirePrivilege to enable SeCreateSymbolicLinkPrivilege for the current thread; sets the symlink reparse point; and then reverts the current thread via RtlReleasePrivilege. It goes through these same steps whether or not the privilege is already enabled in the process, so there's no chance of a race condition between competing threads.

I'm not a good judge for the correctness of this, but lacking any counter arguments I've assumed this to be true. I'll be happy to undo this bit as needed.

[pfmoore]

I'm happy to go with @eryksun's view on this - he has a much better understanding than I do of the lower-level details involved. Thanks for providing the link back to the original discussion.

[zooba]

Looks good to me, with the NEWS grammar tweaks.

[zooba]

And yeah, I believe Eryk is correct here. We shouldn't have to do anything extra to enable the privilege for the process, but the call will fail if the privilege cannot be added due to enforced restrictions.

[vidartf]

I fixed the news grammar. I also did a rebase to put the news entry together with the first commit, and in order to rename the commit according to the dev guide (bpo-xxxx prefix). Other than that:

[...] the call will fail if the privilege cannot be added due to enforced restrictions.

Yes, but currently the error message will change (slightly less informative). I do not know if that is a problem?

Old: OSError: symbolic link privilege not held
New: OSError: [WinError 1314] A required privilege is not held by the client: '<source>' -> '<target>'

[zooba]

Since we're not backporting this (I assume), the changed error message doesn't matter. We should probably add an entry to Doc/whatsnew/3.7.rst in the os module section along the lines of:

os.symlink on Windows will now succeed without administrative privileges when the feature is enabled in the OS. Note that the OSError message has changed for when the required privileges are not held.

Also, no need to rebase PRs. We always do squash merges, so there will only be one commit at the end - rebasing PRs only breaks code reviews.

[vidartf]

We should probably add an entry to Doc/whatsnew/3.7.rst in the os module section [...]

Not sure how I should add this, or if you intended that someone else do this ('we' is rather imprecise here). However, I did take the opportunity to update the doc entry for os.symlink. Is this an OK change?

[eryksun]

It just occurred to me that it's potentially a breaking change if Python stops enabling SeCreateSymbolicLinkPrivilege in the process token. When a server impersonates a client, it uses a duplicate of the client's token that honors the client's requested security quality of service. If the client requests that the impersonation is only "effective", which is the default SQOS setting, then the groups and privileges that are currently disabled in the client token will be removed from the server's impersonation token. Note that removing a group or privilege from a token is permanent; it's not merely disabled.

Here's an example that demonstrates the current behavior with a named pipe. It lists the privileges in the process and thread tokens after calling ImpersonateNamedPipeClient.

from win32pipe import *
from win32file import *
from win32security import *
from win32api import GetCurrentProcess, GetCurrentThread
from win32con import SECURITY_SQOS_PRESENT

hPipeServer = CreateNamedPipe(r'\\?\PIPE\SpamPipe', PIPE_ACCESS_DUPLEX,
                    PIPE_TYPE_BYTE | PIPE_READMODE_BYTE | PIPE_WAIT,
                    PIPE_UNLIMITED_INSTANCES, 512, 512, 0, None)

hPipeClient = CreateFile(r'\\?\PIPE\SpamPipe', GENERIC_READ | GENERIC_WRITE,
                    0, None, OPEN_EXISTING, SECURITY_SQOS_PRESENT |
                    SECURITY_IMPERSONATION | SECURITY_EFFECTIVE_ONLY, None)

WriteFile(hPipeClient, b'Piped Spam')
ReadFile(hPipeServer, 10)

ImpersonateNamedPipeClient(hPipeServer)

hTokenProcess = OpenProcessToken(GetCurrentProcess(), GENERIC_READ)
print('Client Privileges', '=' * 20,
      *sorted(LookupPrivilegeName(None, p[0]) + 
              (' [ENABLED]' if p[1] & SE_PRIVILEGE_ENABLED else '') 
              for p in GetTokenInformation(hTokenProcess, TokenPrivileges)),
      sep='\n')

hTokenThread = OpenThreadToken(GetCurrentThread(), GENERIC_READ, True)
print('\nServer Privileges', '=' * 20,
      *sorted(LookupPrivilegeName(None, p[0])  + 
              (' [ENABLED]' if p[1] & SE_PRIVILEGE_ENABLED else '') 
              for p in GetTokenInformation(hTokenThread, TokenPrivileges)),
      sep='\n')

Output:

Client Privileges
====================
SeBackupPrivilege
SeChangeNotifyPrivilege [ENABLED]
SeCreateGlobalPrivilege [ENABLED]
SeCreatePagefilePrivilege
SeCreateSymbolicLinkPrivilege [ENABLED]
SeDebugPrivilege
SeDelegateSessionUserImpersonatePrivilege
SeImpersonatePrivilege [ENABLED]
SeIncreaseBasePriorityPrivilege
SeIncreaseQuotaPrivilege
SeIncreaseWorkingSetPrivilege
SeLoadDriverPrivilege
SeLockMemoryPrivilege
SeManageVolumePrivilege
SeProfileSingleProcessPrivilege
SeRemoteShutdownPrivilege
SeRestorePrivilege
SeSecurityPrivilege
SeShutdownPrivilege
SeSystemEnvironmentPrivilege
SeSystemProfilePrivilege
SeSystemtimePrivilege
SeTakeOwnershipPrivilege
SeTimeZonePrivilege
SeUndockPrivilege

Server Privileges
====================
SeChangeNotifyPrivilege [ENABLED]
SeCreateGlobalPrivilege [ENABLED]
SeCreateSymbolicLinkPrivilege [ENABLED]
SeImpersonatePrivilege [ENABLED]

SeCreateSymbolicLinkPrivilege is just one of many privileges that are disabled by default and normally removed in this impersonation context. I'm comfortable with changing this because the docs never explicitly stated that Python enables this privilege for the entire process, only that the privilege is required to create symbolic links.

[altamimi513]

good

[vidartf]

Is there anything more that I need to do on this PR, or is this now simply awaiting a favorable time? I'm not familiar with the contribution process for these things.

[izbyshev]

Is there some problem with Windows CI?

But anyway, it wouldn't pass. SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE can't be used unconditionally because it will cause ERROR_INVALID_PARAMETER on any Windows before Creators Update. This fact is not mentioned in CreateSymbolicLink docs. I've tested it on Windows 7 and Windows 8.1, and other people stumbled on that too.

Maybe a feature test could be done instead of old check_CreateSymbolicLink(), for example, call CreateSymbolicLink ("", "", SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE) and check for ERROR_INVALID_PARAMETER.

[izbyshev]

Note that this PR accidentally fixes an unrelated bug by removing code.

The following declaration in Modules/posixmodule.c has an incorrect return type (4-byte DWORD instead of 1-byte BOOLEAN):

/* Grab CreateSymbolicLinkW dynamically from kernel32 */
static DWORD (CALLBACK *Py_CreateSymbolicLinkW)(LPCWSTR, LPCWSTR, DWORD) = NULL;

I've confirmed that this leads to interpreting CreateSymbolicLinkW failure as success if the caller lacks the required privilege (I've had to remove enable_symlink from the original code to make this failure possible), resulting in apparent os.symlink success without creation of the symlink.

I can file a separate issue with a trivial PR fixing the declaration (for backporting only). Alternatively, maybe this PR could be split in two (one that removes code and may be backported; another that adds the feature), but I don't know the workflow. Any advice?

[eryksun]

I'd rather not rely on the order of error checking invalid parameters (i.e. using an invalid path), and I'd also rather not check that the build number is at least 15063. How about using a global value windows_can_symlink_unprivileged, which is initially set. If it's set, try the call with the new flag. If the call fails due to an invalid parameter, retry it without the flag. If that works, then unset windows_can_symlink_unprivileged to avoid the error next time.

[vidartf]

I'd also rather not check that the build number is at least 15063.

Why not? Is this expensive, or is this considered less than ideal for some reason?

How about using a global value windows_can_symlink_unprivileged, which is initially set. If it's set, try the call with the new flag. If the call fails due to an invalid parameter, retry it without the flag. If that works, then unset windows_can_symlink_unprivileged to avoid the error next time.

I could add that, but this does run the risk of misattributing a race condition as a lack of support for this flag, e.g. this scenario:

1. Initial call to CreateSymbolicLink.
2. Some other process modifies something that changes the condition for CreateSymbolicLink failing.
3. Our process checks if CreateSymbolicLink works without flag, now it does, so we wrongly assume that the SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag is unsupported.

As the documentation for CreateSymbolicLink does not mention which conditions cause an ERROR_INVALID_PARAMETER error code, it seems unreliable to use this (but I'm open to the idea that this might be the lesser of evils in this case).

[eryksun]

I'd rather handle the error, but checking the build number works. Ensure it comes from kernel32.dll, since this feature doesn't depend on manifested support for Windows 10. You could factor out _Py_getwindowsversion(OSVERSIONINFO *ver_info) from sys_getwindowsversion.

[izbyshev]

Why not? Is this expensive, or is this considered less than ideal for some reason?

What if the unprivileged symlinks feature were backported to an earlier Windows version? Does Windows 10 Mobile have the same build numbers as normal Windows? Explicit feature testing avoids such kind of questions, and this is what Python does to check availability of various POSIX features at runtime (like syscalls).

I could add that, but this does run the risk of misattributing a race condition as a lack of support for this flag

On the bright side, if the second CreateSymbolicLink succeeds without the new flag, the calling process has the required privilege, so the flag is not needed unless the process privilege set is adjusted afterwards.

Theorizing a bit further, if the second CreateSymbolicLink fails in your scenario, there is an ambiguity too.

• if it fails with ERROR_INVALID_PARAMETER, we can't assume anything about the flag support.
• if it fails with another error, then in presence of races we can't assume anything too because, for example, some check that failed with ERROR_INVALID_PARAMETER before could fail in another way.

So, either we can't set windows_can_symlink_unprivileged if both calls fail or we have to disregard the possibility of races.

[izbyshev]

I'd rather not rely on the order of error checking invalid parameters (i.e. using an invalid path)

It doesn't have to use obviously invalid arguments, for example, it could be CreateSymbolicLink("\\", "\\", SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE). Since arguments are fixed, we can at least test that it works on existing versions of Windows.

I think that if we worry about races, the feature test at module initialization approach is slightly better because the just-in-time error checking approach suffers from exactly the same problem with ERROR_INVALID_PARAMETER being reported due to other reasons than invalid flags but it also deals with arbitrary arguments supplied by the user.

If we don't, error checking seems cleaner.

[vidartf]

Why not? Is this expensive, or is this considered less than ideal for some reason?

What if the unprivileged symlinks feature were backported to an earlier Windows version? [...]

Thanks for clearing that up, I see the motivation now. I'll add the on-use error handling then, but with a comment in the code noting the chance of a race depending on the behavior of CreateSymbolicLink. I can change it to a module load check if there is a consensus for that.

On the bright side, if the second CreateSymbolicLink succeeds without the new flag, the calling process has the required privilege, so the flag is not needed unless the process privilege set is adjusted afterwards.

It might still fail, but with another error code than ERROR_INVALID_PARAMETER, so there's still room for failure ;)

[izbyshev]

You have not removed SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE before retrying CreateSymbolicLinkW.

Also, it'd be better to rename target_is_directory to something like flags.

[vidartf]

Sorry for the commit noise, I'm lacking proper dev env at my current location, so I was abusing CI. It should be OK for a fresh review now.

[izbyshev]

Thank you for the fix. I've left several comments.

[izbyshev]

A note: on systems without the new flag support, we'll do both calls on each os.symlink invocation until the first time the symlink can be created. This is probably not important, but could be avoided if windows_has_symlink_unprivileged_flag was set to FALSE in the case of failure with any error other than ERROR_INVALID_PARAMETER (suffering from the same race as the case of success).

[vidartf]

Thanks for the reviews! It seems things went a little quick yesterday. I've tried to clean up the logic so that there is as few thread blocks as possible, while still enabling threads for all calls out.

[izbyshev]

OK, thank you!

[vidartf]

Rebased to resolve merge conflicts.

[vidartf]

Is there anything more that I need to do on this PR, or is this now simply awaiting a favorable time? I'm not familiar with the contribution process for these things.

[terryjreedy]

Vidar has apparently responded to all comments. Steve, can you re-review and presumably merge?

[brettcannon]

@zooba

[eryksun]

We don't need to suppress the CRT invalid-parameter handler in this code.

[vidartf]

I tried to remove it, but it caused test failures (test_os.Win32SynLinkTests.test_buffer_overflow). See #5989, which introduced these macros here.

[eryksun]

Okay. That makes sense. I forgot that _check_dirW was rewritten using the safe string functions. IMO, it needs improvement. We have path_t strings, so they're null terminated, and we know their length. Also, the check shouldn't be limited to MAX_PATH. But that's for another day.