validate env limits in qfile-dom0-unpacker#209
Open
rishi-jat wants to merge 1 commit into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens parsing of UPDATES_MAX_BYTES and UPDATES_MAX_FILES in qfile-dom0-unpacker to avoid fail-open behavior when environment variables are malformed, enforcing fail-closed semantics.
Changes:
- Introduces
parse_limit_env()usingstrtoll()with strict validation (full parse, no overflow, no negatives). - Ensures invalid environment values cause an error message and immediate exit.
- Replaces
atoll()-based parsing with the new validated parser for both limits.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #209 +/- ##
=======================================
Coverage 69.33% 69.33%
=======================================
Files 10 10
Lines 1275 1275
=======================================
Hits 884 884
Misses 391 391 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Author
|
/cc @ben-grande |
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
rishi-jat
force-pushed
the
fix-validate-env-limits
branch
from
c9e3c08 to
515f84f
Compare
Contributor
|
@rishi-jat If this was assisted by AI you need to mention that it was. See the Qubes OS documentation. (no longer a team member, but familiar with this rule) |
Contributor
|
I am no C expert, I just tested the code. Error messages should contain the reason of the failure, to be able to differentiate amongst them. The last if statement seems to be never reached as errors fall under the previous conditions, but it might be ok to keep it. |
OpenQA test summaryComplete test suite and dependencies: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026060922-devel&flavor=pull-requests Test run included the following:
Upload failures
New failures, excluding unstableCompared to: https://openqa.qubes-os.org/tests/overview?distri=qubesos&version=4.3&build=2026050504-devel&flavor=update
Failed tests19 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/176874#dependencies 24 fixed
Unstable testsDetails
Performance TestsPerformance degradation:28 performance degradations
Remaining performance tests:83 tests
|
Contributor
|
Didn't find failures related to this PR. Because this doesn't feel urgent, we can wait for the next openqarun that will have less failures to better see whats important, as QubesOS/qubes-core-admin#804 has been fixed. |
This was referenced Apr 30, 2026
Contributor
|
This seems similar to https://github.com/QubesOS/qubes-core-qrexec/blob/f6e44eaad81e1eb2465e017b875ae36a03e42277/daemon/qrexec-client.c#L118. Maybe it should be a shared library. Maybe here: https://github.com/QubesOS/qubes-linux-utils/tree/main/qrexec-lib. Wait for Marek to confirm becore making any changes. |
This was referenced May 5, 2026
Member
Yes, good idea. There are a bunch of helper functions in pure.c/pure.h already, so that would be a good place for this one too. |
This was referenced May 15, 2026
This was referenced May 25, 2026
This was referenced Jun 2, 2026
Merged
tests: do not fail dispvm+thunderbird test early if TB is not installed
QubesOS/qubes-core-admin#817
Merged
Member
|
@rishi-jat are you still working on this? |
Author
Sorry for the delay sir , forgot about this issue/PR -- the PR is my queue will update asap. Thanks! |
This was referenced Jun 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change fixes unsafe parsing of UPDATES_MAX_BYTES and UPDATES_MAX_FILES in qfile-dom0-unpacker.
The current implementation uses atoll(), which silently returns 0 on invalid input. Since 0 is interpreted as “no limit”, malformed or unintended values (e.g., non-numeric strings) can effectively disable limits without any error. In the context of copying data into dom0, this results in a fail-open behavior and weakens expected safeguards.
This patch replaces atoll() with strtoll() and adds strict validation. The parsing now:
Any invalid input results in an error message and immediate exit, enforcing fail-closed semantics.
This ensures that user misconfiguration cannot silently remove limits and aligns the behavior with the expectation that invalid input must not degrade safety guarantees.
Fixes QubesOS/qubes-issues#8882