Repository navigation
security(llc): an agent-supplied storage_path is opened unvalidated, reading any text file on the host into the KB #17302
Description
Activity
- added 12 commits that reference this issue
on Sep 23, 2026 Closure evidence. This closed on the merge of #17408 (
a937f5953f) without the AC pass, so here it is, verified against merged code onmainrather than against the PR diff.AC Verdict Evidence on origin/main1 — confined before open(), via the existing helpersmet llc/kb/artifact_ingestor.py:26importsvalidate_path;:100returnsvalidate_path(raw, allowed_roots=(str(root.resolve()),));:312openscontained— the validated value, notraw2 — preferably storage_pathstops being caller-suppliedNOT met llc_local_storage_root()(llc/storage_root.py:40) derives the root, but the per-productstorage_pathis still taken from the row and validated at the boundary. Mitigated, not eliminated3 — a test proves an out-of-root path is refused before open(), asserting nothing was readmet artifact_ingestor_path_containment_17302_test.py:94-95assertstext is Noneandstr(secret) not in opened— the read itself, not the response4 — a test proves a legitimate in-root product still ingests met same file :122,assert text == "# real content\n"5 — rows written before the fix are still opened met same file :137, a legacy row resolves to"written before #17302\n"AC1 used
validate_pathrather than thevalidate_relative_path/resolve_within_sandboxthe criterion named. Same module, same containment property, and it is the helper the rest of this rule's call sites use — recorded because the criterion named specific symbols.AC2 is left unticked deliberately. The wording was "preferably", and the preference was not taken: a caller-supplied
storage_pathis still the input, now validated instead of derived. Reopening is not warranted — the vulnerability is closed — but the stronger design is not what shipped, and ticking it would misrepresent the code.Two findings from the same review, kept separate per one-defect-one-fix:
- security(llc): a confusable extension collapses the attachment basename, so two uploads overwrite each other #17492 — a confusable extension collapses the attachment basename, so two uploads to one work item overwrite each other. Not traversal: containment holds, and the direct-child check added here is what holds it.
- The review's CWE-22 label on that finding was checked and does not survive: one further confusable group resolves above the tenant directory and is refused.
Branch cleanup (per the standing rule that a merged issue with a branch left behind is unfinished):
- remote
issue-17302-llc-path-containment— deleted automatically on squash merge (git ls-remote --headsreturns 0) - local worktree — removed; tree was clean, and all 7 files it touched verified byte-identical to
mainfirst - local branch — deleted (was
540e572c9a)
Severity: high — arbitrary file read, reachable by any authenticated agent
Found by the pre-merge review of #17301 while fixing the sibling defect #17300. Same class, different endpoint, not fixed by that PR.
The defect
WorkProduct.storage_pathis a free-form optional string on the agent request model:It is stored unvalidated via
WorkProductService.createand later opened directly:The only gate between them is
_is_text_path(artifact_ingestor.py:63-65), an extension allowlist —.txt .md .py .json .yaml .yml .toml .csv .sql .html .css .jsx .tsxand similar. There is no containment check against any root.So an authenticated agent can set
storage_pathto any absolute path on the backend host ending in one of those extensions and have the contents read into the knowledge index, where they become retrievable./etc/*.confis not in the allowlist, but plenty of interesting things are:.env-adjacent.yamland.tomlconfig,.jsoncredential files,.pysource,.sqldumps.An extension allowlist answers "is this the kind of file we can parse". It was never a containment check, and it is standing in for one.
Why this matters more than a normal read
The sink is an indexer. The content does not just get returned once to the caller — it is ingested into the KB, so it persists and becomes searchable by anything that queries the index later. The disclosure outlives the request.
Acceptance criteria
storage_pathis confined to the work-product storage root beforeopen(), using the existingautobot_shared/security/path_validator.pyhelpers (validate_relative_path/resolve_within_sandbox) rather than a new check — the same helpers that clear nine other alerts in this rulestorage_pathstops being caller-supplied at all: the service derives it, the way security(llc): an authenticated user can write a file anywhere on disk via the attachment company_id query param #17300 made the attachment route derive its path fromctx.org_id/etc/hostsrenamed to a.txt, or any allowlisted extension) is refused beforeopen()— asserting nothing was read, not merely that the response was an errorstorage_pathalready stored before the fix is still opened on the next ingestRelationship to #17300
Same root cause — an attacker-controlled string becomes an
open()path — and the same shape of near-miss: validation exists (_is_text_path) but is not the validation that was needed. #17300 fixed the write side in attachments; this is the read side in work products, and it was not in that PR's scope.Refs #17300
Refs #17301