microsoft / CsWin32

A source generator to add a user-defined set of Win32 P/Invoke methods and supporting types to a C# project.
MIT License
1.99k stars 84 forks source link

CreateRestrictedToken should probably return a SafeAccessTokenHandle #1070

Open chrischu opened 8 months ago

chrischu commented 8 months ago

We are using CreateRestrictedToken together with WindowsIdentity.RunImpersonatedAsync, but this leads to a problem: WindowsIdentity.RunImpersonatedAsync expects a SafeAccessTokenHandle but CreateRestrictedToken returns a SafeFileHandle.

Would it make sense to change the out parameter of CreateRestrictedToken to SafeAccessTokenHandle?

Thanks in advance!

AArnott commented 8 months ago

I'm afraid .NET's BCL defines more SafeHandle-derived types than are justified by the native functions that release them. SafeFileHandle and SafeAccessTokenHandle both release their handles using the win32 CloseHandle function.

The metadata that CsWin32 is based on discloses the function used to release the handle, and CsWin32 either generates a new SafeHandle for that function or reuses one that already exists in .NET. But an ambiguity arises when .NET has many types that do the same thing, and cswin32 cannot predict for a given API which option would be best suited to match the APIs you intend to use it with.

So... we don't have a great solution here. The only solution I can imagine is a special hard-coded list of win32 APIs and the specific pre-existing SafeHandle-derived type that should be used for that particular win32 function, which is a maintenance burden.

It's unfortunate that RunImpersonatedAsync doesn't accept any SafeHandle object.

Converting between these two types is riddled with dangers. Drafting out one idea, I think this would be safe **but only if no one else has a reference to the original SafeFileHandle:

SafeFileHandle sfh = new SafeFileHandle((nint)123, true);
bool success = false;
sfh.DangerousAddRef(ref success);
sfh.Dispose();
SafeAccessTokenHandle sath = new(sfh.DangerousGetHandle());

This deliberately changes the SafeFileHandle's internal state to never release the handle. The new SafeHandle will release it once it gets finalized. So if others have a reference to the original SafeFileHandle, that handle could be released while they are still using it.

Another complicating factor is that SafeAccessTokenHandle does not offer a constructor that takes an unowned handle, so you can't just pass the handle value into the constructor for sake of calling an API but keep the ref count in the original SafeFileHandle either. :(