Validate blob hashes, offsets and lengths from the destination (closes #155)

A blob hash read back from the downloaded snapshot database or the store listing was trusted unchecked. A hostile remote could set a hash such as "aa/../../etc" and have a decrypted blob written outside the cache directory, or feed a short or negative value that panicked a command.

blobDiskCache.path now refuses any key with a path separator, and ReadAt rejects a negative offset or length, bounding so a sum cannot overflow past the check. A new isBlobHash helper gates FetchBlob, shallow and deep verify, and restore: buildBlobIndexes rejects every hash from the snapshot database before any fetch. The blobs/ and metadata/ listings skip a non-conforming name, and short-hash prefixes in log and error text go through a panic-safe shortHash helper.

Model: opus-4-8

internal/vaultik/destination_validation_test.go

@@ -0,0 +1,262 @@
+package vaultik //nolint:testpackage // drives unexported input validation
+
+import (
+	"context"
+	"errors"
+	"io"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/vaultik/internal/database"
+	"sneak.berlin/go/vaultik/internal/log"
+	"sneak.berlin/go/vaultik/internal/snapshot"
+	"sneak.berlin/go/vaultik/internal/storage"
+	"sneak.berlin/go/vaultik/internal/types"
+)
+
+// These tests treat every hash, offset and length read back from the
+// destination as hostile. A blob hash comes from the downloaded snapshot
+// database or the store listing, neither of which is authenticated (see
+// https://git.eeqj.de/sneak/vaultik/issues/155), so each is validated
+// before it is used to build a path or size an allocation.
+
+// TestBlobCacheRejectsKeyWithSeparator proves the arbitrary-file-write
+// hole is closed: a blob hash that climbs out of the cache directory is
+// refused and nothing is written outside it. This is the exact write the
+// restore path performs, keyed by the hash from the snapshot database.
+func TestBlobCacheRejectsKeyWithSeparator(t *testing.T) {
+	t.Parallel()
+
+	cache, err := newBlobDiskCache(1 << 20)
+	require.NoError(t, err)
+
+	defer func() { _ = cache.Close() }()
+
+	target := filepath.Join(t.TempDir(), "pwned")
+
+	// A hash whose relative form escapes the cache directory to target.
+	key, err := filepath.Rel(cache.dir, target)
+	require.NoError(t, err)
+	require.Contains(t, key, "..")
+
+	err = cache.Put(key, []byte("secret"))
+	require.ErrorIs(t, err, errCacheKeyHasSeparator)
+
+	_, err = cache.PutFromReader(key, strings.NewReader("secret"))
+	require.ErrorIs(t, err, errCacheKeyHasSeparator)
+
+	_, statErr := os.Stat(target)
+	require.Truef(t, os.IsNotExist(statErr),
+		"cache wrote outside its directory at %s", target)
+}
+
+// TestBuildBlobIndexesRejectsHostileHash proves restore refuses a snapshot
+// database whose blob_hash escapes the cache directory. buildBlobIndexes is
+// the first place restore reads these hashes back, and it fails there, before
+// any blob is fetched or written, so a hash containing /../ cannot steer a
+// later write outside the cache directory.
+func TestBuildBlobIndexesRejectsHostileHash(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	db, err := database.New(ctx, filepath.Join(t.TempDir(), "index.sqlite"))
+	require.NoError(t, err)
+
+	defer func() { _ = db.Close() }()
+
+	// A blob cache and a target file just outside it. The hostile hash is
+	// the relative path from the cache to that target, so an unguarded
+	// restore keyed by this hash would write there.
+	cache, err := newBlobDiskCache(1 << 20)
+	require.NoError(t, err)
+
+	defer func() { _ = cache.Close() }()
+
+	target := filepath.Join(t.TempDir(), "pwned")
+	hostile, err := filepath.Rel(cache.dir, target)
+	require.NoError(t, err)
+	require.Contains(t, hostile, "..")
+
+	repos := database.NewRepositories(db)
+	require.NoError(t, repos.Blobs.Create(ctx, nil, &database.Blob{
+		ID:        types.NewBlobID(),
+		Hash:      types.BlobHash(hostile),
+		CreatedTS: time.Now().UTC(),
+	}))
+
+	v := NewForTesting(nil)
+	v.SetContext(ctx)
+
+	_, _, err = v.buildBlobIndexes(repos)
+	require.ErrorIs(t, err, errInvalidBlobHash)
+
+	_, statErr := os.Stat(target)
+	require.Truef(t, os.IsNotExist(statErr),
+		"restore wrote outside the cache directory at %s", target)
+}
+
+// TestBlobCacheReadAtRejectsBadBounds proves a blob_chunks row cannot
+// drive an out-of-range or negative read. offset/length reach ReadAt
+// straight from the database.
+func TestBlobCacheReadAtRejectsBadBounds(t *testing.T) {
+	t.Parallel()
+
+	cache, err := newBlobDiskCache(1 << 20)
+	require.NoError(t, err)
+
+	defer func() { _ = cache.Close() }()
+
+	require.NoError(t, cache.Put("blob", make([]byte, 100)))
+
+	_, err = cache.ReadAt("blob", -1, 10)
+	require.ErrorIs(t, err, errCacheNegativeRead)
+
+	_, err = cache.ReadAt("blob", 0, -1)
+	require.ErrorIs(t, err, errCacheNegativeRead)
+
+	// A length past the end is rejected via the subtraction bound, so a
+	// huge offset+length cannot overflow past the check.
+	_, err = cache.ReadAt("blob", 50, 60)
+	require.ErrorIs(t, err, errCacheReadBeyondBlob)
+}
+
+// TestListAllRemoteBlobsSkipsNonConformingName proves a bogus object name
+// under blobs/ (here a three-character name) is skipped rather than
+// entering the blob map, so prune's later hash[:2]/hash[2:4] path build
+// cannot panic on it.
+func TestListAllRemoteBlobsSkipsNonConformingName(t *testing.T) {
+	// Initialize the global logger before t.Parallel() so the write lands
+	// in the serial phase and cannot race other parallel tests reading it.
+	log.Initialize(log.Config{})
+	t.Parallel()
+
+	good := strings.Repeat("a", blobHashHexLen)
+	store := &stubLister{objects: []storage.ObjectInfo{
+		{Key: "blobs/" + good[:2] + "/" + good[2:4] + "/" + good, Size: 10},
+		{Key: "blobs/a/b/c", Size: 3},
+	}}
+
+	v := &Vaultik{Storage: store}
+	v.SetContext(context.Background())
+
+	blobs, err := v.listAllRemoteBlobs()
+	require.NoError(t, err)
+	require.Contains(t, blobs, good)
+	require.NotContains(t, blobs, "c")
+	require.Len(t, blobs, 1)
+}
+
+// TestVerifyManifestBlobsRejectsShortHash proves a manifest (which is not
+// authenticated) with a short blob hash fails cleanly instead of panicking
+// on blob.Hash[:2].
+func TestVerifyManifestBlobsRejectsShortHash(t *testing.T) {
+	t.Parallel()
+
+	v := &Vaultik{Stdout: io.Discard}
+	manifest := &snapshot.Manifest{
+		Blobs: []snapshot.BlobInfo{{Hash: "abc", CompressedSize: 1}},
+	}
+
+	verified, missing, mismatched, missingSize, err :=
+		v.verifyManifestBlobs(manifest, &VerifyOptions{JSON: true})
+	require.ErrorIs(t, err, errInvalidBlobHash)
+	require.Zero(t, verified)
+	require.Zero(t, missing)
+	require.Zero(t, mismatched)
+	require.Zero(t, missingSize)
+}
+
+// TestVerifyBlobChunksRejectsNegativeLength proves a blob_chunks row with a
+// negative length returns an error rather than reaching make([]byte,
+// length) or streaming an untrusted size.
+func TestVerifyBlobChunksRejectsNegativeLength(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	db, err := database.New(ctx, filepath.Join(t.TempDir(), "index.sqlite"))
+	require.NoError(t, err)
+
+	defer func() { _ = db.Close() }()
+
+	repos := database.NewRepositories(db)
+	blobHash := strings.Repeat("b", blobHashHexLen)
+	blob := &database.Blob{
+		ID:        types.NewBlobID(),
+		Hash:      types.BlobHash(blobHash),
+		CreatedTS: time.Now().UTC(),
+	}
+	require.NoError(t, repos.Blobs.Create(ctx, nil, blob))
+
+	chunkHash := strings.Repeat("c", blobHashHexLen)
+	require.NoError(t, repos.Chunks.Create(ctx, nil,
+		&database.Chunk{ChunkHash: types.ChunkHash(chunkHash), Size: 1024}))
+	require.NoError(t, repos.BlobChunks.Create(ctx, nil, &database.BlobChunk{
+		BlobID:    blob.ID,
+		ChunkHash: types.ChunkHash(chunkHash),
+		Offset:    0,
+		Length:    -1,
+	}))
+
+	v := NewForTesting(nil)
+
+	_, err = v.verifyBlobChunks(db.Conn(), blobHash, strings.NewReader(""))
+	require.ErrorIs(t, err, errNegativeChunkLength)
+}
+
+// errStubUnused marks a stubLister method a test never exercises.
+var errStubUnused = errors.New("stubLister method not used in test")
+
+// stubLister is a storage.Storer whose ListStream yields a fixed set of
+// objects; every other method is unused by the tests here.
+type stubLister struct {
+	objects []storage.ObjectInfo
+}
+
+func (s *stubLister) ListStream(
+	_ context.Context, prefix string,
+) <-chan storage.ObjectInfo {
+	ch := make(chan storage.ObjectInfo, len(s.objects))
+	for _, o := range s.objects {
+		if strings.HasPrefix(o.Key, prefix) {
+			ch <- o
+		}
+	}
+
+	close(ch)
+
+	return ch
+}
+
+func (s *stubLister) Put(_ context.Context, _ string, _ io.Reader) error {
+	return errStubUnused
+}
+
+func (s *stubLister) PutWithProgress(
+	_ context.Context, _ string, _ io.Reader, _ int64, _ storage.ProgressCallback,
+) error {
+	return errStubUnused
+}
+
+func (s *stubLister) Get(_ context.Context, _ string) (io.ReadCloser, error) {
+	return nil, errStubUnused
+}
+
+func (s *stubLister) Stat(_ context.Context, _ string) (*storage.ObjectInfo, error) {
+	return nil, errStubUnused
+}
+
+func (s *stubLister) Delete(_ context.Context, _ string) error {
+	return errStubUnused
+}
+
+func (s *stubLister) List(_ context.Context, _ string) ([]string, error) {
+	return nil, errStubUnused
+}
+
+func (s *stubLister) Info() storage.Info {
+	return storage.Info{}
+}