Skip to content

fix(security): reject path traversal in credential file registration - #3951

Closed
memosr wants to merge 1 commit into
NousResearch:mainfrom
memosr:fix/credential-files-path-traversal
Closed

fix(security): reject path traversal in credential file registration#3951
memosr wants to merge 1 commit into
NousResearch:mainfrom
memosr:fix/credential-files-path-traversal

Conversation

@memosr

@memosr memosr commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

tools/credential_files.py had no containment check on the
relative_path argument passed to register_credential_file().
A malicious skill could declare:

required_credential_files:
  - path: '../../.ssh/id_rsa'

and the function would silently mount the host's SSH private key
(or any other file reachable via ..) into the Docker/Modal sandbox,
leaking it to the skill's execution environment.

The same applies to absolute paths — required_credential_files: ['/etc/passwd']
would also be accepted without any check.

Type of Change

  • 🔒 Security fix

Changes Made

tools/credential_files.pyregister_credential_file():

  • Reject absolute paths immediately with a warning log
  • Resolve the path with Path.resolve() (follows symlinks, normalises ..) before any containment check
  • Verify the resolved path is inside HERMES_HOME via relative_to() — raises ValueError if outside
  • Use the resolved (canonical) path in the registry instead of the raw unresolved path

tests/tools/test_credential_files.py — new TestPathTraversalSecurity class (6 cases):

  • test_dotdot_traversal_rejected../sensitive.json
  • test_deep_traversal_rejected../../.ssh/id_rsa
  • test_absolute_path_rejected/etc/passwd style
  • test_legitimate_file_still_works — normal files inside HERMES_HOME unaffected
  • test_nested_subdir_inside_hermes_home_allowed — subdirs still work
  • test_symlink_traversal_rejected — symlink pointing outside HERMES_HOME

How to Test

Checklist

  • Read the Contributing Guide
  • Commit messages follow Conventional Commits
  • No duplicate PR found
  • PR contains only this security fix
  • pytest passes
  • Tests added for the fix
  • Tested on: Ubuntu 24.04
  • Cross-platform: uses pathlib — N/A

@dieutx

dieutx commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Related: my #2714 adds a read-path deny list for credential and secret files, covering the read side of this attack surface. This PR (rejecting path traversal in credential file registration) is complementary — worth ensuring the path validation logic is consistent between the two. Happy to coordinate if needed.

@memosr

memosr commented Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the heads up! I took a look at #2714 — you're right that
they're complementary. Happy to align the path validation logic if
needed. Let me know what works best.

@memosr
memosr force-pushed the fix/credential-files-path-traversal branch from 6244dea to 2530072 Compare March 30, 2026 12:19
@memosr

memosr commented Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks! I checked out #2714 - the read-side deny list and this write-side
containment check do cover complementary attack surfaces.

I kept the same
pattern (Path.resolve() + relative_to() for containment) so the logic
should be consistent.

Happy to adjust if there's anything that needs to
align better with your implementation.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via PR #4316. Your commit was cherry-picked onto current main with your authorship preserved in git log. We also extended the same path traversal checks to the config-based credential file loader (_load_config_files) as a defense-in-depth follow-up. Thanks @memosr!

@teknium1 teknium1 closed this Mar 31, 2026
teknium1 pushed a commit that referenced this pull request Jul 20, 2026
…dboxes

register_credential_file() takes a skill-declared relative path from
required_credential_files frontmatter and bind-mounts it read-only into the
remote sandbox the skill's own code runs in. It validates that the resolved
path stays inside HERMES_HOME — the docstring names the threat directly:

    so that a malicious skill cannot declare
    required_credential_files: ['../../.ssh/id_rsa'] and exfiltrate
    sensitive host files into a container sandbox

Containment is the wrong boundary on its own, because HERMES_HOME is exactly
where the master credential stores live. Traversal is blocked; asking for the
keys by name is not:

    skill declares               mounted?   agent may read it?
    .env                         YES        DENIED
    auth.json                    YES        DENIED
    .anthropic_oauth.json        YES        DENIED
    cache/bws_cache.json         YES        DENIED
    mcp-tokens/srv.json          YES        DENIED
    google_token.json            YES        allowed
    ../../.ssh/id_rsa            no         n/a

Every row marked DENIED is refused by the canonical read guard
(agent.file_safety.get_read_block_error) — the agent cannot read_file them —
yet one line of hub-installed skill frontmatter gets them bind-mounted where
that skill can cat them. .env alone is every provider API key.

Reuse the canonical deny-list as the mount bar: what the agent is forbidden
to read is not mountable either, so the mount surface cannot hand a skill
what the read surface denies it. Fails CLOSED — if the guard can't be
consulted the mount is refused rather than risked.

The module keeps doing its job: a skill still mounts its own service token
(google_token.json, skills/*), and a refused entry is reported back through
register_credential_files' missing list instead of failing the batch.

The three prior PRs here (#3946, #3951, #4316) all hardened traversal; this
closes the half that traversal validation never covered.
randlee pushed a commit to randlee/hermes-agent that referenced this pull request Aug 11, 2026
…dboxes

register_credential_file() takes a skill-declared relative path from
required_credential_files frontmatter and bind-mounts it read-only into the
remote sandbox the skill's own code runs in. It validates that the resolved
path stays inside HERMES_HOME — the docstring names the threat directly:

    so that a malicious skill cannot declare
    required_credential_files: ['../../.ssh/id_rsa'] and exfiltrate
    sensitive host files into a container sandbox

Containment is the wrong boundary on its own, because HERMES_HOME is exactly
where the master credential stores live. Traversal is blocked; asking for the
keys by name is not:

    skill declares               mounted?   agent may read it?
    .env                         YES        DENIED
    auth.json                    YES        DENIED
    .anthropic_oauth.json        YES        DENIED
    cache/bws_cache.json         YES        DENIED
    mcp-tokens/srv.json          YES        DENIED
    google_token.json            YES        allowed
    ../../.ssh/id_rsa            no         n/a

Every row marked DENIED is refused by the canonical read guard
(agent.file_safety.get_read_block_error) — the agent cannot read_file them —
yet one line of hub-installed skill frontmatter gets them bind-mounted where
that skill can cat them. .env alone is every provider API key.

Reuse the canonical deny-list as the mount bar: what the agent is forbidden
to read is not mountable either, so the mount surface cannot hand a skill
what the read surface denies it. Fails CLOSED — if the guard can't be
consulted the mount is refused rather than risked.

The module keeps doing its job: a skill still mounts its own service token
(google_token.json, skills/*), and a refused entry is reported back through
register_credential_files' missing list instead of failing the batch.

The three prior PRs here (NousResearch#3946, NousResearch#3951, NousResearch#4316) all hardened traversal; this
closes the half that traversal validation never covered.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants