fix(curator-backup): reject non-file tar members in rollback pre-check - #32523
Closed
akaponym-byte wants to merge 1 commit into
Closed
akaponym-byte wants to merge 1 commit into
akaponym-byte wants to merge 1 commit into
Conversation
The rollback path in agent/curator_backup.py uses ``tf.extractall(filter='data')`` when available, with a fallback to plain ``tf.extractall()`` on older interpreters. The pre-check above the call refused tar members whose ``name`` was absolute or contained ``..`` parents, but not members whose *type* was unsafe — so a symlink with a benign-looking name and an attacker-controlled ``linkname`` would slip past. ``filter='data'`` itself catches this on Python 3.12+ and on 3.11.4+ (PEP 706 was backported there), but ``pyproject.toml`` declares ``python>=3.11`` and the explicit ``except TypeError`` branch is only reachable on 3.11.0–3.11.3 where the kwarg is unknown. On those interpreters the fallback materialises the symlink via ``os.symlink(linkname, targetpath)`` and the agent then reads the link target's content on the next ``skill_view``. A malicious tarball under ``~/.hermes/skills/.curator_backups/`` (a co-tenant with write access to the home, a user tricked into importing a third-party "snapshot", or a write-anywhere primitive in another skill) is enough to plant ``skills/leak -> /etc/passwd`` or ``~/.ssh/id_rsa`` ahead of the next ``hermes curator rollback``. The fix mirrors ``hermes_cli/profiles._safe_extract_profile_archive``: extend the pre-check loop to refuse anything that isn't ``isfile()`` or ``isdir()`` — rejecting symlinks, hardlinks, devices and FIFOs at the same gate that already rejects ``/`` and ``..``. This is the same shape as c26af46 ("reject symlinks in skill bundles before install"), one code path over. Regression test forces the legacy fallback (monkeypatches ``extractall`` to raise ``TypeError`` on ``filter='data'``) to exercise the path that 3.11.0– 3.11.3 hits unconditionally, plants a victim file outside the skills tree, and asserts the symlink is never materialised. Without the fix the fallback succeeds and the assertion ``not leak.is_symlink()`` fails. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Author
|
Confirmed duplicate of #23796 — closing. #23796 covers the same
Test there also exercises the legacy 3.11 fallback path and additionally asserts a symlink-mediated file escape, which is strictly stronger than mine. Sorry for the noise — I searched merged PRs for adjacent fixes but didn't think to check open ones with the same scope. |
19 tasks
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
The rollback path in
agent/curator_backup.pyusestf.extractall(..., filter="data")when available, with an explicitexcept TypeErrorfallback to plaintf.extractall(...)on olderinterpreters. The pre-check above the call refused tar members whose
name was absolute or contained
.., but not members whose type wasunsafe — so a tar entry like:
…slipped through the pre-check.
filter="data"itself rejects this on Python 3.12+ and on 3.11.4+ (PEP706 was backported there), but
pyproject.tomldeclarespython>=3.11and the
except TypeErrorbranch is reachable on 3.11.0–3.11.3 wherethe
filterkwarg is unknown. On those interpreters the fallbackmaterialises the symlink via
os.symlink(linkname, targetpath), andthe agent reads the link target's content on the next
skill_view.A malicious tarball under `
/.hermes/skills/.curator_backups/` (e.g. a/.ssh/id_rsa` ahead of the nextco-tenant on a shared host with write access to the home, a user
tricked into importing a third-party 'snapshot', or a write-anywhere
primitive surfaced by another skill) is enough to plant
`skills/leak → /etc/passwd` or `
`hermes curator rollback`.
Fix
Extend the existing pre-check loop with a member-type whitelist —
refuse anything that isn't `isfile()` or `isdir()`. This mirrors
`hermes_cli/profiles._safe_extract_profile_archive`, which already
applies the same gate for profile imports. Same shape as c26af46
('reject symlinks in skill bundles before install'), one code path
over.
Test plan
fallback (monkeypatches `tarfile.TarFile.extractall` to raise
`TypeError` on `filter='data'`), plants a victim file outside the
skills tree, and asserts the symlink is never materialised. Without
the patch the assertion `not leak.is_symlink()` fails.
regressions in sibling curator suites).