fix(snapshot s3): reject object keys with ".." segments and never overwrite a downloaded object - #1155
Conversation
…rwrite a downloaded object
… --exclude-regex Windows drops trailing spaces and dots from a name, so a segment such as ".. " can resolve as "..". Trim before comparing so the check does not depend on the platform running the snapshot. --exclude matches the raw key with a slash-trimmed pattern, so it cannot exclude a key that starts with "/"; --exclude-regex always can. A single filepath.Clean check replaces the two-literal guard for keys that name no file, and the redundant FromSlash is dropped: IsLocal and Join accept both separators on every platform.
|
Claude finished @AlexKantor87's task in 7m 24s —— View job Review — round 8
No new findings. The two items from round 7 both do what they claim, and I have nothing blocking to add. Static analysis
The Windows cross-compile is the one worth calling out: it's the only mechanical check available for the
Verifying the round-7 changesThe contract row is no longer vacuous ( The Two things I checked independentlyBoth path checks are load-bearing, and the PR's framing of them is accurate. I read The same-fingerprint claim holds on modes as well as layout. Also confirmed: the new help line at One note for the human approverThe largest remaining hole is the one the PR scopes out deliberately: two directories differing only in case (or only in Unicode normalisation, on macOS) still merge, because Worth acknowledging
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… directory cannot be made The rule error already named the key and pointed at --exclude-regex; the O_EXCL and MkdirAll errors surfaced a bare temp-dir path instead. One helper now builds every unusable-key error, and the collision test pins the key and the advice.
A disk-full or permission error was carrying "exclude it with
--exclude-regex"; following that on a legitimate object would record a
snapshot with the object silently missing. Filesystem errors now name the
key and wrap the cause with no advice. The rule rejections and an O_EXCL
collision, which the key does cause, keep the advice.
Pins the object-and-prefix case ("a" plus "a/b") and an unwritable
download directory, and documents that only leading "/" is trimmed.
…ising on a rejected key
…ey on download failure os.MkdirAll returns ENOTDIR portably when a parent of the destination is already a file, which only a bucket holding both "a" and "a/b" can cause, so that error now carries the exclusion advice like an O_EXCL collision. The advice no longer presumes an include filter is in use.
…t on Windows, pin the overwrite message
…at --exclude-regex needs escaping
…ror wrap The contract row read one page, and the fake serves one key per page in the contract suite, so a one-element slice was always sorted. Walking the paginator checks the order the snapshot actually consumes; reversing the fake's sort now fails it. A nested key in the permissions test reaches the MkdirAll wrap, which a bare key never did.
Cut the test doc comments that restated their test names, the history and the reference to a sibling test, and reduced each remaining comment to the fact a reader cannot get from the code.
mbevc1
left a comment
mbevc1
left a comment
There was a problem hiding this comment.
I wonder if we could avoid doing downloads or perhaps use temporary names when downloading, instead of relying on target OS compatibility
|
Both good instincts, and the second one is better than I first gave it credit for. Answering properly rather than just deferring to the roadmap. Avoiding the download is where we're heading: that's your Temporary names is the interesting one. It doesn't work on its own, because Two things to know before it does, both from reading your branch rather than the description: It moves the path rule rather than removing it, and the two rules disagree in both directions.
So a bucket that snapshots fine today breaks when the default flips, on keys that have nothing to do with the vulnerability. Worth settling on one rule across both modes before that happens, rather than discovering it in a customer's pipeline.
Keeping this PR as the interim guard, since it's small and can ship now. Happy to be wrong about the ordering if you'd rather go straight at it. |
2 participants
What
kosli snapshot s3downloads objects into a temp directory before fingerprinting. Object keys containing a..path segment are now rejected, and each destination file is created exclusively so two keys can never write the same local file. A key that fails either check fails the snapshot and names the key. Keys that snapshot successfully today keep the same local layout and therefore the same fingerprint.The
..check splits on both/and\, and treats a segment that Windows would normalise to..(trailing spaces or dots, e.g...or...) as.., so the rule does not depend on which platform runs the snapshot.filepath.IsLocalis the OS-specific safety net on top: on Windows it additionally rejects rooted paths, any colon, and reserved device names.Why
A key that normalises to a different path than it names could replace another object's downloaded content, and on Windows a key containing backslash-separated
..could resolve outside the temp directory. Reported privately; tracked internally.Behaviour change
Buckets containing a
..key, or two keys that resolve to one local file, previously produced a fingerprint; they now error and name the key. Two consequences worth knowing:..(e.g.a\..\b) was a literal filename on Linux and macOS and is now rejected everywhere, so the rule is the same on every OS...(e.g....or..) is rejected everywhere, because Windows trims trailing dots and spaces and would resolve it as... A legitimate...directory is a legal name on Linux and macOS and snapshots today; it errors after this change.Use
--exclude-regexfor a legitimate key of these shapes.--excludecannot match a key that starts with/(it slash-trims the pattern, not the key), so the error text and help point at the regex form. When--include/--include-regexis set, exclude filters are ignored (pre-existing precedence), so the advice also says to narrow the include filter.Errors caused by the key (rule rejections, an
O_EXCLcollision) carry that advice; other filesystem errors (disk full, permissions, object-and-prefixENOTDIR, case-foldedEISDIR) name the key and wrap the cause with no advice, so a machine fault never suggests dropping a legitimate object.Doubled slashes, leading slashes,
.segments and backslashes without..are deliberately still accepted and land exactly wherefilepath.Joinput them before.TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeyspins that, and it was verified green against unmodifiedmain.Testing
cmd/kosliTestSnapshotS3needs the local Kosli server on:8001and AWS credentials forkosli-cli-public; it was not runnable locally and is left to CI.make test_integrationwas not run locally for the same reason.Red first.
TestGetS3DataFromClientRejectsKeysWithDotDotSegmentsagainst unmodified code:and a temporary equality assertion confirmed the poisoned bucket's fingerprint was identical to a bucket holding only the attacker's bytes under
protected/release.bin. The three attack tests (RejectsKeysWithDotDotSegments,CollidingKeysAreAnError,DownloadFileFromBucketRefusesToOverwrite) are all red againstmain.Mutations, each restored afterwards. Red: delete the whole
..segment check; split segments on/only (backslash rows red); exact..compare without trimming (..and...rows red);strings.Contains(key, "..")instead of segment compare (..hiddenrow red); replaceO_EXCLwithO_TRUNC;MkdirAll(dest)instead of the parent; drop the leading-slash trim; naive""/"."guard instead ofClean(./.row red); helper returns the key unchanged. Survived: droppingfilepath.IsLocal. On Linux and macOS it can never reject anything the..check has not already rejected, so it has no CI coverage; it is kept because on Windows it is what rejects rooted paths, colons and reserved names (reasoned frominternal/filepathlite/path_windows.go, not run; CI has no Windows job).An independent adversarial pass tried to escape the temp directory or collide silently on all three platforms (case folding, Unicode normalisation, trailing dots and spaces, NUL bytes, file/directory confusion, overlong keys) and found no escape and no silent file collision.
Not done here, deliberately
O_EXCLcovers files only. Pre-existing, noted in the code comment, tracked internally..kosli_ignorehandling (separate ticket).MkdirAll(..., 0770)andOpenFile(..., 0666)are exactly whatutils.CreateFile+os.Createproduced, so the on-disk treedigest.DirSha256walks is unchanged in modes as well as layout; that is what makes the same-fingerprint claim hold.Checklist
🤖 Generated with Claude Code