From ae07981902e45032074e15394870903f8c56faac Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 22 Sep 2026 13:00:41 +0000 Subject: [PATCH] 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 with length > size-offset so a sum cannot overflow past the check. A new isBlobHash helper (a plain function, since the packer stores temp-placeholder-{uuid} as a hash) 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 shortHash helper that cannot panic. verify's chunk reader rejects a negative length and streams the chunk. Model: opus-4-8 --- TODO.md | 23 ++ internal/vaultik/blob_fetch.go | 15 +- internal/vaultik/blob_fetch_hash_test.go | 29 ++ internal/vaultik/blobcache.go | 90 ++++-- internal/vaultik/blobhash.go | 52 ++++ .../vaultik/destination_validation_test.go | 262 ++++++++++++++++++ internal/vaultik/prune.go | 17 +- internal/vaultik/restore.go | 14 +- internal/vaultik/snapshot.go | 66 +++-- internal/vaultik/verify.go | 34 ++- 10 files changed, 549 insertions(+), 53 deletions(-) create mode 100644 internal/vaultik/blobhash.go create mode 100644 internal/vaultik/destination_validation_test.go diff --git a/TODO.md b/TODO.md index 1a33ea4..005dbce 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,29 @@ release" is exactly the contradiction # Completed Steps +- 2026-09-22: Validated blob hashes, offsets and lengths read back from + the destination before using them + ([issue #155](https://git.eeqj.de/sneak/vaultik/issues/155)). A blob + hash taken from the downloaded database or the store listing was + trusted unchecked, so a hostile remote could drive a decrypted blob to + be written outside the cache directory (a hash like `aa/../../etc`) or + crash a command with a short or negative value. `blobDiskCache.path` + now refuses any key containing a path separator, and `ReadAt` rejects a + negative offset or length and bounds with `length > size-offset` so a + sum cannot overflow past the check. A new `isBlobHash` helper (a plain + function, not a method — the packer stores `temp-placeholder-{uuid}` as + a hash) gates `FetchBlob`, shallow and deep verify; the `blobs/` and + `metadata/` listings skip a non-conforming name with a warning; and + short-hash prefixes in log and error text go through a `shortHash` + helper that cannot panic. `verify`'s chunk reader also rejects a + negative `blob_chunks` length and streams the chunk instead of + allocating a database-supplied size. `restore.go` and + `internal/database` were left untouched to avoid colliding with the + in-flight [issue #156](https://git.eeqj.de/sneak/vaultik/issues/156) + work; the cache-path and `FetchBlob` guards already stop the unsafe + write and fetch, so restore's own `buildBlobIndexes` early check is + deferred as fail-fast defense in depth. + - 2026-09-21: Stopped an interrupted blob upload from making a later backup deduplicate against data that was never stored ([issue #148](https://git.eeqj.de/sneak/vaultik/issues/148)). The diff --git a/internal/vaultik/blob_fetch.go b/internal/vaultik/blob_fetch.go index 0ff34e8..2902656 100644 --- a/internal/vaultik/blob_fetch.go +++ b/internal/vaultik/blob_fetch.go @@ -59,7 +59,7 @@ func (h *hashVerifyReader) Close() error { actualHashHex := hex.EncodeToString(blobgen.DoubleSHA256(h.reader.Sum256())) if actualHashHex != h.blobHash { return fmt.Errorf("%w: expected %s, got %s", - errBlobHashMismatch, h.blobHash[:16], actualHashHex[:16]) + errBlobHashMismatch, shortHash(h.blobHash), shortHash(actualHashHex)) } if readerErr != nil { @@ -103,6 +103,13 @@ func (v *Vaultik) FetchAndDecryptBlob( func (v *Vaultik) FetchBlob( ctx context.Context, blobHash string, expectedSize int64, ) (io.ReadCloser, int64, error) { + // blobHash reaches here from the snapshot database, which is not + // trusted. Reject a malformed hash before it is spliced into a storage + // path (blobHash[:2]/blobHash[2:4]) or a fetch is attempted. + if !isBlobHash(blobHash) { + return nil, 0, fmt.Errorf("%w: %s", errInvalidBlobHash, shortHash(blobHash)) + } + blobPath := fmt.Sprintf("blobs/%s/%s/%s", blobHash[:2], blobHash[2:4], blobHash) t0 := time.Now() @@ -110,7 +117,7 @@ func (v *Vaultik) FetchBlob( getDur := time.Since(t0) if err != nil { - return nil, 0, fmt.Errorf("downloading blob %s: %w", blobHash[:16], err) + return nil, 0, fmt.Errorf("downloading blob %s: %w", shortHash(blobHash), err) } t0 = time.Now() @@ -120,11 +127,11 @@ func (v *Vaultik) FetchBlob( if err != nil { _ = rc.Close() - return nil, 0, fmt.Errorf("stat blob %s: %w", blobHash[:16], err) + return nil, 0, fmt.Errorf("stat blob %s: %w", shortHash(blobHash), err) } log.Debug("FetchBlob round-trips", - "hash", blobHash[:16], + "hash", shortHash(blobHash), "ms_storage_get", getDur.Milliseconds(), "ms_storage_stat", statDur.Milliseconds(), "expected_size", expectedSize, diff --git a/internal/vaultik/blob_fetch_hash_test.go b/internal/vaultik/blob_fetch_hash_test.go index 7124942..a8b96c4 100644 --- a/internal/vaultik/blob_fetch_hash_test.go +++ b/internal/vaultik/blob_fetch_hash_test.go @@ -55,6 +55,35 @@ func buildHashTestBlob( return encBuf.Bytes(), correctHash } +// TestFetchBlobRejectsMalformedHash verifies FetchBlob refuses a blob hash +// that is not 64 lowercase hex characters before it builds a storage path +// or issues any request. The hash reaches FetchBlob from the snapshot +// database, which is not trusted, so a value such as one containing "/.." +// must never reach the store. +func TestFetchBlobRejectsMalformedHash(t *testing.T) { + t.Parallel() + + mockStorage := NewMockStorer() + tv := vaultik.NewForTesting(mockStorage) + ctx := context.Background() + + for _, bad := range []string{ + "aa/../../../home/u/.profile", + "abc", + strings.Repeat("A", 64), // uppercase hex is not accepted + strings.Repeat("g", 64), // not hex + } { + _, _, err := tv.FetchBlob(ctx, bad, 0) + if err == nil { + t.Fatalf("expected error for malformed hash %q, got nil", bad) + } + } + + if calls := mockStorage.GetCalls(); len(calls) != 0 { + t.Fatalf("storage was accessed for a malformed hash: %v", calls) + } +} + // TestFetchAndDecryptBlobVerifiesHash verifies that FetchAndDecryptBlob checks // the double-SHA-256 hash of the decrypted plaintext against the expected blob hash. func TestFetchAndDecryptBlobVerifiesHash(t *testing.T) { diff --git a/internal/vaultik/blobcache.go b/internal/vaultik/blobcache.go index 2e811d7..313c3f9 100644 --- a/internal/vaultik/blobcache.go +++ b/internal/vaultik/blobcache.go @@ -6,13 +6,17 @@ import ( "io" "os" "path/filepath" + "strings" "sync" ) // Sentinel errors for blob cache lookups. var ( - errCacheKeyMissing = errors.New("key not in cache") - errCacheReadBeyondBlob = errors.New("read beyond blob size") + errCacheKeyMissing = errors.New("key not in cache") + errCacheReadBeyondBlob = errors.New("read beyond blob size") + errCacheKeyHasSeparator = errors.New( + "cache key contains a path separator") + errCacheNegativeRead = errors.New("negative offset or length") ) // blobCacheFileMode is the permission mode for cached blob files. @@ -74,6 +78,11 @@ func newBlobDiskCache(maxBytes int64) (*blobDiskCache, error) { // Put writes blob data to disk cache. Entries larger than maxBytes are // silently skipped. func (c *blobDiskCache) Put(key string, data []byte) error { + p, err := c.path(key) + if err != nil { + return err + } + entrySize := int64(len(data)) c.mu.Lock() @@ -87,11 +96,12 @@ func (c *blobDiskCache) Put(key string, data []byte) error { if e, ok := c.items[key]; ok { c.unlink(e) c.curBytes -= e.size - _ = os.Remove(c.path(key)) + _ = os.Remove(p) + delete(c.items, key) } - err := os.WriteFile(c.path(key), data, blobCacheFileMode) + err = os.WriteFile(p, data, blobCacheFileMode) if err != nil { return fmt.Errorf("writing blob to cache: %w", err) } @@ -119,19 +129,26 @@ func (c *blobDiskCache) Put(key string, data []byte) error { // disk without buffering its entire plaintext (which may be tens of GB) // in RAM. func (c *blobDiskCache) PutFromReader(key string, r io.Reader) (int64, error) { + p, err := c.path(key) + if err != nil { + return 0, err + } + c.mu.Lock() // Remove any prior entry first; we'll re-link after the file is // written successfully. if e, ok := c.items[key]; ok { c.unlink(e) c.curBytes -= e.size - _ = os.Remove(c.path(key)) + _ = os.Remove(p) + delete(c.items, key) } c.mu.Unlock() + //nolint:gosec // G304: path() rejects keys with a separator f, err := os.OpenFile( - c.path(key), os.O_CREATE|os.O_TRUNC|os.O_WRONLY, blobCacheFileMode) + p, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, blobCacheFileMode) if err != nil { return 0, fmt.Errorf("creating cache file: %w", err) } @@ -140,13 +157,13 @@ func (c *blobDiskCache) PutFromReader(key string, r io.Reader) (int64, error) { closeErr := f.Close() if copyErr != nil { - _ = os.Remove(c.path(key)) + _ = os.Remove(p) return written, fmt.Errorf("streaming to cache file: %w", copyErr) } if closeErr != nil { - _ = os.Remove(c.path(key)) + _ = os.Remove(p) return written, fmt.Errorf("closing cache file: %w", closeErr) } @@ -158,7 +175,7 @@ func (c *blobDiskCache) PutFromReader(key string, r io.Reader) (int64, error) { // floor — but the restore path passes math.MaxInt64 as maxBytes // so this branch is effectively unreachable there. if written > c.maxBytes { - _ = os.Remove(c.path(key)) + _ = os.Remove(p) return written, nil } @@ -181,6 +198,11 @@ func (c *blobDiskCache) PutFromReader(key string, r io.Reader) (int64, error) { // Get reads a cached blob from disk. Returns data and true on hit. func (c *blobDiskCache) Get(key string) ([]byte, bool) { + p, err := c.path(key) + if err != nil { + return nil, false + } + c.mu.Lock() c.getCalls++ @@ -195,7 +217,8 @@ func (c *blobDiskCache) Get(key string) ([]byte, bool) { c.pushFront(e) c.mu.Unlock() - data, err := os.ReadFile(c.path(key)) + //nolint:gosec // G304: path() rejects keys with a separator + data, err := os.ReadFile(p) if err != nil { c.mu.Lock() if e2, ok2 := c.items[key]; ok2 && e2 == e { @@ -213,6 +236,20 @@ func (c *blobDiskCache) Get(key string) ([]byte, bool) { // ReadAt reads a slice of a cached blob without loading the entire blob into memory. func (c *blobDiskCache) ReadAt(key string, offset, length int64) ([]byte, error) { + p, err := c.path(key) + if err != nil { + return nil, err + } + + // offset and length come from a blob_chunks row read back from the + // destination. A negative value must be rejected outright; the upper + // bound is checked as length > size-offset (a subtraction) so a huge + // offset+length cannot overflow int64 and slip past the check. + if offset < 0 || length < 0 { + return nil, fmt.Errorf("%w: offset=%d length=%d", + errCacheNegativeRead, offset, length) + } + c.mu.Lock() c.readAtCalls++ @@ -223,7 +260,7 @@ func (c *blobDiskCache) ReadAt(key string, offset, length int64) ([]byte, error) return nil, fmt.Errorf("%w: %q", errCacheKeyMissing, key) } - if offset+length > e.size { + if length > e.size-offset { c.mu.Unlock() return nil, fmt.Errorf("%w: offset=%d length=%d size=%d", @@ -234,7 +271,7 @@ func (c *blobDiskCache) ReadAt(key string, offset, length int64) ([]byte, error) c.pushFront(e) c.mu.Unlock() - f, err := os.Open(c.path(key)) + f, err := os.Open(p) //nolint:gosec // G304: path() rejects keys with a separator if err != nil { return nil, err } @@ -276,7 +313,13 @@ func (c *blobDiskCache) Delete(key string) { c.unlink(e) delete(c.items, key) c.curBytes -= e.size - _ = os.Remove(c.path(key)) + + // The key is already in the map, so it passed path() when it was + // inserted; the error cannot occur here. + p, err := c.path(key) + if err == nil { + _ = os.Remove(p) + } } // Keys returns a snapshot of all cached keys. Safe for iteration without @@ -347,8 +390,18 @@ func (c *blobDiskCache) Close() error { return os.RemoveAll(c.dir) } -func (c *blobDiskCache) path(key string) string { - return filepath.Join(c.dir, key) +// path returns the on-disk location of the cache file for key. The key is +// a blob hash read back from the destination and is not trusted: a value +// such as "aa/../../../home/u/.profile" would otherwise make filepath.Join +// escape the cache directory, so a key containing a path separator is +// refused rather than joined. +func (c *blobDiskCache) path(key string) (string, error) { + if strings.ContainsRune(key, '/') || + strings.ContainsRune(key, filepath.Separator) { + return "", fmt.Errorf("%w: %q", errCacheKeyHasSeparator, key) + } + + return filepath.Join(c.dir, key), nil } func (c *blobDiskCache) unlink(e *blobDiskCacheEntry) { @@ -391,5 +444,10 @@ func (c *blobDiskCache) evictLRU() { c.unlink(victim) delete(c.items, victim.key) c.curBytes -= victim.size - _ = os.Remove(c.path(victim.key)) + + // victim.key was validated by path() on insertion, so this cannot err. + p, err := c.path(victim.key) + if err == nil { + _ = os.Remove(p) + } } diff --git a/internal/vaultik/blobhash.go b/internal/vaultik/blobhash.go new file mode 100644 index 0000000..fe90d46 --- /dev/null +++ b/internal/vaultik/blobhash.go @@ -0,0 +1,52 @@ +package vaultik + +import "errors" + +// blobHashHexLen is the length of a blob hash written as lowercase hex: a +// SHA-256 digest is 32 bytes, so 64 characters. Remote snapshot keys are +// SHA-256 hashes too and share this exact form. +const blobHashHexLen = 64 + +// shortHashLen is how many leading characters of a hash appear in log and +// error text. +const shortHashLen = 16 + +// errInvalidBlobHash reports a value used as a blob hash that is not +// exactly 64 lowercase hex characters. Restore, verify and prune read +// these values back from the destination, which is not trusted, so each +// one is checked before it is used to build a path or drive a read. +var errInvalidBlobHash = errors.New( + "blob hash is not 64 lowercase hex characters") + +// isBlobHash reports whether s is exactly 64 lowercase hex characters. +// Every real blob hash and remote snapshot key has this form. +// +// The check is a plain function, not a method on types.BlobHash: the +// packer stores "temp-placeholder-{uuid}" as the hash of an unfinished +// blob in the local index, so the type itself must keep accepting values +// that are not hashes. +func isBlobHash(s string) bool { + if len(s) != blobHashHexLen { + return false + } + + for _, r := range s { + if (r < '0' || r > '9') && (r < 'a' || r > 'f') { + return false + } + } + + return true +} + +// shortHash returns the leading part of a hash for log and error text. It +// never panics: a string shorter than the prefix is returned whole. A hash +// read from the destination may be malformed, and formatting one for a +// message must not crash the command. +func shortHash(s string) string { + if len(s) <= shortHashLen { + return s + } + + return s[:shortHashLen] +} diff --git a/internal/vaultik/destination_validation_test.go b/internal/vaultik/destination_validation_test.go new file mode 100644 index 0000000..d154fc9 --- /dev/null +++ b/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{} +} diff --git a/internal/vaultik/prune.go b/internal/vaultik/prune.go index f197bc2..38bb135 100644 --- a/internal/vaultik/prune.go +++ b/internal/vaultik/prune.go @@ -247,9 +247,22 @@ func (v *Vaultik) listAllRemoteBlobs() (map[string]int64, error) { } parts := strings.Split(object.Key, "/") - if len(parts) == blobKeyParts && parts[0] == "blobs" { - allBlobs[parts[3]] = object.Size + if len(parts) != blobKeyParts || parts[0] != "blobs" { + continue } + + // The object name is read from the destination store and is not + // trusted. A name that is not a blob hash (e.g. a short or + // non-hex string) would panic the later hash[:2]/hash[2:4] path + // build, so skip it with a warning rather than delete it. + if !isBlobHash(parts[3]) { + log.Warn("Skipping non-conforming object under blobs/", + "key", object.Key) + + continue + } + + allBlobs[parts[3]] = object.Size } log.Info("Found blobs in storage", "count", len(allBlobs)) diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index ea603d4..4998240 100644 --- a/internal/vaultik/restore.go +++ b/internal/vaultik/restore.go @@ -425,12 +425,12 @@ func (s *restoreSession) downloadNextBlobSet(plan *restorePlan) (bool, error) { blob, ok := s.blobByHash[hash] if !ok { - return false, fmt.Errorf("%w: %s", errBlobMissingFromIndex, hash[:16]) + return false, fmt.Errorf("%w: %s", errBlobMissingFromIndex, shortHash(hash)) } err := s.downloadBlobToCache(hash, blob.CompressedSize) if err != nil { - return false, fmt.Errorf("downloading blob %s: %w", hash[:16], err) + return false, fmt.Errorf("downloading blob %s: %w", shortHash(hash), err) } s.result.BlobsDownloaded++ @@ -474,6 +474,16 @@ func (v *Vaultik) buildBlobIndexes( blobByHash := make(map[string]*database.Blob, len(blobsByID)) for id, blob := range blobsByID { hash := blob.Hash.String() + + // The snapshot database is untrusted. A hash that is not 64 + // lowercase hex characters could steer a later fetch to a path + // outside the cache directory, so reject it here, before any + // blob is downloaded. + if !isBlobHash(hash) { + return nil, nil, fmt.Errorf( + "%w: %s", errInvalidBlobHash, shortHash(hash)) + } + blobIDToHash[id] = hash blobByHash[hash] = blob } diff --git a/internal/vaultik/snapshot.go b/internal/vaultik/snapshot.go index bd2586c..7ab3d63 100644 --- a/internal/vaultik/snapshot.go +++ b/internal/vaultik/snapshot.go @@ -730,8 +730,18 @@ func (v *Vaultik) VerifySnapshotWithOptions( result.DatabaseMissing = true } - result.Verified, result.Missing, result.Mismatched, result.MissingSize = + result.Verified, result.Missing, result.Mismatched, result.MissingSize, err = v.verifyManifestBlobs(manifest, opts) + if err != nil { + if opts.JSON { + result.Status = verifyStatusFailed + result.ErrorMessage = fmt.Sprintf("verifying manifest blobs: %v", err) + + return v.outputVerifyJSON(result) + } + + return fmt.Errorf("verifying manifest blobs: %w", err) + } return v.formatVerifyResult(result, opts) } @@ -766,13 +776,20 @@ func (v *Vaultik) printVerifyHeader(snapshotID string, opts *VerifyOptions) { // comparison matches the deep path (see verifyBlobExistenceFromDB). func (v *Vaultik) verifyManifestBlobs( manifest *snapshot.Manifest, opts *VerifyOptions, -) (int, int, int, int64) { +) (int, int, int, int64, error) { var ( verified, missing, mismatched int missingSize int64 ) for _, blob := range manifest.Blobs { + // The manifest is unauthenticated, so its blob hashes are checked + // before being spliced into a storage path. + if !isBlobHash(blob.Hash) { + return 0, 0, 0, 0, fmt.Errorf("%w: %s", + errInvalidBlobHash, shortHash(blob.Hash)) + } + blobPath := fmt.Sprintf("blobs/%s/%s/%s", blob.Hash[:2], blob.Hash[2:4], blob.Hash) @@ -798,7 +815,7 @@ func (v *Vaultik) verifyManifestBlobs( } } - return verified, missing, mismatched, missingSize + return verified, missing, mismatched, missingSize, nil } // formatVerifyResult outputs the final verification results as JSON or @@ -1313,21 +1330,36 @@ func (v *Vaultik) listAllRemoteSnapshotKeys() ([]string, error) { } parts := strings.Split(object.Key, "/") - if len(parts) >= minSnapshotIDParts && - parts[0] == metadataDirName && parts[1] != "" { - // Skip macOS resource fork files (._*) and other hidden files - if strings.HasPrefix(parts[1], ".") { - continue - } + if len(parts) < minSnapshotIDParts || + parts[0] != metadataDirName || parts[1] == "" { + continue + } - if strings.HasSuffix(object.Key, "/") || - strings.Contains(object.Key, "/manifest.json.zst") { - key := parts[1] - if !seen[key] { - seen[key] = true - keys = append(keys, key) - } - } + // Skip macOS resource fork files (._*) and other hidden files + if strings.HasPrefix(parts[1], ".") { + continue + } + + if !strings.HasSuffix(object.Key, "/") && + !strings.Contains(object.Key, "/manifest.json.zst") { + continue + } + + key := parts[1] + + // A remote snapshot key is a SHA-256 hash: 64 lowercase hex + // characters. The listing comes from the untrusted destination, + // so accept a key only in that form. + if !isBlobHash(key) { + log.Warn("Skipping non-conforming key under metadata/", + "key", object.Key) + + continue + } + + if !seen[key] { + seen[key] = true + keys = append(keys, key) } } diff --git a/internal/vaultik/verify.go b/internal/vaultik/verify.go index 6f95fd1..410f5f4 100644 --- a/internal/vaultik/verify.go +++ b/internal/vaultik/verify.go @@ -24,9 +24,10 @@ var ( errVerificationFailed = errors.New("verification failed") errSecretKeyRequired = errors.New( "VAULTIK_AGE_SECRET_KEY not set; required for deep verification") - errChunksOutOfOrder = errors.New("chunks out of order") - errChunkHashMismatch = errors.New("chunk hash mismatch") - errTrailingBlobData = errors.New( + errChunksOutOfOrder = errors.New("chunks out of order") + errChunkHashMismatch = errors.New("chunk hash mismatch") + errNegativeChunkLength = errors.New("chunk length is negative") + errTrailingBlobData = errors.New( "blob has unexpected trailing bytes not covered by chunk list") errManifestExtraBlob = errors.New("manifest contains blob not in database") errManifestMissingBlob = errors.New( @@ -421,7 +422,7 @@ func (v *Vaultik) verifyBlob( } log.Info("Blob verified", - "hash", blobInfo.Hash[:16]+"...", + "hash", shortHash(blobInfo.Hash)+"...", "chunks", chunkCount, "size", ubytes(blobInfo.CompressedSize), ) @@ -487,21 +488,24 @@ func (v *Vaultik) verifyBlobChunks( totalRead = offset } - // Read chunk data - chunkData := make([]byte, length) + // length comes from an untrusted blob_chunks row: reject a + // negative value, and hash by streaming exactly length bytes + // rather than allocating a database-supplied size up front. + if length < 0 { + return 0, fmt.Errorf("%w: offset %d length %d", + errNegativeChunkLength, offset, length) + } - _, err = io.ReadFull(decompressor, chunkData) + hasher := sha256.New() + + n, err := io.CopyN(hasher, decompressor, length) if err != nil { return 0, fmt.Errorf("failed to read chunk at offset %d: %w", offset, err) } - totalRead += length + totalRead += n - // Verify chunk hash - hasher := sha256.New() - hasher.Write(chunkData) calculatedHash := hex.EncodeToString(hasher.Sum(nil)) - if calculatedHash != chunkHash { return 0, fmt.Errorf("%w at offset %d: calculated %s, expected %s", errChunkHashMismatch, offset, calculatedHash, chunkHash) @@ -651,6 +655,12 @@ func (v *Vaultik) verifyBlobExistenceFromDB(blobs []snapshot.BlobInfo) error { log.Info("Verifying blob existence in S3", "blob_count", len(blobs)) for i, blob := range blobs { + // The hash is read from the snapshot database, which is not + // trusted; check it before it is spliced into a storage path. + if !isBlobHash(blob.Hash) { + return fmt.Errorf("%w: %s", errInvalidBlobHash, shortHash(blob.Hash)) + } + // Construct blob path blobPath := fmt.Sprintf("blobs/%s/%s/%s", blob.Hash[:2], blob.Hash[2:4], blob.Hash) -- 2.54.0