From ad180b4fba24f87f681c722643367e1196da2af1 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 13 Dec 2016 11:31:47 -0800 Subject: [PATCH] brontide: fix bug in final sender/receiver key derivation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes a bug in our key derivation for the final step of the key exchange. In our code we were swapping the order of the salt and input keyeing material to the HKDF function. This was triggered by the argument order of the golang implementation we’re currently using has the “secret” of IKM argument first, instead of second as defined within rfc5869. To fix this, we simply need to swap function arguments in two places: within the split() function and during key rotation. This bug was discovered by Rusty Russell, thanks! --- brontide/noise.go | 7 +++---- brontide/noise_test.go | 7 +------ 2 files changed, 4 insertions(+), 10 deletions(-) diff --git a/brontide/noise.go b/brontide/noise.go index 4c4ab1ea..9fdc794d 100644 --- a/brontide/noise.go +++ b/brontide/noise.go @@ -142,7 +142,7 @@ func (c *cipherState) rotateKey() { ) oldKey := c.secretKey - h := hkdf.New(sha256.New, c.salt[:], oldKey[:], info) + h := hkdf.New(sha256.New, oldKey[:], c.salt[:], info) // hkdf(ck, k, zero) // | @@ -192,7 +192,7 @@ func (s *symmetricState) mixKey(input []byte) { salt := s.chainingKey h := hkdf.New(sha256.New, secret, salt[:], info) - // hkdf(input, ck, zero) + // hkdf(ck, input, zero) // | // | \ // | \ @@ -535,7 +535,6 @@ func (b *BrontideMachine) GenActThree() ([ActThreeSize]byte, error) { ourPubkey := b.localStatic.PubKey().SerializeCompressed() ciphertext := b.EncryptAndHash(ourPubkey) - s := ecdh(b.remoteEphemeral, b.localStatic) b.mixKey(s) @@ -610,7 +609,7 @@ func (b *BrontideMachine) split() { recvKey [32]byte ) - h := hkdf.New(sha256.New, b.chainingKey[:], empty, empty) + h := hkdf.New(sha256.New, empty, b.chainingKey[:], empty) // If we're the initiator the first 32 bytes are used to encrypt our // messages and the second 32-bytes to decrypt their messages. For the diff --git a/brontide/noise_test.go b/brontide/noise_test.go index 3c4e5841..5ea6f14c 100644 --- a/brontide/noise_test.go +++ b/brontide/noise_test.go @@ -63,7 +63,6 @@ func establishTestConnection() (net.Conn, net.Conn, error) { return localConn, remoteConn, nil } - func TestConnectionCorrectness(t *testing.T) { // Create a test connection, grabbing either side of the connection // into local variables. If the initial crypto handshake fails, then @@ -164,7 +163,7 @@ func TestWriteMessageChunking(t *testing.T) { // size. largeMessage := bytes.Repeat([]byte("kek"), math.MaxUint16*3) - // Launch a new goroutine to write the lerge message generated above in + // Launch a new goroutine to write the large message generated above in // chunks. We spawn a new goroutine because otherwise, we may block as // the kernal waits for the buffer to flush. var wg sync.WaitGroup @@ -198,7 +197,3 @@ func TestWriteMessageChunking(t *testing.T) { t.Fatalf("bytes don't match") } } - -func TestNoiseIdentityHiding(t *testing.T) { - // TODO(roasbeef): fin -}