linux: the read-deny glob walk has a budget - #607
Open
ronleizrowice-ant wants to merge 11 commits into
Open
ronleizrowice-ant wants to merge 11 commits into
ronleizrowice-ant wants to merge 11 commits into
Conversation
bubblewrap has no patterns, so a denyRead entry with glob syntax is expanded on the host into the paths to mount over, on every wrap. That walk listed through any symbolic link to a directory, wherever it led; the only link it declined was one leading back up the tree. With a link from a project to /usr and `**/.env` denied, each expansion listed 57,000 entries: 260 to 320 ms per command warm, seconds cold, and no limit for a larger target. A sandboxed command can plant such a link. A link whose target lies outside the pattern's tree is no longer listed through. The tree is the pattern's literal starting directory and what is under it, and a link is judged by where it resolves. Links that stay inside are followed as before, and a link whose own name matches still masks what it leads to. What this gives up is a file the pattern matches only by a name that passes through such a link: macOS never covered it, since patterns are matched against resolved paths there. Where a matched link hides a directory outside the tree that the pattern would have carried on into, nothing is bound back beneath it: it is reported through unlistableDenyDirs like a directory the walk cannot list. Unlisted, an allowed path beneath it would otherwise come back with nothing masked. The walk takes a budget, shared by all the denyRead patterns of one configuration: 2,000,000 directory entries and 10 seconds, the clock read before each listing and at each entry. Out of budget it throws and hands nothing back, and the wrap is refused with LinuxSandboxProfileError `deny_glob_too_large`, the walk's error as its cause. getFsReadConfig() throws the same. A deny list cut short would leave readable what the pattern was written to hide. Each expansion logs one line under SRT_DEBUG: milliseconds, matches, mounts, directories listed, entries looked at, links left unfollowed.
Ten existing cases pinned the listing through a link out of the tree. Five now expect the rule. Five are about something else (a chain of links, a path too long to name, `**` written against text, a bracket expression, a carve-out): they keep their expectations under a base widened to hold the link, and assert the original pattern under the new rule beside it. None is removed. New cases pin the rule from both sides: a link out is never listed, by a count of listings, and a link inside the tree that is the only route to a match is still followed. A link is judged by where it resolves; the tree is the pattern's and not the project's; a matched link still masks its target. Nothing is bound back beneath an unlisted target, shown by a real bubblewrap run in which the file beneath the allowed path is unreadable under both names. The budget: the exact entry boundary, the deadline before and during the walk and inside a small directory, one budget shared by the patterns of a wrap, never a partial result. At the manager the wrap rejects and returns no command, the getter throws the same, and the fields of the cause are what a caller words its own message from.
Open
…llowed A link out of a denyRead pattern's tree is not listed through, and what the pattern would have matched beneath it is not denied. That was recorded on the walk and written to the debug log; a caller had no way to tell its user. getFsReadConfig() now lists each such link in `unfollowedDenyLinks`, with the pattern that came to it, the link and where it resolves. It informs and restricts nothing: no backend reads it. The type is exported as UnfollowedDenyLink.
It lists the links left unfollowed for leading out of a pattern's tree. A link that leads back up the tree, and any link under a pattern that cannot be followed one path component at a time, is not listed through either and is not in the list. The field's comment and the README read as if the list were of every link not followed.
Where a link whose own name matches hides a directory outside the pattern's tree, the expansion reported that directory as one it could not list, so that nothing was bound back beneath it. The wrapper then restores no allowed path at or beneath it, the paths that are always writable included: with a link to a directory above the command's temporary directory, every command failed, and an allowed write path beneath such a directory stopped applying. Through the ancestor that hides it, a second link could take the working directory along. The report is taken out again. A directory outside the tree is never listed, whether or not the link that leads there matches and whatever is allowed beneath it; a matched link still hides its target whole; an allowed path beneath it is bound back as written, as beneath a directory denied literally. `unlistableDenyDirs` again names only a directory the walk could not list, and the code that fills it is what it was. What that gives up is what such a link used to add: what the pattern matched beneath the allowed path through the link's name is not masked again. A link out of the tree can only add to what a pattern covers. A file whose real path is in the tree and matches is found by walking the tree itself, so no link a sandboxed command creates takes a mask away. The unfollowed links are handed back sorted by name, so that the list and the debug line do not depend on the order a directory is read in.
The cases written for the report of an unlisted directory now state the rule as it is: what a matched link out of the tree leads to is hidden whole and not listed, with nothing allowed beneath it and with an allowed path at it, beneath it or written through the link; nothing is handed back as unlistable; a link inside such a directory is never come to. Two real bubblewrap runs pin what follows from it: beneath such a directory an allowed path is readable and the rest stays hidden, under either name, and a write allowed there succeeds. The unfollowed links come back in the same order whichever way the directory is read.
The walk stopped at a symbolic link that leaves the pattern's tree. Every release follows such a link and masks what it finds behind it: with `project/out` a link to `../outside` and `project/**/.env` denied, `outside/deep/.env` is unreadable under both names on 0.0.75, 0.0.76 and 0.0.77. Stopping there left it readable, so the rule lowered what a pattern covers and is taken out. The messages of the earlier commits on this branch that describe it as what other releases and platforms do were wrong on that point. Link handling in walkGlobPattern is what it was. `GlobWalk.unfollowedLinks`, the out-parameter of expandReadDenyGlobLinux, and `unfollowedDenyLinks` with its exported type are removed. What stays is what the walk never had: a budget shared by all the denyRead patterns of one configuration, 2,000,000 directory entries and 10 seconds, the clock read before each listing and at each entry. Out of budget the walk throws and hands nothing back, the wrap is refused with LinuxSandboxProfileError `deny_glob_too_large`, and getFsReadConfig() throws the same. What lies behind a link counts against it like anything else. Each expansion logs one line under SRT_DEBUG with what it cost.
…budget The two existing glob suites are as they were before this branch. The new file keeps the cases for the budget, for the refusal at the manager and for the debug line, and loses the ones written for the rule. Added: a match behind a link to a directory beside the project is denied where it really is, and a real bubblewrap run shows it unreadable under both names; what such a link leads to is spent from the budget, at the walk and at the manager.
ronleizrowice-ant
changed the title
This branch has not been deployed
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
1 participant
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.
On Linux, expanding a
denyReadpattern walks the pattern's tree on the host on every wrap, through symlinked directories too, and nothing bounds that walk. This adds a budget: past it the wrap is refused with a typed error instead of blocking.What was wrong
bubblewrap has no patterns, so a
denyReadentry with glob syntax is expanded on the host into the paths to mount over, each time a command is wrapped. The walk lists every directory under the pattern's literal starting directory and lists through symbolic links to directories, reporting what it finds where it really lives. Nothing limits it: not a count, not a deadline. A project holding a link to a very large tree, or to a slow file system, makes every command wait for the whole walk, and a sandboxed command that may write in the project can create such a link.What changes
denyReadpatterns of one configuration: 2,000,000 directory entries looked at and 10 seconds (GLOB_WALK_MAX_ENTRIES,GLOB_WALK_TIMEOUT_MS). The clock is read before each directory is listed and at each entry.wrapWithSandbox()rejects withLinuxSandboxProfileError, codedeny_glob_too_large, andgetFsReadConfig()throws the same. Its.causeis aGlobWalkBudgetErrorwithpattern,directory,exhausted('entries'or'time'),entries,elapsedMs,maxEntries,timeoutMs. Never a partial list: a deny list cut short would leave readable what the pattern was written to hide.SRT_DEBUG: milliseconds, matches, mounts, directories listed, entries looked at.What a pattern covers does not change. Links are followed exactly as before.
Who sees a difference
getFsReadConfig()can throw for this reason; it did not before. A caller that uses it outside a wrap needs to catch.LinuxSandboxProfileErrorCodegains one code;GlobWalkgains two counters;expandReadDenyGlobLinuxtakes a fourth, optional argument.Known limits
main.Not done here
The
allowReadexpansion and the Windows expansion get no budget; they follow no links but still list the whole real tree under a pattern's base on every wrap.Merge order
Independent. It touches
walkGlobPatterninsandbox-utils.ts, which #575 and #608 also edit; whichever merges second needs a small merge ofmain.Testing
test/sandbox/read-deny-glob-bound.test.ts(new). The budget: the exact entry boundary, the deadline before the walk, during it and inside a small directory, one budget shared by the patterns of a wrap, a tree that only links lead through, what lies behind a link out of the tree, never a partial result. The manager: the wrap rejects and returns no command, the getter throws the same, the fields of the cause. The debug line. And one case with a real bubblewrap run that pins coverage as it is: a file reached through a link out of the tree is unreadable under both names. The two existing glob suites are unchanged.Run end to end on library builds of 0.0.75, 0.0.76, 0.0.77,
mainand this branch, withproject/outa link to../outsideandproject/**/.envdenied:outside/deep/.envis hidden under both names on all five.Both typechecks, eslint and prettier clean. Both runtimes. Whole suite on Linux x86-64, Bun 1.4.2: identical failing names to
main.