diff --git a/changelog/unreleased/pull-21977 b/changelog/unreleased/pull-21977 new file mode 100644 index 000000000..716125469 --- /dev/null +++ b/changelog/unreleased/pull-21977 @@ -0,0 +1,9 @@ +Bugfix: Report an error instead of crashing on a truncated key or config file + +Restic crashed with a slice bounds panic when a repository contained a key +file or a config file that was shorter than the encryption overhead, for +example after an upload was interrupted. Since every command has to read a +key file, the whole repository became unusable until the file was removed. +Restic now reports that the file is too short instead. + +https://github.com/restic/restic/pull/21977 diff --git a/internal/repository/key.go b/internal/repository/key.go index 94a4184e6..e1b0a3dcd 100644 --- a/internal/repository/key.go +++ b/internal/repository/key.go @@ -98,6 +98,10 @@ func openKey(ctx context.Context, s *Repository, id restic.ID, password string) } // decrypt master keys + if len(k.Data) < crypto.CiphertextLength(0) { + return nil, errors.New("invalid key file, data too short") + } + nonce, ciphertext := k.Data[:k.user.NonceSize()], k.Data[k.user.NonceSize():] buf, err := k.user.Open(nil, nonce, ciphertext, nil) if err != nil { diff --git a/internal/repository/repository.go b/internal/repository/repository.go index e52c01f11..8eb9e6a81 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -198,6 +198,10 @@ func (r *Repository) LoadUnpacked(ctx context.Context, t restic.FileType, id res return nil, err } + if len(buf) < crypto.CiphertextLength(0) { + return nil, fmt.Errorf("invalid data in %v file, too short", t) + } + nonce, ciphertext := buf[:r.key.NonceSize()], buf[r.key.NonceSize():] plaintext, err := r.key.Open(ciphertext[:0], nonce, ciphertext, nil) if err != nil { diff --git a/internal/repository/repository_internal_test.go b/internal/repository/repository_internal_test.go index 1b78f07cb..7f9afa175 100644 --- a/internal/repository/repository_internal_test.go +++ b/internal/repository/repository_internal_test.go @@ -14,6 +14,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/klauspost/compress/zstd" "github.com/restic/restic/internal/backend" + "github.com/restic/restic/internal/backend/mem" "github.com/restic/restic/internal/errors" "github.com/restic/restic/internal/repository/crypto" "github.com/restic/restic/internal/repository/index" @@ -576,3 +577,50 @@ func TestStreamPackFallback(t *testing.T) { test(t, true) }) } + +// A key file is just JSON in the repository, so its "data" field can be any +// length an attacker with write access to the backend picks. Make sure a +// short one is rejected instead of tripping a slice bounds panic. +func TestOpenKeyShortData(t *testing.T) { + for _, data := range [][]byte{nil, {}, make([]byte, 1), make([]byte, crypto.CiphertextLength(0)-1)} { + be := mem.New() + repo, err := New(be, Options{}) + rtest.OK(t, err) + + key := Key{ + KDF: "scrypt", + N: 1024, + R: 8, + P: 1, + Salt: make([]byte, 64), + Data: data, + } + buf, err := json.Marshal(key) + rtest.OK(t, err) + + id := restic.Hash(buf) + h := backend.Handle{Type: backend.KeyFile, Name: id.String()} + rtest.OK(t, be.Save(context.TODO(), h, backend.NewByteReader(buf, be.Hasher()))) + + _, err = openKey(context.TODO(), repo, id, "geheim") + rtest.Assert(t, err != nil, "expected an error for key data of length %d", len(data)) + rtest.Assert(t, strings.Contains(err.Error(), "too short"), + "expected a 'too short' error, got %v", err) + } +} + +// The config file is exempt from the ID check in LoadRaw, so a truncated one +// reaches the decryption path unfiltered. +func TestLoadUnpackedShortData(t *testing.T) { + be := mem.New() + repo, err := New(be, Options{}) + rtest.OK(t, err) + + h := backend.Handle{Type: backend.ConfigFile, Name: restic.ID{}.String()} + rtest.OK(t, be.Save(context.TODO(), h, backend.NewByteReader([]byte{}, be.Hasher()))) + + repo.key = crypto.NewRandomKey() + _, err = repo.LoadUnpacked(context.TODO(), restic.ConfigFile, restic.ID{}) + rtest.Assert(t, err != nil && strings.Contains(err.Error(), "too short"), + "expected a 'too short' error, got %v", err) +}