[security] fix(download): contain archive extraction paths - #289
Closed
Hinotoi-agent wants to merge 1 commit into
Closed
[security] fix(download): contain archive extraction paths#289Hinotoi-agent wants to merge 1 commit into
Hinotoi-agent wants to merge 1 commit into
Conversation
Author
|
Closing this in favor of #227. I rechecked the overlap: #227 already includes the same core archive-extraction containment direction using resolved path ancestry checks, plus broader Windows/diagnostics updates. This PR is the narrower security-only variant, but keeping both open would split maintainer review on the same extraction boundary. If maintainers prefer the focused security-only patch instead of the broader #227, I can reopen or rebase this branch. |
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 PR hardens CloakBrowser's downloaded archive extraction boundary so archive member paths must resolve inside the intended binary cache directory before extraction.
../out_evil/owned.txt.Security issues covered
Before this PR
_extract_tar()and_extract_zip()compared paths withstr(member_path).startswith(str(dest_dir.resolve())).../../../etc/passwd, but it does not prove filesystem ancestry.../out_evil/owned.txtcan resolve to a path whose string still starts with the destination path prefix when the destination is namedout.After this PR
Why this matters
CloakBrowser downloads and extracts Chromium archives into a local cache. That cache boundary should remain intact even if an archive is malformed, malicious, or unexpectedly served from a compromised/misconfigured distribution path.
A string prefix is not a filesystem containment check. Enforcing real path ancestry prevents archive entries from escaping into adjacent paths under the same parent directory.
How this differs from related issue/PR
This overlaps the archive-hardening portion of #227, but keeps the contribution intentionally narrower:
cloakbrowser doctorCLI.cloakbrowser/download.pyplus focused regression tests.Attack flow
Affected code
cloakbrowser/download.py,tests/test_extract.pyRoot cause
Issue: archive member path containment used string-prefix comparison.
Path.resolve()was called, but the containment decision was made with stringstartswith().CVSS assessment
CVSS:3.1/AV:N/AC:H/PR:N/UI:R/S:U/C:H/I:H/A:HRationale:
Safe reproduction steps
out.../out_evil/owned.txt._extract_tar(archive, dest)on the vulnerable code.out_evil/owned.txtnext to the intended destination.Expected vulnerable behavior
Changes in this PR
_is_relative_to()to perform resolved path ancestry checks withPath.relative_to().safe_members.Files changed
cloakbrowser/download.pytests/test_extract.pyMaintainer impact
Fix rationale
Path.relative_to()after resolution expresses the intended boundary directly: the archive member's resolved destination must be inside the resolved extraction directory. This avoids sibling-prefix false positives while keeping the existing extraction flow and macOS bundle symlink support intact.Type of change
Test plan
python -m pytest tests/test_extract.py -q— 14 passedpython -m pytest tests/test_update.py tests/test_proxy.py tests/test_build_args.py tests/test_geoip.py tests/test_config.py -q— 186 passedpython -m ruff check cloakbrowser/download.py tests/test_extract.pypython -m ruff format --check cloakbrowser/download.py tests/test_extract.pypython -m compileall -q cloakbrowser/download.py tests/test_extract.pygit diff --checkpython -m pytest -q— attempted locally, but this environment does not haveplaywrightinstalled; the run stopped withModuleNotFoundError: No module named 'playwright'in browser/stealth tests after the extraction-focused tests had passed.Disclosure notes