Unify SECURITY_ATTRIBUTES code from FileSystemAclExtensions and SafeFileHandle

@adamsitnik

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

@carlossanlop

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

@deeprobin

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

Navigation Menu

Repository navigation

Unify SECURITY_ATTRIBUTES code from FileSystemAclExtensions and SafeFileHandle #64017

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	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);
	}
	}

	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);
	}

Unify SECURITY_ATTRIBUTES code from FileSystemAclExtensions and SafeFileHandle #64017

Description

Activity

deeprobin commented on Oct 10, 2023

carlossanlop commented on Oct 10, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions