Skip to content

fix(curator): reject symlinks in tarball pre-check for Python < 3.12 - #60007

Open
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/tarfile-symlink-traversal-py311
Open

fix(curator): reject symlinks in tarball pre-check for Python < 3.12#60007
AlexFucuson9 wants to merge 1 commit into
NousResearch:mainfrom
AlexFucuson9:fix/tarfile-symlink-traversal-py311

Conversation

@AlexFucuson9

Copy link
Copy Markdown
Contributor

Summary

Reject symlinks and hard links in the tarball pre-extraction check for Python < 3.12.

Problem

agent/curator_backup.py:624 — the pre-extraction loop rejects absolute paths and .. components but does not reject symlinks. On Python < 3.12 where filter='data' is unavailable, a malicious tarball can exploit symlinks for zip-slip:

  1. Include a symlink foo -> ../../etc
  2. Include a regular file foo/passwd with attacker content
  3. The pre-check sees foo/passwd (safe path), but extraction follows the symlink and writes to /etc/passwd

The filter='data' path on Python 3.12+ already handles this, but the fallback path is vulnerable.

Fix

Add member.issym() and member.islnk() checks to the pre-extraction validation loop, raising TarError if any symlink or hard link is found.

Testing

1-file change. The existing curator backup tests exercise the extraction path.

The pre-extraction loop rejects absolute paths and .. components, but
does not reject symlinks. On Python < 3.12 where filter='data' is
unavailable, a malicious tarball can exploit symlinks for zip-slip:

1. Include a symlink 'foo' -> '../../etc'
2. Include a regular file 'foo/passwd' with attacker content
3. The pre-check sees 'foo/passwd' (safe path), but extraction follows
   the symlink and writes to /etc/passwd

Fix: also reject symlinks and hard links in the pre-check loop.
The filter='data' path on Python 3.12+ already handles this, but the
fallback needs the explicit guard.
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jul 7, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #23796 (earliest open fix for the same curator rollback tar symlink/hardlink zip-slip on the Python <3.12 fallback). Same mechanism as the saturated cluster: #43942, #48595, #27375 (and closed #32523/#34062). Fixes issue #23794.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused security hardening. The reported legacy fallback issue remains present on current main: agent/curator_backup.py:624-634 validates only member names before the TypeError branch calls unfiltered extractall().

Problems

  • This PR changes only agent/curator_backup.py; it adds no regression test. Existing tests/agent/test_curator_backup.py:236-259 covers .. traversal but not symlink/hardlink members or the legacy fallback branch.

Suggested changes

  • Add a test that forces extractall(..., filter="data") to raise TypeError, supplies a symlink plus a later file below it, and verifies no write escapes the skills directory. The earlier duplicate Reject unsafe curator rollback tar members #23796 includes that legacy-fallback test scenario.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 15, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was generated by AI during triage.

Summary

Two PRs address the unsafe legacy tar extraction fallback in agent/curator_backup.py: #60007 adds a blanket link rejection to the shared pre-check, while #65579 rejects links and special files specifically after the filtered-extraction fallback is triggered. Both overlap #23796, which the discussion identifies as the earlier fix with legacy-fallback regression coverage and preservation of safe in-root links.

Related pull requests

  • #60007 related — (+6/-0) — close as duplicate of #23796: the diff rejects every symlink and hardlink in the shared pre-check, but adds no forced-legacy-fallback regression test. Despite the automated keep-open verdict on #60007, its own evidence points to #23796 for the missing test scenario, and the contributor review identifies #23796 as the earlier fix for the same vulnerability.
  • #65579 duplicate — (+13/-1) — close as duplicate of #23796: the fallback-only guard additionally rejects special files, but its blanket link rejection is not equivalent to filter="data" and breaks legitimate in-tree symlink snapshots. Despite the keep_open review on #65579, the complete diff and subsequent contributor review show the unresolved design flaw, while #23796 covers the same fallback path and preserves safe in-root links.

Duplicates

#60007 and #65579 substantially duplicate each other on symlink/hardlink rejection in the Python <3.12 fallback security path; both are downstream duplicates of #23796.

Suggested consolidation

Close #60007 and #65579 as duplicates of #23796. Consolidate on #23796 because the supplied discussion identifies it as the earliest open fix for the same legacy fallback, with the forced-TypeError regression scenario missing from both PRs and target-aware validation that avoids rejecting legitimate in-root links.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup60007 ["PRs duplicating each other"]
        P60007["PR #60007 (open)"]
        P65579["PR #65579 (open)"]
    end
    class P60007 open
    class P65579 open
    class P60007 target
    click P60007 "https://github.com/NousResearch/hermes-agent/pull/60007"
    click P65579 "https://github.com/NousResearch/hermes-agent/pull/65579"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 2 kB of PR diffs, 4 kB of issue/PR text, 5 kB of discussion (6 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants