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
This commit was merged in pull request #176.
This commit is contained in:
@@ -38,6 +38,19 @@ release" is exactly the contradiction
|
|||||||
recorded with no remote backend are marked uploaded so this invariant
|
recorded with no remote backend are marked uploaded so this invariant
|
||||||
holds uniformly.
|
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
|
- 2026-09-21: Stopped `--json` from silencing stderr diagnostics
|
||||||
([issue #112](https://git.eeqj.de/sneak/vaultik/issues/112)). `--json`
|
([issue #112](https://git.eeqj.de/sneak/vaultik/issues/112)). `--json`
|
||||||
used to be folded into `Quiet`, which pinned the log level to `WARN`,
|
used to be folded into `Quiet`, which pinned the log level to `WARN`,
|
||||||
|
|||||||
+91
-11
@@ -11,6 +11,7 @@ import (
|
|||||||
"math"
|
"math"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
"filippo.io/age"
|
"filippo.io/age"
|
||||||
@@ -30,10 +31,12 @@ var (
|
|||||||
"Set the VAULTIK_AGE_SECRET_KEY environment variable to your " +
|
"Set the VAULTIK_AGE_SECRET_KEY environment variable to your " +
|
||||||
"age private key:\n" +
|
"age private key:\n" +
|
||||||
" export VAULTIK_AGE_SECRET_KEY='AGE-SECRET-KEY-...'")
|
" export VAULTIK_AGE_SECRET_KEY='AGE-SECRET-KEY-...'")
|
||||||
errBlobMissingFromIndex = errors.New("blob hash missing from blob index")
|
errBlobMissingFromIndex = errors.New("blob hash missing from blob index")
|
||||||
errChunkNotInAnyBlob = errors.New("chunk not found in any blob")
|
errChunkNotInAnyBlob = errors.New("chunk not found in any blob")
|
||||||
errBlobIDNotInHashIndex = errors.New("blob id missing from hash index")
|
errBlobIDNotInHashIndex = errors.New("blob id missing from hash index")
|
||||||
errShortChunkRead = errors.New("short read")
|
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
|
// restoreDirMode is the permission mode for directories created while
|
||||||
@@ -760,13 +763,85 @@ type restoreSession struct {
|
|||||||
runningAsRoot bool
|
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.
|
// restoreFile dispatches to the right per-kind restorer.
|
||||||
func (s *restoreSession) restoreFile(file *database.File) error {
|
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)
|
parentDir := filepath.Dir(targetPath)
|
||||||
|
|
||||||
err := s.v.Fs.MkdirAll(parentDir, restoreDirMode)
|
err = s.v.Fs.MkdirAll(parentDir, restoreDirMode)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return fmt.Errorf("creating parent directory: %w", err)
|
return fmt.Errorf("creating parent directory: %w", err)
|
||||||
}
|
}
|
||||||
@@ -1062,17 +1137,22 @@ func (v *Vaultik) verifyRestoredFiles(
|
|||||||
return ctx.Err()
|
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 {
|
if err != nil {
|
||||||
log.Error("File verification failed", "path", file.Path, "error", err)
|
log.Error("File verification failed", "path", file.Path, "error", err)
|
||||||
|
|
||||||
result.FilesFailed++
|
result.FilesFailed++
|
||||||
result.FailedFiles = append(result.FailedFiles, file.Path.String())
|
result.FailedFiles = append(result.FailedFiles, file.Path.String())
|
||||||
} else {
|
|
||||||
result.FilesVerified++
|
|
||||||
result.BytesVerified += bytesVerified
|
|
||||||
}
|
}
|
||||||
|
|
||||||
bytesProcessed += file.Size
|
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)
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user