From 9174fb7892e1e4a95673f80f11f9a77f71ebe277 Mon Sep 17 00:00:00 2001 From: Zaki Manian Date: Thu, 14 Nov 2019 01:45:17 -0800 Subject: [PATCH] p2p: make SecretConnection non-malleable (#3668) ## Issue: This is an approach to fixing secret connection that is more noise-ish than actually noise. but it essentially fixes the problem that #3315 is trying to solve by making the secret connection handshake non-malleable. It's easy to understand and I think will be acceptable to @jaekwon .. the formal reasoning is basically, if the "view" of the transcript between diverges between the sender and the receiver at any point in the protocol, the handshake would terminate. The base protocol of Station to Station mistakenly assumes that if the sender and receiver arrive at shared secret they have the same view. This is only true for a DH on prime order groups. This robustly solves the problem by having each cryptographic operation commit to operators view of the protocol. Another nice thing about a transcript is it provides the basis for "secure" (barring cryptographic breakages, horrible design flaws, or implementation bugs) downgrades, where a backwards compatible handshake can be used to offer newer protocol features/extensions, peers agree to the common subset of what they support, and both sides have to agree on what the other offered for the transcript MAC to verify. With something like Protos/Amino you already get "extensions" for free (TLS uses a simple TLV format https://tools.ietf.org/html/rfc8446#section-4.2 for extensions not too far off from Protos/Amino), so as long as you cryptographically commit to what they contain in the transcript, it should be possible to extend the protocol in a backwards-compatible manner. ## Commits: * Minimal changes to remove malleability of secret connection removes the need to check for lower order points. Breaks compatibility. Secret connections that have no been updated will fail * Remove the redundant blacklist * remove remainders of blacklist in tests to make the code compile again Signed-off-by: Ismail Khoffi * Apply suggestions from code review Apply Ismail's error handling Co-Authored-By: Ismail Khoffi * fix error check for io.ReadFull Signed-off-by: Ismail Khoffi * Update p2p/conn/secret_connection.go Co-Authored-By: Ismail Khoffi * Update p2p/conn/secret_connection.go Co-Authored-By: Bot from GolangCI <42910462+golangcibot@users.noreply.github.com> * update changelog and format the code * move hkdfInit closer to where it's used --- CHANGELOG_PENDING.md | 3 + p2p/conn/secret_connection.go | 96 +++++++++++------------------- p2p/conn/secret_connection_test.go | 45 -------------- 3 files changed, 39 insertions(+), 105 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 37413640c..626768293 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -32,6 +32,9 @@ program](https://hackerone.com/tendermint). - Go API - [libs/pubsub] [\#4070](https://github.com/tendermint/tendermint/pull/4070) `Query#(Matches|Conditions)` returns an error. +- P2P Protocol + - [p2p] [\3668](https://github.com/tendermint/tendermint/pull/3668) Make `SecretConnection` non-malleable + ### FEATURES: ### IMPROVEMENTS: diff --git a/p2p/conn/secret_connection.go b/p2p/conn/secret_connection.go index 6dfa02e53..9d6825df8 100644 --- a/p2p/conn/secret_connection.go +++ b/p2p/conn/secret_connection.go @@ -77,24 +77,40 @@ type SecretConnection struct { // Caller should call conn.Close() // See docs/sts-final.pdf for more information. func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (*SecretConnection, error) { - locPubKey := locPrivKey.PubKey() + var ( + hash = sha256.New + hdkfState = new([32]byte) + locPubKey = locPrivKey.PubKey() + ) // Generate ephemeral keys for perfect forward secrecy. locEphPub, locEphPriv := genEphKeys() // Write local ephemeral pubkey and receive one too. - // NOTE: every 32-byte string is accepted as a Curve25519 public key - // (see DJB's Curve25519 paper: http://cr.yp.to/ecdh/curve25519-20060209.pdf) + // NOTE: every 32-byte string is accepted as a Curve25519 public key (see + // DJB's Curve25519 paper: http://cr.yp.to/ecdh/curve25519-20060209.pdf) remEphPub, err := shareEphPubKey(conn, locEphPub) if err != nil { return nil, err } // Sort by lexical order. - loEphPub, _ := sort32(locEphPub, remEphPub) + loEphPub, hiEphPub := sort32(locEphPub, remEphPub) + + hkdfInit := hkdf.New(hash, []byte("INIT_HANDSHAKE"), nil, []byte("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH")) + if _, err := io.ReadFull(hkdfInit, hdkfState[:]); err != nil { + return nil, err + } + + hkdfLoEphPub := hkdf.New(hash, loEphPub[:], hdkfState[:], []byte("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH")) + if _, err := io.ReadFull(hkdfLoEphPub, hdkfState[:]); err != nil { + return nil, err + } + + hkdfHiEphPub := hkdf.New(hash, hiEphPub[:], hdkfState[:], []byte("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH")) - // Check if the local ephemeral public key - // was the least, lexicographically sorted. + // Check if the local ephemeral public key was the least, lexicographically + // sorted. locIsLeast := bytes.Equal(locEphPub[:], loEphPub[:]) // Compute common diffie hellman secret using X25519. @@ -102,9 +118,19 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (* if err != nil { return nil, err } + if _, err := io.ReadFull(hkdfHiEphPub, hdkfState[:]); err != nil { + return nil, err + } + + hkdfSecret := hkdf.New(hash, dhSecret[:], hdkfState[:], []byte("TENDERMINT_SECRET_CONNECTION_TRANSCRIPT_HASH")) + if _, err := io.ReadFull(hkdfSecret, hdkfState[:]); err != nil { + return nil, err + } - // generate the secret used for receiving, sending, challenge via hkdf-sha2 on dhSecret - recvSecret, sendSecret, challenge := deriveSecretAndChallenge(dhSecret, locIsLeast) + // Generate the secret used for receiving, sending, challenge via HKDF-SHA2 + // on the transcript state (which itself also uses HKDF-SHA2 to derive a key + // from the dhSecret). + recvSecret, sendSecret, challenge := deriveSecretAndChallenge(hdkfState, locIsLeast) sendAead, err := chacha20poly1305.New(sendSecret[:]) if err != nil { @@ -114,7 +140,7 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (* if err != nil { return nil, errors.New("invalid receive SecretConnection Key") } - // Construct SecretConnection. + sc := &SecretConnection{ conn: conn, recvBuffer: nil, @@ -134,11 +160,9 @@ func MakeSecretConnection(conn io.ReadWriteCloser, locPrivKey crypto.PrivKey) (* } remPubKey, remSignature := authSigMsg.Key, authSigMsg.Sig - if _, ok := remPubKey.(ed25519.PubKeyEd25519); !ok { return nil, errors.Errorf("expected ed25519 pubkey, got %T", remPubKey) } - if !remPubKey.VerifyBytes(challenge[:], remSignature) { return nil, errors.New("challenge verification failed") } @@ -285,9 +309,7 @@ func shareEphPubKey(conn io.ReadWriter, locEphPub *[32]byte) (remEphPub *[32]byt if err2 != nil { return nil, err2, true // abort } - if hasSmallOrder(_remEphPub) { - return nil, ErrSmallOrderRemotePubKey, true - } + return _remEphPub, nil, false }, ) @@ -303,52 +325,6 @@ func shareEphPubKey(conn io.ReadWriter, locEphPub *[32]byte) (remEphPub *[32]byt return &_remEphPub, nil } -// use the samne blacklist as lib sodium (see https://eprint.iacr.org/2017/806.pdf for reference): -// https://github.com/jedisct1/libsodium/blob/536ed00d2c5e0c65ac01e29141d69a30455f2038/src/libsodium/crypto_scalarmult/curve25519/ref10/x25519_ref10.c#L11-L17 -var blacklist = [][32]byte{ - // 0 (order 4) - {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, - // 1 (order 1) - {0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00}, - // 325606250916557431795983626356110631294008115727848805560023387167927233504 - // (order 8) - {0xe0, 0xeb, 0x7a, 0x7c, 0x3b, 0x41, 0xb8, 0xae, 0x16, 0x56, 0xe3, - 0xfa, 0xf1, 0x9f, 0xc4, 0x6a, 0xda, 0x09, 0x8d, 0xeb, 0x9c, 0x32, - 0xb1, 0xfd, 0x86, 0x62, 0x05, 0x16, 0x5f, 0x49, 0xb8, 0x00}, - // 39382357235489614581723060781553021112529911719440698176882885853963445705823 - // (order 8) - {0x5f, 0x9c, 0x95, 0xbc, 0xa3, 0x50, 0x8c, 0x24, 0xb1, 0xd0, 0xb1, - 0x55, 0x9c, 0x83, 0xef, 0x5b, 0x04, 0x44, 0x5c, 0xc4, 0x58, 0x1c, - 0x8e, 0x86, 0xd8, 0x22, 0x4e, 0xdd, 0xd0, 0x9f, 0x11, 0x57}, - // p-1 (order 2) - {0xec, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f}, - // p (=0, order 4) - {0xed, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f}, - // p+1 (=1, order 1) - {0xee, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x7f}, -} - -func hasSmallOrder(pubKey [32]byte) bool { - isSmallOrderPoint := false - for _, bl := range blacklist { - if subtle.ConstantTimeCompare(pubKey[:], bl[:]) == 1 { - isSmallOrderPoint = true - break - } - } - return isSmallOrderPoint -} - func deriveSecretAndChallenge( dhSecret *[32]byte, locIsLeast bool, diff --git a/p2p/conn/secret_connection_test.go b/p2p/conn/secret_connection_test.go index 0438ec259..e0d5f4fac 100644 --- a/p2p/conn/secret_connection_test.go +++ b/p2p/conn/secret_connection_test.go @@ -101,51 +101,6 @@ 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: - for _, remLowOrderPubKey := range blacklist { - remLowOrderPubKey := remLowOrderPubKey - _, _ = cmn.Parallel( - func(_ int) (val interface{}, err error, abort bool) { - _, err = shareEphPubKey(fooConn, locEphPub) - - require.Error(t, err) - require.Equal(t, err, ErrSmallOrderRemotePubKey) - - return nil, nil, false - }, - func(_ int) (val interface{}, err error, abort bool) { - readRemKey, err := shareEphPubKey(barConn, &remLowOrderPubKey) - - require.NoError(t, err) - require.Equal(t, locEphPub, readRemKey) - - return nil, nil, false - }) - } -} - -// 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 { - remLowOrderPubKey := remLowOrderPubKey - 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)