From bb92b487f72171c28f4a9fbd8662e19895a78f12 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 3 Feb 2024 17:35:46 +0100 Subject: [PATCH 01/13] repository: fix repack test --- internal/repository/repair_pack_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/repository/repair_pack_test.go b/internal/repository/repair_pack_test.go index 6b20dbffb..3f9477945 100644 --- a/internal/repository/repair_pack_test.go +++ b/internal/repository/repair_pack_test.go @@ -38,17 +38,17 @@ func TestRepairBrokenPack(t *testing.T) { func testRepairBrokenPack(t *testing.T, version uint) { tests := []struct { name string - damage func(repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) + damage func(t *testing.T, repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) }{ { "valid pack", - func(repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) { + func(t *testing.T, repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) { return packsBefore, restic.NewBlobSet() }, }, { "broken pack", - func(repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) { + func(t *testing.T, repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) { wrongBlob := createRandomWrongBlob(t, repo) damagedPacks := findPacksForBlobs(t, repo, restic.NewBlobSet(wrongBlob)) return damagedPacks, restic.NewBlobSet(wrongBlob) @@ -56,7 +56,7 @@ func testRepairBrokenPack(t *testing.T, version uint) { }, { "partially broken pack", - func(repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) { + func(t *testing.T, repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) { // damage one of the pack files damagedID := packsBefore.List()[0] replaceFile(t, repo, backend.Handle{Type: backend.PackFile, Name: damagedID.String()}, @@ -79,7 +79,7 @@ func testRepairBrokenPack(t *testing.T, version uint) { }, }, { "truncated pack", - func(repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) { + func(t *testing.T, repo restic.Repository, packsBefore restic.IDSet) (restic.IDSet, restic.BlobSet) { // damage one of the pack files damagedID := packsBefore.List()[0] replaceFile(t, repo, backend.Handle{Type: backend.PackFile, Name: damagedID.String()}, @@ -112,7 +112,7 @@ func testRepairBrokenPack(t *testing.T, version uint) { packsBefore := listPacks(t, repo) blobsBefore := listBlobs(repo) - toRepair, damagedBlobs := test.damage(repo, packsBefore) + toRepair, damagedBlobs := test.damage(t, repo, packsBefore) rtest.OK(t, repository.RepairPacks(context.TODO(), repo, toRepair, &progress.NoopPrinter{})) // reload index From 16e3f79e8b3cf0d9672e6cca2f0760e45d3d0624 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 3 Feb 2024 17:47:36 +0100 Subject: [PATCH 02/13] repository: make repo.Options configurable for test repos --- internal/archiver/archiver_test.go | 2 +- internal/migrations/upgrade_repo_v2_test.go | 2 +- internal/repository/fuzz_test.go | 3 +-- internal/repository/testing.go | 9 +++++---- internal/restic/lock_test.go | 2 +- 5 files changed, 9 insertions(+), 9 deletions(-) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index c6daed5bb..46ef44251 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -1880,7 +1880,7 @@ func TestArchiverContextCanceled(t *testing.T) { }) // Ensure that the archiver itself reports the canceled context and not just the backend - repo := repository.TestRepositoryWithBackend(t, &noCancelBackend{mem.New()}, 0) + repo := repository.TestRepositoryWithBackend(t, &noCancelBackend{mem.New()}, 0, repository.Options{}) back := restictest.Chdir(t, tempdir) defer back() diff --git a/internal/migrations/upgrade_repo_v2_test.go b/internal/migrations/upgrade_repo_v2_test.go index 40153d3ca..845d20e92 100644 --- a/internal/migrations/upgrade_repo_v2_test.go +++ b/internal/migrations/upgrade_repo_v2_test.go @@ -69,7 +69,7 @@ func TestUpgradeRepoV2Failure(t *testing.T) { Backend: be, } - repo := repository.TestRepositoryWithBackend(t, be, 1) + repo := repository.TestRepositoryWithBackend(t, be, 1, repository.Options{}) if repo.Config().Version != 1 { t.Fatal("test repo has wrong version") } diff --git a/internal/repository/fuzz_test.go b/internal/repository/fuzz_test.go index b4036288c..80372f8e0 100644 --- a/internal/repository/fuzz_test.go +++ b/internal/repository/fuzz_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/restic/restic/internal/backend/mem" "github.com/restic/restic/internal/restic" "golang.org/x/sync/errgroup" ) @@ -19,7 +18,7 @@ func FuzzSaveLoadBlob(f *testing.F) { } id := restic.Hash(blob) - repo := TestRepositoryWithBackend(t, mem.New(), 2) + repo := TestRepositoryWithVersion(t, 2) var wg errgroup.Group repo.StartPackUploader(context.TODO(), &wg) diff --git a/internal/repository/testing.go b/internal/repository/testing.go index d79137425..dbbdbeb07 100644 --- a/internal/repository/testing.go +++ b/internal/repository/testing.go @@ -44,7 +44,7 @@ const TestChunkerPol = chunker.Pol(0x3DA3358B4DC173) // TestRepositoryWithBackend returns a repository initialized with a test // password. If be is nil, an in-memory backend is used. A constant polynomial // is used for the chunker and low-security test parameters. -func TestRepositoryWithBackend(t testing.TB, be backend.Backend, version uint) restic.Repository { +func TestRepositoryWithBackend(t testing.TB, be backend.Backend, version uint, opts Options) restic.Repository { t.Helper() TestUseLowSecurityKDFParameters(t) restic.TestDisableCheckPolynomial(t) @@ -53,7 +53,7 @@ func TestRepositoryWithBackend(t testing.TB, be backend.Backend, version uint) r be = TestBackend(t) } - repo, err := New(be, Options{}) + repo, err := New(be, opts) if err != nil { t.Fatalf("TestRepository(): new repo failed: %v", err) } @@ -79,6 +79,7 @@ func TestRepository(t testing.TB) restic.Repository { func TestRepositoryWithVersion(t testing.TB, version uint) restic.Repository { t.Helper() dir := os.Getenv("RESTIC_TEST_REPO") + opts := Options{} if dir != "" { _, err := os.Stat(dir) if err != nil { @@ -86,7 +87,7 @@ func TestRepositoryWithVersion(t testing.TB, version uint) restic.Repository { if err != nil { t.Fatalf("error creating local backend at %v: %v", dir, err) } - return TestRepositoryWithBackend(t, be, version) + return TestRepositoryWithBackend(t, be, version, opts) } if err == nil { @@ -94,7 +95,7 @@ func TestRepositoryWithVersion(t testing.TB, version uint) restic.Repository { } } - return TestRepositoryWithBackend(t, nil, version) + return TestRepositoryWithBackend(t, nil, version, opts) } // TestOpenLocal opens a local repository. diff --git a/internal/restic/lock_test.go b/internal/restic/lock_test.go index 13b66a432..0d282aaf7 100644 --- a/internal/restic/lock_test.go +++ b/internal/restic/lock_test.go @@ -66,7 +66,7 @@ func (be *failLockLoadingBackend) Load(ctx context.Context, h backend.Handle, le func TestMultipleLockFailure(t *testing.T) { be := &failLockLoadingBackend{Backend: mem.New()} - repo := repository.TestRepositoryWithBackend(t, be, 0) + repo := repository.TestRepositoryWithBackend(t, be, 0, repository.Options{}) restic.TestSetLockTimeout(t, 5*time.Millisecond) lock1, err := restic.NewLock(context.TODO(), repo) From c01a0c6da71383a4aae74174e82c33a0e45d8ee6 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Fri, 2 Feb 2024 21:15:39 +0100 Subject: [PATCH 03/13] backup: verify blobs before upload This only covers the blobs themselves, the pack header is not verified so far. Unpacked files are also not covered by the integrity check. --- internal/repository/repository.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 9db83a4df..fadb120ce 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -423,6 +423,11 @@ func (r *Repository) saveAndEncrypt(ctx context.Context, t restic.BlobType, data // encrypt blob ciphertext = r.key.Seal(ciphertext, nonce, data, nil) + if err := r.verifyCiphertext(ciphertext, uncompressedLength, id); err != nil { + // FIXME call to action + return 0, fmt.Errorf("detected data corruption while saving blob %v: %w", id, err) + } + // find suitable packer and add blob var pm *packerManager @@ -438,6 +443,27 @@ func (r *Repository) saveAndEncrypt(ctx context.Context, t restic.BlobType, data return pm.SaveBlob(ctx, t, id, ciphertext, uncompressedLength) } +func (r *Repository) verifyCiphertext(buf []byte, uncompressedLength int, id restic.ID) error { + nonce, ciphertext := buf[:r.key.NonceSize()], buf[r.key.NonceSize():] + plaintext, err := r.key.Open(nil, nonce, ciphertext, nil) + if err != nil { + return fmt.Errorf("decryption failed: %w", err) + } + if uncompressedLength != 0 { + // DecodeAll will allocate a slice if it is not large enough since it + // knows the decompressed size (because we're using EncodeAll) + plaintext, err = r.getZstdDecoder().DecodeAll(plaintext, nil) + if err != nil { + return fmt.Errorf("decompression failed: %w", err) + } + } + if !restic.Hash(plaintext).Equal(id) { + return errors.New("hash mismatch") + } + + return nil +} + func (r *Repository) compressUnpacked(p []byte) ([]byte, error) { // compression is only available starting from version 2 if r.cfg.Version < 2 { From 30a84e90035c53079d422ecf4ee0346e6a6c2886 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 3 Feb 2024 17:30:58 +0100 Subject: [PATCH 04/13] backup: verify unpacked files before upload --- internal/repository/repository.go | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/internal/repository/repository.go b/internal/repository/repository.go index fadb120ce..2584f42c7 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -500,7 +500,8 @@ func (r *Repository) decompressUnpacked(p []byte) ([]byte, error) { // SaveUnpacked encrypts data and stores it in the backend. Returned is the // storage hash. -func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, p []byte) (id restic.ID, err error) { +func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, buf []byte) (id restic.ID, err error) { + p := buf if t != restic.ConfigFile { p, err = r.compressUnpacked(p) if err != nil { @@ -515,6 +516,11 @@ func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, p []by ciphertext = r.key.Seal(ciphertext, nonce, p, nil) + if err := r.verifyUnpacked(ciphertext, t, buf); err != nil { + // FIXME call to action + return restic.ID{}, fmt.Errorf("detected data corruption while saving file of type %v: %w", t, err) + } + if t == restic.ConfigFile { id = restic.ID{} } else { @@ -532,6 +538,25 @@ func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, p []by return id, nil } +func (r *Repository) verifyUnpacked(buf []byte, t restic.FileType, expected []byte) error { + nonce, ciphertext := buf[:r.key.NonceSize()], buf[r.key.NonceSize():] + plaintext, err := r.key.Open(nil, nonce, ciphertext, nil) + if err != nil { + return fmt.Errorf("decryption failed: %w", err) + } + if t != restic.ConfigFile { + plaintext, err = r.decompressUnpacked(plaintext) + if err != nil { + return fmt.Errorf("decompression failed: %w", err) + } + } + + if !bytes.Equal(plaintext, expected) { + return errors.New("data mismatch") + } + return nil +} + // Flush saves all remaining packs and the index func (r *Repository) Flush(ctx context.Context) error { if err := r.flushPacks(ctx); err != nil { From 2dbb18128c4923f271691ad2530cb24b60d520b5 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 3 Feb 2024 17:47:48 +0100 Subject: [PATCH 05/13] repository: Allow skipping verification for tests Some tests have to explicitly create pack files with blobs that don't match their ID. For those blobs the builtin verification of the repository must be disabled. --- internal/repository/repack_test.go | 6 ++++-- internal/repository/repair_pack_test.go | 3 ++- internal/repository/repository.go | 13 +++++++++++-- 3 files changed, 17 insertions(+), 5 deletions(-) diff --git a/internal/repository/repack_test.go b/internal/repository/repack_test.go index 63845d5b1..c07c0a943 100644 --- a/internal/repository/repack_test.go +++ b/internal/repository/repack_test.go @@ -336,7 +336,8 @@ func TestRepackWrongBlob(t *testing.T) { } func testRepackWrongBlob(t *testing.T, version uint) { - repo := repository.TestRepositoryWithVersion(t, version) + // disable verification to allow adding corrupted blobs to the repository + repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoVerifyPack: true}) seed := time.Now().UnixNano() rand.Seed(seed) @@ -361,7 +362,8 @@ func TestRepackBlobFallback(t *testing.T) { } func testRepackBlobFallback(t *testing.T, version uint) { - repo := repository.TestRepositoryWithVersion(t, version) + // disable verification to allow adding corrupted blobs to the repository + repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoVerifyPack: true}) seed := time.Now().UnixNano() rand.Seed(seed) diff --git a/internal/repository/repair_pack_test.go b/internal/repository/repair_pack_test.go index 3f9477945..c9b0badfc 100644 --- a/internal/repository/repair_pack_test.go +++ b/internal/repository/repair_pack_test.go @@ -102,7 +102,8 @@ func testRepairBrokenPack(t *testing.T, version uint) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { - repo := repository.TestRepositoryWithVersion(t, version) + // disable verification to allow adding corrupted blobs to the repository + repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoVerifyPack: true}) seed := time.Now().UnixNano() rand.Seed(seed) diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 2584f42c7..706e84876 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -59,8 +59,9 @@ type Repository struct { } type Options struct { - Compression CompressionMode - PackSize uint + Compression CompressionMode + PackSize uint + NoVerifyPack bool } // CompressionMode configures if data should be compressed. @@ -444,6 +445,10 @@ func (r *Repository) saveAndEncrypt(ctx context.Context, t restic.BlobType, data } func (r *Repository) verifyCiphertext(buf []byte, uncompressedLength int, id restic.ID) error { + if r.opts.NoVerifyPack { + return nil + } + nonce, ciphertext := buf[:r.key.NonceSize()], buf[r.key.NonceSize():] plaintext, err := r.key.Open(nil, nonce, ciphertext, nil) if err != nil { @@ -539,6 +544,10 @@ func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, buf [] } func (r *Repository) verifyUnpacked(buf []byte, t restic.FileType, expected []byte) error { + if r.opts.NoVerifyPack { + return nil + } + nonce, ciphertext := buf[:r.key.NonceSize()], buf[r.key.NonceSize():] plaintext, err := r.key.Open(nil, nonce, ciphertext, nil) if err != nil { From 193140525c1358096216c6198866c5306015cd3e Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 3 Feb 2024 18:13:34 +0100 Subject: [PATCH 06/13] repository: test verification of blobs/unpacked data --- .../repository/repository_internal_test.go | 98 +++++++++++++++++++ 1 file changed, 98 insertions(+) diff --git a/internal/repository/repository_internal_test.go b/internal/repository/repository_internal_test.go index eed99c7e0..0c7115bc9 100644 --- a/internal/repository/repository_internal_test.go +++ b/internal/repository/repository_internal_test.go @@ -351,3 +351,101 @@ func testStreamPack(t *testing.T, version uint) { } }) } + +func TestBlobVerification(t *testing.T) { + repo := TestRepository(t).(*Repository) + + type DamageType string + const ( + damageData DamageType = "data" + damageCompressed DamageType = "compressed" + damageCiphertext DamageType = "ciphertext" + ) + + for _, test := range []struct { + damage DamageType + msg string + }{ + {"", ""}, + {damageData, "hash mismatch"}, + {damageCompressed, "decompression failed"}, + {damageCiphertext, "ciphertext verification failed"}, + } { + plaintext := rtest.Random(800, 1234) + id := restic.Hash(plaintext) + if test.damage == damageData { + plaintext[42] ^= 0x42 + } + + uncompressedLength := uint(len(plaintext)) + plaintext = repo.getZstdEncoder().EncodeAll(plaintext, nil) + + if test.damage == damageCompressed { + plaintext = plaintext[:len(plaintext)-8] + } + + nonce := crypto.NewRandomNonce() + ciphertext := append([]byte{}, nonce...) + ciphertext = repo.Key().Seal(ciphertext, nonce, plaintext, nil) + + if test.damage == damageCiphertext { + ciphertext[42] ^= 0x42 + } + + err := repo.verifyCiphertext(ciphertext, int(uncompressedLength), id) + if test.msg == "" { + rtest.Assert(t, err == nil, "expected no error, got %v", err) + } else { + rtest.Assert(t, strings.Contains(err.Error(), test.msg), "expected error to contain %q, got %q", test.msg, err) + } + } +} + +func TestUnpackedVerification(t *testing.T) { + repo := TestRepository(t).(*Repository) + + type DamageType string + const ( + damageData DamageType = "data" + damageCompressed DamageType = "compressed" + damageCiphertext DamageType = "ciphertext" + ) + + for _, test := range []struct { + damage DamageType + msg string + }{ + {"", ""}, + {damageData, "data mismatch"}, + {damageCompressed, "decompression failed"}, + {damageCiphertext, "ciphertext verification failed"}, + } { + plaintext := rtest.Random(800, 1234) + orig := append([]byte{}, plaintext...) + if test.damage == damageData { + plaintext[42] ^= 0x42 + } + + compressed := []byte{2} + compressed = repo.getZstdEncoder().EncodeAll(plaintext, compressed) + + if test.damage == damageCompressed { + compressed = compressed[:len(compressed)-8] + } + + nonce := crypto.NewRandomNonce() + ciphertext := append([]byte{}, nonce...) + ciphertext = repo.Key().Seal(ciphertext, nonce, compressed, nil) + + if test.damage == damageCiphertext { + ciphertext[42] ^= 0x42 + } + + err := repo.verifyUnpacked(ciphertext, restic.IndexFile, orig) + if test.msg == "" { + rtest.Assert(t, err == nil, "expected no error, got %v", err) + } else { + rtest.Assert(t, strings.Contains(err.Error(), test.msg), "expected error to contain %q, got %q", test.msg, err) + } + } +} From 66e8971659bdd84a0363799e2206ef310fbc8fc2 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 3 Feb 2024 18:17:18 +0100 Subject: [PATCH 07/13] Make --no-verify-pack globally available Verifying all blobs before upload comes with a notable performance impact. Allow users to skip it if necessary. --- cmd/restic/global.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cmd/restic/global.go b/cmd/restic/global.go index 6def9bf83..49cb894fa 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -67,6 +67,7 @@ type GlobalOptions struct { CleanupCache bool Compression repository.CompressionMode PackSize uint + NoVerifyPack bool backend.TransportOptions limiter.Limits @@ -139,6 +140,7 @@ func init() { f.BoolVar(&globalOptions.InsecureTLS, "insecure-tls", false, "skip TLS certificate verification when connecting to the repository (insecure)") f.BoolVar(&globalOptions.CleanupCache, "cleanup-cache", false, "auto remove old cache directories") f.Var(&globalOptions.Compression, "compression", "compression mode (only available for repository format version 2), one of (auto|off|max) (default: $RESTIC_COMPRESSION)") + f.BoolVar(&globalOptions.NoVerifyPack, "no-verify-pack", false, "skip verification of data before upload") f.IntVar(&globalOptions.Limits.UploadKb, "limit-upload", 0, "limits uploads to a maximum `rate` in KiB/s. (default: unlimited)") f.IntVar(&globalOptions.Limits.DownloadKb, "limit-download", 0, "limits downloads to a maximum `rate` in KiB/s. (default: unlimited)") f.UintVar(&globalOptions.PackSize, "pack-size", 0, "set target pack `size` in MiB, created pack files may be larger (default: $RESTIC_PACK_SIZE)") @@ -453,8 +455,9 @@ func OpenRepository(ctx context.Context, opts GlobalOptions) (*repository.Reposi } s, err := repository.New(be, repository.Options{ - Compression: opts.Compression, - PackSize: opts.PackSize * 1024 * 1024, + Compression: opts.Compression, + PackSize: opts.PackSize * 1024 * 1024, + NoVerifyPack: opts.NoVerifyPack, }) if err != nil { return nil, errors.Fatal(err.Error()) From c97a271e89a704a0524c3a3b654e0bff90bbf5ba Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sat, 3 Feb 2024 18:42:02 +0100 Subject: [PATCH 08/13] repository: ask users to report corrupted data while saving blobs --- internal/repository/repository.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 706e84876..f8b21586a 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -425,8 +425,7 @@ func (r *Repository) saveAndEncrypt(ctx context.Context, t restic.BlobType, data ciphertext = r.key.Seal(ciphertext, nonce, data, nil) if err := r.verifyCiphertext(ciphertext, uncompressedLength, id); err != nil { - // FIXME call to action - return 0, fmt.Errorf("detected data corruption while saving blob %v: %w", id, err) + return 0, fmt.Errorf("detected data corruption while saving blob %v: %w\nCorrupted blobs are either caused by hardware problems or bugs in restic. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting", id, err) } // find suitable packer and add blob @@ -522,8 +521,7 @@ func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, buf [] ciphertext = r.key.Seal(ciphertext, nonce, p, nil) if err := r.verifyUnpacked(ciphertext, t, buf); err != nil { - // FIXME call to action - return restic.ID{}, fmt.Errorf("detected data corruption while saving file of type %v: %w", t, err) + return restic.ID{}, fmt.Errorf("detected data corruption while saving file of type %v: %w\nCorrupted data is either caused by hardware problems or bugs in restic. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting", t, err) } if t == restic.ConfigFile { From c32e5e2abba39cdff12ad1ede34078b0c6598d30 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 4 Feb 2024 11:58:29 +0100 Subject: [PATCH 09/13] pack: verify integrity of pack file header --- internal/pack/pack.go | 43 +++++++++++++++------ internal/pack/pack_internal_test.go | 58 +++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 11 deletions(-) diff --git a/internal/pack/pack.go b/internal/pack/pack.go index 211af7bfb..04042744e 100644 --- a/internal/pack/pack.go +++ b/internal/pack/pack.go @@ -1,6 +1,7 @@ package pack import ( + "bytes" "context" "encoding/binary" "fmt" @@ -74,7 +75,7 @@ func (p *Packer) Finalize() error { p.m.Lock() defer p.m.Unlock() - header, err := p.makeHeader() + header, err := makeHeader(p.blobs) if err != nil { return err } @@ -83,6 +84,11 @@ func (p *Packer) Finalize() error { nonce := crypto.NewRandomNonce() encryptedHeader = append(encryptedHeader, nonce...) encryptedHeader = p.k.Seal(encryptedHeader, nonce, header, nil) + encryptedHeader = binary.LittleEndian.AppendUint32(encryptedHeader, uint32(len(encryptedHeader))) + + if err := verifyHeader(p.k, encryptedHeader, p.blobs); err != nil { + return fmt.Errorf("detected data corruption while writing pack-file header: %w\nCorrupted data is either caused by hardware problems or bugs in restic. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting", err) + } // append the header n, err := p.wr.Write(encryptedHeader) @@ -90,18 +96,33 @@ func (p *Packer) Finalize() error { return errors.Wrap(err, "Write") } - hdrBytes := len(encryptedHeader) - if n != hdrBytes { + if n != len(encryptedHeader) { return errors.New("wrong number of bytes written") } + p.bytes += uint(len(encryptedHeader)) - // write length - err = binary.Write(p.wr, binary.LittleEndian, uint32(hdrBytes)) + return nil +} + +func verifyHeader(k *crypto.Key, header []byte, expected []restic.Blob) error { + // do not offer a way to skip the pack header verification, as pack headers are usually small enough + // to not result in a significant performance impact + + decoded, hdrSize, err := List(k, bytes.NewReader(header), int64(len(header))) if err != nil { - return errors.Wrap(err, "binary.Write") + return fmt.Errorf("header decoding failed: %w", err) + } + if hdrSize != uint32(len(header)) { + return fmt.Errorf("unexpected header size %v instead of %v", hdrSize, len(header)) + } + if len(decoded) != len(expected) { + return fmt.Errorf("pack header size mismatch") + } + for i := 0; i < len(decoded); i++ { + if decoded[i] != expected[i] { + return fmt.Errorf("pack header entry mismatch got %v instead of %v", decoded[i], expected[i]) + } } - p.bytes += uint(hdrBytes + binary.Size(uint32(0))) - return nil } @@ -111,10 +132,10 @@ func (p *Packer) HeaderOverhead() int { } // makeHeader constructs the header for p. -func (p *Packer) makeHeader() ([]byte, error) { - buf := make([]byte, 0, len(p.blobs)*int(entrySize)) +func makeHeader(blobs []restic.Blob) ([]byte, error) { + buf := make([]byte, 0, len(blobs)*int(entrySize)) - for _, b := range p.blobs { + for _, b := range blobs { switch { case b.Type == restic.DataBlob && b.UncompressedLength == 0: buf = append(buf, 0) diff --git a/internal/pack/pack_internal_test.go b/internal/pack/pack_internal_test.go index c1a4867ea..2e7400ad0 100644 --- a/internal/pack/pack_internal_test.go +++ b/internal/pack/pack_internal_test.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/binary" "io" + "strings" "testing" "github.com/restic/restic/internal/crypto" @@ -177,3 +178,60 @@ func TestReadRecords(t *testing.T) { } } } + +func TestUnpackedVerification(t *testing.T) { + // create random keys + k := crypto.NewRandomKey() + blobs := []restic.Blob{ + { + BlobHandle: restic.NewRandomBlobHandle(), + Length: 42, + Offset: 0, + UncompressedLength: 2 * 42, + }, + } + + type DamageType string + const ( + damageData DamageType = "data" + damageCiphertext DamageType = "ciphertext" + damageLength DamageType = "length" + ) + + for _, test := range []struct { + damage DamageType + msg string + }{ + {"", ""}, + {damageData, "pack header entry mismatch"}, + {damageCiphertext, "ciphertext verification failed"}, + {damageLength, "header decoding failed"}, + } { + header, err := makeHeader(blobs) + rtest.OK(t, err) + + if test.damage == damageData { + header[8] ^= 0x42 + } + + encryptedHeader := make([]byte, 0, crypto.CiphertextLength(len(header))) + nonce := crypto.NewRandomNonce() + encryptedHeader = append(encryptedHeader, nonce...) + encryptedHeader = k.Seal(encryptedHeader, nonce, header, nil) + encryptedHeader = binary.LittleEndian.AppendUint32(encryptedHeader, uint32(len(encryptedHeader))) + + if test.damage == damageCiphertext { + encryptedHeader[8] ^= 0x42 + } + if test.damage == damageLength { + encryptedHeader[len(encryptedHeader)-1] ^= 0x42 + } + + err = verifyHeader(k, encryptedHeader, blobs) + if test.msg == "" { + rtest.Assert(t, err == nil, "expected no error, got %v", err) + } else { + rtest.Assert(t, strings.Contains(err.Error(), test.msg), "expected error to contain %q, got %q", test.msg, err) + } + } +} From 7d31180fe65f0b4a0442673c0e6318c593263b1f Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 4 Feb 2024 15:48:11 +0100 Subject: [PATCH 10/13] add data verification changelog entry --- changelog/unreleased/issue-4529 | 14 ++++++++++++++ 1 file changed, 14 insertions(+) create mode 100644 changelog/unreleased/issue-4529 diff --git a/changelog/unreleased/issue-4529 b/changelog/unreleased/issue-4529 new file mode 100644 index 000000000..2e8bbbed7 --- /dev/null +++ b/changelog/unreleased/issue-4529 @@ -0,0 +1,14 @@ +Enhancement: Verify data integrity before upload + +Hardware issues or a bug in restic could cause restic to create corrupted files +that were then uploaded to the repository. Detecting such corruption usually +required explicitly running the `check --read-data` command. + +To prevent the upload of corrupted data to the repository, restic now +additionally verifies that files can be decoded and contain the correct data +beforehand. This increases the CPU usage during backups. If absolutely +necessary, you can disable the verification using the option +`--no-verify-pack`. + +https://github.com/restic/restic/issues/4529 +https://github.com/restic/restic/pull/4681 From 86b38a0b17f4b67080047516913def0d0bc5e0a0 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 4 Feb 2024 16:50:50 +0100 Subject: [PATCH 11/13] rename `--no-verify-pack` to `--no-extra-verify` --- changelog/unreleased/issue-4529 | 2 +- cmd/restic/global.go | 10 +++++----- internal/repository/repack_test.go | 4 ++-- internal/repository/repair_pack_test.go | 2 +- internal/repository/repository.go | 10 +++++----- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/changelog/unreleased/issue-4529 b/changelog/unreleased/issue-4529 index 2e8bbbed7..c3ec69510 100644 --- a/changelog/unreleased/issue-4529 +++ b/changelog/unreleased/issue-4529 @@ -8,7 +8,7 @@ To prevent the upload of corrupted data to the repository, restic now additionally verifies that files can be decoded and contain the correct data beforehand. This increases the CPU usage during backups. If absolutely necessary, you can disable the verification using the option -`--no-verify-pack`. +`--no-extra-verify`. https://github.com/restic/restic/issues/4529 https://github.com/restic/restic/pull/4681 diff --git a/cmd/restic/global.go b/cmd/restic/global.go index 49cb894fa..08342435a 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -67,7 +67,7 @@ type GlobalOptions struct { CleanupCache bool Compression repository.CompressionMode PackSize uint - NoVerifyPack bool + NoExtraVerify bool backend.TransportOptions limiter.Limits @@ -140,7 +140,7 @@ func init() { f.BoolVar(&globalOptions.InsecureTLS, "insecure-tls", false, "skip TLS certificate verification when connecting to the repository (insecure)") f.BoolVar(&globalOptions.CleanupCache, "cleanup-cache", false, "auto remove old cache directories") f.Var(&globalOptions.Compression, "compression", "compression mode (only available for repository format version 2), one of (auto|off|max) (default: $RESTIC_COMPRESSION)") - f.BoolVar(&globalOptions.NoVerifyPack, "no-verify-pack", false, "skip verification of data before upload") + f.BoolVar(&globalOptions.NoExtraVerify, "no-extra-verify", false, "skip verification of data before upload") f.IntVar(&globalOptions.Limits.UploadKb, "limit-upload", 0, "limits uploads to a maximum `rate` in KiB/s. (default: unlimited)") f.IntVar(&globalOptions.Limits.DownloadKb, "limit-download", 0, "limits downloads to a maximum `rate` in KiB/s. (default: unlimited)") f.UintVar(&globalOptions.PackSize, "pack-size", 0, "set target pack `size` in MiB, created pack files may be larger (default: $RESTIC_PACK_SIZE)") @@ -455,9 +455,9 @@ func OpenRepository(ctx context.Context, opts GlobalOptions) (*repository.Reposi } s, err := repository.New(be, repository.Options{ - Compression: opts.Compression, - PackSize: opts.PackSize * 1024 * 1024, - NoVerifyPack: opts.NoVerifyPack, + Compression: opts.Compression, + PackSize: opts.PackSize * 1024 * 1024, + NoExtraVerify: opts.NoExtraVerify, }) if err != nil { return nil, errors.Fatal(err.Error()) diff --git a/internal/repository/repack_test.go b/internal/repository/repack_test.go index c07c0a943..e5e46ac2a 100644 --- a/internal/repository/repack_test.go +++ b/internal/repository/repack_test.go @@ -337,7 +337,7 @@ func TestRepackWrongBlob(t *testing.T) { func testRepackWrongBlob(t *testing.T, version uint) { // disable verification to allow adding corrupted blobs to the repository - repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoVerifyPack: true}) + repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoExtraVerify: true}) seed := time.Now().UnixNano() rand.Seed(seed) @@ -363,7 +363,7 @@ func TestRepackBlobFallback(t *testing.T) { func testRepackBlobFallback(t *testing.T, version uint) { // disable verification to allow adding corrupted blobs to the repository - repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoVerifyPack: true}) + repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoExtraVerify: true}) seed := time.Now().UnixNano() rand.Seed(seed) diff --git a/internal/repository/repair_pack_test.go b/internal/repository/repair_pack_test.go index c9b0badfc..b950245aa 100644 --- a/internal/repository/repair_pack_test.go +++ b/internal/repository/repair_pack_test.go @@ -103,7 +103,7 @@ func testRepairBrokenPack(t *testing.T, version uint) { for _, test := range tests { t.Run(test.name, func(t *testing.T) { // disable verification to allow adding corrupted blobs to the repository - repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoVerifyPack: true}) + repo := repository.TestRepositoryWithBackend(t, nil, version, repository.Options{NoExtraVerify: true}) seed := time.Now().UnixNano() rand.Seed(seed) diff --git a/internal/repository/repository.go b/internal/repository/repository.go index f8b21586a..917b7318f 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -59,9 +59,9 @@ type Repository struct { } type Options struct { - Compression CompressionMode - PackSize uint - NoVerifyPack bool + Compression CompressionMode + PackSize uint + NoExtraVerify bool } // CompressionMode configures if data should be compressed. @@ -444,7 +444,7 @@ func (r *Repository) saveAndEncrypt(ctx context.Context, t restic.BlobType, data } func (r *Repository) verifyCiphertext(buf []byte, uncompressedLength int, id restic.ID) error { - if r.opts.NoVerifyPack { + if r.opts.NoExtraVerify { return nil } @@ -542,7 +542,7 @@ func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, buf [] } func (r *Repository) verifyUnpacked(buf []byte, t restic.FileType, expected []byte) error { - if r.opts.NoVerifyPack { + if r.opts.NoExtraVerify { return nil } From a737fe1e478f7a06c1ae7c5e0a8569edccaf3b47 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 4 Feb 2024 17:11:49 +0100 Subject: [PATCH 12/13] add documentation for --no-extra-verify option --- doc/047_tuning_backup_parameters.rst | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/doc/047_tuning_backup_parameters.rst b/doc/047_tuning_backup_parameters.rst index 6ea39dc75..5773ac161 100644 --- a/doc/047_tuning_backup_parameters.rst +++ b/doc/047_tuning_backup_parameters.rst @@ -60,6 +60,17 @@ only applied for the single run of restic. The option can also be set via the en variable ``RESTIC_COMPRESSION``. +Data Verification +================= + +To prevent the upload of corrupted data to the repository, restic verifies that files can +be decoded and contain the correct data beforehand. This increases the CPU usage during +backups. If necessary, you can disable this verification using the option ``--no-extra-verify``. +However, in this case you should verify the repository integrity more actively using +``restic check --read-data``. Otherwise, data corruption due to hardware issues or software +bugs might go unnoticed. + + File Read Concurrency ===================== From 5957417b1f85e0921dae56e88ef68f3d5ce88e9f Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 4 Feb 2024 18:09:32 +0100 Subject: [PATCH 13/13] Apply changelog entry / documentation improvements from review --- changelog/unreleased/issue-4529 | 22 +++++++++++++--------- cmd/restic/global.go | 2 +- doc/047_tuning_backup_parameters.rst | 15 +++++++++------ internal/pack/pack.go | 3 ++- internal/repository/repository.go | 6 ++++-- 5 files changed, 29 insertions(+), 19 deletions(-) diff --git a/changelog/unreleased/issue-4529 b/changelog/unreleased/issue-4529 index c3ec69510..fed726d2d 100644 --- a/changelog/unreleased/issue-4529 +++ b/changelog/unreleased/issue-4529 @@ -1,14 +1,18 @@ -Enhancement: Verify data integrity before upload +Enhancement: Add extra verification of data integrity before upload -Hardware issues or a bug in restic could cause restic to create corrupted files -that were then uploaded to the repository. Detecting such corruption usually -required explicitly running the `check --read-data` command. +Hardware issues, or a bug in restic or its dependencies, could previously cause +corruption in the files restic created and stored in the repository. Detecting +such corruption previously required explicitly running the `check --read-data` +or `check --read-data-subset` commands. -To prevent the upload of corrupted data to the repository, restic now -additionally verifies that files can be decoded and contain the correct data -beforehand. This increases the CPU usage during backups. If absolutely -necessary, you can disable the verification using the option -`--no-extra-verify`. +To further ensure data integrity, even in the case of hardware issues or +software bugs, restic now performs additional verification of the files about +to be uploaded to the repository. + +These extra checks will increase CPU usage during backups. They can therefore, +if absolutely necessary, be disabled using the `--no-extra-verify` global +option. Please note that this should be combined with more active checking +using the previously mentioned check commands. https://github.com/restic/restic/issues/4529 https://github.com/restic/restic/pull/4681 diff --git a/cmd/restic/global.go b/cmd/restic/global.go index 08342435a..da01aa732 100644 --- a/cmd/restic/global.go +++ b/cmd/restic/global.go @@ -140,7 +140,7 @@ func init() { f.BoolVar(&globalOptions.InsecureTLS, "insecure-tls", false, "skip TLS certificate verification when connecting to the repository (insecure)") f.BoolVar(&globalOptions.CleanupCache, "cleanup-cache", false, "auto remove old cache directories") f.Var(&globalOptions.Compression, "compression", "compression mode (only available for repository format version 2), one of (auto|off|max) (default: $RESTIC_COMPRESSION)") - f.BoolVar(&globalOptions.NoExtraVerify, "no-extra-verify", false, "skip verification of data before upload") + f.BoolVar(&globalOptions.NoExtraVerify, "no-extra-verify", false, "skip additional verification of data before upload (see documentation)") f.IntVar(&globalOptions.Limits.UploadKb, "limit-upload", 0, "limits uploads to a maximum `rate` in KiB/s. (default: unlimited)") f.IntVar(&globalOptions.Limits.DownloadKb, "limit-download", 0, "limits downloads to a maximum `rate` in KiB/s. (default: unlimited)") f.UintVar(&globalOptions.PackSize, "pack-size", 0, "set target pack `size` in MiB, created pack files may be larger (default: $RESTIC_PACK_SIZE)") diff --git a/doc/047_tuning_backup_parameters.rst b/doc/047_tuning_backup_parameters.rst index 5773ac161..d8fb2c9b6 100644 --- a/doc/047_tuning_backup_parameters.rst +++ b/doc/047_tuning_backup_parameters.rst @@ -63,12 +63,15 @@ variable ``RESTIC_COMPRESSION``. Data Verification ================= -To prevent the upload of corrupted data to the repository, restic verifies that files can -be decoded and contain the correct data beforehand. This increases the CPU usage during -backups. If necessary, you can disable this verification using the option ``--no-extra-verify``. -However, in this case you should verify the repository integrity more actively using -``restic check --read-data``. Otherwise, data corruption due to hardware issues or software -bugs might go unnoticed. +To prevent the upload of corrupted data to the repository, which can happen due +to hardware issues or software bugs, restic verifies that generated files can +be decoded and contain the correct data beforehand. This increases the CPU usage +during backups. If necessary, you can disable this verification using the +``--no-extra-verify`` option of the ``backup`` command. However, in this case +you should verify the repository integrity more actively using +``restic check --read-data`` (or the similar ``--read-data-subset`` option). +Otherwise, data corruption due to hardware issues or software bugs might go +unnoticed. File Read Concurrency diff --git a/internal/pack/pack.go b/internal/pack/pack.go index 04042744e..cd118ab03 100644 --- a/internal/pack/pack.go +++ b/internal/pack/pack.go @@ -87,7 +87,8 @@ func (p *Packer) Finalize() error { encryptedHeader = binary.LittleEndian.AppendUint32(encryptedHeader, uint32(len(encryptedHeader))) if err := verifyHeader(p.k, encryptedHeader, p.blobs); err != nil { - return fmt.Errorf("detected data corruption while writing pack-file header: %w\nCorrupted data is either caused by hardware problems or bugs in restic. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting", err) + //nolint:revive // ignore linter warnings about error message spelling + return fmt.Errorf("Detected data corruption while writing pack-file header: %w\nCorrupted data is either caused by hardware issues or software bugs. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting.", err) } // append the header diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 917b7318f..a20f71ab1 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -425,7 +425,8 @@ func (r *Repository) saveAndEncrypt(ctx context.Context, t restic.BlobType, data ciphertext = r.key.Seal(ciphertext, nonce, data, nil) if err := r.verifyCiphertext(ciphertext, uncompressedLength, id); err != nil { - return 0, fmt.Errorf("detected data corruption while saving blob %v: %w\nCorrupted blobs are either caused by hardware problems or bugs in restic. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting", id, err) + //nolint:revive // ignore linter warnings about error message spelling + return 0, fmt.Errorf("Detected data corruption while saving blob %v: %w\nCorrupted blobs are either caused by hardware issues or software bugs. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting.", id, err) } // find suitable packer and add blob @@ -521,7 +522,8 @@ func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, buf [] ciphertext = r.key.Seal(ciphertext, nonce, p, nil) if err := r.verifyUnpacked(ciphertext, t, buf); err != nil { - return restic.ID{}, fmt.Errorf("detected data corruption while saving file of type %v: %w\nCorrupted data is either caused by hardware problems or bugs in restic. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting", t, err) + //nolint:revive // ignore linter warnings about error message spelling + return restic.ID{}, fmt.Errorf("Detected data corruption while saving file of type %v: %w\nCorrupted data is either caused by hardware issues or software bugs. Please open an issue at https://github.com/restic/restic/issues/new/choose for further troubleshooting.", t, err) } if t == restic.ConfigFile {