Various bugfixes #139
Various bugfixes #139
Conversation
It currently fails (QubesOS/qubes-issues#9089).
Previously these were ignored. Treat them as errors instead and terminate the search. Fixes: QubesOS/qubes-issues#9089
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #139 +/- ##
==========================================
+ Coverage 76.00% 76.81% +0.81%
==========================================
Files 52 52
Lines 8951 9111 +160
==========================================
+ Hits 6803 6999 +196
+ Misses 2148 2112 -36 ☔ View full report in Codecov by Sentry. |
DemiMarie
force-pushed
the
bugfixes
branch
7 times, most recently
from
087f0fb to
26c579b
Compare
QREXEC_SERVICE_ARGUMENT, QREXEC_REQUESTED_TARGET, and QREXEC_REQUESTED_TARGET_KEYWORD should not be in the environment of the child process unless explicitly set by the qrexec call. The current code fails to unset them.
QREXEC_SERVICE_ARGUMENT, QREXEC_REQUESTED_TARGET and QREXEC_REQUESTED_TARGET_KEYWORD should not be in the environment of the child process unless explicitly set by the qrexec call. Explicitly unset them. Also avoid relying on QREXEC_SERVICE_ARGUMENT not containing glob characters or characters in $IFS. Commands sent from a VM cannot have them due to the sanitization in qrexec-daemon, but commands sent from dom0 could. Fixes: QubesOS/qubes-issues#9091
DemiMarie
force-pushed
the
bugfixes
branch
4 times, most recently
from
27d2d3e to
9b0691c
Compare
The test currently fails due to QubesOS/qubes-issues#9090. The test for socket services will be added along with the fix, as without the fix it hangs.
DemiMarie
force-pushed
the
bugfixes
branch
2 times, most recently
from
9ebd8c0 to
6bfec77
Compare
Previously, qubes.Service+ was searched for if the call was from a VM, but not if the call was from dom0. Furthermore, a call from dom0 with no argument would skip the check for a too-long service name. In the future, service arguments should be required and not passing one should be treated as an error. That's for R5.0, though: there is too much code in dom0 that depends on the current behavior, and Mirage also depends on receiving a call with no argument at all. QREXEC_SERVICE_FULL_NAME (for executable services) and the call metadata (for socket services) still include the actual command, even if the command does not include "+". Therefore, code that needs to differentiate between "no argument" and "empty argument" can do so for calls from dom0. Fixes: QubesOS/qubes-issues#9090
This would have previously been allowed. The test currently fails. Also add various tests for the modern protocol version, which all pass, and a test for QSB-089.
There is no point in allowing calls with an empty service name: if the argument was empty, the call would be unconditionally rejected by the policy daemon, and even if it was _not_ empty, libqrexec would refuse to execute the call. Furthermore, MSG_TRIGGER_SERVICE3 already checks for empty service names and sends MSG_SERVICE_REFUSED without even querying the policy daemon, so querying the policy daemon for MSG_TRIGGER_SERVICE is inconsistent. Fixes: QubesOS/qubes-issues#9098
This avoids a privilege escalation from unprivileged users (not in the "qubes" group). Fixes: QubesOS/qubes-issues#9097
Service config for qubes.Service+arg should also be checked for in the location for qubes.Service. Add a test for that.
Fails because of QubesOS/qubes-issues#9099.
Previously they were skipped. Fixes: QubesOS/qubes-issues#9099
This fails because of QubesOS/qubes-issues#9100
This includes the case where there is an I/O error or it is e.g. a symlink to a nonexistent file. Fixes: QubesOS/qubes-issues#9100
Previously all such errors were ignored. Fixes: QubesOS/qubes-issues#9101
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024041017-4.2&flavor=pull-requests New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.2&build=2024031904-4.2&flavor=update
Failed tests63 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/94176#dependencies 45 fixed
Unstable tests
|
Instead, directly execute the command from C. Marked as draft for five reasons: 1. MSG_JUST_EXEC is now unable to invoke services. This means that wait=False qrexec calls from the Admin API made in dom0 do not work. 2. There is no logging of the service's stderr anymore. 3. libqrexec-utils has an ABI break, meaning that a new library cannot work with old programs and visa versa. 4. This PR is based on another PR (QubesOS#139), not main. 5. All variables with names beginning with QREXEC_ are stripped from the environment. This is a change in behavior compared to the current code. 1, 2, 3, and 4 must be fixed before this can be merged. 5 is a design decision that could go either way.
Instead, directly execute the command from C. Marked as draft for five reasons: 1. MSG_JUST_EXEC is now unable to invoke services. This means that wait=False qrexec calls from the Admin API made in dom0 do not work. 2. There is no logging of the service's stderr anymore. 3. libqrexec-utils has an ABI break, meaning that a new library cannot work with old programs and visa versa. 4. This PR is based on another PR (QubesOS#139), not main. 5. All variables with names beginning with QREXEC_ are stripped from the environment. This is a change in behavior compared to the current code. 1, 2, 3, and 4 must be fixed before this can be merged. 5 is a design decision that could go either way. Fixes: QubesOS/qubes-issues#9062
Instead, directly execute the command from C. Marked as draft for five reasons: 1. MSG_JUST_EXEC is now unable to invoke services. This means that wait=False qrexec calls from the Admin API made in dom0 do not work. 2. There is no logging of the service's stderr anymore. 3. libqrexec-utils has an ABI break, meaning that a new library cannot work with old programs and visa versa. 4. This PR is based on another PR (QubesOS#139), not main. 5. All variables with names beginning with QREXEC_ are stripped from the environment. This is a change in behavior compared to the current code. 1, 2, 3, and 4 must be fixed before this can be merged. 5 is a design decision that could go either way. Fixes: QubesOS/qubes-issues#9062
Instead, directly execute the command from C. Marked as draft for four reasons: 1. There is no logging of the service's stderr anymore. 2. libqrexec-utils has an ABI break, meaning that a new library cannot work with old programs and visa versa. 3. This PR is based on another PR (QubesOS#139), not main. 4. All variables with names beginning with QREXEC_ are stripped from the environment. This is a change in behavior compared to the current code. 1, 2, and 3 must be fixed before this can be merged. 4 is a design decision that could go either way. Fixes: QubesOS/qubes-issues#9062
Instead, directly execute the command from C. Marked as draft for four reasons: 1. There is no logging of the service's stderr anymore. 2. This PR is based on another PR (QubesOS#139), not main. 3. All variables with names beginning with QREXEC_ are stripped from the environment. This is a change in behavior compared to the current code. 1 and 2 must be fixed before this can be merged. 3 is a design decision that could go either way. Fixes: QubesOS/qubes-issues#9062
Instead, directly execute the command from C. Marked as draft for two reasons: 1. This PR is based on another PR (QubesOS#139), not main. 2. All variables with names beginning with QREXEC_ are stripped from the environment, except for QREXEC_SERVICE_PATH. This is a change in behavior compared to the current code. 1 must be fixed before this can be merged. 3 is a design decision that could go either way. Fixes: QubesOS/qubes-issues#9062
Instead, directly execute the command from C. Marked as draft for two reasons: 1. This PR is based on another PR (QubesOS#139), not main. 2. All variables with names beginning with QREXEC_ are stripped from the environment, except for QREXEC_SERVICE_PATH. This is a change in behavior compared to the current code. 1 must be fixed before this can be merged. 2 is a design decision that could go either way. Fixes: QubesOS/qubes-issues#9062
Instead, directly execute the command from C. Marked as draft for two reasons: 1. This PR is based on another PR (QubesOS#139), not main. 2. All variables with names beginning with QREXEC_ are stripped from the environment, except for QREXEC_SERVICE_PATH. This is a change in behavior compared to the current code. 1 must be fixed before this can be merged. 2 is a design decision that could go either way. Fixes: QubesOS/qubes-issues#9062
Instead, directly execute the command from C. Marked as draft for two reasons: 1. This PR is based on another PR (QubesOS#139), not main. 2. All variables with names beginning with QREXEC_ are stripped from the environment, except for QREXEC_SERVICE_PATH. This is a change in behavior compared to the current code. 1 must be fixed before this can be merged. 2 is a design decision that could go either way. Fixes: QubesOS/qubes-issues#9062
The first six commits fix user-visible bugs. The last two fix a bug that is currently unobservable, but the code is fragile and I want to make sure it doesn’t break in the future.
Each of the fixes but the last is split into two commits. The first commit adds a unit test marked as
@unittest.expectedFailure. The second commit is the actual fix and removes the@unittest.expectedFailuredecorator. This allowed me to ensure that the tests failed before the bug was fixed, and passed afterwards.Fixes: QubesOS/qubes-issues#9089
Fixes: QubesOS/qubes-issues#9090
Fixes: QubesOS/qubes-issues#9091
Fixes: QubesOS/qubes-issues#9097
Fixes: QubesOS/qubes-issues#9098
Fixes: QubesOS/qubes-issues#9099
Fixes: QubesOS/qubes-issues#9100
Fixes: QubesOS/qubes-issues#9101