diff --git a/internal/blobgen/reader.go b/internal/blobgen/reader.go index 7c6ebeb..27bad18 100644 --- a/internal/blobgen/reader.go +++ b/internal/blobgen/reader.go @@ -2,6 +2,7 @@ package blobgen import ( "crypto/sha256" + "errors" "fmt" "hash" "io" @@ -56,6 +57,22 @@ func (r *Reader) Read(p []byte) (int, error) { n, err := r.teeReader.Read(p) r.bytesRead += int64(n) + // When the ciphertext is cut right after the age header plus its + // 16-byte nonce, the age reader's first read fails with + // io.ErrUnexpectedEOF, and the zstd decoder maps that to a clean + // io.EOF at frame start. That makes a truncated stream look like a + // valid empty one. Distinguish the two: on EOF, read once more from + // the age reader. A genuine end leaves it at (0, io.EOF); a truncated + // stream leaves its stored io.ErrUnexpectedEOF, which we surface. + if errors.Is(err, io.EOF) { + var probe [1]byte + + m, ageErr := r.decryptor.Read(probe[:]) + if m != 0 || !errors.Is(ageErr, io.EOF) { + return n, io.ErrUnexpectedEOF + } + } + return n, err } diff --git a/internal/blobgen/truncation_test.go b/internal/blobgen/truncation_test.go new file mode 100644 index 0000000..d21dcfd --- /dev/null +++ b/internal/blobgen/truncation_test.go @@ -0,0 +1,70 @@ +package blobgen_test + +import ( + "bytes" + "io" + "testing" + + "filippo.io/age" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/blobgen" +) + +// TestReaderRejectsHeaderNonceTruncation guards against a stream cut right +// after the age header plus its 16-byte nonce. age.Decrypt still succeeds on +// such an object, and the zstd decoder maps the age reader's +// io.ErrUnexpectedEOF to a clean io.EOF at frame start, so without the extra +// check the truncated stream would read as a valid empty one. Reading it must +// now fail. +func TestReaderRejectsHeaderNonceTruncation(t *testing.T) { + t.Parallel() + + identity, err := age.GenerateX25519Identity() + require.NoError(t, err) + + // Encrypting empty plaintext yields header + nonce(16) + a single + // 16-byte final chunk tag. Dropping the trailing tag leaves exactly the + // age header plus its nonce — the truncation point that triggers the bug. + var full bytes.Buffer + + w, err := age.Encrypt(&full, identity.Recipient()) + require.NoError(t, err) + require.NoError(t, w.Close()) + + truncated := full.Bytes()[:full.Len()-16] + + reader, err := blobgen.NewReader(bytes.NewReader(truncated), identity) + require.NoError(t, err) + + defer func() { _ = reader.Close() }() + + _, err = io.ReadAll(reader) + require.Error(t, err) + require.ErrorIs(t, err, io.ErrUnexpectedEOF) +} + +// TestReaderReadsGenuinelyEmptyBlob confirms the truncation check does not +// reject a legitimately empty payload: a blob written with no data must round +// trip back to zero bytes with no error. +func TestReaderReadsGenuinelyEmptyBlob(t *testing.T) { + t.Parallel() + + identity, err := age.GenerateX25519Identity() + require.NoError(t, err) + + var encrypted bytes.Buffer + + writer, err := blobgen.NewWriter( + &encrypted, 3, []string{identity.Recipient().String()}) + require.NoError(t, err) + require.NoError(t, writer.Close()) + + reader, err := blobgen.NewReader(bytes.NewReader(encrypted.Bytes()), identity) + require.NoError(t, err) + + defer func() { _ = reader.Close() }() + + data, err := io.ReadAll(reader) + require.NoError(t, err) + require.Empty(t, data) +} diff --git a/internal/vaultik/restore.go b/internal/vaultik/restore.go index ca5b29b..118a68b 100644 --- a/internal/vaultik/restore.go +++ b/internal/vaultik/restore.go @@ -48,6 +48,10 @@ var ( "restore loop ended with files still pending") errSnapshotDBMismatch = errors.New( "decrypted database is not the requested snapshot") + // errEmptySnapshotDB is returned when the decrypted metadata database has + // zero length, which happens when the object was truncated or replaced + // with an empty payload. Rejected before any schema is built on it. + errEmptySnapshotDB = errors.New("decrypted snapshot database is empty") ) // snapshotDBFilename is the name the decrypted snapshot database is @@ -758,6 +762,11 @@ func (v *Vaultik) materializeSnapshotDB( log.Debug("Created restore database", "path", dbPath, "size", ubytes(written)) + // Reject an empty database before OpenReadOnly builds a schema on it. + if written == 0 { + return nil, "", errEmptySnapshotDB + } + db, err := database.OpenReadOnly(v.ctx, dbPath) if err != nil { return nil, "", fmt.Errorf("opening restore database: %w", err) diff --git a/internal/vaultik/restore_truncated_db_test.go b/internal/vaultik/restore_truncated_db_test.go new file mode 100644 index 0000000..3991f3d --- /dev/null +++ b/internal/vaultik/restore_truncated_db_test.go @@ -0,0 +1,81 @@ +package vaultik_test + +import ( + "bytes" + "context" + "io" + "path/filepath" + "testing" + + "filippo.io/age" + "github.com/spf13/afero" + "github.com/stretchr/testify/require" + "sneak.berlin/go/vaultik/internal/log" + "sneak.berlin/go/vaultik/internal/snapshot" + "sneak.berlin/go/vaultik/internal/ui" + "sneak.berlin/go/vaultik/internal/vaultik" +) + +// TestRestoreRejectsTruncatedMetadataDB backs up a real tree, then replaces +// the snapshot's db.zst.age with a stream cut right after the age header and +// its 16-byte nonce. age.Decrypt still accepts such an object and the zstd +// decoder turns the truncated read into a clean EOF, so before the fix restore +// built a fresh empty schema and reported success. Restore must now fail. +func TestRestoreRejectsTruncatedMetadataDB(t *testing.T) { + log.Initialize(log.Config{}) + t.Parallel() + + fs := afero.NewOsFs() + tempDir := t.TempDir() + + dataDir := filepath.Join(tempDir, "source") + storeDir := filepath.Join(tempDir, "remote") + restoreDir := filepath.Join(tempDir, "restored") + dbPath := filepath.Join(tempDir, "index.sqlite") + + chunkSize := int64(64 * 1024) + maxBlobSize := int64(512 * 1024) + + setupE2ESourceTree(t, fs, dataDir, chunkSize) + + ctx := context.Background() + + cfg, storer, snapshotID := runFileStorageBackup( + ctx, t, fs, dataDir, storeDir, dbPath, chunkSize, maxBlobSize) + + // Encrypting empty plaintext to the snapshot recipient yields + // header + nonce(16) + a single 16-byte final chunk tag. Dropping the + // trailing tag leaves exactly the age header plus its nonce — the + // truncation an attacker can write over metadata without any key. + recipient, err := age.ParseX25519Recipient(testAgePublicKey) + require.NoError(t, err) + + var full bytes.Buffer + + w, err := age.Encrypt(&full, recipient) + require.NoError(t, err) + require.NoError(t, w.Close()) + + truncated := full.Bytes()[:full.Len()-16] + + dbKeyPath := filepath.Join(storeDir, "metadata", + snapshot.RemoteSnapshotKey(snapshotID), "db.zst.age") + require.NoError(t, afero.WriteFile(fs, dbKeyPath, truncated, 0o644)) + + restoreVaultik := &vaultik.Vaultik{ + Config: cfg, + Storage: storer, + Fs: fs, + Stdout: io.Discard, + Stderr: io.Discard, + UI: ui.NewWithColor(io.Discard, false), + } + restoreVaultik.SetContext(ctx) + + err = restoreVaultik.Restore(&vaultik.RestoreOptions{ + SnapshotID: snapshotID, + TargetDir: restoreDir, + Verify: true, + }) + require.Error(t, err) +}