NamedPipe: CurrentUserOnly, quick fixes for Unix #27463
Conversation
pjanotti
commented
Feb 26, 2018
pjanotti
commented
- The path for the directory when using current user only was wrong and not using the intended folder.
- Added getpeerid validation on the server side (see Expose PipeOptions.CurrentUserOnly and add implementation when flag is passed #26395 (comment))
* The path for the directory of when using current user only was wrong and not using the intended folder. * Added getpeerid validation on the server side.
| @@ -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> | |||
| { | ||
| using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, PipeOptions.CurrentUserOnly)) | ||
| { | ||
| Assert.Throws<TimeoutException>(() => client.Connect(1)); |
There was a problem hiding this comment.
Is TimeoutException really the right type to be throwing here? It seems like a more helpful exception type could be thrown.
There was a problem hiding this comment.
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.
| @@ -41,6 +41,20 @@ public static void CreateServer_ConnectClient() | |||
| } | |||
| } | |||
|
|
|||
| [Fact] | |||
| [PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix domain socket should have different location in this case. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| [PlatformSpecific(TestPlatforms.AnyUnix)] // On Unix domain socket should have different location in this case. | ||
| public static void CreateServerNotCurrentUserOnly_ClientCurrentUserOnly_ThrowsTimeout_OnUnix() | ||
| { | ||
| var name = GetUniquePipeName(); |
| var name = GetUniquePipeName(); | ||
| using (var server = new NamedPipeServerStream(name, PipeDirection.InOut, 1, PipeTransmissionMode.Byte)) | ||
| { | ||
| using (var client = new NamedPipeClientStream(".", name, PipeDirection.InOut, PipeOptions.CurrentUserOnly)) |
There was a problem hiding this comment.
Does the same issue apply if the server is CurrentUserOnly and the client is not? We should test that direction as well.
There was a problem hiding this comment.
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.
|
I'm going to merge this as it is, since it fixes the incorrect path but let's continue the discussion about removing the extra directory. |
|
Failure on macOS 10.13 is unrelated. |
* NamedPipe: CurrentUserOnly, quick fixes for Unix * The path for the directory of when using current user only was wrong and not using the intended folder. * Added getpeerid validation on the server side. * Clean some dirty changes used for quick validation. * PR feedback round dotnet/corefx#1 Commit migrated from dotnet/corefx@5c7137c
