Fix FileSystemAclExtensions.Create when passing a null FileSecurity #59389

carlossanlop · 2021-09-21T02:40:04Z

Fixes in this PR:

The ACL extension method FileStream.Create should accept a null FileSecurity parameter so the behavior can match .NET Framework. The ref definition had to be changed to a nullable parameter. @bartonjs @GrabYourPitchforks @stephentoub Do I need to go through API review to request this change or can we validate it here? ~~This will be marked as a breaking change because an exception case was removed~~.
FileStream throws ArgumentOutOfRangeException if the passed FileAccess is not valid. In .NET Framework, this was not a problem because all the ACL logic was contained within the FileStream constructor, so it was possible to simply pass the FileSystemRights enum value casted as a FileAccess enum value to the p/invoke. Here, the ACL logic is in an extension method, and we don't want to do such casting. To address this, I am now mapping all the writeable FileSystemRights values to a FileAccess write or read value. I left a detailed explanation here of what we were doing before.
Additional bug fix: The FileShare.Inheritable bit 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). Otherwise, the P/Invoke causes an IOException to be thrown with the message The parameter is incorrect.
Added unit tests to verify all the write combinations when the FileSecurity argument is null, which also includes the code example in the original issue.

ghost · 2021-09-21T02:40:09Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2021-09-21T02:40:13Z

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

Issue Details

Fixes #57768

Fixes in this PR:

