Skip to content

fix(security): strip directory components from Teams recording display_name to prevent path traversal - #28173

Closed
memosr wants to merge 1 commit into
NousResearch:mainfrom
memosr:fix/teams-pipeline-recording-path-traversal
Closed

fix(security): strip directory components from Teams recording display_name to prevent path traversal#28173
memosr wants to merge 1 commit into
NousResearch:mainfrom
memosr:fix/teams-pipeline-recording-path-traversal

Conversation

@memosr

@memosr memosr commented May 18, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

The Teams meeting pipeline (plugins/teams_pipeline/pipeline.py,
landed in v0.14.0 via #22007) downloads meeting recording artifacts
to a per-job temp directory before passing them to STT. The local
filename used to write the artifact comes straight from the Graph
API's recording.display_name:

# Before — plugins/teams_pipeline/pipeline.py:461-462
recording_name = recording.display_name or f"{recording.artifact_id}.mp4"
recording_path = Path(tmp_dir) / recording_name

Path / user_input does not strip .. segments — it just
concatenates. If display_name contains directory components, the
resulting recording_path escapes tmp_dir.

Attack scenario

recording.display_name is sourced from
payload.get("displayName") in meetings.py:102, which is the
recording's displayName field as returned by the Graph
/communications/callRecords/{id}/sessions/{sid}/recordings
endpoint. That field is ultimately set by the meeting organizer
when the recording is created.

A meeting organizer (any M365 user with rights to record a meeting
the agent is connected to) creates a recording with:

displayName = "../../../../etc/cron.d/hermes-pwn"

When the pipeline picks the artifact up:

recording_path = Path(tmp_dir) / "../../../../etc/cron.d/hermes-pwn"
# → /etc/cron.d/hermes-pwn  (on Linux, if process has write)

download_recording_artifact then opens that path for writing and
streams the attacker-controlled recording bytes into it. The
attacker now controls a file at an arbitrary path the Hermes
process can write to — common targets include:

  • ~/.ssh/authorized_keys (any user-writable path)
  • ~/.hermes/auth.json (overwrite agent credentials)
  • /etc/cron.d/... (if Hermes runs as root, e.g. some Docker setups)
  • ~/.bashrc, ~/.zshrc (next-shell-spawn code execution)

The artifact body is fully attacker-controlled — they uploaded
whatever they wanted as the recording content. Combined with the
filename traversal, that's an arbitrary write primitive triggered
by every meeting the agent processes.

CVSS 3.1 estimate

AV:N/AC:L/PR:L/UI:N/S:C/C:N/I:H/A:N7.3 (HIGH)

PR:L because the attacker needs to be a meeting participant /
organizer the agent is connected to, not anonymous. S:C and I:H
because arbitrary file write at the Hermes process's privilege
level can pivot to credential theft, persistence, or code
execution depending on what's writable.

Fix

One-line — strip directory components with Path(...).name:

raw_name = recording.display_name or f"{recording.artifact_id}.mp4"
recording_name = Path(raw_name).name or f"{recording.artifact_id}.mp4"
recording_path = Path(tmp_dir) / recording_name

Path("../../etc/cron.d/evil").name"evil" — directory
components are dropped, only the basename survives. The
or f"{...}.mp4" fallback covers the edge case where the
sanitized name is empty (e.g. display_name = "../""").

Why this matters

  • meetings.py:201 already handles display_name correctly — it
    uses Path(display_name).suffix which is safe.
  • Only pipeline.py:461 was joining the raw value into a path.
  • This is a small, surgical fix at the trust boundary — no behavior
    change for legitimate recordings whose displayName is just a
    human-readable label.

Mirrors the defense-in-depth pattern in:

  • #21277 — dashboard plugin SRI integrity
  • #19597 — Meet node localhost binding + chmod
  • #22432 — Google Chat sender_type coercion
  • #27825 — LSP diagnostic sanitization (in review)

Type of Change

  • 🔒 Security fix (HIGH — path traversal → arbitrary file write via Graph-sourced filename)

Checklist

  • Read the Contributing Guide
  • Commit messages follow Conventional Commits
  • Fix applied at the trust boundary where untrusted data enters the path join
  • Fallback for empty sanitized name preserves behavior for edge cases
  • No change in behavior for legitimate recordings
  • Defense-in-depth — works alongside any future server-side validation Graph may add

@outsourc-e

Copy link
Copy Markdown
Contributor

Did a local stack audit. The Teams path-traversal hardening is valid, but this PR is stacked on unrelated mobile dashboard/web commits (head includes 6fa1701 / #28127). I opened a clean replacement with the security fix plus regression coverage here: #28177. Verification on the clean branch: scripts/run_tests.sh tests/plugins/test_teams_pipeline_plugin.py -> 11 passed.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P0 Critical — data loss, security, crash loop comp/plugins Plugin system and bundled plugins platform/webhook Webhook / API server labels May 18, 2026
@BoardJames-Bot

Copy link
Copy Markdown

BoardJames CI triage:

  • Current red check is only Tests / test; it ran 20m16s and GitHub cancelled it at 95% with ##[error]The operation was canceled. No pytest failure summary/traceback was emitted.
  • All other checks are green: e2e, ruff/ty, attribution, history, nix, supply-chain, amd64/arm64 docker builds.
  • Focused local validation for the Teams pipeline area passes on this head (b2a269b0): ./scripts/run_tests.sh tests/plugins/test_teams_pipeline_plugin.py tests/hermes_cli/test_teams_pipeline_plugin_cli.py tests/gateway/test_teams.py tests/gateway/test_teams_pipeline_runtime_wiring.py => 72 passed.
  • This matches the current systemic full-suite timeout/cancel pattern on main (recent main Tests / test runs are also cancelled around the 20-minute limit), not a branch-specific failure in this PR.

No branch fix pushed; recommendation is to re-run/merge once the shared full-suite CI budget issue is fixed or the test workflow timeout is raised/split.

@memosr

memosr commented May 19, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @outsourc-e for the test coverage and salvage! Authorship
preserved via cherry-pick (254198a) and your 9ba5c1a regression test
is exactly what was missing. Happy to close #28173 once this lands.

@egilewski

Copy link
Copy Markdown
Contributor

Recommendation: needs rework; keep this PR superseded rather than merging it as-is.

Checked current upstream/main ee7948ea6e3b7b6187d40702a76204927b6688e3 against PR head b2a269b0ae703fabc8ce40b7ff5e70656045ea50.

Validation:

  • Reproduced the current-main issue with a mocked _transcribe_recording() probe: display_name="../../outside.mp4" produced a download destination resolving outside the generated teams-recording-* temp directory.
  • Ran the same probe on this PR patch: that nested traversal case stayed inside the generated temp directory as outside.mp4.
  • Focused tests passed on the patched tree: /home/mac/hermes-agent/.venv/bin/python -m pytest tests/plugins/test_teams_pipeline_plugin.py -q -o addopts='' -> 11 passed in 0.53s.
  • git apply --index applied the PR patch cleanly on current main, and git diff --check HEAD was clean.
  • CodeRabbit completed on the uncommitted patch and found a remaining edge: Path("..").name returns "..", so display_name=".." still resolves outside the generated temp directory. I validated that the downloader path fails at os.replace(..., tmp_dir / "..") rather than writing an arbitrary chosen file, but it still violates the intended safe-file-basename invariant and needs regression coverage.

Since #28177 is already the clean replacement with the focused regression test, I would not merge #28173. Please carry the fix forward in the replacement by rejecting empty, ., and .. basenames before joining with the temp directory.

Signed: GPT-5.5-xhigh in Codex

@memosr

memosr commented Jun 5, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @egilewski for the thorough audit — the . / .. basename
gap is a real find. Agreed that #28177 is the cleaner shape with
proper regression coverage, so closing this in favor of carrying
the fix forward there with the empty/./.. rejection added before
the temp-dir join.

@teknium1

teknium1 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

For the record: your Teams recording path-traversal fix (this PR's commit, authored by @memosr) shipped to main via #56198, with your authorship preserved in git log. The clean superset was routed through #28177; the merged version also closes a follow-up edge case (bare '..'/'.'/'' display names). Thanks!

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

Labels

comp/plugins Plugin system and bundled plugins P0 Critical — data loss, security, crash loop platform/webhook Webhook / API server type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants