app/vmrestore: disallow restoring parts outside the configured -storageDataPath directory (#1051)

A specifically crafted backup source (compromised S3/GCS/Azure bucket) could use `..`
components in object names to write files outside the `-storageDataPath`
directory.

Fix by validating all source parts against the destination directory
before restore begins (restore.go), and adding a defense-in-depth panic
guard in `NewDirectWriteCloser` (fslocal.go).

PR https://github.com/VictoriaMetrics/VictoriaMetrics-enterprise/pull/1051

Co-authored-by: Roman Khavronenko <roman@victoriametrics.com>

docs/victoriametrics/changelog/CHANGELOG.md

@@ -39,6 +39,7 @@ See also [LTS releases](https://docs.victoriametrics.com/victoriametrics/lts-rel
 * BUGFIX: [vmbackup](https://docs.victoriametrics.com/vmbackup/), [vmbackupmanager](https://docs.victoriametrics.com/victoriametrics/vmbackupmanager/): do not fail backup list if directory is absent while using `fs://` destination to align with other protocols. See [6c3c548](https://github.com/VictoriaMetrics/VictoriaMetrics/commit/6c3c548ddb0385b749e731f52276f130e2a4e4a8)
 * BUGFIX: [vmsingle](https://docs.victoriametrics.com/victoriametrics/single-server-victoriametrics/) and `vmstorage` in [VictoriaMetrics cluster](https://docs.victoriametrics.com/victoriametrics/cluster-victoriametrics/): prevent more cases of panic during directory deletion on `NFS`-based mounts. See [#11060](https://github.com/VictoriaMetrics/VictoriaMetrics/issues/11060).
 * BUGFIX: [vmctl](https://docs.victoriametrics.com/victoriametrics/vmctl/): push metrics to configured `-pushmetrics.url` on shutdown when migration fails. Previously, metrics were not pushed if vmctl exited with an error. See [#11081](https://github.com/VictoriaMetrics/VictoriaMetrics/pull/11081).
+* BUGFIX: [vmrestore](https://docs.victoriametrics.com/victoriametrics/vmrestore/): disallow restoring parts outside the configured `-storageDataPath` directory.
 
 ## [v1.145.0](https://github.com/VictoriaMetrics/VictoriaMetrics/releases/tag/v1.145.0)
 

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 {

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

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)
+}

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