fix: reject non-local snapshot chunk names in AddChunk - #1247
fix: reject non-local snapshot chunk names in AddChunk#1247basavaraj-sm05 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR hardens snapshot chunk handling by preventing path traversal in pebbleSnapshotLoader.AddChunk, ensuring snapshot senders cannot write files outside the loader directory, and adds a regression test for the vulnerability.
Changes:
- Validate snapshot chunk filenames with
filepath.IsLocalbefore writing chunk data to disk - Add a regression test to confirm traversal attempts are rejected and no outside file is created
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| oxiad/dataserver/database/kvstore/kv_pebble.go | Adds filename validation in AddChunk to block path traversal writes outside the snapshot loader directory |
| oxiad/dataserver/database/kvstore/kv_pebble_test.go | Adds a regression test ensuring traversal chunk names are rejected and do not create files outside the shard data directory |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // fileName arrives from the snapshot sender over the network. Reject any | ||
| // name that isn't confined to the loader directory so a peer can't use | ||
| // path components like "../" to write outside of it. | ||
| if !filepath.IsLocal(fileName) { | ||
| return errors.Errorf("invalid snapshot chunk file name: %q", fileName) | ||
| } | ||
| sl.file, err = os.OpenFile(filepath.Join(sl.dbPath, fileName), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0644) |
There was a problem hiding this comment.
Good point, and you're right that IsLocal on its own is too loose here. I checked it: IsLocal(".") is true and so is IsLocal("sub/dir/file"), so both would have slipped through.
The sender side settles it. newPebbleSnapshot builds its file list with os.ReadDir over the checkpoint dir and skips anything where de.IsDir(), so ps.files only ever holds plain base names of regular files sitting directly in that dir. Nothing legitimate ever carries a separator. Tightened the check to require exactly that:
if fileName != filepath.Base(fileName) || fileName == "." || !filepath.IsLocal(fileName) {
The Base comparison is what rejects nested paths and trailing separators, and it stays platform-correct since Base follows the OS separator rules. "." needs calling out on its own because it survives both Base and IsLocal. I kept IsLocal on the end rather than replacing it, since it still buys the absolute-path case and the Windows reserved device names, which a bare Base check would happily accept.
Extended the test to cover the newly rejected names, plus an accept case with a real marker file name so the check can't quietly get strict enough to break a legitimate snapshot.
| dataDir := t.TempDir() | ||
| factory, err := NewPebbleKVFactory(&FactoryOptions{ | ||
| DataDir: dataDir, | ||
| CacheSizeMB: 1, | ||
| }) | ||
| assert.NoError(t, err) | ||
|
|
||
| loader, err := factory.NewSnapshotLoader(constant.DefaultNamespace, 1) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
Makes sense, switched the three setup assertions to require.NoError. The factory and loader ones are the ones that actually matter, since a nil loader there turns a clear construction failure into a nil-pointer panic several lines later. Left the assertions on the behavior under test as assert so a failure still reports everything it found.
Signed-off-by: basavaraj-sm05 <basavaraj@digiscrypt.com>
Signed-off-by: basavaraj-sm05 <basavaraj@digiscrypt.com>
9997f58 to
8d087b1
Compare
|
Pushed a fix for the DCO failure: same two commits, rebased on current main and signed off. |
When a follower installs a snapshot it streams SnapshotChunk messages from the sending peer and writes each one to a file named by the chunk. pebbleSnapshotLoader.AddChunk takes that name straight off the wire and joins it onto the loader directory with filepath.Join before opening the file for writing. Because filepath.Join cleans the result but does not keep it under the base, a name that walks upward with ../ lands outside the snapshot directory, so the sender controls where the bytes go rather than just what they contain. I noticed it while reading the receive path in follower_controller.loadSnapshotChunks, where firstChunk.Name and snapChunk.Name flow into the loader with no validation, and confirmed a chunk named to escape the shard directory gets written above it. The sender only ever emits plain file names from the checkpoint dir, so the fix is to reject anything that is not a local path with filepath.IsLocal before opening it. I kept the check inside AddChunk so every caller of the loader is covered, and added a regression test alongside the existing loader test.