Path Traversal / Local File Write during restore

PoC (demonstrates exploitable path traversal prior to fix): Assumptions: - storageDataPath (Dir) is /data/storage - A backup source contains a part with a path that traverses outside the storage directory, e.g. "../outside/evil.txt" or similar. - The restore logic uses Part.Path to compute the write path without proper validation. Steps: 1) Create a crafted backup part with Path: "../outside/evil.txt" and destination Dir: "/data/storage". 2) During Restore.Run, the code would (in vulnerable versions) map this to a writable location outside /data/storage and proceed to write there. 3) With the fix, the guard in the restore code checks IsLocalPathInsideDir(dir) for every part and rejects the operation if any part would escape /data/storage. 4) As an additional defense, NewDirectWriteCloser panics if an outside write is attempted, preventing any write through the local filesystem layer. Minimal runnable illustration (conceptual, uses internal types): package main import ( "fmt" "github.com/VictoriaMetrics/VictoriaMetrics/lib/backup/common" fslocal "github.com/VictoriaMetrics/VictoriaMetrics/lib/backup/fslocal" ) func main() { storageDir := "/data/storage" p := common.Part{Path: "../outside/evil.txt"} // Before fix: this would likely be allowed to write outside storageDir // After fix: p.IsLocalPathInsideDir(storageDir) would return false and Restore would error out. inside := p.IsLocalPathInsideDir(storageDir) fmt.Printf("IsLocalPathInsideDir(%q) => %v\n", storageDir, inside) // Attempting direct write would panic under the defensive check in NewDirectWriteCloser if outside. fs := &fslocal.FS{Dir: storageDir} if !inside { fmt.Println("Attempt would be blocked by restore guard (and panic in NewDirectWriteCloser).") } else { // This path would only be taken if the path were inside, which it is not in this PoC. w, err := fs.NewDirectWriteCloser(p) if err != nil { fmt.Println("Write error:", err) return } _ = w.Close() } }

diff --git a/cve/..foo.txt b/cve/..foo.txt new file mode 100644 index 0000000000000..e69de29bb2d1d diff --git a/lib/backup/actions/restore.go b/lib/backup/actions/restore.go index 2c816a0e173f4..10fc8ff4f39bd 100644 --- a/lib/backup/actions/restore.go +++ b/lib/backup/actions/restore.go @@ -91,6 +91,11 @@ func (r *Restore) Run(ctx context.Context) error { if err != nil { return fmt.Errorf("cannot list src parts: %w", err) } + for _, srcPart := range srcParts { + if !srcPart.IsLocalPathInsideDir(r.Dst.Dir) { + return fmt.Errorf("part file %s would be written outside storage directory %s", srcPart.Path, r.Dst.Dir) + } + } logger.Infof("obtaining list of parts at %s", dst) dstParts, err := dst.ListParts() if err != nil { diff --git a/lib/backup/common/part.go b/lib/backup/common/part.go index 84e3d1d7007f4..bf3cd9263fc9a 100644 --- a/lib/backup/common/part.go +++ b/lib/backup/common/part.go @@ -120,6 +120,17 @@ func (p *Part) ParseFromRemotePath(remotePath string) bool { return true } +// IsLocalPathInsideDir returns true if the part's local path resolves inside dir. +// It resolves ../../ sequences and prevents path traversal outside dir. +func (p *Part) IsLocalPathInsideDir(dir string) bool { + dir = filepath.Clean(dir) + if dir == `/` { + return true + } + + return strings.HasPrefix(p.LocalPath(dir), dir+string(filepath.Separator)) +} + // MaxPartSize is the maximum size for each part. // // The MaxPartSize reduces bandwidth usage during retires on network errors diff --git a/lib/backup/common/part_test.go b/lib/backup/common/part_test.go new file mode 100644 index 0000000000000..29f473c73827f --- /dev/null +++ b/lib/backup/common/part_test.go @@ -0,0 +1,54 @@ +package common + +import ( + "testing" +) + +func TestIsLocalPathInsideDir(t *testing.T) { + f := func(dir, path string, expected bool) { + t.Helper() + p := Part{Path: path} + if got := p.IsLocalPathInsideDir(dir); got != expected { + t.Fatalf("IsLocalPathInsideDir(%q, %q): got %v, want %v", dir, path, got, expected) + } + } + + // normal path inside dir + f("/data/storage", "parts/segment1/data.bin", true) + + // dir with trailing slash is normalized + f("/data/storage/", "parts/segment1/data.bin", true) + + // deeply nested path + f("/data/storage", "a/b/c/d/e/file.dat", true) + + // traversal that stays inside dir + f("/data/storage", "foo/../bar/file.dat", true) + + // root dir allows any path + f("/", "any/path/here", true) + + // root dir allows traversal attempts since nothing is outside / + f("/", "../outside/marker.txt", true) + + // path with leading slash is treated as relative by filepath.Join and stays inside dir + f("/data/storage", "/outside/marker.txt", true) + + // dir with .. components is normalized; path inside resolved dir + f("/data/storage/../foo", "parts/file.dat", true) + + // dir with .. components is normalized; traversal outside resolved dir + f("/data/storage/../foo", "../storage/evil.txt", false) + + // simple traversal + f("/data/storage", "../outside/marker.txt", false) + + // traversal with trailing slash in dir + f("/data/storage/", "../outside/marker.txt", false) + + // deep traversal + f("/data/storage", "a/../../outside/marker.txt", false) + + // sibling directory whose name shares a prefix with dir + f("/data/storage", "../storagefoo/evil.txt", false) +} diff --git a/lib/backup/fslocal/fslocal.go b/lib/backup/fslocal/fslocal.go index 43d8eec791a4a..fde2507c85756 100644 --- a/lib/backup/fslocal/fslocal.go +++ b/lib/backup/fslocal/fslocal.go @@ -129,6 +129,10 @@ func (fs *FS) NewReadCloser(p common.Part) (io.ReadCloser, error) { // On platforms with preallocation, writes go to a .tmp file that must be // finalized with FinalizeFile. func (fs *FS) NewDirectWriteCloser(p common.Part) (io.WriteCloser, error) { + if !p.IsLocalPathInsideDir(fs.Dir) { + logger.Fatalf("BUG: part file %s would be written outside storage directory %s", p.Path, fs.Dir) + } + path := fs.writePath(p) if err := fs.mkdirAll(path); err != nil { return nil, err