From e0adc5e807c8c2ea4ac03f8eeeb006b1a154d453 Mon Sep 17 00:00:00 2001 From: Ismail Khoffi Date: Sat, 23 Feb 2019 16:25:57 +0100 Subject: [PATCH] secret connection check all zeroes (#3347) * reject the shared secret if is all zeros in case the blacklist was not sufficient * Add test that verifies lower order pub-keys are rejected at the DH step * Update changelog * fix typo in test-comment --- CHANGELOG_PENDING.md | 4 +++- p2p/conn/secret_connection.go | 28 ++++++++++++++++++++++++---- p2p/conn/secret_connection_test.go | 17 +++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index c123f1c34..65fc4bd25 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -22,4 +22,6 @@ Special thanks to external contributors on this release: ### IMPROVEMENTS: ### BUG FIXES: -- [libs/pubsub] \#951, \#1880 use non-blocking send when dispatching messages [ADR-33](https://github.com/tendermint/tendermint/blob/develop/docs/architecture/adr-033-pubsub.md) \ No newline at end of file + +- [p2p/conn] \#3347 Reject all-zero shared secrets in the Diffie-Hellman step of secret-connection +- [libs/pubsub] \#951, \#1880 use non-blocking send when dispatching messages [ADR-33](https://github.com/tendermint/tendermint/blob/develop/docs/architecture/adr-033-pubsub.md) diff --git a/p2p/conn/secret_connection.go b/p2p/conn/secret_connection.go index d2ba6fb51..2a06945b0 100644 --- a/p2p/conn/secret_connection.go +++ b/p2p/conn/secret_connection.go @@ -29,7 +29,10 @@ const aeadSizeOverhead = 16 // overhead of poly 1305 authentication tag const aeadKeySize = chacha20poly1305.KeySize const aeadNonceSize = chacha20poly1305.NonceSize -var ErrSmallOrderRemotePubKey = errors.New("detected low order point from remote peer") +var ( + ErrSmallOrderRemotePubKey = errors.New("detected low order point from remote peer") + ErrSharedSecretIsZero = errors.New("shared secret is all zeroes") +) // SecretConnection implements net.Conn. // It is an implementation of the STS protocol. @@ -90,7 +93,10 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (* locIsLeast := bytes.Equal(locEphPub[:], loEphPub[:]) // Compute common diffie hellman secret using X25519. - dhSecret := computeDHSecret(remEphPub, locEphPriv) + dhSecret, err := computeDHSecret(remEphPub, locEphPriv) + if err != nil { + return nil, err + } // generate the secret used for receiving, sending, challenge via hkdf-sha2 on dhSecret recvSecret, sendSecret, challenge := deriveSecretAndChallenge(dhSecret, locIsLeast) @@ -230,9 +236,12 @@ func (sc *SecretConnection) SetWriteDeadline(t time.Time) error { func genEphKeys() (ephPub, ephPriv *[32]byte) { var err error + // TODO: Probably not a problem but ask Tony: different from the rust implementation (uses x25519-dalek), + // we do not "clamp" the private key scalar: + // see: https://github.com/dalek-cryptography/x25519-dalek/blob/34676d336049df2bba763cc076a75e47ae1f170f/src/x25519.rs#L56-L74 ephPub, ephPriv, err = box.GenerateKey(crand.Reader) if err != nil { - panic("Could not generate ephemeral keypairs") + panic("Could not generate ephemeral key-pair") } return } @@ -349,9 +358,20 @@ func deriveSecretAndChallenge(dhSecret *[32]byte, locIsLeast bool) (recvSecret, return } -func computeDHSecret(remPubKey, locPrivKey *[32]byte) (shrKey *[32]byte) { +// computeDHSecret computes a shared secret Diffie-Hellman secret +// from a the own local private key and the others public key. +// +// It returns an error if the computed shared secret is all zeroes. +func computeDHSecret(remPubKey, locPrivKey *[32]byte) (shrKey *[32]byte, err error) { shrKey = new([32]byte) curve25519.ScalarMult(shrKey, locPrivKey, remPubKey) + + // reject if the returned shared secret is all zeroes + // related to: https://github.com/tendermint/tendermint/issues/3010 + zero := new([32]byte) + if subtle.ConstantTimeCompare(shrKey[:], zero[:]) == 1 { + return nil, ErrSharedSecretIsZero + } return } diff --git a/p2p/conn/secret_connection_test.go b/p2p/conn/secret_connection_test.go index 5c389ee3a..7e264e913 100644 --- a/p2p/conn/secret_connection_test.go +++ b/p2p/conn/secret_connection_test.go @@ -100,8 +100,12 @@ func TestSecretConnectionHandshake(t *testing.T) { } } +// Test that shareEphPubKey rejects lower order public keys based on an +// (incomplete) blacklist. func TestShareLowOrderPubkey(t *testing.T) { var fooConn, barConn = makeKVStoreConnPair() + defer fooConn.Close() + defer barConn.Close() locEphPub, _ := genEphKeys() // all blacklisted low order points: @@ -126,6 +130,19 @@ func TestShareLowOrderPubkey(t *testing.T) { } } +// Test that additionally that the Diffie-Hellman shared secret is non-zero. +// The shared secret would be zero for lower order pub-keys (but tested against the blacklist only). +func TestComputeDHFailsOnLowOrder(t *testing.T) { + _, locPrivKey := genEphKeys() + for _, remLowOrderPubKey := range blacklist { + shared, err := computeDHSecret(&remLowOrderPubKey, locPrivKey) + assert.Error(t, err) + + assert.Equal(t, err, ErrSharedSecretIsZero) + assert.Empty(t, shared) + } +} + func TestConcurrentWrite(t *testing.T) { fooSecConn, barSecConn := makeSecretConnPair(t) fooWriteText := cmn.RandStr(dataMaxSize)