Keep restore writes inside the target directory (closes #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. Since age decryption proves a snapshot is readable but not honest, and restore usually runs as root, a forged snapshot became an arbitrary file write. Both call sites now go through containedRestorePath: it rejects a stored path unless filepath.IsLocal accepts it with the leading separator removed (barring "..", absolute, and empty paths), then Lstats each existing ancestor below the target and refuses to descend through a symlink. The target directory itself may be a symlink, and honest symlinks pointing outside the tree are still written verbatim. Model: opus-4-8
2026-09-22 07:25:19 +00:00
parent 994e5de613
commit 3441eec48c
3 changed files with 279 additions and 11 deletions
@@ -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
@@ -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)
+}