Using TCP qrexec for updates proxy hangs on EOF

[marmarek]

How to file a helpful issue

Qubes OS release

R4.2

Brief summary

Normal updates proxy operation works, but getting the "filtered" page (used by whonix to check if updates proxy uses tor or not) hangs. The issue does not happen when using executable service.

Steps to reproduce

1. Use Use new TCP support in qrexec instead of calling socat qubes-core-agent-linux#495
2. Start a template (or other qube that is set to use updates proxy)
3. curl -v http://127.0.0.1:8082/

Expected behavior

Get the 403 message and curl terminates

Actual behavior

Get the 403 message, but curl remain waiting.

$ curl -v http://127.0.0.1:8082/
* processing: http://127.0.0.1:8082/
*   Trying 127.0.0.1:8082...
* Connected to 127.0.0.1 (127.0.0.1) port 8082
> GET / HTTP/1.1
> Host: 127.0.0.1:8082
> User-Agent: curl/8.2.1
> Accept: */*
> 
* HTTP 1.0, assume close after body
< HTTP/1.0 403 Filtered
< Server: tinyproxy/1.10.0
< Content-Type: text/html
< Connection: close
< 
<?xml version="1.0" encoding="UTF-8" ?>
...

The issue is most likely related to HTTP 1.0 - and curl waiting for EOF (with HTTP 1.1 it doesn't need to wait).

I guess it may be caused by #9142, but I'm opening separate issue anyway, because I'm not sure.

[marmarek]

I tried also setting StandardError=journal in [email protected] (which is a good idea anyway), but it didn't help.

[marmarek]

@DemiMarie

[marmarek]

I suspected it might be related to using close() on stdout by qrexec-client-vm (called from [email protected]), instead of shutdown(..., SHUT_WR). But it doesn't explain why executable service works. Anyway, part of exploring this idea was checking why close_stdout() treats fd == 1 differently, and the answer is in this commit: QubesOS/qubes-core-agent-linux@6bddcfc
In the end, it's probably irrelevant, but if anyone would wonder why while looking there, here is the answer.

[DemiMarie]

@marmarek got this, working on it.

[marmarek]

I'm attaching strace of a failed state:

• pid1 (that starts qrexec-client-vm) from the template

• qrexec agent in the updatevm

I did the curl call as above, it hanged, and then I interrupted it after several seconds. You can see which FDs are closed and which aren't at that point.

[DemiMarie]

qrexec-client-vm is launched via qubes-updates-proxy-forwarder.socket, which has Accept=yes set. This means that qrexec-client-vm has the same file description for both stdin and stdout, so even though it closes stdout, it could still write to the socket via stdin. Therefore curl does not receive EOF and hangs.

To trigger this bug, all of the following conditions must be met:

1. The remote side must close the writing side of the connection.
2. If the remote service is executable, it must not exit, as otherwise MSG_DATA_EXIT_CODE would call close_stdout(), which in this case closes stdin (FD 0) of qrexec-client-vm.
3. qrexec-client-vm must not get EOF on stdin.
4. Either the remote side does not close its stdin, or the local side does not continue to write data. In this case, I believe tinyproxy does not close the socket to prevent a connection reset, which would cause client buffers to be lost.

If all four conditions are met, the local caller of qrexec-client-vm should get EOF but will not. To fix this, qrexec-client-vm must call shutdown(1, SHUT_WR) if FD 1 is a socket. The previous behavior is intentional, but it cannot be fixed without either leaving this bug unfixed or using a disgusting hack (below).

The reason that the previous service worked is that after either direction of the channel is shut down, socat will wait for a timeout and then shut down the other side of the channel, even if the other direction of the channel is continuing to transmit data. The default timeout is 0.5 seconds, so this would add a pointless 0.5 second delay. I’m not particularly inclined to reimplement this behavior in qrexec, but I will if this is needed for the R4.2 backport.

[marmarek]

The code already has a path for using shutdown(1, SHUT_WR), but it isn't used for this case (because stdout_fd == 1) - see my earlier comment. What about adding an option to force using shutdown anyway?

[DemiMarie]

That’s fine, but it will need a new API in libqrexec and a corresponding change in both qrexec-client-vm and qrexec-client. Is that okay?

[marmarek]

Ugh, I guess it means a change to the struct process_io_request, which is allocated by the process main code, and then used by libqrexec, right? Can you find some way for a backward compatible change? Maybe you can use some padding in the existing structure for a new field (if I'm not mistaken, there should be some near 3 bools)? If such padding is zeroed in the old version of course, but I think it is.

[DemiMarie]

That’s not guaranteed to work with GCC, as GCC doesn’t initialize padding when a designated initializer is used, though clang does (GCC bug 77992). A cleaner solution is to use stdout_fd == stdin_fd as the trigger for calling shutdown(). This condition is currently true for socket services and is false otherwise. In particular, it is currently always false when file descriptors 0 and 1 are used. Therefore, this won’t break compatibility.

[marmarek]

I guess that would need to be enough, but do document it in the header. And then have an option like --use-fd0-socket (any idea for a better name?) ?

[DemiMarie]

Is it safe to use closefrom() in non-fuzzer builds? Since I will be using close() instead of shutdown() I need to guarantee that there are no other references to the sockets that will be closed. closefrom() allows avoiding the hard-coded limit in fix_fds().

[marmarek]

Since I will be using close() instead of shutdown()

I'm confused, isn't this exactly the opposite? You said earlier you need to use shutdown() to fix the issue. And also, with close() you can't half-close (needed when client sends EOF, but still wanting to retrieve response, for example)

[DemiMarie]

Since I will be using close() instead of shutdown()

I'm confused, isn't this exactly the opposite? You said earlier you need to use shutdown() to fix the issue. And also, with close() you can't half-close (needed when client sends EOF, but still wanting to retrieve response, for example)

The fix (QubesOS/qubes-core-qrexec#151) does some refactoring: instead of calling shutdown() unless the file descriptor was inherited, it calls shutdown() iff the stdin and stdout file descriptors are the same. This is much cleaner, but means that shutdown() will not be called for executable services. Executable services never need half-closing (and if they did, it would be done in do_fork_exec(), not process_io()), but they can hold on to any leaked file descriptors for a while in qrexec-agent, as qrexec-agent keeps a process open to handle PAM. So I had to explicitly close the parent side of the file descriptors if they exceeded 256 (maximum value that fix_fds() would close) to avoid hangs. closefrom() avoids this problem simply because it has no upper limit.

[marmarek]

Executable services never need half-closing

This isn't exactly true: https://github.com/QubesOS/qubes-core-qrexec/blob/main/libqrexec/process_io.c#L195-L214 (but you changed this too)

Anyway, closefrom() seems to be available in glibc >= 2.34, Debian bookworm has 2.36, so it should be fine.

[DemiMarie]

Executable services never need half-closing

This isn't exactly true: https://github.com/QubesOS/qubes-core-qrexec/blob/main/libqrexec/process_io.c#L195-L214 (but you changed this too)

You might want to edit this to include the exact commit sha1, so that future viewers do not get confused.

Anyway, closefrom() seems to be available in glibc >= 2.34, Debian bookworm has 2.36, so it should be fine.

Thanks! I used close_range(), which is the underlying syscall.

[DemiMarie]

Having to pass manually --use-stdin-socket isn’t great. What if the code automatically detected (perhaps via stat()?) when the file descriptions for stdin and stdout are the same and both are sockets, and switched to shutdown() automatically in that case? This is actually less code than wiring in the command-line option.

I’ll write a regression test for this later today. It turns out to be quite easy: send MSG_DATA_STDOUT with an empty value, read from the stdin/stdout socket, and check that EOF is received.

[marmarek]

Thanks! I used close_range(), which is the underlying syscall.

Actually, I'd prefer closefrom() since it does include a fallback for older kernels.

[marmarek]

Having to pass manually --use-stdin-socket isn’t great. What if the code automatically detected (perhaps via stat()?) when the file descriptions for stdin and stdout are the same and both are sockets, and switched to shutdown() automatically in that case? This is actually less code than wiring in the command-line option.

This is interesting idea, but I fear it may be unreliable. Explicit request (via SIGUSR1 on the service side, via cmdline switch on the client side) is more reliable.

[DemiMarie]

Having to pass manually --use-stdin-socket isn’t great. What if the code automatically detected (perhaps via stat()?) when the file descriptions for stdin and stdout are the same and both are sockets, and switched to shutdown() automatically in that case? This is actually less code than wiring in the command-line option.

This is interesting idea, but I fear it may be unreliable. Explicit request (via SIGUSR1 on the service side, via cmdline switch on the client side) is more reliable.

For the service side I agree, but why is it unreliable on the client side? I suspect that passing the same file description for both stdin and stdout is uncommon, and that by far the most likely cause is either using Accept=true for systemd, or using a different program that does something similar. In both cases this is correct.

[marmarek]

the most likely cause

I agree, "most likely". But in the unlikely case you still end up with a deadlock.

[DemiMarie]

Thanks! I used close_range(), which is the underlying syscall.

Actually, I'd prefer closefrom() since it does include a fallback for older kernels.

I decided to open-code it, as neither close_range() nor closefrom() are available on musl and I don’t want to break the Alpine template being worked on by the community. If SYS_close_range is undefined, or if close_range() fails with ENOSYS, the code falls back to opening /proc/self/fd and iterating over it. I tested both the happy path (SYS_close_range defined and close_range() works) and the case where SYS_close_range is undefined.

[DemiMarie]

the most likely cause

I agree, "most likely". But in the unlikely case you still end up with a deadlock.

What is the unlikely case you are referring to? The one I was referring to is where someone passes the same socket file description for both stdin and stdout and expects to be able to use the socket after qrexec-client or qrexec-client-vm returns.

[marmarek]

That too - in fact this was the very reason why shutdown was not used for FD 0/1 (linked here before). Lets not re-introduce this issue.

[marmarek]

I suspect that passing the same file description for both stdin and stdout is uncommon

In fact it is very common - all kind of interactive use does it all the time. If you happen to have a shell connected to a socket instead of TTY, your proposed approach would break stuff.

[DemiMarie]

I suspect that passing the same file description for both stdin and stdout is uncommon

In fact it is very common - all kind of interactive use does it all the time. If you happen to have a shell connected to a socket instead of TTY, your proposed approach would break stuff.

1 socket or 2?

[marmarek]

If shell is called with the same socket on stdin/stdout, the child will have the same too.

[DemiMarie]

Another option might be to have a service configuration option to exit if either side of the communication channel closes, rather than waiting until both close. That would allow QubesOS/qubes-core-agent-linux#495 to be merged without breaking existing Whonix VMs (e.g. restored from backup).

[DemiMarie]

The remaining work is tracked by #9176.