Unify SECURITY_ATTRIBUTES code from FileSystemAclExtensions and SafeFileHandle

[carlossanlop]

Follow up of the PR comment where this was suggested: #61297 (comment) (but wait for that PR to be merged).

Unify these two methods:

runtime/src/libraries/System.IO.FileSystem.AccessControl/src/System/IO/FileSystemAclExtensions.cs

Lines 294 to 308 in 57bfe47

	fixed (byte* pSecurityDescriptor = security.GetSecurityDescriptorBinaryForm())
	{
	var secAttrs = new Interop.Kernel32.SECURITY_ATTRIBUTES
	{
	nLength = (uint)sizeof(Interop.Kernel32.SECURITY_ATTRIBUTES),
	bInheritHandle = ((share & FileShare.Inheritable) != 0) ? Interop.BOOL.TRUE : Interop.BOOL.FALSE,
	lpSecurityDescriptor = (IntPtr)pSecurityDescriptor
	};
	using (DisableMediaInsertionPrompt.Create())
	{
	handle = Interop.Kernel32.CreateFile(fullPath, (int)rights, share, &secAttrs, mode, flagsAndAttributes, IntPtr.Zero);
	ValidateFileHandle(handle, fullPath);
	}
	}

runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

Lines 52 to 102 in 39ece73

	private static unsafe SafeFileHandle CreateFile(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options)
	{
	Interop.Kernel32.SECURITY_ATTRIBUTES secAttrs = default;
	if ((share & FileShare.Inheritable) != 0)
	{
	secAttrs = new Interop.Kernel32.SECURITY_ATTRIBUTES
	{
	nLength = (uint)sizeof(Interop.Kernel32.SECURITY_ATTRIBUTES),
	bInheritHandle = Interop.BOOL.TRUE
	};
	}
	int fAccess =
	((access & FileAccess.Read) == FileAccess.Read ? Interop.Kernel32.GenericOperations.GENERIC_READ : 0) |
	((access & FileAccess.Write) == FileAccess.Write ? Interop.Kernel32.GenericOperations.GENERIC_WRITE : 0);
	// Our Inheritable bit was stolen from Windows, but should be set in
	// the security attributes class. Don't leave this bit set.
	share &= ~FileShare.Inheritable;
	// Must use a valid Win32 constant here...
	if (mode == FileMode.Append)
	{
	mode = FileMode.OpenOrCreate;
	}
	int flagsAndAttributes = (int)options;
	// For mitigating local elevation of privilege attack through named pipes
	// make sure we always call CreateFile with SECURITY_ANONYMOUS so that the
	// named pipe server can't impersonate a high privileged client security context
	// (note that this is the effective default on CreateFile2)
	flagsAndAttributes |= (Interop.Kernel32.SecurityOptions.SECURITY_SQOS_PRESENT | Interop.Kernel32.SecurityOptions.SECURITY_ANONYMOUS);
	SafeFileHandle fileHandle = Interop.Kernel32.CreateFile(fullPath, fAccess, share, &secAttrs, mode, flagsAndAttributes, IntPtr.Zero);
	if (fileHandle.IsInvalid)
	{
	// Return a meaningful exception with the full path.
	// NT5 oddity - when trying to open "C:\" as a Win32FileStream,
	// we usually get ERROR_PATH_NOT_FOUND from the OS. We should
	// probably be consistent w/ every other directory.
	int errorCode = Marshal.GetLastPInvokeError();
	if (errorCode == Interop.Errors.ERROR_PATH_NOT_FOUND && fullPath!.Length == PathInternal.GetRootLength(fullPath))
	{
	errorCode = Interop.Errors.ERROR_ACCESS_DENIED;
	}
	throw Win32Marshal.GetExceptionForWin32Error(errorCode, fullPath);
	}

Since they are both very similar.

cc @adamsitnik @jozkee

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Follow up of the PR comment where this was suggested: #61297 (comment)

Unify these two methods:

runtime/src/libraries/System.IO.FileSystem.AccessControl/src/System/IO/FileSystemAclExtensions.cs

Lines 294 to 308 in 57bfe47

	fixed (byte* pSecurityDescriptor = security.GetSecurityDescriptorBinaryForm())
	{
	var secAttrs = new Interop.Kernel32.SECURITY_ATTRIBUTES
	{
	nLength = (uint)sizeof(Interop.Kernel32.SECURITY_ATTRIBUTES),
	bInheritHandle = ((share & FileShare.Inheritable) != 0) ? Interop.BOOL.TRUE : Interop.BOOL.FALSE,
	lpSecurityDescriptor = (IntPtr)pSecurityDescriptor
	};
	using (DisableMediaInsertionPrompt.Create())
	{
	handle = Interop.Kernel32.CreateFile(fullPath, (int)rights, share, &secAttrs, mode, flagsAndAttributes, IntPtr.Zero);
	ValidateFileHandle(handle, fullPath);
	}
	}

runtime/src/libraries/System.Private.CoreLib/src/Microsoft/Win32/SafeHandles/SafeFileHandle.Windows.cs

Lines 52 to 102 in 39ece73

	private static unsafe SafeFileHandle CreateFile(string fullPath, FileMode mode, FileAccess access, FileShare share, FileOptions options)
	{
	Interop.Kernel32.SECURITY_ATTRIBUTES secAttrs = default;
	if ((share & FileShare.Inheritable) != 0)
	{
	secAttrs = new Interop.Kernel32.SECURITY_ATTRIBUTES
	{
	nLength = (uint)sizeof(Interop.Kernel32.SECURITY_ATTRIBUTES),
	bInheritHandle = Interop.BOOL.TRUE
	};
	}
	int fAccess =
	((access & FileAccess.Read) == FileAccess.Read ? Interop.Kernel32.GenericOperations.GENERIC_READ : 0) |
	((access & FileAccess.Write) == FileAccess.Write ? Interop.Kernel32.GenericOperations.GENERIC_WRITE : 0);
	// Our Inheritable bit was stolen from Windows, but should be set in
	// the security attributes class. Don't leave this bit set.
	share &= ~FileShare.Inheritable;
	// Must use a valid Win32 constant here...
	if (mode == FileMode.Append)
	{
	mode = FileMode.OpenOrCreate;
	}
	int flagsAndAttributes = (int)options;
	// For mitigating local elevation of privilege attack through named pipes
	// make sure we always call CreateFile with SECURITY_ANONYMOUS so that the
	// named pipe server can't impersonate a high privileged client security context
	// (note that this is the effective default on CreateFile2)
	flagsAndAttributes |= (Interop.Kernel32.SecurityOptions.SECURITY_SQOS_PRESENT | Interop.Kernel32.SecurityOptions.SECURITY_ANONYMOUS);
	SafeFileHandle fileHandle = Interop.Kernel32.CreateFile(fullPath, fAccess, share, &secAttrs, mode, flagsAndAttributes, IntPtr.Zero);
	if (fileHandle.IsInvalid)
	{
	// Return a meaningful exception with the full path.
	// NT5 oddity - when trying to open "C:\" as a Win32FileStream,
	// we usually get ERROR_PATH_NOT_FOUND from the OS. We should
	// probably be consistent w/ every other directory.
	int errorCode = Marshal.GetLastPInvokeError();
	if (errorCode == Interop.Errors.ERROR_PATH_NOT_FOUND && fullPath!.Length == PathInternal.GetRootLength(fullPath))
	{
	errorCode = Interop.Errors.ERROR_ACCESS_DENIED;
	}
	throw Win32Marshal.GetExceptionForWin32Error(errorCode, fullPath);
	}

Since they are both very similar.

cc @adamsitnik @jozkee

Author:	carlossanlop
Assignees:	-
Labels:	area-System.IO
Milestone:	Future

[deeprobin]

@carlossanlop I could have a look at it. Gladly assign me.

[carlossanlop]

Thank you for offering your help, @deeprobin ! I assigned this issue to you.