The ACL extension method FileStream.Create should accept a null FileSecurity parameter so the behavior can match .NET Framework. The ref definition had to be changed to a nullable parameter. @bartonjs @GrabYourPitchforks @stephentoub Do I need to go through API review to request this change or can we validate it here? This will be marked as a breaking change because an exception case was removed.
FileStream throws ArgumentOutOfRangeException if the passed FileAccess is not valid. In .NET Framework, this was not a problem because all the ACL logic was contained within the FileStream constructor, so it was possible to simply pass the FileSystemRights enum value casted as a FileAccess enum value to the p/invoke. Here, the ACL logic is in an extension method, and we don't want to do such casting. To address this, I am now mapping all the writeable FileSystemRights values to a FileAccess write or read value. I left a detailed explanation here of what we were doing before.
Added unit tests to verify all the write combinations when the FileSecurity argument is null, which also includes the code example in the original issue.

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.IO`
Milestone:	7.0.0

bartonjs · 2021-09-21T17:12:16Z

The ACL extension method FileStream.Create should accept a null FileSecurity parameter so the behavior can match .NET Framework. ... API Review ... ?

Moving from not allowing null to allowing null is fine. I don't think as reviewers we have the muscles developed to be good at inquiring about the state of question marks, and that direction isn't a breaking change.

This will be marked as a breaking change because an exception case was removed.

This the poster child for a "visible behavioral change" that isn't a breaking change. (ArgumentNullException means the problem was in their code. So if we allow null now and they didn't want to, that error is still in their code)

https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/breaking-change-rules.md (Exceptions, Allowed, last bullet)

jeffhandley

Looks good to me. Let's get another review from @adamsitnik or @jozkee though.

jozkee · 2021-10-22T02:42:45Z

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

Since we already have a SafeFileHandle that was opened with the correct FileSystemRights, the sole purpose of passing FileAccess here is to indicate whether the Stream supports reading/writing (or both).

With that said, I think this mapping logic is too complex and doesn't really achieve much. What do you think of simplifying it to always return Read as default and include Write in the special cases?

jozkee

Left some suggestions for you to consider.

jozkee · 2021-10-22T02:57:30Z

src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs

What things can you do with a file created with a non-null FileSecurity argument vs one created with a null one?

You can create a file with specific access rules and audit rules to allow or deny permissions.

jozkee · 2021-10-22T03:04:06Z

src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs

I wouldn't include variable buffer size in this tests, I don't see how that relates with ACL.

jozkee · 2021-10-22T03:04:22Z

src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs

What about readables?

They are disallowed.

If you run a test console app on .NET Framework:

string path = "file.txt"; FileMode mode = FileMode.CreateNew; FileSystemRights rights = FileSystemRights.ReadAndExecute; FileShare share = FileShare.None; int bufferSize = 4096; FileOptions options = FileOptions.None; using (FileStream fs = new FileStream(path, mode, rights, share, bufferSize, options, null)){}

You get the following exception:

'Combining FileMode: CreateNew with FileSystemRights: ReadAndExecute is invalid.' This exception was originally thrown at this call stack: System.IO.FileStream.Init(string, System.IO.FileMode, System.IO.FileAccess, int, bool, System.IO.FileShare, int, System.IO.FileOptions, Microsoft.Win32.Win32Native.SECURITY_ATTRIBUTES, string, bool, bool, bool) in filestream.cs System.IO.FileStream.FileStream(string, System.IO.FileMode, System.Security.AccessControl.FileSystemRights, System.IO.FileShare, int, System.IO.FileOptions, System.Security.AccessControl.FileSecurity) in filestream.cs ConsoleFramework.Program.Main() in Program.cs

So we do a similar verification in .NET 7:

System.ArgumentException : Combining FileMode.CreateNew with FileSystemRights.ReadAndExecute is invalid. Stack Trace: C:\Users\calope\source\repos\runtime\src\libraries\System.IO.FileSystem.AccessControl\src\System\IO\FileSystemAclExtensions.cs(202,0): at System.IO.FileSystemAclExtensions.Create(FileInfo fileInfo, FileMode mode, FileSystemRights rights, FileShare share, Int32 bufferSize, FileOptions options, FileSecurity fileSecurity)

I'm comparing the output of .NET Framework vs .NET 7 with all the combinations of FileMode and FileSystemRights to determine if I need to special-case some unit tests.

jozkee · 2021-10-22T03:04:39Z

src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs

Why not test all values in the enum?

jozkee · 2021-10-22T03:07:10Z

src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs

This should be tested with null and non-null FileSecurity.

jozkee · 2021-10-22T03:09:44Z

src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs

What does this return when you create the file with a null FileSecurity? I assume it doesn't return null.

It returns a FileSecurity object that contains zero access rules. I added a test for that case.

jozkee · 2021-10-22T03:31:36Z

This will be marked as a breaking change because an exception case was removed.

That's not a breaking change, see https://docs.microsoft.com/en-us/dotnet/core/compatibility/#exceptions

✔️ ALLOWED: Removing an exception to enable more robust behavior or new scenarios

adamsitnik

GetFileStreamFileAccess should not return 0, as FileStream is simply going to throw. Please see my comments.

Do we have a unit test that ensures that FileSystemRights.AppendData is working the way it was shown in #57768?

adamsitnik · 2021-10-25T08:29:58Z

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

0 is illegal value for FileAccess, the FileStream ctor is going to throw:

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

Line 73 in 9e795c0

else if (access < FileAccess.Read || access > FileAccess.ReadWrite)

What was the Full .NET Framework doing when user was passing zero for FileSystemRights?

Weird. The unit tests are passing, the FileStream constructor is not throwing. I'm investigating.

Also, notice that we pass (FileAccess)0 as an argument to Init in .NET Framework:
https://referencesource.microsoft.com/#mscorlib/system/io/filestream.cs,569

adamsitnik · 2021-10-25T08:33:42Z

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

Synchronize is not related to reading or writing:

runtime/src/libraries/Common/src/Interop/Windows/NtDll/Interop.NtCreateFile.cs

Lines 565 to 569 in 9e795c0

/// <summary>

/// The right to use the object for synchronization. Enables a thread to wait until the object

/// is in the signaled state. This is required if opening a synchronous handle.

/// </summary>

SYNCHRONIZE = 0x00100000,

Suggested change

(rights & FileSystemRights.Synchronize) != 0 ||

adamsitnik · 2021-10-25T09:11:03Z

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

As I wrote above, currently 0 is invalid value for FileAccess.

I would start with ReadWrite and just negate for cases when read|write intent was not specified.

private static FileAccess GetFileStreamFileAccess(FileSystemRights rights) { FileAccess access = FileAccess.ReadWrite; if ((rights & FileSystemRights.Read) == 0 && ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_READ) == 0) access &= ~FileAccess.Read; if ((rights & FileSystemRights.Write) == 0 && ((int)rights & Interop.Kernel32.GenericOperations.GENERIC_WRITE) == 0) access &= ~FileAccess.Write; if (access == 0) // do what Full Framework did here (use a default value or throw, based on what it actually was) return access; }

But using the code provided above, we still end up with few cases where we end up with 0:

static void Main(string[] args) { foreach (FileSystemRights flag in Enum.GetValues<FileSystemRights>().Select(flag => (int)flag).Distinct()) { Console.WriteLine($"{flag} {GetFileStreamFileAccess(flag)}"); } }

ReadData Read CreateFiles Write AppendData Write ReadExtendedAttributes Read WriteExtendedAttributes Write ExecuteFile 0 DeleteSubdirectoriesAndFiles 0 ReadAttributes Read WriteAttributes Write Write Write Delete 0 ReadPermissions Read Read Read ReadAndExecute Read Modify ReadWrite ChangePermissions 0 TakeOwnership 0 Synchronize 0 FullControl ReadWrite

I don't know what Full .NET Framework was doing in cases where the intent was not specified, example:

FileSystemRights.Synchronize

it would be great to find out what is was doing and either:

use the same default value

throw the same exception (but improve error message if needed)

I am also not sure why anyone would like to create a FileStream without read or write rights. Perhaps just to access FileStream.SafeFileHandle and never use any of the FileStream.Read* and FileStream.Write* methods?

cc @JeremyKuhne who might know the answer

I am also not sure why anyone would like to create a FileStream without read or write rights.

To use as a sentinel perhaps. If all you want to do is keep a file handle open you wouldn't need anything but delete I believe.

…le FileSystemRights.

… FileAccess

carlossanlop · 2021-11-04T22:43:09Z

src/libraries/System.IO.FileSystem.AccessControl/tests/FileSystemAclExtensionsTests.cs

@joperezr Do we still need to check for PlatformDetection.IsNetFramework or can I remove those checks?

carlossanlop · 2021-11-07T21:40:29Z

Closing and resending PR to make it easier to review.

carlossanlop added the area-System.IO label Sep 21, 2021

carlossanlop added this to the 7.0.0 milestone Sep 21, 2021

carlossanlop requested review from adamsitnik, jozkee and stephentoub September 21, 2021 02:40

carlossanlop self-assigned this Sep 21, 2021

ghost added the new-api-needs-documentation label Sep 21, 2021

jeffhandley approved these changes Oct 21, 2021

View reviewed changes

jozkee reviewed Oct 22, 2021

View reviewed changes

adamsitnik reviewed Oct 25, 2021

View reviewed changes

carlossanlop added 6 commits November 4, 2021 13:46

ref: Make FileSecurity parameter nullable.

4c03f14

src: Make FileSecurity optional in FileStream.Create, but always hand…

de8fa27

…le FileSystemRights.

test: Remove exception when FileSecurity is null. Add missing Dispose.

2369ffa

src: Extend writeable FileSystemRights values when converting them to…

cbc630d

… FileAccess

test: Add multiple valid argument combos when FileSecurity is null.

63847f6

Spacing

7ad3a16

carlossanlop commented Nov 4, 2021

View reviewed changes

carlossanlop added 2 commits November 4, 2021 20:13

Fix bug preventing using FileShare.Inheritable.

46f7f7a

Address unit test feedback.

6e8f5e1

carlossanlop closed this Nov 7, 2021

carlossanlop deleted the FSACnull branch November 7, 2021 21:40

carlossanlop mentioned this pull request Nov 7, 2021

Fix FileSystemAclExtensions.Create when passing a null FileSecurity #61297

Merged

ghost locked as resolved and limited conversation to collaborators Dec 8, 2021

	/// <summary>
	/// The right to use the object for synchronization. Enables a thread to wait until the object
	/// is in the signaled state. This is required if opening a synchronous handle.
	/// </summary>
	SYNCHRONIZE = 0x00100000,

Fix FileSystemAclExtensions.Create when passing a null FileSecurity #59389

Fix FileSystemAclExtensions.Create when passing a null FileSecurity #59389

Uh oh!

Conversation

carlossanlop commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 21, 2021

Uh oh!

ghost commented Sep 21, 2021

Uh oh!

bartonjs commented Sep 21, 2021

Uh oh!

jeffhandley left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jozkee commented Oct 22, 2021

Uh oh!

adamsitnik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlossanlop commented Nov 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

carlossanlop commented Sep 21, 2021 •

edited

Loading

carlossanlop Nov 4, 2021 •

edited

Loading