fix(cloud-storage): resolve symlinks when enforcing local path boundaries - #3810
Open
huangjiahua wants to merge 1 commit into
Open
fix(cloud-storage): resolve symlinks when enforcing local path boundaries#3810huangjiahua wants to merge 1 commit into
huangjiahua wants to merge 1 commit into
Conversation
…ries allowedLocalRoots and the download tool's destination_dir both decided whether a path was in bounds by inspecting the path string, then handed the unresolved name to os.Open/os.OpenFile. A symbolic link planted under an allowed root therefore passed the check and opened its target, wherever that was: upload_object would read a file outside the root and send it to Cloud Storage, and download_object would write through the link. The write escape did not require overwrite=true. O_CREATE|O_EXCL does reject a symlinked final component, but an intermediate directory link -- root/dir -> /elsewhere -- still lands the file outside the root under the default overwrite=false. Both boundaries now compare the symlink-resolved target in addition to the name. Roots are resolved as well, so a root reached through a link (/tmp on macOS, a symlinked workspace) keeps matching, and links resolving back inside a root keep working. Destinations that do not exist yet resolve as deeply as the filesystem allows; a dangling link is rejected rather than treated as a missing name, since creating that path would follow it. Two limits are documented rather than fixed: hard links are indistinguishable from regular files, and the check still precedes the open, so anything able to write into an allowed root can swap a component in between. OS permissions remain the enforcing boundary.
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces robust symbolic link resolution and validation for local filesystem access within the cloud storage source. It updates validateLocalPath and ResolveWithinDir to resolve symlinks for both the input paths and the allowed root directories before performing boundary checks, preventing potential directory traversal bypasses via symlinks. Comprehensive unit tests and documentation updates have been added to cover these changes. No review comments were provided, so there is no feedback to address.
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.
Description
allowedLocalRoots(source) anddestination_dir(the download tool) both decided whether a local path was in bounds by inspecting the path string, then handed the unresolved name toos.Open/os.OpenFile. Neither resolved symbolic links, so a link placed under an allowed root passed the check and opened its target wherever that was:cloud-storage-upload-objectreads a file outside the root and sends it to Cloud Storage.cloud-storage-download-objectwrites through the link to a file outside the root.The write escape does not require
overwrite=true.O_CREATE|O_EXCLdoes reject a symlinked final component, but an intermediate directory link —root/dir -> /elsewhere— still lands the file outside the root under the defaultoverwrite=false.Solution
New
cloudstoragecommon.ResolveSymlinksreturns a path's final filesystem target, handling two cases a barefilepath.EvalSymlinksdoes not:EvalSymlinksreturnsENOENTfor a path that doesn't exist yet, which is the normal case for a download destination. It resolves the deepest existing ancestor and reattaches the missing components literally.ENOENT, and treating one as "just a missing file" would reopen the escape, since a later create follows the link. Rejected instead.Both boundaries now compare the resolved target in addition to the name:
Source.validateLocalPathrequires the path to be under an allowed root both as written and after resolution. Keeping the name check first means rejection messages still name what the caller asked for, and guarantees the new behavior is never looser than the old.ResolveWithinDirgets the same treatment fordestination_dir, which had the identical flaw.Roots are resolved too, so a root reached through a link (
/tmpon macOS, a symlinked workspace) keeps matching, and links resolving back inside a root keep working.Known limits (documented, not fixed)
os.Root/openat2at the point of I/O — a larger change; happy to follow up if reviewers prefer it here.source.mdnow states the guarantee and both limits, and keeps the guidance that OS permissions are the enforcing boundary.Behavior change
A workspace that legitimately symlinks out of an allowed root is now rejected. That is the intent of the fix, but it may surface as a bug report.
PR Checklist
./cmd/... ./internal/...),go vet, andgofmtpass.golangci-lintwas not available locally and has not been run!if this involve a breaking change — tightens validation rather than changing an API, so filed asfix; see "Behavior change" aboveTesting
Each fix was reverted in turn to confirm the new tests fail without it.
TestAllowedLocalRootSymlinkEscapereproduces the reported scenario against the unpatched source:All four
destination_direscape cases likewise fail against the unpatchedResolveWithinDir. Coverage added for: escaping link leaf, escaping intermediate directory link, new file under an escaping directory link, dangling link, links resolving back inside the root, roots reached through a link, and unsetallowedLocalRootsremaining unrestricted — across both boundaries.🛠️ Fixes #<issue_number_goes_here>