repository: reject key and config files that are too short (#21977)

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>

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

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 {

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 {

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)
+}