Do not close FDs 0, 1, or 2 #186
Conversation
DemiMarie
force-pushed
the
no-close-low-fd
branch
from
29a0392 to
e123ed7
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #186 +/- ##
==========================================
+ Coverage 78.84% 79.09% +0.24%
==========================================
Files 55 55
Lines 10146 10182 +36
==========================================
+ Hits 8000 8053 +53
+ Misses 2146 2129 -17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Codecov appears to not be testing what happens in the child process after fork() and the error path of “cannot open |
|
AFAIR unit tests do not cover the PAM handling part, as they are not running as a system service, test runners don't have necessary PAM configuration etc. |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025022106-4.3&flavor=pull-requests Test run included the following:
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2025020404-4.3&flavor=update
Failed tests26 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/127852#dependencies 28 fixed
Unstable testsPerformance TestsPerformance degradation:No issues Remaining performance tests:72 tests
|
This is the only qrexec PR in this test run, so the above failures seems to be regression caused by this change. |
DemiMarie
force-pushed
the
no-close-low-fd
branch
from
e123ed7 to
c7a7826
Compare
libqrexec/libqrexec-utils.h
Outdated
| void buffer_remove(struct buffer *b, int len); | ||
| int buffer_len(struct buffer *b); | ||
| void *buffer_data(struct buffer *b); | ||
| /* Open /dev/null and keep it from being closed before the exec func is called. |
There was a problem hiding this comment.
Isn't it simpler (and safer) to simply open /dev/null just before dup-ing it over 0,1,2 (in the child process already)?
There was a problem hiding this comment.
It’s definitely simpler, but I didn’t want the extra call to open(). That said, giving each child process the same open file description is less than great, as it introduces shared state that should not be there. I don’t know if /dev/null has any such state, but still it isn’t great, so I went ahead and switched to your approach.
|
But also, I question usefulness of this PR as a whole - the closing of standard FDs happens in a process that has a single purpose - wait for the child process and then exit, in the very same function as closing happens. There are few PAM cleanup calls, but it's very unlikely for them to be a problem (especially, it isn't a problem now, or for the last 10 or so years). |
|
Whether or not the last commit in the PR is merged, I definitely think the other commits should be merged. In particular, it turned out that the “close the FD” functionality had no unit tests because the unit tests took a codepath that was too different from the production code. This PR makes the production and test code follow the same path, with the result that the actual bug (
PAM cleanup calls into PAM modules, so it can do anything. I suspect Qubes OS only gets away with it because we have a fairly simple PAM stack by default. PAM cleanup is used for e.g. unmounting filesystems and closing encrypted volumes. The best approach would be for PAM to run with stdin pointed at |
Indeed I was talking about the last commit (which until the last force-push was the only commit in this PR).
Aren't PAM modules expected handle proper logging themselves? I don't think they are supposed to touch calling process's stdin/out/err in any case. And if they would do, that likely would interfere also with cases where they aren't closed (and then replaced with with unrelated thing) - for example it could interfere with an application log file on stderr that is expected in a specific format (different than PAM messages). |
|
As for the other commits, won't that have some non-trivial conflicts with #141 (which I hope is quite close to merge-able state)? |
I can include them in #141 or rebase this PR on top of it. I can also close this PR if you prefer, but I’d prefer that at least the bug fixes and testability changes go in. |
DemiMarie
force-pushed
the
no-close-low-fd
branch
from
c7a7826 to
4b941e6
Compare
Recent run with db5faa9 has only one failure (1 failed out of 4 times this test was run) I tried to get more logs, but it isn't very interesting: I don't see actual reason for the failure here... |
DemiMarie
force-pushed
the
no-close-low-fd
branch
from
db5faa9 to
ca34aa7
Compare
These should never happen, but call exit() if they do. Also avoid freeing an uninitialized PAM handle in such an error case. I do not consider this a security vulnerability because there is no reasonable way I know of for an attacker to trigger this failure, but this commit should still be backported.
Saves an (admittedly cheap) system call.
No functional change intended.
This will be used by tests later. No functional change intended.
This will be used by tests later. No functional change intended.
This also fixes a bug: basename can mutate its argument, so a copy must be passed to it.
This makes the unit test code more like the actual code used by end-users, and therefore makes the tests more accurate. This trips a bug in the code which will be fixed later, requiring a test to be changed to compensate.
If they are closed, another file descriptor could be created with these numbers, and so standard library functions that use them might write to an unwanted place. dup2() a file descriptor to /dev/null over them instead. Also statically initialize trigger_fd to -1, which is the conventional value for an invalid file descriptor. This requires care to avoid closing the file descriptor to /dev/null in fix_fds(), which took over an hour to debug.
DemiMarie
force-pushed
the
no-close-low-fd
branch
from
ca34aa7 to
ae2281c
Compare
|
This PR (currently) introduces a regression, and since it isn't very useful anyway (#186 (comment)) and Demi left the team, lets abandon it instead of handing it to somebody else. |
3 participants
If they are closed, another file descriptor could be created with these numbers, and so standard library functions that use them might write to an unwanted place. dup2() a file descriptor to /dev/null over them instead.