validate env limits in qfile-dom0-unpacker#209
Open
rishi-jat wants to merge 3 commits 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 71.72% 71.74% +0.02%
==========================================
Files 12 12
Lines 1337 1338 +1
==========================================
+ Hits 959 960 +1
Misses 378 378 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Author
|
/cc @ben-grande |
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=2026061619-devel&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=2026050504-devel&flavor=update
Failed tests16 failures
Fixed failuresCompared to: https://openqa.qubes-os.org/tests/176874#dependencies 30 fixed
Unstable testsDetails
Performance TestsPerformance degradation:29 performance degradations
Remaining performance tests:82 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
This was referenced May 31, 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
Author
|
@marmarek PTAL. Thanks! |
Contributor
@rishi-jat please see this comment above. |
This was referenced Jun 15, 2026
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
- remove unreachable negative value check (never reached after initial digit check) - addresses reviewer note that some error paths seemed unreachable
rishi-jat
force-pushed
the
fix-validate-env-limits
branch
from
1bd68f9 to
3cc072c
Compare
Signed-off-by: Rishi Jat <rishijat098@gmail.com>
Member
|
On Tue, Jun 16, 2026 at 04:36:16PM -0700, bhavishyaa-sahuu wrote:
bhavishyaa-sahuu left a comment (QubesOS/qubes-core-admin-linux#209)
For some reason I don't see this comment on the github UI...
Applied maintainer feedback:
- Improved error messages with specific reasons (already in previous commit).
- Removed the unreachable `if (limit < 0)` branch in `parse_limit_env` (it could never be hit due to the leading-digit check before `strtoll`; this addresses the note that some conditions seemed never reached).
- Cleaned up branch history: rebased the two feature commits cleanly on top of current `main` (the branch previously carried unrelated merge history from other PRs). PR diff is now minimal and only touches the intended file.
Regarding the shared library suggestion (#209 (comment) and follow-ups): a general helper (similar to `parse_int` in qrexec or the pure validation funcs) belongs in `qubes-linux-utils/qrexec-lib/pure.h` + impl (as @marmarek suggested). This PR keeps a small static helper local to the unpacker because:
- the function does I/O + exit on error (getenv, fprintf+exit), while pure/ is for side-effect free validators
- it can be extracted/moved later without API break once a shared `qubes_pure_strtonum` / `qubes_parse_limit` etc. is introduced and the `qubes-utils-devel` BuildRequires is bumped.
This should be ready to merge (the functional change is isolated, tested for error paths, and history clean). OpenQA etc. can be re-run as needed.
Currently this fails to build, looks like the PR to linux-utils package
is missing (with the qubes_pure_parse_nonneg_ll function).
…--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
|
Author
Yep i also seen that message on mail but not able to see on GitHub Ui -- I haven't raised PR to Linux-utils package yet will open asap. Thanks! |
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