Draft: CommunicationHandle (WinIO): re-open handle in child process with overlap flag

[sheaf]

NB: Only look at the diff compared to #308, i.e. the last commit (as of writing this description, this was: 79cf59aaeb66921884a841d39f1fd5232e19f474).

@Mistuke suggested on the GHC issue tracker that, when communicating via a named pipe on Windows, if the child process wants to use asynchronous I/O, it should re-open the handle that it was passed with the overlap flag set.
I attempt to do that in this PR, but it seems this operation always fails with a "invalid permissions" error. Discussing with @bgamari, we came to the conclusion that Windows does not seem to allow re-opening one end of a pipe created with mkNamedPipe unless one is using the listen-style ConnectPipe API (that we are deliberately not using). See e.g. this StackOverflow answer which says:

You did specify PIPE_UNLIMITED_INSTANCES for the nMaxInstances parameter in CreateNamedPipe call, but as you never called ConnectNamedPipe to create other endpoints, only one CreateFile was allowed.

@Mistuke, are we missing something? Is there a way to call re-open file in this situation that is expected to work?

[Mistuke]

NB: Only look at the diff compared to #308, i.e. the last commit (as of writing this description, this was: 79cf59a).

@Mistuke, are we missing something? Is there a way to call re-open file in this situation that is expected to work?

Arg, I forgot that for named pipes I set the maximum allowed clients to 1 to prevent snooping. That's indeed a problem.

I think the only way this'll work for pipes is when both client and server agree. And so the choice should be explicit. In other words, for async communication we don't strip away the flag during the creation of the pipe.

The reason for the stripping is when we don't know what the other side will use. So I think we need a parameter here sadly.

[sheaf]

Arg, I forgot that for named pipes I set the maximum allowed clients to 1 to prevent snooping. That's indeed a problem.

OK, but I believe that isn't the only problem: when trying to make this change work, I also used PIPE_UNLIMITED_INSTANCES and removed the FILE_FLAG_FIRST_PIPE_INSTANCE flag in the createNamedPipe call in the C code. Yet we still get a permission denied error; is that expected?

I think the only way this'll work for pipes is when both client and server agree. And so the choice should be explicit. In other words, for async communication we don't strip away the flag during the creation of the pipe.

The reason for the stripping is when we don't know what the other side will use. So I think we need a parameter here sadly.

That was what @bgamari and I concluded. We're not sure it's worth making the API more complex for this situation, but we don't entirely understand the negative performance implications of always using synchronous I/O here.

[Mistuke]

Arg, I forgot that for named pipes I set the maximum allowed clients to 1 to prevent snooping. That's indeed a problem.

OK, but I believe that isn't the only problem: when trying to make this change work, I also used PIPE_UNLIMITED_INSTANCES and removed the FILE_FLAG_FIRST_PIPE_INSTANCE flag in the createNamedPipe call in the C code. Yet we still get a permission denied error; is that expected?

ReOpenFile only works with file handles openes through CreateFile. Which I thought is how I open the client side of the handle. So I'll have to debug the code. Do you have a test binary?

I think the only way this'll work for pipes is when both client and server agree. And so the choice should be explicit. In other words, for async communication we don't strip away the flag during the creation of the pipe. The reason for the stripping is when we don't know what the other side will use. So I think we need a parameter here sadly.

That was what @bgamari and I concluded. We're not sure it's worth making the API more complex for this situation, but we don't entirely understand the negative performance implications of always using synchronous I/O here.

