FileSystemAclExtensions.Create broken in multiple ways

[stephentoub]

On .NET Framework, this:

using System;
using System.IO;
using System.Security.AccessControl;

internal class Program
{
    static void Main()
    {
        string path = @"tmp.txt";
        using (FileStream fs = new FileStream(path, FileMode.Create, FileSystemRights.AppendData, FileShare.None, 1, FileOptions.None, null))
        {
            for (int i = 0; i < 26; i++)
            {
                fs.WriteByte((byte)('a' + i));
                fs.Position = 0;
            }
        }

        const string Expected = "abcdefghijklmnopqrstuvwxyz";
        string text = File.ReadAllText(path);
        Console.WriteLine(text == Expected ?
            "Success" :
            $"Expected {Expected}, got {text}");
    }
}

produces:

Success

On .NET 5/6, you should be able to use FileStreamAclExtensions.Create in the exact same way (its whole purpose for existing is to provide the exact same functionality on core):

using System;
using System.IO;
using System.Security.AccessControl;

internal class Program
{
    static void Main()
    {
        string path = @"tmp.txt";
        using (FileStream fs = FileSystemAclExtensions.Create(new FileInfo(path), FileMode.Create, FileSystemRights.AppendData, FileShare.None, 1, FileOptions.None, null))
        {
            for (int i = 0; i < 26; i++)
            {
                fs.WriteByte((byte)('a' + i));
                fs.Position = 0;
            }
        }

        const string Expected = "abcdefghijklmnopqrstuvwxyz";
        string text = File.ReadAllText(path);
        Console.WriteLine(text == Expected ?
            "Success" :
            $"Expected {Expected}, got {text}");
    }
}

but that blows up with an ArgumentNullException:

Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'fileSecurity')
   at System.IO.FileSystemAclExtensions.Create(FileInfo fileInfo, FileMode mode, FileSystemRights rights, FileShare share, Int32 bufferSize, FileOptions options, FileSecurity fileSecurity)
   at Program.Main()

If you then fix that to allow null, it then fails deeper with how it's mapping to FileStream validation of FileAccess:

Unhandled exception. System.ArgumentOutOfRangeException: Enum value was out of legal range. (Parameter 'access')
   at System.IO.FileStream.ValidateHandle(SafeFileHandle handle, FileAccess access, Int32 bufferSize) in D:\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\FileStream.cs:line 75
   at System.IO.FileStream.ValidateHandle(SafeFileHandle handle, FileAccess access, Int32 bufferSize, Boolean isAsync) in D:\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\FileStream.cs:line 89
   at System.IO.FileStream..ctor(SafeFileHandle handle, FileAccess access, Int32 bufferSize, Boolean isAsync) in D:\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\FileStream.cs:line 115
   at System.IO.FileSystemAclExtensions.Create(FileInfo fileInfo, FileMode mode, FileSystemRights rights, FileShare share, Int32 bufferSize, FileOptions options, FileSecurity fileSecurity) in D:\repos\runtime\src\libraries\System.IO.FileSystem.AccessControl\src\System\IO\FileSystemAclExtensions.cs:line 204
   at Program.Main()

[ghost]

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

Issue Details

---

On .NET Framework, this:

using System;
using System.IO;
using System.Security.AccessControl;

internal class Program
{
    static void Main()
    {
        string path = @"tmp.txt";
        using (FileStream fs = new FileStream(path, FileMode.Create, FileSystemRights.AppendData, FileShare.None, 1, FileOptions.None, null))
        {
            for (int i = 0; i < 26; i++)
            {
                fs.WriteByte((byte)('a' + i));
                fs.Position = 0;
            }
        }

        const string Expected = "abcdefghijklmnopqrstuvwxyz";
        string text = File.ReadAllText(path);
        Console.WriteLine(text == Expected ?
            "Success" :
            $"Expected {Expected}, got {text}");
    }
}

produces:

Success

On .NET 5/6, you should be able to use FileStreamAclExtensions.Create in the exact same way (its whole purpose for existing is to provide the exact same functionality on core):

using System;
using System.IO;
using System.Security.AccessControl;

internal class Program
{
    static void Main()
    {
        string path = @"tmp.txt";
        using (FileStream fs = FileSystemAclExtensions.Create(new FileInfo(path), FileMode.Create, FileSystemRights.AppendData, FileShare.None, 1, FileOptions.None, null))
        {
            for (int i = 0; i < 26; i++)
            {
                fs.WriteByte((byte)('a' + i));
                fs.Position = 0;
            }
        }

        const string Expected = "abcdefghijklmnopqrstuvwxyz";
        string text = File.ReadAllText(path);
        Console.WriteLine(text == Expected ?
            "Success" :
            $"Expected {Expected}, got {text}");
    }
}

but that blows up with an ArgumentNullException:

Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'fileSecurity')
   at System.IO.FileSystemAclExtensions.Create(FileInfo fileInfo, FileMode mode, FileSystemRights rights, FileShare share, Int32 bufferSize, FileOptions options, FileSecurity fileSecurity)
   at Program.Main()

If you then fix that to allow null, it then fails deeper with how it's mapping to FileStream validation of FileAccess:

