allow FileStream to open any types of files from path

[adamsitnik]

So far FileStream on Windows could be created from a path that was pointing only to a regular file. Sockets and pipes (which can be reffered to using device path like: \\.\pipe\$pipeName) were not supported:

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

Lines 153 to 154 in 9d771a2

	intfileType=handle.GetFileType();
	if(fileType!=Interop.Kernel32.FileTypes.FILE_TYPE_DISK)

This limitation was applied only to non-extended paths:

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

Lines 147 to 151 in 9d771a2

	if(!PathInternal.IsExtended(originalPath))
	{
	// To help avoid stumbling into opening COM/LPT ports by accident, we will block on non file handles unless
	// we were explicitly passed a path that has \\?\. GetFullPath() will turn paths like C:\foo\con.txt into
	// \\.\CON, so we'll only allow the \\?\ syntax.

Which would still allow for opening UNKNOWN (GetFileType returning 0) files like device interfaces from #54143

Since we don't have such limitation for Unix where we just disallow for opening directories using FileStream (it's ok to open a pipe or socket from the path):

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

Line 76 in 9d771a2

if((status.Mode&Interop.Sys.FileTypes.S_IFMT)==Interop.Sys.FileTypes.S_IFDIR)

And it's an additional sys-call for file opening, I think that we can just remove this limitation.

I am going to post benchmark numbers as soon as I rebuild the repo in Release

[ghost]

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

Issue Details

---

So far FileStream on Windows could be created from a path that was pointing only to a regular file. Sockets and pipes (which can be reffered to using device path like: \\.\pipe\$pipeName) were not supported:

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

Lines 153 to 154 in 9d771a2

	intfileType=handle.GetFileType();
	if(fileType!=Interop.Kernel32.FileTypes.FILE_TYPE_DISK)

This limitation was applied only to non-extended paths:

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

Lines 147 to 151 in 9d771a2

	if(!PathInternal.IsExtended(originalPath))
	{
	// To help avoid stumbling into opening COM/LPT ports by accident, we will block on non file handles unless
	// we were explicitly passed a path that has \\?\. GetFullPath() will turn paths like C:\foo\con.txt into
	// \\.\CON, so we'll only allow the \\?\ syntax.

Which would still allow for opening UNKNOWN (GetFileType returning 0) files like device interfaces from #54143

Since we don't have such limitation for Unix where we just disallow for opening directories using FileStream (it's ok to open a pipe or socket from the path):

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

Line 76 in 9d771a2

if((status.Mode&Interop.Sys.FileTypes.S_IFMT)==Interop.Sys.FileTypes.S_IFDIR)

And it's an additional sys-call for file opening, I think that we can just remove this limitation.

I am going to post benchmark numbers as soon as I rebuild the repo in Release

Author:	adamsitnik
Assignees:	-
Labels:	area-System.IO, os-windows
Milestone:	6.0.0

[filipnavara]

Can this have some security implications where application accepting file path would now be able to inadvertently write to a pipe? I think it's okay but it may need to be listed as a breaking change.

[adamsitnik]

Opening files would be 2-3% faster:

Details

Method	Toolchain	fileSize	options	Mean	Ratio	Allocated
OpenClose	\after\corerun.exe	1024	None	20.59 us	0.97	232 B
OpenClose	\before\corerun.exe	1024	None	21.21 us	1.00	232 B
OpenClose	\after\corerun.exe	1024	Asynchronous	22.00 us	0.98	264 B
OpenClose	\before\corerun.exe	1024	Asynchronous	22.42 us	1.00	264 B

[adamsitnik]

Can this have some security implications where application accepting file path would now be able to inadvertently write to a pipe?

I would not consider it a security issue but would prefer experts like @GrabYourPitchforks or @bartonjs to state their opinion.

As of today someone can create a FileStream that points to a pipe on Windows by:

• Opening the handle on their own by using one of the *CreateFile* sys-calls and provide it to FileStream ctor
• using extended path \??\ prefix to point to a pipe
• creating a symbolic link that uses extended path and points to a pipe as well.

To be honest I don't know why we do have this limitation. Perhaps @JeremyKuhne knows?

[filipnavara]

I didn't mean that it would break any security guarantees at the OS or sandbox level. Purely talking about application level validation of input here and a scenario where this additional validation disappears with simple recompilation of app against new framework.

[GrabYourPitchforks]

Can this have some security implications where application accepting file path would now be able to inadvertently write to a pipe?

I don't think this is a huge concern. The only way I can see this happening is if the server allows the untrusted client to provide the entire path to open (not even using concatenation, as buggy as people's implementations of this are) with no validation whatsoever. And at that point, if I wanted to do bad things to your server, I'd overwrite your server's executables on disk directly rather than try to write to a pipe. Basically, I think your app already has to be in a very bad way if this were to open a new attack vector.

One thing that could be interesting is whether this API now allows people to read & write NTFS alternate data streams, which IIRC were blocked previously. (Somebody check me on that.) This would still indicate a bug (lack of validation) on the application's part, but we know for a fact that the majority of developers don't know how to write proper validation code when accepting arbitrary untrusted filenames as inputs. So I don't think it would introduce any new holes, but it could expand existing holes in apps.

[JeremyKuhne]

No security implications here. There were much more aggressive checks on Framework which, afaik, were a combination of:

• Supporting NT4 which didn't have the same protections against writing directly to devices in dangerous ways.
• Lack of clarity on the various path formats and what they represent.

As a side note, I'd push people to \\?\ instead of \??\. They're functionally equivalent, but many Win32 APIs don't recognize the \??\ variant.

[filipnavara]

I don't think this is a huge concern.

I'm not necessarily saying it's a huge concern, more like questioning whether it needs an extra note in release notes or something akin to that.

Here's a scenario I was running in my head:
In our application we receive untrusted HTML content from emails. These emails can contain file URLs. No matter how much we'd like to block and strip everything outright the companies sometimes rely on this for internal reports and ERP tools. So we use a semi-trusted approach where we disallow all local paths (in any form) but allow UNC paths under a user-visible warning when trying to access such a resource. As anyone doing UI can tell you users don't read message boxes, they just click on whatever gets them through. So, third level of validation for us was actually the framework code rejecting things like UNC pipe paths. If I remember correctly UNC pipe reads can be blocking and behave differently than files in that aspect. So if the first couple of validations were bypassed it could lead to some unintentional freeze from a malicious content. Granted, it's very unlikely and this scenario is entirely speculative but I wanted to point it our nevertheless.

[GrabYourPitchforks]

Agree that a change like this would absolutely merit a mention in the release notes.

[stephentoub]

LGTM, thanks (though there are now conflicts to address).