diff --git a/TODO.md b/TODO.md index 0242f5a..1a33ea4 100644 --- a/TODO.md +++ b/TODO.md @@ -38,6 +38,19 @@ release" is exactly the contradiction recorded with no remote backend are marked uploaded so this invariant holds uniformly. +- 2026-09-22: Made restore refuse any snapshot path that would write + outside the target directory + ([issue #154](https://git.eeqj.de/sneak/vaultik/issues/154)). + `restoreFile` and `verifyRestoredFiles` joined the stored path onto the + target with no containment check, so a `..` segment or an absolute path + escaped the target and a restored symlink could redirect a later child + write anywhere on disk. Every stored path is now rejected unless + `filepath.IsLocal` accepts it with the leading separator removed, and + each existing ancestor directory below the target is `Lstat`ed to refuse + descending through a symlink; honest symlinks pointing outside the tree + are still written verbatim. age decryption proves a snapshot is + readable, not honest, and restore usually runs as root. + - 2026-09-21: Stopped `--json` from silencing stderr diagnostics ([issue #112](https://git.eeqj.de/sneak/vaultik/issues/112)). `--json` used to be folded into `Quiet`, which pinned the log level to `WARN`, diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index c46ff5f..e847939 100644 --- a/internal/vaultik/restore.go +++ b/internal/vaultik/restore.go @@ -11,6 +11,7 @@ import ( "math" "os" "path/filepath" + "strings" "time" "filippo.io/age" @@ -30,10 +31,12 @@ var ( "Set the VAULTIK_AGE_SECRET_KEY environment variable to your " + "age private key:\n" + " export VAULTIK_AGE_SECRET_KEY='AGE-SECRET-KEY-...'") - errBlobMissingFromIndex = errors.New("blob hash missing from blob index") - errChunkNotInAnyBlob = errors.New("chunk not found in any blob") - errBlobIDNotInHashIndex = errors.New("blob id missing from hash index") - errShortChunkRead = errors.New("short read") + errBlobMissingFromIndex = errors.New("blob hash missing from blob index") + errChunkNotInAnyBlob = errors.New("chunk not found in any blob") + errBlobIDNotInHashIndex = errors.New("blob id missing from hash index") + errShortChunkRead = errors.New("short read") + errRestorePathEscapesTarget = errors.New( + "refusing to restore path outside the target directory") ) // restoreDirMode is the permission mode for directories created while @@ -760,13 +763,85 @@ type restoreSession struct { runningAsRoot bool } +// containedRestorePath resolves rel — a path read from the snapshot +// database — to its location under targetDir and confirms the write will +// stay inside the target. +// +// age decryption proves a snapshot is readable, not that it is honest, so +// every stored path is treated as hostile. rel is rejected unless +// filepath.IsLocal accepts it once the leading separator is stripped: +// stored paths are absolute and the join to targetDir drops that +// separator, so "/etc/passwd" is judged as the relative "etc/passwd" it +// becomes on disk. This bars "..", absolute, and empty paths. +// +// A stored symlink whose target points outside the tree is still honest +// (and restored verbatim), but a later entry must not be written through +// it. Each existing ancestor directory below the target is therefore +// Lstat'ed and a symlink among them is refused. The leaf itself is not +// traversed: honest snapshots restore symlinks at leaf positions, and the +// unique-path constraint keeps a leaf from being both a symlink and a +// regular file. The target directory itself may be a symlink; only +// components below it are checked. +func containedRestorePath(fs afero.Fs, targetDir, rel string) (string, error) { + local := strings.TrimPrefix(rel, string(filepath.Separator)) + if !filepath.IsLocal(local) { + return "", fmt.Errorf("%w: %s", errRestorePathEscapesTarget, rel) + } + + local = filepath.Clean(local) + targetPath := filepath.Join(targetDir, local) + + relDir := filepath.Dir(local) + if relDir == "." { + return targetPath, nil + } + + current := targetDir + for component := range strings.SplitSeq(relDir, string(filepath.Separator)) { + current = filepath.Join(current, component) + + info, err := lstatIfPossible(fs, current) + if err != nil { + if os.IsNotExist(err) { + continue + } + + return "", fmt.Errorf("checking restore path %s: %w", current, err) + } + + if info.Mode()&os.ModeSymlink != 0 { + return "", fmt.Errorf("%w: %s descends through symlink %s", + errRestorePathEscapesTarget, rel, current) + } + } + + return targetPath, nil +} + +// lstatIfPossible performs a symlink-aware stat when the filesystem +// supports it. afero.OsFs does; MemMapFs, which has no symlinks, reports +// that Lstat was not used and its result never carries ModeSymlink. +func lstatIfPossible(fs afero.Fs, name string) (os.FileInfo, error) { + if lstater, ok := fs.(afero.Lstater); ok { + info, _, err := lstater.LstatIfPossible(name) + + return info, err + } + + return fs.Stat(name) +} + // restoreFile dispatches to the right per-kind restorer. func (s *restoreSession) restoreFile(file *database.File) error { - targetPath := filepath.Join(s.opts.TargetDir, file.Path.String()) + targetPath, err := containedRestorePath( + s.v.Fs, s.opts.TargetDir, file.Path.String()) + if err != nil { + return err + } parentDir := filepath.Dir(targetPath) - err := s.v.Fs.MkdirAll(parentDir, restoreDirMode) + err = s.v.Fs.MkdirAll(parentDir, restoreDirMode) if err != nil { return fmt.Errorf("creating parent directory: %w", err) } @@ -1062,17 +1137,22 @@ func (v *Vaultik) verifyRestoredFiles( return ctx.Err() } - targetPath := filepath.Join(targetDir, file.Path.String()) + targetPath, err := containedRestorePath(v.Fs, targetDir, file.Path.String()) + if err == nil { + var bytesVerified int64 + + bytesVerified, err = v.verifyFile(ctx, repos, file, targetPath) + if err == nil { + result.FilesVerified++ + result.BytesVerified += bytesVerified + } + } - bytesVerified, err := v.verifyFile(ctx, repos, file, targetPath) if err != nil { log.Error("File verification failed", "path", file.Path, "error", err) result.FilesFailed++ result.FailedFiles = append(result.FailedFiles, file.Path.String()) - } else { - result.FilesVerified++ - result.BytesVerified += bytesVerified } bytesProcessed += file.Size diff --git a/internal/vaultik/restore_containment_test.go b/internal/vaultik/restore_containment_test.go new file mode 100644 index 0000000..becbfa0 --- /dev/null +++ b/internal/vaultik/restore_containment_test.go @@ -0,0 +1,175 @@ +package vaultik //nolint:testpackage // drives unexported restore internals + +import ( + "context" + "io" + "os" + "path/filepath" + "testing" + "time" + + "github.com/spf13/afero" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/config" + "sneak.berlin/go/vaultik/internal/database" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/types" + "sneak.berlin/go/vaultik/internal/ui" +) + +// These tests exercise the path-containment guard that keeps restore from +// writing outside its target directory. age decryption proves only that a +// snapshot is readable, not that its recorded paths are honest, so restore +// treats every stored path as hostile: a compromised backed-up host could +// forge a snapshot that decrypts cleanly, and restore usually runs as root. +// +// They drive restoreAllFiles directly (rather than the full Restore, which +// downloads and decrypts the metadata database from storage) so a snapshot +// database with adversarial rows can be handed to the restore loop without +// the surrounding blob/storage machinery. Directory and symlink entries +// carry no chunks, so no blobs are needed. + +// containmentDirMode marks a File row as a directory for the restore loop. +const containmentDirMode = uint32(os.ModeDir | 0o755) + +// newContainmentVaultik builds the minimal Vaultik needed to run +// restoreAllFiles against fs. +func newContainmentVaultik(ctx context.Context, fs afero.Fs) *Vaultik { + v := &Vaultik{ + Config: &config.Config{ + BlobSizeLimit: config.Size(10 * 1024 * 1024), + }, + Fs: fs, + Stdout: io.Discard, + Stderr: io.Discard, + UI: ui.NewWithColor(io.Discard, false), + } + v.SetContext(ctx) + + return v +} + +// makeFiles inserts the given rows into a fresh in-memory snapshot database +// and returns them (with IDs assigned) plus the repositories. +func makeFiles( + ctx context.Context, t *testing.T, rows []*database.File, +) ([]*database.File, *database.Repositories) { + t.Helper() + + db, err := database.New(ctx, filepath.Join(t.TempDir(), "index.sqlite")) + require.NoError(t, err) + t.Cleanup(func() { _ = db.Close() }) + + repos := database.NewRepositories(db) + for _, f := range rows { + require.NoError(t, repos.Files.Create(ctx, nil, f)) + } + + return rows, repos +} + +func TestRestoreRejectsPathTraversal(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + tests := []struct { + name string + // rows are inserted in order; the escape entry is restored after + // any entry it depends on (the symlink case needs its link first). + rows func(outsideDir string) []*database.File + // escaped is the path, outside the target, that must not appear. + escaped func(tempDir, outsideDir string) string + }{ + { + name: "relative dotdot", + rows: func(_ string) []*database.File { + return []*database.File{{ + Path: "../escaped-relative", + Mode: containmentDirMode, + }} + }, + escaped: func(tempDir, _ string) string { + return filepath.Join(tempDir, "escaped-relative") + }, + }, + { + name: "absolute with dotdot", + rows: func(_ string) []*database.File { + return []*database.File{{ + Path: "/a/../../escaped-absolute", + Mode: containmentDirMode, + }} + }, + escaped: func(tempDir, _ string) string { + return filepath.Join(tempDir, "escaped-absolute") + }, + }, + { + name: "child through symlink", + rows: func(outsideDir string) []*database.File { + return []*database.File{ + // Restored first: an in-target symlink pointing out. + {Path: "linkdir", LinkTarget: types.FilePath(outsideDir)}, + // Restored second: a child written through that link. + {Path: "linkdir/child", Mode: containmentDirMode}, + } + }, + escaped: func(_, outsideDir string) string { + return filepath.Join(outsideDir, "child") + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + fs := afero.NewOsFs() + tempDir := t.TempDir() + targetDir := filepath.Join(tempDir, "target") + outsideDir := filepath.Join(tempDir, "outside") + require.NoError(t, fs.MkdirAll(outsideDir, 0o755)) + + rows, repos := makeFiles(ctx, t, tc.rows(outsideDir)) + v := newContainmentVaultik(ctx, fs) + + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: targetDir}, nil, nil) + + require.ErrorIs(t, err, errRestorePathEscapesTarget) + + escaped := tc.escaped(tempDir, outsideDir) + _, statErr := os.Lstat(escaped) + require.Truef(t, os.IsNotExist(statErr), + "restore wrote outside the target at %s", escaped) + }) + } +} + +// TestRestoreAllowsSymlinkPointingOutsideTree confirms the guard does not +// over-block: an honest snapshot may contain a symlink whose target lies +// outside the restored tree, and it must still be restored verbatim. +func TestRestoreAllowsSymlinkPointingOutsideTree(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + ctx := context.Background() + fs := afero.NewOsFs() + tempDir := t.TempDir() + targetDir := filepath.Join(tempDir, "target") + linkTarget := filepath.Join(tempDir, "outside", "data") + + rows, repos := makeFiles(ctx, t, []*database.File{ + {Path: "goodlink", LinkTarget: types.FilePath(linkTarget), MTime: time.Unix(0, 0)}, + }) + v := newContainmentVaultik(ctx, fs) + + _, err := v.restoreAllFiles(rows, repos, + &RestoreOptions{TargetDir: targetDir}, nil, nil) + require.NoError(t, err) + + got, err := os.Readlink(filepath.Join(targetDir, "goodlink")) + require.NoError(t, err) + require.Equal(t, linkTarget, got) +}