From c22688b9238a1c21de2f72acf0c5b67b79e82fee Mon Sep 17 00:00:00 2001 From: Patrick Kaeding Date: Fri, 4 Sep 2026 12:12:09 -0400 Subject: [PATCH 1/2] =?UTF-8?q?test(client):=20Agent=20Skills=20=E2=80=94?= =?UTF-8?q?=20the=20root-swap=20races=20the=20descriptor=20walk=20does=20n?= =?UTF-8?q?ot=20close?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three expected-failure tests (TestRootSwapRaces) for SEC-8985 row 2. The SDR response says every destructive operation runs relative to a descriptor held for the duration of the reconcile. The code pins / per operation and never holds the root: _resolve_root validates it once and returns a path, and each write and prune re-opens / by path with O_NOFOLLOW, which guards only the final component. A root swapped for a symlink after validation redirects the open, and every descriptor-relative step behind it, into the attacker's directory. Precondition is write permission on the root's parent, which the README checklist does not mention. The tests state the contract (nothing lands outside the root, no outside file is overwritten, no outside file is removed) and are marked xfail(strict=True, raises=AssertionError) so the suite stays green, the gap is recorded next to the other race tests, and the fix cannot land without removing the marker. Run with --runxfail to see the three escapes. Co-Authored-By: Claude Fable 5.1 --- packages/client/tests/test_skills_fs.py | 175 ++++++++++++++++++++++++ 1 file changed, 175 insertions(+) diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index 6d5ab2d..9c0d101 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -198,6 +198,56 @@ def install(self, monkeypatch: pytest.MonkeyPatch) -> _SwapDirectoryDuring: return self +class _SwapRootDuring: + """Fires the *root*-swap race at the exact instant of an operation. + + ``_SwapDirectoryDuring`` one level up: renames the managed root itself aside + and leaves a symlink to *outside* in its place, then lets the intercepted + call proceed. ``O_NOFOLLOW`` guards only the final path component, so an + ``os.mkdir`` or ``os.open`` of ``/`` issued after the swap is + resolved by the kernel *through* the link: the descriptor that comes back is + pinned to ``/``, and every descriptor-relative step that + follows is relative to the wrong directory. + + The attacker capability is write permission on the root's *parent*, not on + the root. The README's privilege-separation checklist denies the agent + identity the root, the skill directories, the files and the manifest, and + says nothing about the parent; in the documented layout + (``/.claude/skills``) that parent is ``.claude``, which the agent + identity typically owns. + + Fires once, on the first call whose path argument is ``/``; every + other call passes straight through. + """ + + def __init__(self, attribute: str, root: Path, key: str, outside: Path) -> None: + self.attribute = attribute + self.root = Path(os.path.realpath(root)) + self.trigger = self.root / key + self.moved_to = self.root.parent / f"{self.root.name}.real" + self.outside = outside + self.swapped = False + self._real = getattr(os, attribute) + + def __call__(self, first: Any, *args: Any, **kwargs: Any) -> Any: + # ``os.mkdir(path, mode)`` and ``os.open(path, flags, ...)`` both take the + # path first; descriptor-relative calls pass a bare name, which never + # compares equal to the absolute trigger. + if ( + not self.swapped + and isinstance(first, (str, os.PathLike)) + and Path(os.fspath(first)) == self.trigger + ): + os.rename(self.root, self.moved_to) + os.symlink(self.outside, self.root, target_is_directory=True) + self.swapped = True + return self._real(first, *args, **kwargs) + + def install(self, monkeypatch: pytest.MonkeyPatch) -> _SwapRootDuring: + monkeypatch.setattr(safe_fs_module.os, self.attribute, self) + return self + + _needs_dir_fd = pytest.mark.skipif( not safe_fs_module.SUPPORTS_DIR_FD, reason="no *at() family on this platform; the per-component lstat floor applies", @@ -1104,6 +1154,131 @@ async def test_directory_swapped_at_the_prune_cannot_redirect_the_unlink( assert [a.action for a in report.actions if a.key == "a"] == ["removed"] +_root_not_held = pytest.mark.xfail( + strict=True, + raises=AssertionError, + reason=( + "SEC-8985 row 2: the managed root is validated once but never held open, " + "so a root swapped for a symlink before / is opened redirects " + "the operation out of the root. Remove this marker once write_skills pins " + "the root and walks from it (dir_fd=root_fd)." + ), +) +""" +Strict, so the day the fix lands these tests fail as XPASS and the marker has +to go — they cannot silently stay "expected to fail" past the fix. Narrowed to +``AssertionError`` so a harness breakage (an unexpected ``OSError``, a race +that never fires) is reported as a real failure rather than absorbed as the +known gap. +""" + + +@pytest.mark.skipif( + not hasattr(os, "symlink"), reason="platform has no symlink support" +) +class TestRootSwapRaces: + """The swap one level up: the managed *root*, not ``/``. + + ``_resolve_root`` validates the root once and returns a path; nothing holds + it open. Each write and each prune then opens ``/`` *by path* + with ``O_NOFOLLOW | O_DIRECTORY`` and pins that. ``O_NOFOLLOW`` guards only + the final component, so the root and every ancestor are re-resolved on + every such open, and a root swapped for a symlink after validation + redirects the open — and with it every descriptor-relative step behind it + — into the attacker's directory. ``atomic_write_in`` does refuse the + manifest rewrite afterwards, but by then the skill file is already outside + the root. + + ``TestSymlinkAttacks`` proves the ``/`` swap is closed. These + three state the same contract for the root — nothing lands outside it, no + outside file is overwritten, no outside file is removed — and are marked + expected-to-fail because the code does not yet meet it. The fix is to pin + the root once at the top of ``write_skills`` and walk from it: + ``os.mkdir(key, dir_fd=root_fd)``, ``os.open(key, ..., dir_fd=root_fd)``, + and ``atomic_write(..., dir_fd=root_fd)`` for the manifest. + """ + + @_needs_dir_fd + @_root_not_held + async def test_root_swapped_at_the_skill_directory_create_cannot_redirect_the_write( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """First reconcile against a fresh root: the skill directory does not + exist yet, so ``open_or_create_directory`` calls ``os.mkdir(/a)``. + The swap fires there; the ``mkdir`` and the ``O_NOFOLLOW`` open that + follows both resolve through the link, and ``SKILL.md`` is written into + ``/a/``.""" + root = tmp_path / "skills" + root.mkdir() + outside = tmp_path / "outside" + outside.mkdir() + race = _SwapRootDuring("mkdir", root, "a", outside).install(monkeypatch) + + report = await write_skills([_skill("a")], root) + + if not race.swapped: + pytest.fail("the race never fired; the test proves nothing") + assert list(outside.iterdir()) == [] + # Either the skill landed in the real root or the run says it did not. + assert report.ok is False or ( + (race.moved_to / "a" / "SKILL.md").read_text(encoding="utf-8") == SKILL_BODY + ) + + @_needs_dir_fd + @_root_not_held + async def test_root_swapped_at_the_skill_directory_open_cannot_clobber_an_outside_file( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Update of an already-managed skill: the swap fires at the first + ``O_NOFOLLOW`` open of ``/a`` (the orphan-temp sweep's), and the + write re-opens by path and inherits it. ``/a/SKILL.md`` — a + file the manifest never recorded — is replaced with LaunchDarkly-served + content.""" + root = tmp_path / "skills" + root.mkdir() + outside = tmp_path / "outside" + (outside / "a").mkdir(parents=True) + victim = outside / "a" / "SKILL.md" + victim.write_text("precious\n", encoding="utf-8") + _place_managed(root, "a", SKILL_BODY) + race = _SwapRootDuring("open", root, "a", outside).install(monkeypatch) + + report = await write_skills([_skill("a", 2, "served update\n")], root) + + if not race.swapped: + pytest.fail("the race never fired; the test proves nothing") + assert victim.read_text(encoding="utf-8") == "precious\n" + assert report.ok is False or ( + (race.moved_to / "a" / "SKILL.md").read_text(encoding="utf-8") + == "served update\n" + ) + + @_needs_dir_fd + @_root_not_held + async def test_root_swapped_at_the_prune_cannot_redirect_the_unlink( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """The destructive side. A prune of a formerly-managed key opens + ``/a`` by path, unlinks ``SKILL.md`` relative to that descriptor, + then ``rmdir``s the directory — all three resolve through the swapped + root, so an outside file and its directory are removed.""" + root = tmp_path / "skills" + root.mkdir() + outside = tmp_path / "outside" + (outside / "a").mkdir(parents=True) + victim = outside / "a" / "SKILL.md" + victim.write_text("precious\n", encoding="utf-8") + _place_managed(root, "a", SKILL_BODY) + race = _SwapRootDuring("open", root, "a", outside).install(monkeypatch) + + report = await write_skills([], root) + + if not race.swapped: + pytest.fail("the race never fired; the test proves nothing") + assert victim.exists() and victim.read_text(encoding="utf-8") == "precious\n" + assert report.ok is False or not (race.moved_to / "a" / "SKILL.md").exists() + + class TestWithoutDirFd: """The full-path fallback for platforms with no ``*at()`` family. From 37ddb6aa7e4241bf5dfb8b6ff30e27ed0eb588be Mon Sep 17 00:00:00 2001 From: Patrick Kaeding Date: Fri, 4 Sep 2026 12:41:25 -0400 Subject: [PATCH 2/2] test(client): let the root-swap races fail outright The expected-failure marker made the suite green while the contract was violated. A red run is the demonstration: the tests assert the contract, the code does not meet it, and they go green when the root is pinned for the reconcile, with nothing to remove. Co-Authored-By: Claude Fable 5.1 --- packages/client/tests/test_skills_fs.py | 37 ++++--------------------- 1 file changed, 6 insertions(+), 31 deletions(-) diff --git a/packages/client/tests/test_skills_fs.py b/packages/client/tests/test_skills_fs.py index 9c0d101..812eff7 100644 --- a/packages/client/tests/test_skills_fs.py +++ b/packages/client/tests/test_skills_fs.py @@ -1154,25 +1154,6 @@ async def test_directory_swapped_at_the_prune_cannot_redirect_the_unlink( assert [a.action for a in report.actions if a.key == "a"] == ["removed"] -_root_not_held = pytest.mark.xfail( - strict=True, - raises=AssertionError, - reason=( - "SEC-8985 row 2: the managed root is validated once but never held open, " - "so a root swapped for a symlink before / is opened redirects " - "the operation out of the root. Remove this marker once write_skills pins " - "the root and walks from it (dir_fd=root_fd)." - ), -) -""" -Strict, so the day the fix lands these tests fail as XPASS and the marker has -to go — they cannot silently stay "expected to fail" past the fix. Narrowed to -``AssertionError`` so a harness breakage (an unexpected ``OSError``, a race -that never fires) is reported as a real failure rather than absorbed as the -known gap. -""" - - @pytest.mark.skipif( not hasattr(os, "symlink"), reason="platform has no symlink support" ) @@ -1191,15 +1172,14 @@ class TestRootSwapRaces: ``TestSymlinkAttacks`` proves the ``/`` swap is closed. These three state the same contract for the root — nothing lands outside it, no - outside file is overwritten, no outside file is removed — and are marked - expected-to-fail because the code does not yet meet it. The fix is to pin - the root once at the top of ``write_skills`` and walk from it: + outside file is overwritten, no outside file is removed. The code does not + yet meet it, so they fail, and the red run is the demonstration. The fix is + to pin the root once at the top of ``write_skills`` and walk from it: ``os.mkdir(key, dir_fd=root_fd)``, ``os.open(key, ..., dir_fd=root_fd)``, and ``atomic_write(..., dir_fd=root_fd)`` for the manifest. """ @_needs_dir_fd - @_root_not_held async def test_root_swapped_at_the_skill_directory_create_cannot_redirect_the_write( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -1216,8 +1196,7 @@ async def test_root_swapped_at_the_skill_directory_create_cannot_redirect_the_wr report = await write_skills([_skill("a")], root) - if not race.swapped: - pytest.fail("the race never fired; the test proves nothing") + assert race.swapped is True, "the race never fired; the test proves nothing" assert list(outside.iterdir()) == [] # Either the skill landed in the real root or the run says it did not. assert report.ok is False or ( @@ -1225,7 +1204,6 @@ async def test_root_swapped_at_the_skill_directory_create_cannot_redirect_the_wr ) @_needs_dir_fd - @_root_not_held async def test_root_swapped_at_the_skill_directory_open_cannot_clobber_an_outside_file( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -1245,8 +1223,7 @@ async def test_root_swapped_at_the_skill_directory_open_cannot_clobber_an_outsid report = await write_skills([_skill("a", 2, "served update\n")], root) - if not race.swapped: - pytest.fail("the race never fired; the test proves nothing") + assert race.swapped is True, "the race never fired; the test proves nothing" assert victim.read_text(encoding="utf-8") == "precious\n" assert report.ok is False or ( (race.moved_to / "a" / "SKILL.md").read_text(encoding="utf-8") @@ -1254,7 +1231,6 @@ async def test_root_swapped_at_the_skill_directory_open_cannot_clobber_an_outsid ) @_needs_dir_fd - @_root_not_held async def test_root_swapped_at_the_prune_cannot_redirect_the_unlink( self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: @@ -1273,8 +1249,7 @@ async def test_root_swapped_at_the_prune_cannot_redirect_the_unlink( report = await write_skills([], root) - if not race.swapped: - pytest.fail("the race never fired; the test proves nothing") + assert race.swapped is True, "the race never fired; the test proves nothing" assert victim.exists() and victim.read_text(encoding="utf-8") == "precious\n" assert report.ok is False or not (race.moved_to / "a" / "SKILL.md").exists()