security(file_svc): containment check on save_file to block agent-contact path traversal#3380
Merged
Merged
Conversation
added 2 commits
May 18, 2026 19:21
…ersal
file_svc.save_file builds its target path as os.path.join(target_dir, filename)
and writes payload bytes there with no validation of the resulting location.
It is reached from the agent contact handlers with an attacker-controlled
filename:
* app/contacts/contact_ftp.py:217 — FTP submit_uploaded_file
* app/contacts/contact_dns.py:491 — DNS _submit_uploaded_file
* app/contacts/contact_gist.py:212 — Gist contact upload
* app/contacts/contact_slack.py:206 — Slack contact upload
A filename of e.g. '../../data/object_store' escapes the exfil tree and lets
an attacker write arbitrary bytes to any path the server has permission to
write. data_svc.restore_state pickle.loads('data/object_store') on the next
restart, which turns that write primitive into RCE on the team server. The
file is Fernet-encrypted with conf encryption_key, but with the documented
default encryption_key=ADMIN123 a remote attacker can forge a valid blob.
save_file has two legitimate call modes:
(1) target_dir + basename — sink mode used by data_svc, app_svc, c_operation,
base_knowledge_svc, and the four agent contact handlers above.
(2) '' + relative-path — path mode used by ability_api_manager,
base_api_manager, and rest_svc, which assemble paths from sanitised IDs.
A simple "filename must be a basename" check breaks mode (2). Instead, resolve
the final path with realpath and require it to stay under its parent. This:
* rejects '..' / absolute-path escape in BOTH call modes, with one check;
* leaves all in-tree callers' happy paths intact (object_store, contact
reports, operation logs, ability/source YAML writes);
* provides defense-in-depth for any future caller, not just the four contact
handlers known to be vulnerable today.
Includes a regression test that exercises three traversal vectors and
confirms each is rejected before any bytes are written.
Reported externally on 2026-05-18 alongside the broader audit pass.
The post-condition assertion 'not os.path.exists(realpath(...))' used '/etc/passwd' as one of the resolved-traversal targets. That file exists on every Linux runner, so the guard would fail even when the production code correctly raised ValueError before any I/O — what we saw in CI. The security check (pytest.raises(ValueError, match='escapes parent')) is unchanged; it's the only assertion that proves the fix works. The post-condition guard is now scoped to canary basenames that cannot pre-exist by accident, so it actually exercises 'rejected before write' without depending on system-file absence.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a containment check in FileSvc.save_file to prevent path traversal via attacker-controlled filename reaching the sink from unauthenticated agent contact handlers (DNS/FTP/Gist/Slack). The resolved final path is now required to stay under its parent (or cwd when target_dir is empty), defending both call modes (basename-in-target_dir and relative-path-with-empty-target_dir) with a single check.
Changes:
- Resolve final write path with
os.path.realpathand raiseValueErrorif it escapes its parent. - Add regression test
test_save_file_rejects_path_traversalcovering several..-based payloads.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/service/file_svc.py | Adds realpath-based containment check in save_file before delegating to _save. |
| tests/services/test_file_svc.py | Regression test asserting traversal attempts raise ValueError and write nothing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
1 similar comment
|
|
❌ The last analysis has failed. |
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
`file_svc.save_file` builds its target path as `os.path.join(target_dir, filename)` and writes payload bytes there with no validation of the resulting location. It is reached from the agent contact handlers with an attacker-controlled `filename`:
A filename of e.g. `../../data/object_store` escapes the exfil tree and writes arbitrary bytes to any path the server can write. `data_svc.restore_state` calls `pickle.loads('data/object_store')` on the next restart, which turns that write primitive into RCE on the team server. The file is Fernet-encrypted with conf `encryption_key`, but with the documented default `encryption_key=ADMIN123` a remote attacker can forge a valid blob.
Fix
`save_file` has two legitimate call modes:
A naive "filename must be a basename" check would break mode (2). Instead, resolve the final path with `os.path.realpath` and require it to stay under its parent. This:
```python
async def save_file(self, filename, payload, target_dir, encrypt=True, encoding=None):
if encoding:
payload = await self._decode_contents(payload, encoding)
full_path = os.path.join(target_dir, filename)
parent = os.path.realpath(target_dir) if target_dir else os.path.realpath(os.getcwd())
final = os.path.realpath(full_path)
if final != parent and not final.startswith(parent + os.sep):
raise ValueError('save_file: path %r escapes parent %r ...' % ...)
self._save(full_path, payload, encrypt)
```
Test plan
Regression test `test_save_file_rejects_path_traversal` added to `tests/services/test_file_svc.py`, exercising:
Each must raise `ValueError` with `'escapes parent'` in the message, AND nothing must be written outside the temp dir.
Local standalone smoke test (outside the pytest fixture chain) also confirms:
Why this is the right shape
Earlier PR #3311 attempted to defend the same class by adding `@check_authorization` to the v1 `/file/download` endpoint. That gate would 401 every `sandcat`/`manx` bootstrap (the endpoint is intentionally unauthenticated — agents have no creds at bootstrap). The right defense is at the sink, not at the gate: keep the endpoint open as documented, but make the sink incapable of writing outside its tree.
Refs