For threaded rts it's not an issue as only the calling thread is blocked and the I/O manager has its own native thread handling requests and cancelation. Throughput would drop slightly as pipe reads won't be batched with other I/O operations and you lose the ability to cancel them. (you can't cancel non-async I/O which was one of the big reasons to go to async I/O).

The biggest difficulties I expected would come from the non-threaded rts as it would block the main thread in Haskell (from memory) as the call won't get to I/O manager.

Operationally it should work though as e.g. Std handles to console buffers (i.e. Stdin etc) also can't be handled asynchronously.

So you can try it, but it would be a shame to lose async here. The only throughput loss would be on big reads larger than the buffer size (which I believe is 4k) in async mode we'd buffer the next chunk in the background as we get ERR_MORE_DATA

[sheaf]

@Mistuke Here are two binaries, parent test.exe and child cli-child.exe: process-tests.zip. Let me know if they work for you; after unzipping you should be able to run test.exe +RTS --io-manager=native and trigger the issue.

Running the parent should create two pipes, passing one end of each pipe to the child without the overlapped flag set. The child will try to re-open the handle it gets with the overlapped flag set, and runs into an error:

> test.exe +RTS --io-manager=native
testCommunicationHandle {
parentUsesWinIO: True
cli-child {
 childUsesWinIO: True
cli-child.exe: reOpenFileOverlapped: invalid argument (All pipe instances are busy.)
test.exe: testCommunicationHandle: child exited with exception ExitFailure 1

I now realise that the code might need to be slightly re-structured; currently, the write end of the pipe is created (or rather opened) using CreateFile, but the read end is created using CreateNamedPipe. Given that you say above that we need the handle to have been created with CreateFile to be able to re-open it, we might need to change it so that the side that is passed to the child is opened using CreateFile. At any rate, in the test I gave above, I am re-opening the write side and running into an error. Re-opening the read side gives a "permission denied" error instead of "all pipe instances are busy".

[sheaf]

In this commit (not pushed to this PR), I have updated the mkNamedPipe C function so that the parent end always is the end created by CreateNamedPipeW, with the other end (that we get by calling CreateFileW) being the child end. This experiment left me wondering whether we should instead be calling CreateFileW in the child process (with the overlap mode we want), as opposed to calling it in the parent process. Would that make sense?

[sheaf]

In this commit (not pushed to this PR), I have updated the mkNamedPipe C function so that the parent end always is the end created by CreateNamedPipeW, with the other end (that we get by calling CreateFileW) being the child end. This experiment left me wondering whether we should instead be calling CreateFileW in the child process (with the overlap mode we want), as opposed to calling it in the parent process. Would that make sense?

@bgamari points out that this significantly increases the raciness of the program, as another process might connect to the pipe before the intended child process does.

[Mistuke]

In this commit (not pushed to this PR), I have updated the mkNamedPipe C function so that the parent end always is the end created by CreateNamedPipeW, with the other end (that we get by calling CreateFileW) being the child end. This experiment left me wondering whether we should instead be calling CreateFileW in the child process (with the overlap mode we want), as opposed to calling it in the parent process. Would that make sense?

@bgamari points out that this significantly increases the raciness of the program, as another process might connect to the pipe before the intended child process does.

Indeed, we shouldn't do this as it makes it possible for an adversarial program to connect before the correct. One of the reasons for using the single client limit was because with anonymous pipes we didn't have to worry about the security issue of a third party being able to connect to the pipe remotely. With named pipes the objects have a name so anyone can connect

[Mistuke]

@Mistuke Here are two binaries, parent test.exe and child cli-child.exe: process-tests.zip. Let me know if they work for you; after unzipping you should be able to run test.exe +RTS --io-manager=native and trigger the issue.

Running the parent should create two pipes, passing one end of each pipe to the child without the overlapped flag set. The child will try to re-open the handle it gets with the overlapped flag set, and runs into an error:

> test.exe +RTS --io-manager=native
testCommunicationHandle {
parentUsesWinIO: True
cli-child {
 childUsesWinIO: True
cli-child.exe: reOpenFileOverlapped: invalid argument (All pipe instances are busy.)
test.exe: testCommunicationHandle: child exited with exception ExitFailure 1

I now realise that the code might need to be slightly re-structured; currently, the write end of the pipe is created (or rather opened) using CreateFile, but the read end is created using CreateNamedPipe. Given that you say above that we need the handle to have been created with CreateFile to be able to re-open it, we might need to change it so that the side that is passed to the child is opened using CreateFile. At any rate, in the test I gave above, I am re-opening the write side and running into an error. Re-opening the read side gives a "permission denied" error instead of "all pipe instances are busy".

Thanks. Am heading home now and will take a look.

[sheaf]

One of the reasons for using the single client limit was because with anonymous pipes we didn't have to worry about the security issue of a third party being able to connect to the pipe remotely. With named pipes the objects have a name so anyone can connect.

I thought that anonymous pipes on Windows were named pipes with randomly chosen names. An adversary would be able to look at the existing pipes on the system and connect to them all the same.

[Mistuke]

One of the reasons for using the single client limit was because with anonymous pipes we didn't have to worry about the security issue of a third party being able to connect to the pipe remotely. With named pipes the objects have a name so anyone can connect.

I thought that anonymous pipes on Windows were named pipes with randomly chosen names. An adversary would be able to look at the existing pipes on the system and connect to them all the same.

That's not true as they are created with a single client limit. Which was the point I was making above. An adversary cannot connect to a pipe which has already reached it's connection limit.

In your suggestion the attacker can because for the child to connect by name it must still be allowing new connections.

[bgamari]

That's not true as they are created with a single client limit. Which was the point I was making above. An adversary cannot connect to a pipe which has already reached it's connection limit.

IIUC, even under the status quo there is still a race since pipe creation is in fact a two step process: CreateNamedPipe followed by CreateFile to open the other end. Of course, at least in this case we will know if this race is exploited as CreateFile will fail. We should likely do more to recover in this case.

[Mistuke]

That's not true as they are created with a single client limit. Which was the point I was making above. An adversary cannot connect to a pipe which has already reached it's connection limit.

IIUC, even under the status quo there is still a race since pipe creation is in fact a two step process: CreateNamedPipe followed by CreateFile to open the other end. Of course, at least in this case we will know if this race is exploited as CreateFile will fail. We should likely do more to recover in this case.

That's quite unlikely though, and if you have that much control there are easier ways to do so. You'd need to hook into the API to figure out what the name is. In which case you can just replace any pipe you want and don't need the name at all.

My point was, and you're both getting to far away from this, is that using a named pipe and passing a name to a child makes it unnecessarily easy to hijack the pipe. As easy as first year computer science student can do.

[Mistuke]

And as @sheaf says. Internally CreatePipe which creates anonymous pipes with a random name does the same two step approach. The windows kernel does not have a single step process to create two pipes. So the same race condition exists in the Windows API.

To do what you're suggesting you'd need an equivalent of LD_PRELOAD at which point whether there's a few sys calls in between the creation of the two ends lf the pipe doesn't matter. An attacker can simply replace both ends.

[Mistuke]

That's not true as they are created with a single client limit. Which was the point I was making above. An adversary cannot connect to a pipe which has already reached it's connection limit.

IIUC, even under the status quo there is still a race since pipe creation is in fact a two step process: CreateNamedPipe followed by CreateFile to open the other end. Of course, at least in this case we will know if this race is exploited as CreateFile will fail. We should likely do more to recover in this case.

I think what you're forgetting is that unlike on Unix, there is no syscall that creates two pipes.

these are the officially listed kernel I/O function https://learn.microsoft.com/en-us/windows/win32/devnotes/nt-create-named-pipe-file these are the unofficial ones http://undocumented.ntinternals.net/index.html?page=UserMode%2FUndocumented%20Functions%2FNT%20Objects%2FFile%2FNtCreateNamedPipeFile.html

Notice how NtCreatePipe does not exist. So even with the API that returns two pipe handles, it's emulated by making two system calls.

Here's e.g. the ReactOS implementation of CreatePipe https://doxygen.reactos.org/df/d77/npipe_8c_source.html

and the same can be seen by disassembling the runtime dll in Windows.

So I'm not sure what's being suggested as the alternative here :)

[sheaf]

I'm closing this PR, on the grounds that:

• on Windows you can't re-open a handle that has been created in asynchronous mode in synchronous mode;
• the alternative, of passing the pipe name instead of the handle to the child process, presents too severe security flaws to be considered.

This means that the only way we can use an asynchronous handle in the child is if the parent creates the handle in asynchronous mode to start with.