Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

NamedPipe: CurrentUserOnly, quick fixes for Unix #27463

Merged
merged 3 commits into from
Feb 27, 2018
Merged

NamedPipe: CurrentUserOnly, quick fixes for Unix #27463

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6526b4b
NamedPipe: CurrentUserOnly, quick fixes for Unix
Feb 26, 2018
73574ee
Clean some dirty changes used for quick validation.
Feb 26, 2018
4c03e6a
PR feedback round #1
Feb 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/System.IO.Pipes/src/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,4 +285,7 @@
<data name="UnauthorizedAccess_NotOwnedByCurrentUser" xml:space="preserve">
<value>Could not connect to the pipe because it was not owned by the current user.</value>
</data>
<data name="UnauthorizedAccess_ClientIsNotCurrentUser" xml:space="preserve">
<value>Client connection refused because it was not owned by the current user.</value>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?"Was refused"

</data>
</root>
17 changes: 17 additions & 0 deletions src/System.IO.Pipes/src/System/IO/Pipes/NamedPipeServerStream.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,25 @@ async Task WaitForConnectionAsyncCore() =>
private void HandleAcceptedSocket(Socket acceptedSocket)
{
var serverHandle = new SafePipeHandle(acceptedSocket);

try
{
if (IsCurrentUserOnly)
{
uint serverEUID = Interop.Sys.GetEUid();

uint peerID;
if (Interop.Sys.GetPeerID(serverHandle, out peerID) == -1)
{
throw CreateExceptionForLastError(_instance?.PipeName);
}

if (serverEUID != peerID)
{
throw new UnauthorizedAccessException(SR.UnauthorizedAccess_ClientIsNotCurrentUser);
}
}

ConfigureSocket(acceptedSocket, serverHandle, _direction, _inBufferSize, _outBufferSize, _inheritability);
}
catch
Expand Down
2 changes: 1 addition & 1 deletion src/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ internal static string GetPipePath(string serverName, string pipeName, bool isCu
throw CreateExceptionForLastError();
}

return Path.Combine(directory, s_pipePrefix + pipeName);
return Path.Combine(directory, pipeName);
}

return s_pipePrefix + pipeName;
Expand Down
14 changes: 14 additions & 0 deletions src/System.IO.Pipes/tests/NamedPipeTests/NamedPipeTest.CurrentUserOnly.netcoreapp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,20 @@ public static void CreateServer_ConnectClient()
}
}

[Fact]
[PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix domain socket should have different location in this case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I admit I didn't yet review the CurrentUserOnly code that went in and so haven't thought deeply about it. But it's a little worrying that CurrentUserOnly when applied on just the server or client means that the current user can't connect to it. Do we have other approaches we could take that would enable it? Could we get away with only checking user ID or we believe the extra security provided by the directory is critical? Could we try one path and then the other if we fail to connect on the first? Etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could get away with removing the extra directory to avoid this behavior, the downside seems that we need throw on the server side, see #26395 (comment)

There is also a small incorrect thing that I just noticed about the current code: we chmod the directory but don't check actual ownership so for instance root can chmode but owner is still old user (the code ignores that for now).

public static void CreateServerNotCurrentUserOnly_ClientCurrentUserOnly_ThrowsTimeout_OnUnix()
{
var name = GetUniquePipeName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: var => string

using (var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte))
{
using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, PipeOptions.CurrentUserOnly))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the same issue apply if the server is CurrentUserOnly and the client is not? We should test that direction as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently due to the extra folder yes. The test basically ensures that they have a different location when current user only is set. Before I add the reverse test, let's consider if we don't want to instead get rid of the extra directory.

{
Assert.Throws<TimeoutException>(() => client.Connect(1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is TimeoutException really the right type to be throwing here? It seems like a more helpful exception type could be thrown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the client is not going to find it so that is the "natural" exception, the alternative is to go with getpeerid and do not use the directory, see #26395 (comment) for some comments about it.

}
}
}

[Fact]
public static void CreateMultipleServers_ConnectMultipleClients()
{
Expand Down