From e1b80859f27f995d48dd1e77e59286ad70514777 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 28 Oct 2017 10:59:55 +0200 Subject: [PATCH 1/8] Make crypto.Key implement cipher.AEAD --- internal/crypto/crypto.go | 144 ++++++++++++++++++++++++++++- internal/crypto/crypto_int_test.go | 41 ++++---- 2 files changed, 166 insertions(+), 19 deletions(-) diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index aaa42b54f..62ec62e25 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -147,7 +147,9 @@ func NewRandomKey() *Key { return k } -func newIV() []byte { +// NewRandomNonce returns a new random nonce. It panics on error so that the +// program is safely terminated. +func NewRandomNonce() []byte { iv := make([]byte, ivSize) n, err := rand.Read(iv) if n != ivSize || err != nil { @@ -233,6 +235,144 @@ func (k *EncryptionKey) Valid() bool { // holds the plaintext. var ErrInvalidCiphertext = errors.New("invalid ciphertext, same slice used for plaintext") +// validNonce checks that nonce is not all zero. +func validNonce(nonce []byte) bool { + sum := 0 + for b := range nonce { + sum += b + } + return sum > 0 +} + +// statically ensure that *Key implements crypto/cipher.AEAD +var _ cipher.AEAD = &Key{} + +// NonceSize returns the size of the nonce that must be passed to Seal +// and Open. +func (k *Key) NonceSize() int { + return ivSize +} + +// Overhead returns the maximum difference between the lengths of a +// plaintext and its ciphertext. +func (k *Key) Overhead() int { + return macSize +} + +// Seal encrypts and authenticates plaintext, authenticates the +// additional data and appends the result to dst, returning the updated +// slice. The nonce must be NonceSize() bytes long and unique for all +// time, for a given key. +// +// The plaintext and dst may alias exactly or not at all. To reuse +// plaintext's storage for the encrypted output, use plaintext[:0] as dst. +func (k *Key) Seal(dst, nonce, plaintext, additionalData []byte) []byte { + if !k.Valid() { + panic("key is invalid") + } + + if len(additionalData) > 0 { + panic("additional data is not supported") + } + + if len(nonce) != ivSize { + panic("incorrect nonce length") + } + + if !validNonce(nonce) { + panic("nonce is invalid") + } + + // extend dst so that the ciphertext fits + ciphertextLength := len(plaintext) + k.Overhead() + pos := len(dst) + + capacity := cap(dst) - len(dst) + if capacity < ciphertextLength { + dst = dst[:cap(dst)] + dst = append(dst, make([]byte, ciphertextLength-capacity)...) + } else { + dst = dst[:pos+ciphertextLength] + } + + c, err := aes.NewCipher(k.EncryptionKey[:]) + if err != nil { + panic(fmt.Sprintf("unable to create cipher: %v", err)) + } + e := cipher.NewCTR(c, nonce) + e.XORKeyStream(dst[pos:pos+len(plaintext)], plaintext) + + // truncate to only cover the ciphertext + dst = dst[:pos+len(plaintext)] + + mac := poly1305MAC(dst[pos:], nonce, &k.MACKey) + dst = append(dst, mac...) + + return dst +} + +// Open decrypts and authenticates ciphertext, authenticates the +// additional data and, if successful, appends the resulting plaintext +// to dst, returning the updated slice. The nonce must be NonceSize() +// bytes long and both it and the additional data must match the +// value passed to Seal. +// +// The ciphertext and dst may alias exactly or not at all. To reuse +// ciphertext's storage for the decrypted output, use ciphertext[:0] as dst. +// +// Even if the function fails, the contents of dst, up to its capacity, +// may be overwritten. +func (k *Key) Open(dst, nonce, ciphertext, additionalData []byte) ([]byte, error) { + if !k.Valid() { + return nil, errors.New("invalid key") + } + + // check parameters + if len(nonce) != ivSize { + panic("incorrect nonce length") + } + + if !validNonce(nonce) { + return nil, errors.New("nonce is invalid") + } + + // check for plausible length + if len(ciphertext) < k.Overhead() { + return nil, errors.Errorf("trying to decrypt invalid data: ciphertext too small") + } + + // extract mac + l := len(ciphertext) - macSize + ct, mac := ciphertext[:l], ciphertext[l:] + + // verify mac + if !poly1305Verify(ct, nonce, &k.MACKey, mac) { + return nil, ErrUnauthenticated + } + + // extend dst so that the plaintext fits + plaintextLength := len(ct) + pos := len(dst) + + capacity := cap(dst) - len(dst) + if capacity < plaintextLength { + dst = dst[:cap(dst)] + dst = append(dst, make([]byte, plaintextLength-capacity)...) + } else { + dst = dst[:pos+plaintextLength] + } + + // decrypt data + c, err := aes.NewCipher(k.EncryptionKey[:]) + if err != nil { + panic(fmt.Sprintf("unable to create cipher: %v", err)) + } + e := cipher.NewCTR(c, nonce) + e.XORKeyStream(dst[pos:], ct) + + return dst, nil +} + // Encrypt encrypts and authenticates data. Stored in ciphertext is IV || Ciphertext || // MAC. Encrypt returns the new ciphertext slice, which is extended when // necessary. ciphertext and plaintext may not point to (exactly) the same @@ -255,7 +395,7 @@ func (k *Key) Encrypt(ciphertext []byte, plaintext []byte) ([]byte, error) { ciphertext = append(ciphertext, make([]byte, ext)...) } - iv := newIV() + iv := NewRandomNonce() copy(ciphertext, iv[:]) c, err := aes.NewCipher(k.EncryptionKey[:]) diff --git a/internal/crypto/crypto_int_test.go b/internal/crypto/crypto_int_test.go index 3ace3f393..a5995a9ad 100644 --- a/internal/crypto/crypto_int_test.go +++ b/internal/crypto/crypto_int_test.go @@ -113,43 +113,50 @@ func TestCrypto(t *testing.T) { MACKey: tv.skey, } - msg, err := k.Encrypt(msg, tv.plaintext) - if err != nil { - t.Fatal(err) - } + nonce := NewRandomNonce() + ciphertext := k.Seal(msg, nonce, tv.plaintext, nil) // decrypt message buf := make([]byte, len(tv.plaintext)) - n, err := k.Decrypt(buf, msg) + buf, err := k.Open(buf, nonce, ciphertext, nil) if err != nil { t.Fatal(err) } - buf = buf[:n] - // change mac, this must fail - msg[len(msg)-8] ^= 0x23 - - if _, err = k.Decrypt(buf, msg); err != ErrUnauthenticated { - t.Fatal("wrong MAC value not detected") + if !bytes.Equal(buf, tv.plaintext) { + t.Fatalf("wrong plaintext returned") } + // change mac, this must fail + ciphertext[len(ciphertext)-8] ^= 0x23 + + if _, err = k.Open(buf, nonce, ciphertext, nil); err != ErrUnauthenticated { + t.Fatal("wrong MAC value not detected") + } // reset mac - msg[len(msg)-8] ^= 0x23 + ciphertext[len(ciphertext)-8] ^= 0x23 + + // tamper with nonce, this must fail + nonce[2] ^= 0x88 + if _, err = k.Open(buf, nonce, ciphertext, nil); err != ErrUnauthenticated { + t.Fatal("tampered nonce not detected") + } + // reset nonce + nonce[2] ^= 0x88 // tamper with message, this must fail - msg[16+5] ^= 0x85 - - if _, err = k.Decrypt(buf, msg); err != ErrUnauthenticated { + ciphertext[16+5] ^= 0x85 + if _, err = k.Open(buf, nonce, ciphertext, nil); err != ErrUnauthenticated { t.Fatal("tampered message not detected") } // test decryption p := make([]byte, len(tv.ciphertext)) - n, err = k.Decrypt(p, tv.ciphertext) + nonce, ciphertext = tv.ciphertext[:16], tv.ciphertext[16:] + p, err = k.Open(p, nonce, ciphertext, nil) if err != nil { t.Fatal(err) } - p = p[:n] if !bytes.Equal(p, tv.plaintext) { t.Fatalf("wrong plaintext: expected %q but got %q\n", tv.plaintext, p) From 6fc133ad6af4da8e9eb1ccb0012a21be9a00fa2f Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 28 Oct 2017 11:24:15 +0200 Subject: [PATCH 2/8] Run tests on Seal/Open --- internal/crypto/crypto_int_test.go | 12 +- internal/crypto/crypto_test.go | 211 +++++++++++++++++++++++------ 2 files changed, 177 insertions(+), 46 deletions(-) diff --git a/internal/crypto/crypto_int_test.go b/internal/crypto/crypto_int_test.go index a5995a9ad..9473d1382 100644 --- a/internal/crypto/crypto_int_test.go +++ b/internal/crypto/crypto_int_test.go @@ -114,10 +114,10 @@ func TestCrypto(t *testing.T) { } nonce := NewRandomNonce() - ciphertext := k.Seal(msg, nonce, tv.plaintext, nil) + ciphertext := k.Seal(msg[0:], nonce, tv.plaintext, nil) // decrypt message - buf := make([]byte, len(tv.plaintext)) + buf := make([]byte, 0, len(tv.plaintext)) buf, err := k.Open(buf, nonce, ciphertext, nil) if err != nil { t.Fatal(err) @@ -130,7 +130,7 @@ func TestCrypto(t *testing.T) { // change mac, this must fail ciphertext[len(ciphertext)-8] ^= 0x23 - if _, err = k.Open(buf, nonce, ciphertext, nil); err != ErrUnauthenticated { + if _, err = k.Open(buf[:0], nonce, ciphertext, nil); err != ErrUnauthenticated { t.Fatal("wrong MAC value not detected") } // reset mac @@ -138,7 +138,7 @@ func TestCrypto(t *testing.T) { // tamper with nonce, this must fail nonce[2] ^= 0x88 - if _, err = k.Open(buf, nonce, ciphertext, nil); err != ErrUnauthenticated { + if _, err = k.Open(buf[:0], nonce, ciphertext, nil); err != ErrUnauthenticated { t.Fatal("tampered nonce not detected") } // reset nonce @@ -146,14 +146,14 @@ func TestCrypto(t *testing.T) { // tamper with message, this must fail ciphertext[16+5] ^= 0x85 - if _, err = k.Open(buf, nonce, ciphertext, nil); err != ErrUnauthenticated { + if _, err = k.Open(buf[:0], nonce, ciphertext, nil); err != ErrUnauthenticated { t.Fatal("tampered message not detected") } // test decryption p := make([]byte, len(tv.ciphertext)) nonce, ciphertext = tv.ciphertext[:16], tv.ciphertext[16:] - p, err = k.Open(p, nonce, ciphertext, nil) + p, err = k.Open(p[:0], nonce, ciphertext, nil) if err != nil { t.Fatal(err) } diff --git a/internal/crypto/crypto_test.go b/internal/crypto/crypto_test.go index f1a7b7994..94f22588b 100644 --- a/internal/crypto/crypto_test.go +++ b/internal/crypto/crypto_test.go @@ -24,18 +24,17 @@ func TestEncryptDecrypt(t *testing.T) { for _, size := range tests { data := rtest.Random(42, size) - buf := make([]byte, size+crypto.Extension) + buf := make([]byte, 0, size+crypto.Extension) - ciphertext, err := k.Encrypt(buf, data) - rtest.OK(t, err) - rtest.Assert(t, len(ciphertext) == len(data)+crypto.Extension, + nonce := crypto.NewRandomNonce() + ciphertext := k.Seal(buf[:0], nonce, data, nil) + rtest.Assert(t, len(ciphertext) == len(data)+k.Overhead(), "ciphertext length does not match: want %d, got %d", len(data)+crypto.Extension, len(ciphertext)) - plaintext := make([]byte, len(ciphertext)) - n, err := k.Decrypt(plaintext, ciphertext) + plaintext := make([]byte, 0, len(ciphertext)) + plaintext, err := k.Open(plaintext[:0], nonce, ciphertext, nil) rtest.OK(t, err) - plaintext = plaintext[:n] rtest.Assert(t, len(plaintext) == len(data), "plaintext length does not match: want %d, got %d", len(data), len(plaintext)) @@ -52,8 +51,9 @@ func TestSmallBuffer(t *testing.T) { _, err := io.ReadFull(rand.Reader, data) rtest.OK(t, err) - ciphertext := make([]byte, size/2) - ciphertext, err = k.Encrypt(ciphertext, data) + ciphertext := make([]byte, 0, size/2) + nonce := crypto.NewRandomNonce() + ciphertext = k.Seal(ciphertext[:0], nonce, data, nil) // this must extend the slice rtest.Assert(t, cap(ciphertext) > size/2, "expected extended slice, but capacity is only %d bytes", @@ -61,9 +61,8 @@ func TestSmallBuffer(t *testing.T) { // check for the correct plaintext plaintext := make([]byte, len(ciphertext)) - n, err := k.Decrypt(plaintext, ciphertext) + plaintext, err = k.Open(plaintext[:0], nonce, ciphertext, nil) rtest.OK(t, err) - plaintext = plaintext[:n] rtest.Assert(t, bytes.Equal(plaintext, data), "wrong plaintext returned") } @@ -78,37 +77,169 @@ func TestSameBuffer(t *testing.T) { ciphertext := make([]byte, 0, size+crypto.Extension) - ciphertext, err = k.Encrypt(ciphertext, data) - rtest.OK(t, err) + nonce := crypto.NewRandomNonce() + ciphertext = k.Seal(ciphertext, nonce, data, nil) // use the same buffer for decryption - n, err := k.Decrypt(ciphertext, ciphertext) + ciphertext, err = k.Open(ciphertext[:0], nonce, ciphertext, nil) rtest.OK(t, err) - ciphertext = ciphertext[:n] rtest.Assert(t, bytes.Equal(ciphertext, data), "wrong plaintext returned") } -func TestCornerCases(t *testing.T) { +func encrypt(t testing.TB, k *crypto.Key, data, ciphertext, nonce []byte) []byte { + prefixlen := len(ciphertext) + ciphertext = k.Seal(ciphertext, nonce, data, nil) + if len(ciphertext) != len(data)+k.Overhead()+prefixlen { + t.Fatalf("destination slice has wrong length, want %d, got %d", + len(data)+k.Overhead(), len(ciphertext)) + } + + return ciphertext +} + +func decryptNewSliceAndCompare(t testing.TB, k *crypto.Key, data, ciphertext, nonce []byte) { + plaintext := make([]byte, 0, len(ciphertext)) + decryptAndCompare(t, k, data, ciphertext, nonce, plaintext) +} + +func decryptAndCompare(t testing.TB, k *crypto.Key, data, ciphertext, nonce, dst []byte) { + prefix := make([]byte, len(dst)) + copy(prefix, dst) + + plaintext, err := k.Open(dst, nonce, ciphertext, nil) + if err != nil { + t.Fatalf("unable to decrypt ciphertext: %v", err) + } + + if len(data)+len(prefix) != len(plaintext) { + t.Fatalf("wrong plaintext returned, want %d bytes, got %d", len(data)+len(prefix), len(plaintext)) + } + + if !bytes.Equal(plaintext[:len(prefix)], prefix) { + t.Fatal("prefix is wrong") + } + + if !bytes.Equal(plaintext[len(prefix):], data) { + t.Fatal("wrong plaintext returned") + } +} + +func TestAppendSeal(t *testing.T) { + k := crypto.NewRandomKey() + nonce := crypto.NewRandomNonce() + + data := make([]byte, 600) + _, err := io.ReadFull(rand.Reader, data) + rtest.OK(t, err) + ciphertext := encrypt(t, k, data, nil, nonce) + + // we need to test several different cases: + // * destination slice is nil + // * destination slice is empty and has enough capacity + // * destination slice is empty and does not have enough capacity + // * destination slice contains data and has enough capacity + // * destination slice contains data and does not have enough capacity + + // destination slice is nil + t.Run("nil", func(t *testing.T) { + var plaintext []byte + decryptAndCompare(t, k, data, ciphertext, nonce, plaintext) + }) + + // destination slice is empty and has enough capacity + t.Run("empty-large", func(t *testing.T) { + plaintext := make([]byte, 0, len(data)+100) + decryptAndCompare(t, k, data, ciphertext, nonce, plaintext) + }) + + // destination slice is empty and does not have enough capacity + t.Run("empty-small", func(t *testing.T) { + plaintext := make([]byte, 0, len(data)/2) + decryptAndCompare(t, k, data, ciphertext, nonce, plaintext) + }) + + // destination slice contains data and has enough capacity + t.Run("prefix-large", func(t *testing.T) { + plaintext := make([]byte, 0, len(data)+100) + plaintext = append(plaintext, []byte("foobar")...) + decryptAndCompare(t, k, data, ciphertext, nonce, plaintext) + }) + + // destination slice contains data and does not have enough capacity + t.Run("prefix-small", func(t *testing.T) { + plaintext := make([]byte, 0, len(data)/2) + plaintext = append(plaintext, []byte("foobar")...) + decryptAndCompare(t, k, data, ciphertext, nonce, plaintext) + }) +} + +func TestAppendOpen(t *testing.T) { k := crypto.NewRandomKey() - // nil plaintext should encrypt to the empty string - // nil ciphertext should allocate a new slice for the ciphertext - c, err := k.Encrypt(nil, nil) + data := make([]byte, 600) + _, err := io.ReadFull(rand.Reader, data) rtest.OK(t, err) - rtest.Assert(t, len(c) == crypto.Extension, - "wrong length returned for ciphertext, expected 0, got %d", - len(c)) + // we need to test several different cases: + // * destination slice is nil + // * destination slice is empty and has enough capacity + // * destination slice is empty and does not have enough capacity + // * destination slice contains data and has enough capacity + // * destination slice contains data and does not have enough capacity - // this should decrypt to nil - n, err := k.Decrypt(nil, c) - rtest.OK(t, err) - rtest.Equals(t, 0, n) + // destination slice is nil + t.Run("nil", func(t *testing.T) { + nonce := crypto.NewRandomNonce() + var ciphertext []byte - // test encryption for same slice, this should return an error - _, err = k.Encrypt(c, c) - rtest.Equals(t, crypto.ErrInvalidCiphertext, err) + ciphertext = encrypt(t, k, data, ciphertext, nonce) + decryptNewSliceAndCompare(t, k, data, ciphertext, nonce) + }) + + // destination slice is empty and has enough capacity + t.Run("empty-large", func(t *testing.T) { + nonce := crypto.NewRandomNonce() + ciphertext := make([]byte, 0, len(data)+100) + + ciphertext = encrypt(t, k, data, ciphertext, nonce) + decryptNewSliceAndCompare(t, k, data, ciphertext, nonce) + }) + + // destination slice is empty and does not have enough capacity + t.Run("empty-small", func(t *testing.T) { + nonce := crypto.NewRandomNonce() + ciphertext := make([]byte, 0, len(data)/2) + + ciphertext = encrypt(t, k, data, ciphertext, nonce) + decryptNewSliceAndCompare(t, k, data, ciphertext, nonce) + }) + + // destination slice contains data and has enough capacity + t.Run("prefix-large", func(t *testing.T) { + nonce := crypto.NewRandomNonce() + ciphertext := make([]byte, 0, len(data)+100) + ciphertext = append(ciphertext, []byte("foobar")...) + + ciphertext = encrypt(t, k, data, ciphertext, nonce) + if string(ciphertext[:6]) != "foobar" { + t.Errorf("prefix is missing") + } + decryptNewSliceAndCompare(t, k, data, ciphertext[6:], nonce) + }) + + // destination slice contains data and does not have enough capacity + t.Run("prefix-small", func(t *testing.T) { + nonce := crypto.NewRandomNonce() + ciphertext := make([]byte, 0, len(data)/2) + ciphertext = append(ciphertext, []byte("foobar")...) + + ciphertext = encrypt(t, k, data, ciphertext, nonce) + if string(ciphertext[:6]) != "foobar" { + t.Errorf("prefix is missing") + } + decryptNewSliceAndCompare(t, k, data, ciphertext[6:], nonce) + }) } func TestLargeEncrypt(t *testing.T) { @@ -123,10 +254,9 @@ func TestLargeEncrypt(t *testing.T) { _, err := io.ReadFull(rand.Reader, data) rtest.OK(t, err) - ciphertext, err := k.Encrypt(make([]byte, size+crypto.Extension), data) - rtest.OK(t, err) - - plaintext, err := k.Decrypt([]byte{}, ciphertext) + nonce := crypto.NewRandomNonce() + ciphertext := k.Seal(make([]byte, size+k.Overhead()), nonce, data, nil) + plaintext, err := k.Open([]byte{}, nonce, ciphertext, nil) rtest.OK(t, err) rtest.Equals(t, plaintext, data) @@ -139,13 +269,13 @@ func BenchmarkEncrypt(b *testing.B) { k := crypto.NewRandomKey() buf := make([]byte, len(data)+crypto.Extension) + nonce := crypto.NewRandomNonce() b.ResetTimer() b.SetBytes(int64(size)) for i := 0; i < b.N; i++ { - _, err := k.Encrypt(buf, data) - rtest.OK(b, err) + _ = k.Seal(buf, nonce, data, nil) } } @@ -155,17 +285,18 @@ func BenchmarkDecrypt(b *testing.B) { k := crypto.NewRandomKey() - plaintext := make([]byte, size) - ciphertext := make([]byte, size+crypto.Extension) + plaintext := make([]byte, 0, size) + ciphertext := make([]byte, 0, size+crypto.Extension) + nonce := crypto.NewRandomNonce() + ciphertext = k.Seal(ciphertext, nonce, data, nil) - ciphertext, err := k.Encrypt(ciphertext, data) - rtest.OK(b, err) + var err error b.ResetTimer() b.SetBytes(int64(size)) for i := 0; i < b.N; i++ { - _, err = k.Decrypt(plaintext, ciphertext) + _, err = k.Open(plaintext, nonce, ciphertext, nil) rtest.OK(b, err) } } From a5f0e9ab654d5a60b1ca19eb4a5110ae1f84d921 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sat, 28 Oct 2017 11:24:09 +0200 Subject: [PATCH 3/8] Remove custom Encrypt/Decrypt methods --- internal/crypto/crypto.go | 87 --------------------------------------- 1 file changed, 87 deletions(-) diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index 62ec62e25..07ea3cc43 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -373,93 +373,6 @@ func (k *Key) Open(dst, nonce, ciphertext, additionalData []byte) ([]byte, error return dst, nil } -// Encrypt encrypts and authenticates data. Stored in ciphertext is IV || Ciphertext || -// MAC. Encrypt returns the new ciphertext slice, which is extended when -// necessary. ciphertext and plaintext may not point to (exactly) the same -// slice or non-intersecting slices. -func (k *Key) Encrypt(ciphertext []byte, plaintext []byte) ([]byte, error) { - if !k.Valid() { - return nil, errors.New("invalid key") - } - - ciphertext = ciphertext[:cap(ciphertext)] - - // test for same slice, if possible - if len(plaintext) > 0 && len(ciphertext) > 0 && &plaintext[0] == &ciphertext[0] { - return nil, ErrInvalidCiphertext - } - - // extend ciphertext slice if necessary - if len(ciphertext) < len(plaintext)+Extension { - ext := len(plaintext) + Extension - len(ciphertext) - ciphertext = append(ciphertext, make([]byte, ext)...) - } - - iv := NewRandomNonce() - copy(ciphertext, iv[:]) - - c, err := aes.NewCipher(k.EncryptionKey[:]) - if err != nil { - panic(fmt.Sprintf("unable to create cipher: %v", err)) - } - e := cipher.NewCTR(c, ciphertext[:ivSize]) - e.XORKeyStream(ciphertext[ivSize:], plaintext) - - // truncate to only cover iv and actual ciphertext - ciphertext = ciphertext[:ivSize+len(plaintext)] - - mac := poly1305MAC(ciphertext[ivSize:], ciphertext[:ivSize], &k.MACKey) - ciphertext = append(ciphertext, mac...) - - return ciphertext, nil -} - -// Decrypt verifies and decrypts the ciphertext. Ciphertext must be in the form -// IV || Ciphertext || MAC. plaintext and ciphertext may point to (exactly) the -// same slice. -func (k *Key) Decrypt(plaintext []byte, ciphertextWithMac []byte) (int, error) { - if !k.Valid() { - return 0, errors.New("invalid key") - } - - // check for plausible length - if len(ciphertextWithMac) < Extension { - return 0, errors.Errorf("trying to decrypt invalid data: ciphertext too small") - } - - // check buffer length for plaintext - plaintextLength := len(ciphertextWithMac) - Extension - if len(plaintext) < plaintextLength { - return 0, errors.Errorf("plaintext buffer too small, %d < %d", len(plaintext), plaintextLength) - } - - // extract mac - l := len(ciphertextWithMac) - macSize - ciphertextWithIV, mac := ciphertextWithMac[:l], ciphertextWithMac[l:] - - // extract iv - iv, ciphertext := ciphertextWithIV[:ivSize], ciphertextWithIV[ivSize:] - - // verify mac - if !poly1305Verify(ciphertext, iv, &k.MACKey, mac) { - return 0, ErrUnauthenticated - } - - if len(ciphertext) != plaintextLength { - panic("plaintext and ciphertext lengths do not match") - } - - // decrypt data - c, err := aes.NewCipher(k.EncryptionKey[:]) - if err != nil { - panic(fmt.Sprintf("unable to create cipher: %v", err)) - } - e := cipher.NewCTR(c, iv) - e.XORKeyStream(plaintext, ciphertext) - - return plaintextLength, nil -} - // Valid tests if the key is valid. func (k *Key) Valid() bool { return k.EncryptionKey.Valid() && k.MACKey.Valid() From 931e6ed2ac76841931421a2317aac293263d9844 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Sun, 29 Oct 2017 11:33:57 +0100 Subject: [PATCH 4/8] Use Seal/Open everywhere --- internal/archiver/archiver_test.go | 9 +++-- internal/checker/checker.go | 6 ++-- internal/pack/pack.go | 18 ++++++---- internal/repository/key.go | 12 ++++--- internal/repository/repack.go | 9 +++-- internal/repository/repository.go | 54 +++++++++++------------------- 6 files changed, 49 insertions(+), 59 deletions(-) diff --git a/internal/archiver/archiver_test.go b/internal/archiver/archiver_test.go index 01e9d9e74..293de9152 100644 --- a/internal/archiver/archiver_test.go +++ b/internal/archiver/archiver_test.go @@ -32,6 +32,7 @@ type Rdr interface { func benchmarkChunkEncrypt(b testing.TB, buf, buf2 []byte, rd Rdr, key *crypto.Key) { rd.Seek(0, 0) ch := chunker.New(rd, testPol) + nonce := crypto.NewRandomNonce() for { chunk, err := ch.Next(buf) @@ -42,12 +43,10 @@ func benchmarkChunkEncrypt(b testing.TB, buf, buf2 []byte, rd Rdr, key *crypto.K rtest.OK(b, err) - // reduce length of buf rtest.Assert(b, uint(len(chunk.Data)) == chunk.Length, "invalid length: got %d, expected %d", len(chunk.Data), chunk.Length) - _, err = key.Encrypt(buf2, chunk.Data) - rtest.OK(b, err) + _ = key.Seal(buf2[:0], nonce, chunk.Data, nil) } } @@ -71,6 +70,7 @@ func BenchmarkChunkEncrypt(b *testing.B) { func benchmarkChunkEncryptP(b *testing.PB, buf []byte, rd Rdr, key *crypto.Key) { ch := chunker.New(rd, testPol) + nonce := crypto.NewRandomNonce() for { chunk, err := ch.Next(buf) @@ -78,8 +78,7 @@ func benchmarkChunkEncryptP(b *testing.PB, buf []byte, rd Rdr, key *crypto.Key) break } - // reduce length of chunkBuf - key.Encrypt(chunk.Data, chunk.Data) + _ = key.Seal(chunk.Data[:0], nonce, chunk.Data, nil) } } diff --git a/internal/checker/checker.go b/internal/checker/checker.go index c557ab475..efef7ae94 100644 --- a/internal/checker/checker.go +++ b/internal/checker/checker.go @@ -724,15 +724,15 @@ func checkPack(ctx context.Context, r restic.Repository, id restic.ID) error { continue } - n, err := r.Key().Decrypt(buf, buf) + nonce, ciphertext := buf[:r.Key().NonceSize()], buf[r.Key().NonceSize():] + plaintext, err := r.Key().Open(ciphertext[:0], nonce, ciphertext, nil) if err != nil { debug.Log(" error decrypting blob %v: %v", blob.ID.Str(), err) errs = append(errs, errors.Errorf("blob %v: %v", i, err)) continue } - buf = buf[:n] - hash := restic.Hash(buf) + hash := restic.Hash(plaintext) if !hash.Equal(blob.ID) { debug.Log(" Blob ID does not match, want %v, got %v", blob.ID.Str(), hash.Str()) errs = append(errs, errors.Errorf("Blob ID does not match, want %v, got %v", blob.ID.Str(), hash.Str())) diff --git a/internal/pack/pack.go b/internal/pack/pack.go index 292f9dcb0..577b4c763 100644 --- a/internal/pack/pack.go +++ b/internal/pack/pack.go @@ -75,10 +75,10 @@ func (p *Packer) Finalize() (uint, error) { return 0, err } - encryptedHeader, err := p.k.Encrypt(nil, hdrBuf.Bytes()) - if err != nil { - return 0, err - } + encryptedHeader := make([]byte, 0, hdrBuf.Len()+p.k.Overhead()+p.k.NonceSize()) + nonce := crypto.NewRandomNonce() + encryptedHeader = append(encryptedHeader, nonce...) + encryptedHeader = p.k.Seal(encryptedHeader, nonce, hdrBuf.Bytes(), nil) // append the header n, err := p.wr.Write(encryptedHeader) @@ -268,15 +268,19 @@ func List(k *crypto.Key, rd io.ReaderAt, size int64) (entries []restic.Blob, err return nil, err } - n, err := k.Decrypt(buf, buf) + if len(buf) < k.NonceSize()+k.Overhead() { + return nil, errors.New("invalid header, too small") + } + + nonce, buf := buf[:k.NonceSize()], buf[k.NonceSize():] + buf, err = k.Open(buf[:0], nonce, buf, nil) if err != nil { return nil, err } - buf = buf[:n] hdrRd := bytes.NewReader(buf) - entries = make([]restic.Blob, 0, uint(n)/entrySize) + entries = make([]restic.Blob, 0, uint(len(buf))/entrySize) pos := uint(0) for { diff --git a/internal/repository/key.go b/internal/repository/key.go index e378991cb..e29e09257 100644 --- a/internal/repository/key.go +++ b/internal/repository/key.go @@ -87,12 +87,12 @@ func OpenKey(ctx context.Context, s *Repository, name string, password string) ( } // decrypt master keys - buf := make([]byte, len(k.Data)) - n, err := k.user.Decrypt(buf, k.Data) + nonce, ciphertext := k.Data[:k.user.NonceSize()], k.Data[k.user.NonceSize():] + buf := make([]byte, 0, len(ciphertext)) + buf, err = k.user.Open(buf, nonce, ciphertext, nil) if err != nil { return nil, err } - buf = buf[:n] // restore json k.master = &crypto.Key{} @@ -221,7 +221,11 @@ func AddKey(ctx context.Context, s *Repository, password string, template *crypt return nil, errors.Wrap(err, "Marshal") } - newkey.Data, err = newkey.user.Encrypt(nil, buf) + nonce := crypto.NewRandomNonce() + ciphertext := make([]byte, 0, len(buf)+newkey.user.Overhead()+newkey.user.NonceSize()) + ciphertext = append(ciphertext, nonce...) + ciphertext = newkey.user.Seal(ciphertext, nonce, buf, nil) + newkey.Data = ciphertext // dump as json buf, err = json.Marshal(newkey) diff --git a/internal/repository/repack.go b/internal/repository/repack.go index 3bba26803..3eacea182 100644 --- a/internal/repository/repack.go +++ b/internal/repository/repack.go @@ -90,14 +90,13 @@ func Repack(ctx context.Context, repo restic.Repository, packs restic.IDSet, kee h, tempfile.Name(), len(buf), n) } - n, err = repo.Key().Decrypt(buf, buf) + nonce, ciphertext := buf[:repo.Key().NonceSize()], buf[repo.Key().NonceSize():] + plaintext, err := repo.Key().Open(ciphertext[:0], nonce, ciphertext, nil) if err != nil { return nil, err } - buf = buf[:n] - - id := restic.Hash(buf) + id := restic.Hash(plaintext) if !id.Equal(entry.ID) { debug.Log("read blob %v/%v from %v: wrong data returned, hash is %v", h.Type, h.ID, tempfile.Name(), id) @@ -105,7 +104,7 @@ func Repack(ctx context.Context, repo restic.Repository, packs restic.IDSet, kee h, tempfile.Name(), id) } - _, err = repo.SaveBlob(ctx, entry.Type, buf, entry.ID) + _, err = repo.SaveBlob(ctx, entry.Type, plaintext, entry.ID) if err != nil { return nil, err } diff --git a/internal/repository/repository.go b/internal/repository/repository.go index 161eb8734..9bf686548 100644 --- a/internal/repository/repository.go +++ b/internal/repository/repository.go @@ -79,13 +79,13 @@ func (r *Repository) LoadAndDecrypt(ctx context.Context, t restic.FileType, id r return nil, errors.Errorf("load %v: invalid data returned", h) } - // decrypt - n, err := r.decryptTo(buf, buf) + nonce, ciphertext := buf[:r.key.NonceSize()], buf[r.key.NonceSize():] + plaintext, err := r.key.Open(ciphertext[:0], nonce, ciphertext, nil) if err != nil { return nil, err } - return buf[:n], nil + return plaintext, nil } // sortCachedPacks moves all cached pack files to the front of blobs. @@ -156,20 +156,22 @@ func (r *Repository) loadBlob(ctx context.Context, id restic.ID, t restic.BlobTy } // decrypt - n, err = r.decryptTo(plaintextBuf, plaintextBuf) + nonce, ciphertext := plaintextBuf[:r.key.NonceSize()], plaintextBuf[r.key.NonceSize():] + plaintext, err := r.key.Open(ciphertext[:0], nonce, ciphertext, nil) if err != nil { lastError = errors.Errorf("decrypting blob %v failed: %v", id, err) continue } - plaintextBuf = plaintextBuf[:n] // check hash - if !restic.Hash(plaintextBuf).Equal(id) { + if !restic.Hash(plaintext).Equal(id) { lastError = errors.Errorf("blob %v returned invalid hash", id) continue } - return len(plaintextBuf), nil + // move decrypted data to the start of the provided buffer + copy(plaintextBuf[0:], plaintext) + return len(plaintext), nil } if lastError != nil { @@ -210,11 +212,12 @@ func (r *Repository) SaveAndEncrypt(ctx context.Context, t restic.BlobType, data ciphertext := getBuf() defer freeBuf(ciphertext) + ciphertext = ciphertext[:0] + nonce := crypto.NewRandomNonce() + ciphertext = append(ciphertext, nonce...) + // encrypt blob - ciphertext, err := r.Encrypt(ciphertext, data) - if err != nil { - return restic.ID{}, err - } + ciphertext = r.key.Seal(ciphertext, nonce, data, nil) // find suitable packer and add blob var pm *packerManager @@ -266,10 +269,11 @@ func (r *Repository) SaveJSONUnpacked(ctx context.Context, t restic.FileType, it // storage hash. func (r *Repository) SaveUnpacked(ctx context.Context, t restic.FileType, p []byte) (id restic.ID, err error) { ciphertext := restic.NewBlobBuffer(len(p)) - ciphertext, err = r.Encrypt(ciphertext, p) - if err != nil { - return restic.ID{}, err - } + ciphertext = ciphertext[:0] + nonce := crypto.NewRandomNonce() + ciphertext = append(ciphertext, nonce...) + + ciphertext = r.key.Seal(ciphertext, nonce, p, nil) id = restic.Hash(ciphertext) h := restic.Handle{Type: t, Name: id.String()} @@ -522,26 +526,6 @@ func (r *Repository) init(ctx context.Context, password string, cfg restic.Confi return err } -// decrypt authenticates and decrypts ciphertext and stores the result in -// plaintext. -func (r *Repository) decryptTo(plaintext, ciphertext []byte) (int, error) { - if r.key == nil { - return 0, errors.New("key for repository not set") - } - - return r.key.Decrypt(plaintext, ciphertext) -} - -// Encrypt encrypts and authenticates the plaintext and saves the result in -// ciphertext. -func (r *Repository) Encrypt(ciphertext, plaintext []byte) ([]byte, error) { - if r.key == nil { - return nil, errors.New("key for repository not set") - } - - return r.key.Encrypt(ciphertext, plaintext) -} - // Key returns the current master key. func (r *Repository) Key() *crypto.Key { return r.key From ba43c8bab52486e35ba5816644a014df402d37d0 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 1 Nov 2017 09:34:00 +0100 Subject: [PATCH 5/8] crypto: Fix nonce test, make it faster --- internal/crypto/crypto.go | 6 +++--- internal/crypto/crypto_int_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index 07ea3cc43..01cd82c45 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -237,9 +237,9 @@ var ErrInvalidCiphertext = errors.New("invalid ciphertext, same slice used for p // validNonce checks that nonce is not all zero. func validNonce(nonce []byte) bool { - sum := 0 - for b := range nonce { - sum += b + var sum byte + for _, b := range nonce { + sum |= b } return sum > 0 } diff --git a/internal/crypto/crypto_int_test.go b/internal/crypto/crypto_int_test.go index 9473d1382..769f34d1e 100644 --- a/internal/crypto/crypto_int_test.go +++ b/internal/crypto/crypto_int_test.go @@ -163,3 +163,30 @@ func TestCrypto(t *testing.T) { } } } + +func TestNonceVadlid(t *testing.T) { + nonce := make([]byte, ivSize) + + if validNonce(nonce) { + t.Error("null nonce detected as valid") + } + + for i := 0; i < 100; i++ { + nonce = NewRandomNonce() + if !validNonce(nonce) { + t.Errorf("random nonce not detected as valid: %02x", nonce) + } + } +} + +func BenchmarkNonceValid(b *testing.B) { + nonce := NewRandomNonce() + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + if !validNonce(nonce) { + b.Fatal("nonce is invalid") + } + } +} From 2a67d7a6c2c260ef46a63216850fd33ffcc55154 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 1 Nov 2017 09:46:05 +0100 Subject: [PATCH 6/8] crypto: Correct test function names --- internal/crypto/crypto_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/crypto/crypto_test.go b/internal/crypto/crypto_test.go index 94f22588b..47debe896 100644 --- a/internal/crypto/crypto_test.go +++ b/internal/crypto/crypto_test.go @@ -125,7 +125,7 @@ func decryptAndCompare(t testing.TB, k *crypto.Key, data, ciphertext, nonce, dst } } -func TestAppendSeal(t *testing.T) { +func TestAppendOpen(t *testing.T) { k := crypto.NewRandomKey() nonce := crypto.NewRandomNonce() @@ -174,7 +174,7 @@ func TestAppendSeal(t *testing.T) { }) } -func TestAppendOpen(t *testing.T) { +func TestAppendSeal(t *testing.T) { k := crypto.NewRandomKey() data := make([]byte, 600) From bb435b39d9ffc3276d5895e81c0e1f796f0074e1 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 1 Nov 2017 09:50:46 +0100 Subject: [PATCH 7/8] crypto: Rework Seal/Open to use sliceForAppend --- internal/crypto/crypto.go | 58 +++++++++++++++++---------------------- 1 file changed, 25 insertions(+), 33 deletions(-) diff --git a/internal/crypto/crypto.go b/internal/crypto/crypto.go index 01cd82c45..b56a9da2e 100644 --- a/internal/crypto/crypto.go +++ b/internal/crypto/crypto.go @@ -259,6 +259,23 @@ func (k *Key) Overhead() int { return macSize } +// sliceForAppend takes a slice and a requested number of bytes. It returns a +// slice with the contents of the given slice followed by that many bytes and a +// second slice that aliases into it and contains only the extra bytes. If the +// original slice has sufficient capacity then no allocation is performed. +// +// taken from the stdlib, crypto/aes/aes_gcm.go +func sliceForAppend(in []byte, n int) (head, tail []byte) { + if total := len(in) + n; cap(in) >= total { + head = in[:total] + } else { + head = make([]byte, total) + copy(head, in) + } + tail = head[len(in):] + return +} + // Seal encrypts and authenticates plaintext, authenticates the // additional data and appends the result to dst, returning the updated // slice. The nonce must be NonceSize() bytes long and unique for all @@ -283,32 +300,19 @@ func (k *Key) Seal(dst, nonce, plaintext, additionalData []byte) []byte { panic("nonce is invalid") } - // extend dst so that the ciphertext fits - ciphertextLength := len(plaintext) + k.Overhead() - pos := len(dst) - - capacity := cap(dst) - len(dst) - if capacity < ciphertextLength { - dst = dst[:cap(dst)] - dst = append(dst, make([]byte, ciphertextLength-capacity)...) - } else { - dst = dst[:pos+ciphertextLength] - } + ret, out := sliceForAppend(dst, len(plaintext)+k.Overhead()) c, err := aes.NewCipher(k.EncryptionKey[:]) if err != nil { panic(fmt.Sprintf("unable to create cipher: %v", err)) } e := cipher.NewCTR(c, nonce) - e.XORKeyStream(dst[pos:pos+len(plaintext)], plaintext) + e.XORKeyStream(out, plaintext) - // truncate to only cover the ciphertext - dst = dst[:pos+len(plaintext)] + mac := poly1305MAC(out[:len(plaintext)], nonce, &k.MACKey) + copy(out[len(plaintext):], mac) - mac := poly1305MAC(dst[pos:], nonce, &k.MACKey) - dst = append(dst, mac...) - - return dst + return ret } // Open decrypts and authenticates ciphertext, authenticates the @@ -341,7 +345,6 @@ func (k *Key) Open(dst, nonce, ciphertext, additionalData []byte) ([]byte, error return nil, errors.Errorf("trying to decrypt invalid data: ciphertext too small") } - // extract mac l := len(ciphertext) - macSize ct, mac := ciphertext[:l], ciphertext[l:] @@ -350,27 +353,16 @@ func (k *Key) Open(dst, nonce, ciphertext, additionalData []byte) ([]byte, error return nil, ErrUnauthenticated } - // extend dst so that the plaintext fits - plaintextLength := len(ct) - pos := len(dst) + ret, out := sliceForAppend(dst, len(ct)) - capacity := cap(dst) - len(dst) - if capacity < plaintextLength { - dst = dst[:cap(dst)] - dst = append(dst, make([]byte, plaintextLength-capacity)...) - } else { - dst = dst[:pos+plaintextLength] - } - - // decrypt data c, err := aes.NewCipher(k.EncryptionKey[:]) if err != nil { panic(fmt.Sprintf("unable to create cipher: %v", err)) } e := cipher.NewCTR(c, nonce) - e.XORKeyStream(dst[pos:], ct) + e.XORKeyStream(out, ct) - return dst, nil + return ret, nil } // Valid tests if the key is valid. From 6d46824fb0e150c2f90690c08e7de85612ba4e30 Mon Sep 17 00:00:00 2001 From: Alexander Neumann Date: Wed, 1 Nov 2017 09:58:59 +0100 Subject: [PATCH 8/8] Pass in a nil buffer to Open() --- internal/repository/key.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal/repository/key.go b/internal/repository/key.go index e29e09257..781670bad 100644 --- a/internal/repository/key.go +++ b/internal/repository/key.go @@ -88,8 +88,7 @@ func OpenKey(ctx context.Context, s *Repository, name string, password string) ( // decrypt master keys nonce, ciphertext := k.Data[:k.user.NonceSize()], k.Data[k.user.NonceSize():] - buf := make([]byte, 0, len(ciphertext)) - buf, err = k.user.Open(buf, nonce, ciphertext, nil) + buf, err := k.user.Open(nil, nonce, ciphertext, nil) if err != nil { return nil, err }