From 3df2ca128d12a20c63fb05fd20d37132b7576760 Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Wed, 25 Oct 2017 19:56:08 -0700 Subject: [PATCH 1/2] make PrivateKey + Signature comparisons use constant time comparisons Fixes https://github.com/tendermint/go-crypto/issues/43 Avoid susceptibility to timing/side channel attacks by ensuring that private key and signature comparisons use `subtle.ConstantTimeCompare` instead of `bytes.Equal` --- priv_key.go | 14 +++++++++++--- signature.go | 14 +++++++++++--- 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/priv_key.go b/priv_key.go index 0c6bd2ae7..456985615 100644 --- a/priv_key.go +++ b/priv_key.go @@ -1,7 +1,7 @@ package crypto import ( - "bytes" + "crypto/subtle" secp256k1 "github.com/btcsuite/btcd/btcec" "github.com/tendermint/ed25519" @@ -57,7 +57,11 @@ func (privKey PrivKeyEd25519) PubKey() PubKey { func (privKey PrivKeyEd25519) Equals(other PrivKey) bool { if otherEd, ok := other.Unwrap().(PrivKeyEd25519); ok { - return bytes.Equal(privKey[:], otherEd[:]) + // It is essential that we constant time compare + // private keys and signatures instead of bytes.Equal, + // to avoid susceptibility to timing/side channel attacks. + // See Issue https://github.com/tendermint/go-crypto/issues/43 + return subtle.ConstantTimeCompare(privKey[:], otherEd[:]) == 0 } else { return false } @@ -144,7 +148,11 @@ func (privKey PrivKeySecp256k1) PubKey() PubKey { func (privKey PrivKeySecp256k1) Equals(other PrivKey) bool { if otherSecp, ok := other.Unwrap().(PrivKeySecp256k1); ok { - return bytes.Equal(privKey[:], otherSecp[:]) + // It is essential that we constant time compare + // private keys and signatures instead of bytes.Equal, + // to avoid susceptibility to timing/side channel attacks. + // See Issue https://github.com/tendermint/go-crypto/issues/43 + return subtle.ConstantTimeCompare(privKey[:], otherSecp[:]) == 0 } else { return false } diff --git a/signature.go b/signature.go index 5b1d6cb05..fb07aba96 100644 --- a/signature.go +++ b/signature.go @@ -1,7 +1,7 @@ package crypto import ( - "bytes" + "crypto/subtle" "fmt" "github.com/tendermint/go-wire" @@ -46,7 +46,11 @@ func (sig SignatureEd25519) String() string { return fmt.Sprintf("/%X.../", Fing func (sig SignatureEd25519) Equals(other Signature) bool { if otherEd, ok := other.Unwrap().(SignatureEd25519); ok { - return bytes.Equal(sig[:], otherEd[:]) + // It is essential that we constant time compare + // private keys and signatures instead of bytes.Equal, + // to avoid susceptibility to timing/side channel attacks. + // See Issue https://github.com/tendermint/go-crypto/issues/43 + return subtle.ConstantTimeCompare(sig[:], otherEd[:]) == 0 } else { return false } @@ -82,7 +86,11 @@ func (sig SignatureSecp256k1) String() string { return fmt.Sprintf("/%X.../", Fi func (sig SignatureSecp256k1) Equals(other Signature) bool { if otherEd, ok := other.Unwrap().(SignatureSecp256k1); ok { - return bytes.Equal(sig[:], otherEd[:]) + // It is essential that we constant time compare + // private keys and signatures instead of bytes.Equal, + // to avoid susceptibility to timing/side channel attacks. + // See Issue https://github.com/tendermint/go-crypto/issues/43 + return subtle.ConstantTimeCompare(sig[:], otherEd[:]) == 0 } else { return false } From eaf4b8c79500e7be9b227c5dca8b9813bb75842d Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 21 Dec 2017 19:51:57 -0500 Subject: [PATCH 2/2] fix Equals --- priv_key.go | 16 ++++++---------- signature.go | 16 ++++------------ signature_test.go | 24 ++++++++++++++++++++++++ 3 files changed, 34 insertions(+), 22 deletions(-) diff --git a/priv_key.go b/priv_key.go index 456985615..613d94593 100644 --- a/priv_key.go +++ b/priv_key.go @@ -55,13 +55,11 @@ func (privKey PrivKeyEd25519) PubKey() PubKey { return PubKeyEd25519(pubBytes).Wrap() } +// Equals - you probably don't need to use this. +// Runs in constant time based on length of the keys. func (privKey PrivKeyEd25519) Equals(other PrivKey) bool { if otherEd, ok := other.Unwrap().(PrivKeyEd25519); ok { - // It is essential that we constant time compare - // private keys and signatures instead of bytes.Equal, - // to avoid susceptibility to timing/side channel attacks. - // See Issue https://github.com/tendermint/go-crypto/issues/43 - return subtle.ConstantTimeCompare(privKey[:], otherEd[:]) == 0 + return subtle.ConstantTimeCompare(privKey[:], otherEd[:]) == 1 } else { return false } @@ -146,13 +144,11 @@ func (privKey PrivKeySecp256k1) PubKey() PubKey { return pub.Wrap() } +// Equals - you probably don't need to use this. +// Runs in constant time based on length of the keys. func (privKey PrivKeySecp256k1) Equals(other PrivKey) bool { if otherSecp, ok := other.Unwrap().(PrivKeySecp256k1); ok { - // It is essential that we constant time compare - // private keys and signatures instead of bytes.Equal, - // to avoid susceptibility to timing/side channel attacks. - // See Issue https://github.com/tendermint/go-crypto/issues/43 - return subtle.ConstantTimeCompare(privKey[:], otherSecp[:]) == 0 + return subtle.ConstantTimeCompare(privKey[:], otherSecp[:]) == 1 } else { return false } diff --git a/signature.go b/signature.go index fb07aba96..be1f24905 100644 --- a/signature.go +++ b/signature.go @@ -1,7 +1,7 @@ package crypto import ( - "crypto/subtle" + "bytes" "fmt" "github.com/tendermint/go-wire" @@ -46,11 +46,7 @@ func (sig SignatureEd25519) String() string { return fmt.Sprintf("/%X.../", Fing func (sig SignatureEd25519) Equals(other Signature) bool { if otherEd, ok := other.Unwrap().(SignatureEd25519); ok { - // It is essential that we constant time compare - // private keys and signatures instead of bytes.Equal, - // to avoid susceptibility to timing/side channel attacks. - // See Issue https://github.com/tendermint/go-crypto/issues/43 - return subtle.ConstantTimeCompare(sig[:], otherEd[:]) == 0 + return bytes.Equal(sig[:], otherEd[:]) } else { return false } @@ -85,12 +81,8 @@ func (sig SignatureSecp256k1) IsZero() bool { return len(sig) == 0 } func (sig SignatureSecp256k1) String() string { return fmt.Sprintf("/%X.../", Fingerprint(sig[:])) } func (sig SignatureSecp256k1) Equals(other Signature) bool { - if otherEd, ok := other.Unwrap().(SignatureSecp256k1); ok { - // It is essential that we constant time compare - // private keys and signatures instead of bytes.Equal, - // to avoid susceptibility to timing/side channel attacks. - // See Issue https://github.com/tendermint/go-crypto/issues/43 - return subtle.ConstantTimeCompare(sig[:], otherEd[:]) == 0 + if otherSecp, ok := other.Unwrap().(SignatureSecp256k1); ok { + return bytes.Equal(sig[:], otherSecp[:]) } else { return false } diff --git a/signature_test.go b/signature_test.go index 5e9f06723..4801e5fef 100644 --- a/signature_test.go +++ b/signature_test.go @@ -141,3 +141,27 @@ func TestWrapping(t *testing.T) { } } + +func TestPrivKeyEquality(t *testing.T) { + { + privKey := GenPrivKeySecp256k1().Wrap() + privKey2 := GenPrivKeySecp256k1().Wrap() + assert.False(t, privKey.Equals(privKey2)) + assert.False(t, privKey2.Equals(privKey)) + + privKeyCopy := privKey // copy + assert.True(t, privKey.Equals(privKeyCopy)) + assert.True(t, privKeyCopy.Equals(privKey)) + } + + { + privKey := GenPrivKeyEd25519().Wrap() + privKey2 := GenPrivKeyEd25519().Wrap() + assert.False(t, privKey.Equals(privKey2)) + assert.False(t, privKey2.Equals(privKey)) + + privKeyCopy := privKey // copy + assert.True(t, privKey.Equals(privKeyCopy)) + assert.True(t, privKeyCopy.Equals(privKey)) + } +}