Unhandled exception. System.ArgumentOutOfRangeException: Enum value was out of legal range. (Parameter 'access')
   at System.IO.FileStream.ValidateHandle(SafeFileHandle handle, FileAccess access, Int32 bufferSize) in D:\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\FileStream.cs:line 75
   at System.IO.FileStream.ValidateHandle(SafeFileHandle handle, FileAccess access, Int32 bufferSize, Boolean isAsync) in D:\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\FileStream.cs:line 89
   at System.IO.FileStream..ctor(SafeFileHandle handle, FileAccess access, Int32 bufferSize, Boolean isAsync) in D:\repos\runtime\src\libraries\System.Private.CoreLib\src\System\IO\FileStream.cs:line 115
   at System.IO.FileSystemAclExtensions.Create(FileInfo fileInfo, FileMode mode, FileSystemRights rights, FileShare share, Int32 bufferSize, FileOptions options, FileSecurity fileSecurity) in D:\repos\runtime\src\libraries\System.IO.FileSystem.AccessControl\src\System\IO\FileSystemAclExtensions.cs:line 204
   at Program.Main()

Author:	stephentoub
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[jozkee]

I don't think this meets the bar for 6.0, we should probably address it in 7.0 though.

[adamsitnik]

The fix would most probably be easy, the question is whether we would be able to include it in 6.0

[jeffhandley]

As a regression from .NET Framework, this would be a reasonable fix to take in RC2 so long as we're very confident in the changes and we evaluate/mitigate the potential risks. Ideally we'd get validation from main.

[carlossanlop]

Is the behavior not expected to be the same if the last argument is new FileSecurity() instead of null?

[carlossanlop]

This was introduced in 5.0, and also the FileSecurity argument was not approved in the API proposal as nullable. But we can change this in 7.0, and backport it to 6.0.

[stephentoub]

Is the behavior not expected to be the same if the last argument is new FileSecurity() instead of null?

No, the implementation on .NET Framework has multiple checks that differentiate behavior when it's null and not null, e.g.
https://referencesource.microsoft.com/#mscorlib/system/io/filestream.cs,1095

also the FileSecurity argument was not approved in the API proposal as nullable

The API reviewers aren't experts in all the functionality for each area and trust the proposer for things like this. If it should be nullable, we can make it nullable.

[carlossanlop]

I have a partial fix for this - The API now accepts a null FileSecurity argument and handles that case by creating a SECURITY_ATTRIBUTES without a security descriptor. This handles the first exception.

For the second exception, I am still trying to decide how to best address this.

In .NET Framework, if the FileSecurity is null, but the user passes a valid FileSystemRights value, then the FileAccess value that is passed to the file creation p/invoke is the FileSystemRights value casted to a FileAccess value: https://referencesource.microsoft.com/#mscorlib/system/io/filestream.cs,705

And because all this ACL logic was done directly in the FileStream class, at that point there was no FileAccess out of range validation.

In our .NET 7.0, we generate the ACL logic in the extension method, then pass the handle and arguments to the FileStream constructor, which only allows the 3 expected FileAccess values:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamHelpers.cs

Lines 86 to 89 in 6115ca2

	else if (access < FileAccess.Read || access > FileAccess.ReadWrite)
	{
	badArg = nameof(access);
	}

And because not all of their values match, we can't just cast the FileSystemRights enum value to a FileAccess like before:

• FileAccess only has 3 values (Read=1, Write=2, ReadWrite=3): https://referencesource.microsoft.com/#mscorlib/system/io/fileaccess.cs,5cc5644ae3acd24b
• FileSystemRights does match the 3 values from FileAccess, but it has many more values. For example, AppendData, which was used in this issue's repro code, has a value of 0x000004: https://referencesource.microsoft.com/#mscorlib/system/security/accesscontrol/filesecurity.cs,39

So I'm inclined to fix this by analyzing all the values of FileSystemRights and choosing the most appropriate FileAccess value to which they belong, particularly because of the comment on the top of the method that currently does that conversion:

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

Lines 259 to 275 in 57bfe47

	// In the context of a FileStream, the only ACCESS_MASK ACE rights we care about are reading/writing data and the generic read/write rights.
	// See: https://docs.microsoft.com/en-us/windows/win32/secauthz/access-mask
	private static FileAccess GetFileStreamFileAccess(FileSystemRights rights)
	{
	FileAccess access = 0;
	if ((rights & FileSystemRights.ReadData) != 0 || ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_READ) != 0)
	{
	access = FileAccess.Read;
	}
	if ((rights & FileSystemRights.WriteData) != 0 || ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_WRITE) != 0)
	{
	access = access == FileAccess.Read ? FileAccess.ReadWrite : FileAccess.Write;
	}
	return access;
	}

FileSystemRights values casted to FileAccess.Read:

• ReadData
• ListDirectory (same as ReadData)
• ReadExtendedAttributes
• ExecuteFile
• Traverse (same as ExecuteFile)
• ReadAttributes
• ReadPermissions
• Read (contains all reads)
• ReadAndExecute (contains all reads + execute)

FileSystemRights values casted to FileAccess.Write:

• WriteData
• CreateFiles (same as WriteData)
• AppendData
• CreateDirectories (same as AppendData)
• WriteExtendedAttributes
• DeleteSubdirectoriesAndFiles
• WriteAttributes
• Delete
• ChangePermissions
• TakeOwnership
• Synchronize
• Write (contains all writes)

FileSystemRights values casted to FileAccess.ReadWrite:

• FullControl
• Modify (contains all reads, writes, executes)

[FlsZen]

@carlossanlop I have code that was just migrated from .NET 4.8 to .NET 6 and ran into FileSystemRights.AppendData being not mapped to FileAccess.Write (audit log files that we create with ReadData | AppendData rights). Glad to see it will be fixed in the future. 👍

[carlossanlop]

I found another bug in the extension method: If FileShare.Inheritable is passed, an IOException is thrown with the message The parameter is incorrect.

This does not happen in .NET Framework.

I found the root cause: The FileShare.Inheritable needs to be saved in the SECURITY_ATTRIBUTES struct (we are already doing this), but should not be passed to the CreateFile P/Invoke (we were not doing this).

I'll include this fix in the PR.