Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

sheaf
Copy link
Contributor

@sheaf sheaf commented Apr 4, 2024

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

@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?

@sheaf sheaf marked this pull request as draft April 5, 2024 08:23
@sheaf sheaf force-pushed the communication-handle-reopen branch 2 times, most recently from 79cf59a to fc25548 Compare April 5, 2024 13:53
This commit moves the process testsuite to a separate package.
The rationale is that later commits require the test-suite to use a
Custom setup in order to work around Cabal bug #9854. This introduces a
setup-depends dependency on the Cabal library, and we would very much
like to avoid the dependency cycle:

  Cabal depends on process
  process depends on Cabal

Instead, splitting up the test-suite, we have:

  Cabal depends on process
  process-tests depends on process and on Cabal

To run the test-suite, you can use either of the following commands:

> cabal run process-tests:test
> stack test process-tests

There are a few other auxiliary changes, such as:

  - Using Cabal version >= 2.4 in the .cabal files,
    and fixing the associated warnings,
  - Using the CPP mingw32_HOST_OS define to check for Windows;
    this avoids having to define the same variable twice in two
    different packages.
@sheaf sheaf force-pushed the communication-handle-reopen branch from fc25548 to c4e9278 Compare April 5, 2024 14:22
This commit adds the System.Process.CommunicationHandle module, which
provides the cross-platform CommunicationHandle abstraction which allows
Handles to be passed to child processes for inter-process communication.

A high-level API is provided by the function
`readCreateProcessWithExitCodeCommunicationHandle`, which can be
consulted for further details about how the functionality is meant to be
used.

To test this functionality, we created a new "cli-child" executable
component to the process-tests package. To work around Cabal bug #9854,
it was necessary to change the build-type of the package to `Custom`, in
order to make the "cli-child" executable visible when running the test-suite.
The custom Setup.hs script contains more details about the problem.
@sheaf sheaf force-pushed the communication-handle-reopen branch from c4e9278 to 1e23acf Compare April 5, 2024 15:00
@Mistuke
Copy link
Contributor

Mistuke commented Apr 9, 2024

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
Copy link
Contributor Author

sheaf commented Apr 9, 2024

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
Copy link
Contributor

Mistuke commented Apr 9, 2024

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
Copy link
Contributor Author

sheaf commented Apr 10, 2024

@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
Copy link
Contributor Author

sheaf commented Apr 10, 2024

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
Copy link
Contributor Author

sheaf commented Apr 10, 2024

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
Copy link
Contributor

Mistuke commented Apr 10, 2024

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
Copy link
Contributor

Mistuke commented Apr 10, 2024

@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
Copy link
Contributor Author

sheaf commented Apr 11, 2024

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
Copy link
Contributor

Mistuke commented Apr 11, 2024

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
Copy link
Contributor

bgamari commented Apr 11, 2024

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
Copy link
Contributor

Mistuke commented Apr 11, 2024

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
Copy link
Contributor

Mistuke commented Apr 11, 2024

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
Copy link
Contributor

Mistuke commented Apr 11, 2024

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
Copy link
Contributor Author

sheaf commented Apr 22, 2024

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.

@sheaf sheaf closed this Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants