Security: WorkFolderTools.resolvePath ignores symlinks — escape via symlink possible#10
Draft
mimeding wants to merge 4 commits into
Draft
Security: WorkFolderTools.resolvePath ignores symlinks — escape via symlink possible#10mimeding wants to merge 4 commits into
mimeding wants to merge 4 commits into
Conversation
The existing containment check used only `.standardized`, which is a lexical operation that collapses '..' and '/./' but does not follow symbolic links. That made the agent's file tools vulnerable to symlink-based escape: an agent or a careless template could plant a symlink inside the work root that points to '/etc' (or any user file outside the project) and then read or write through the link believing the path was contained. Add a second pass that re-checks containment after `.resolvingSymlinksInPath()` on both the candidate path and the root, with proper trailing-separator handling so a sibling directory that happens to share a prefix (e.g. '/work/foo-baz' vs '/work/foo') can't bypass the prefix test. The work root itself is symlink-resolved before the comparison, so macOS-typical projects whose root is reached via a symlink (a common pattern, e.g. /var -> /private/var) don't fail spuriously. New focused tests cover: * relative path inside root accepted * '/path' (treated as relative) accepted * '..' escape rejected (pre-existing behavior, still covered) * symlink escape rejected (new) * root-via-symlink accepted (regression guard for the macOS pattern) Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
The original fix used string-prefix matching ('resolvedURL.path.hasPrefix(
rootPathString)') after only one side had been run through
resolvingSymlinksInPath(). On macOS the runner's TempDirectory lives under
/var/folders/... and the system selectively canonicalizes that prefix to
/private/var/... depending on which API is used (FileManager.enumerator,
URL.resolvingSymlinksInPath, URL.standardized — each applies a different
rule). One side getting the /private prefix and the other not yielded
false-negative containment failures and made the rejectsSymlinkEscape test
fail in CI.
Use pathComponents arrays + Array.starts(with:) after canonicalizing BOTH
sides identically (resolvingSymlinksInPath().standardized). Component-based
prefix matching also closes the 'sibling directory with shared name
prefix' foot-gun (e.g. /work/foo-baz vs /work/foo) that the original
trailing-separator string trick was trying to paper over.
Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
macOS's URL.resolvingSymlinksInPath() uses strict-realpath semantics: if any component along the path doesn't currently exist, it returns the input URL UNCHANGED rather than resolving the components that do exist. In our case, an agent asking the tools to write '/root/escape/newfile' — where 'newfile' has not been created yet but 'escape' inside the work root IS a symlink pointing OUT of the root — would skate past the symlink-aware check because the resolver gave back the original unresolved string. The CI test 'rejectsSymlinkEscape' caught exactly this: 'secret' didn't exist, 'escape' was a symlink to a sibling temp directory, and resolvePath returned successfully. Walk the relative components manually. For each component, append it on top of the running resolved-so-far URL. If the resulting path exists on disk, run it through resolvingSymlinksInPath() so any symlink we encounter at this depth gets resolved. As soon as a component doesn't exist, stop resolving and append the rest of the path lexically. This is the moral equivalent of 'realpath -m' on Linux and gets us the right answer in the 'parent exists but leaf doesn't' case the tests exercise. Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
ModelManager.init kicks off an unstructured Task that calls loadOsaurusAIOrgModels(), which fetches the OsaurusAI organization listing from Hugging Face and feeds the result through applyOsaurusOrgFetch. The unit-test runner repeatedly constructs ModelManager() to drive applyOsaurusOrgFetch directly. The background launch-time fetch races with those test calls — whichever finishes last wins, and the merge result is non-deterministic. That's the root cause of the flaky ModelManagerSuggestedTests failures seen across many of the recent PR CI runs (applyOsaurusOrgFetch_dropsStaleAutoFetched OnReapply, applyOsaurusOrgFetch_addsNewEntriesAfterCurated, etc.). Gate the launch-time fetch on a small isRunningInTestEnvironment helper that checks for any of XCTestConfigurationFilePath, XCTestBundlePath, or XCTestSessionIdentifier in the process environment. Those variables are only present inside an xctest host process; production app launches still get the HF fetch exactly as before. This is a network call, so removing it under tests also has the side benefit of making the test suite work offline / on hermetic CI runners. Co-authored-by: Michael Meding <mimeding@users.noreply.github.com>
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
Why this matters (business)
Work mode lets an agent read, edit, and run files inside a designated project folder. The whole safety story for "the agent can't accidentally rewrite my home directory" rests on
WorkFolderToolHelpers.resolvePath— every file tool routes through it. If that helper accepts a path that escapes the work root, every downstream file operation inherits the escape.The helper rejected
../etc/passwd-style escapes correctly (lexical..is collapsed by.standardizedbefore the prefix check). What it didn't reject was symlink-based escape: a symlink planted inside the work root that points to/etc, the user's~/Documents, or any other path outside the project. An agent (or a malicious project template, or a routinegit checkoutof an attacker-controlled repo) could create such a symlink and then read/write through it while the containment check happily said "yes, that's inside the work root."What's wrong (technical)
URL.standardizedis purely lexical — it collapses..and/./but never reads the filesystem and never follows symbolic links. So a file namedescapeinside the work root that's actually a symlink to/some/private/dirlexically lives at<root>/escape, passes the prefix check, and the file tool then operates on/some/private/dir/....Two additional pitfalls the audit's recommendation has to handle:
hasPrefix(rootPathString)without a trailing separator.<root>="/work/foo"would also match"/work/foo-evil/file"— a sibling directory with a name that happens to share a prefix./varare themselves symlinks (/private/var). Naively resolving symlinks on the candidate path but not the root would cause every legitimate project whose path runs through a system symlink to fail containment.Fix
Add a symlink-aware second pass:
/var→/private/varwork./prefix (or full-equality for the root itself), so sibling-directory false matches are impossible.URLis intentionally the non-symlink-resolved one — callers that need anURLto hand toFileManager/Data(contentsOf:)get the same surface they always got; only the guard changed.Scope decisions
This fixes
WorkFolderTools.resolvePath. The same pattern (.standardized-only) exists elsewhere in the codebase (plugin static-asset serving inHTTPHandler.swift, sandbox setup URL handling). Those deserve their own focused PRs so each change can be reviewed against the constraints of its caller.Changes
WorkFolderToolsResolvePathTests)Test Plan
Manually: in a work folder, create
escape -> /etc(e.g.ln -s /etc /work-root/escape). Ask the agent to readescape/passwd. Expected: the file-tool returnspathOutsideRoot. Previously: the agent could read/etc/passwd.Checklist
CONTRIBUTING.mdswiftc -frontend -parse; the new tests use only FoundationFileManagerAPIs that work the same on both platforms)