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.

Sign up for GitHub

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

Jump to bottom

qrexec wrongly skips broken symbolic links when searching for services #9089

Closed
DemiMarie opened this issue Apr 4, 2024 · 0 comments · Fixed by QubesOS/qubes-core-qrexec#139
Closed

qrexec wrongly skips broken symbolic links when searching for services #9089

DemiMarie opened this issue Apr 4, 2024 · 0 comments · Fixed by QubesOS/qubes-core-qrexec#139
Labels
affects-4.1 This issue affects Qubes OS 4.1. affects-4.2 This issue affects Qubes OS 4.2. C: core diagnosed Technical diagnosis has been performed (see issue comments). P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. pr submitted A pull request has been submitted for this issue. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists.

Comments

@DemiMarie
Copy link

How to file a helpful issue

Qubes OS release

R4.2, but I think R4.1 is probably affected too.

Brief summary

When searching for a service to execute, qrexec skips broken symbolic links. Instead, it should treat a broken symbolic link as an error and terminate the search.

Steps to reproduce

Run this script as root:

# /dev/null/doesnotexist is guaranteed to not exist by POSIX
ln -s /dev/null/doesnotexist /etc/qubes-rpc/qubes.TestService+a
ln -s /bin/true /etc/qubes-rpc/qubes.TestService

Then invoke qubes.TestService+a from any VM.

Expected behavior

Fails

Actual behavior

Succeeds
@DemiMarie DemiMarie added T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists. C: core P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. labels Apr 4, 2024
@andrewdavidwong andrewdavidwong added affects-4.1 This issue affects Qubes OS 4.1. affects-4.2 This issue affects Qubes OS 4.2. needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. labels Apr 4, 2024
@DemiMarie DemiMarie added diagnosed Technical diagnosis has been performed (see issue comments). and removed needs diagnosis Requires technical diagnosis from developer. Replace with "diagnosed" or remove if otherwise closed. labels Apr 4, 2024
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 5, 2024
@DemiMarie
Add test for broken symbolic links as services
f74981f 
It currently fails (QubesOS/qubes-issues#9089).
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 5, 2024
@DemiMarie
Add test for missing service arguments
dcb1f0c 
The test currently fails due to QubesOS/qubes-issues#9089.
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 5, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
b928655 
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.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but the complexity of this makes it undesirable
for a bug fix.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 5, 2024
@DemiMarie
Add test for missing service arguments
074a692 
The test currently fails due to QubesOS/qubes-issues#9089.
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 5, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
de2e7fc 
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.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but the complexity of this makes it undesirable
for a bug fix.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 5, 2024
@DemiMarie
Add test for missing service arguments
50ce10d 
The test currently fails due to QubesOS/qubes-issues#9089.  The test for
socket services will be added along with the fix, as without the fix it
hangs.
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 5, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
18da240 
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.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but the complexity of this makes it undesirable
for a bug fix.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 5, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
6724d4b 
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.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but that would mean parsing the call twice,
which is undesirable.

In addition to qubes.Service+ not being searched for, the call metadata
(for socket services) and QREXEC_SERVICE_FULL_NAME (for executable
services) didn't include the "+".  Fix that as well, so that
qubes.Service and qubes.Service+ behave identically.

A test for executable services was added in the previous commit.  Also
add a test for socket services.  This test hangs if qubes.Service+ is
not used, so it is not included in the previous commit.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 6, 2024
@DemiMarie
Add test for missing service arguments
d65d02d 
The test currently fails due to QubesOS/qubes-issues#9089.  The test for
socket services will be added along with the fix, as without the fix it
hangs.
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 6, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
279876e 
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.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but the complexity of this makes it undesirable
for a bug fix.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 6, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
81c4a98 
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.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but the complexity of this makes it undesirable
for a bug fix.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 6, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
f2079d7 
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.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but the complexity of this makes it undesirable
for a bug fix.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 6, 2024
@DemiMarie
Add test for missing service arguments
03feaf7 
The test currently fails due to QubesOS/qubes-issues#9089.  The test for
socket services will be added along with the fix, as without the fix it
hangs.
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 6, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
c103ce3 
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.  It would
be cleaner for qrexec-client to add a "+" to the service descriptor if
no argument is present, but the complexity of this makes it undesirable
for a bug fix.

In the future, service arguments should be required and not passing one
should be treated as an error.  That's for R5.0, though, as there is too
much code in dom0 that depends on the current behavior.

Fixes: QubesOS/qubes-issues#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 6, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
33a17c4 
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, as there is too
much code in dom0 that depends on the current behavior.

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#9089
DemiMarie added a commit to DemiMarie/qubes-core-qrexec that referenced this issue Apr 6, 2024
@DemiMarie
Search for qubes.Service+ if call for qubes.Service is made
16b1121 
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#9089
@DemiMarie DemiMarie mentioned this issue Apr 7, 2024
Merged
@andrewdavidwong andrewdavidwong added the pr submitted A pull request has been submitted for this issue. label Apr 11, 2024
This was referenced May 9, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
affects-4.1 This issue affects Qubes OS 4.1. affects-4.2 This issue affects Qubes OS 4.2. C: core diagnosed Technical diagnosis has been performed (see issue comments). P: default Priority: default. Default priority for new issues, to be replaced given sufficient information. pr submitted A pull request has been submitted for this issue. T: bug Type: bug report. A problem or defect resulting in unintended behavior in something that exists.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Various bugfixes DemiMarie/qubes-core-qrexec
2 participants
@DemiMarie @andrewdavidwong