diff --git a/whisper/whisperv6/api.go b/whisper/whisperv6/api.go index 0e8490b419..4f38188cd1 100644 --- a/whisper/whisperv6/api.go +++ b/whisper/whisperv6/api.go @@ -277,7 +277,7 @@ func (api *PublicWhisperAPI) Post(ctx context.Context, req NewMessage) (bool, er if params.KeySym, err = api.w.GetSymKey(req.SymKeyID); err != nil { return false, err } - if !validateSymmetricKey(params.KeySym) { + if !validateRandomData(params.KeySym, aesKeyLength) { return false, ErrInvalidSymmetricKey } } @@ -383,7 +383,7 @@ func (api *PublicWhisperAPI) Messages(ctx context.Context, crit Criteria) (*rpc. if err != nil { return nil, err } - if !validateSymmetricKey(key) { + if !validateRandomData(key, aesKeyLength) { return nil, ErrInvalidSymmetricKey } filter.KeySym = key @@ -555,7 +555,7 @@ func (api *PublicWhisperAPI) NewMessageFilter(req Criteria) (string, error) { if keySym, err = api.w.GetSymKey(req.SymKeyID); err != nil { return "", err } - if !validateSymmetricKey(keySym) { + if !validateRandomData(keySym, aesKeyLength) { return "", ErrInvalidSymmetricKey } } diff --git a/whisper/whisperv6/doc.go b/whisper/whisperv6/doc.go index eae2dcb77f..abb8012e45 100644 --- a/whisper/whisperv6/doc.go +++ b/whisper/whisperv6/doc.go @@ -54,7 +54,7 @@ const ( TopicLength = 4 // in bytes signatureLength = 65 // in bytes aesKeyLength = 32 // in bytes - AESNonceLength = 12 // in bytes; also returned by aesgcm.NonceSize() + aesNonceLength = 12 // in bytes; for more info please see cipher.gcmStandardNonceSize & aesgcm.NonceSize() keyIdSize = 32 // in bytes bloomFilterSize = 64 // in bytes diff --git a/whisper/whisperv6/message.go b/whisper/whisperv6/message.go index 45b33ec557..21849060e2 100644 --- a/whisper/whisperv6/message.go +++ b/whisper/whisperv6/message.go @@ -55,7 +55,7 @@ type sentMessage struct { } // ReceivedMessage represents a data packet to be received through the -// Whisper protocol. +// Whisper protocol and successfully decrypted. type ReceivedMessage struct { Raw []byte @@ -71,7 +71,7 @@ type ReceivedMessage struct { Dst *ecdsa.PublicKey // Message recipient (identity used to decode the message) Topic TopicType - SymKeyHash common.Hash // The Keccak256Hash of the key, associated with the Topic + SymKeyHash common.Hash // The Keccak256Hash of the key EnvelopeHash common.Hash // Message envelope hash to act as a unique id } @@ -131,9 +131,6 @@ func (msg *sentMessage) appendPadding(params *MessageParams) error { if params.Src != nil { rawSize += signatureLength } - if params.KeySym != nil { - rawSize += AESNonceLength - } odd := rawSize % padSizeLimit paddingSize := padSizeLimit - odd pad := make([]byte, paddingSize) @@ -141,7 +138,7 @@ func (msg *sentMessage) appendPadding(params *MessageParams) error { if err != nil { return err } - if !validateSymmetricKey(pad) { + if !validateRandomData(pad, paddingSize) { return errors.New("failed to generate random padding of size " + strconv.Itoa(paddingSize)) } msg.Raw = append(msg.Raw, pad...) @@ -183,8 +180,8 @@ func (msg *sentMessage) encryptAsymmetric(key *ecdsa.PublicKey) error { // encryptSymmetric encrypts a message with a topic key, using AES-GCM-256. // nonce size should be 12 bytes (see cipher.gcmStandardNonceSize). func (msg *sentMessage) encryptSymmetric(key []byte) (err error) { - if !validateSymmetricKey(key) { - return errors.New("invalid key provided for symmetric encryption") + if !validateRandomData(key, aesKeyLength) { + return errors.New("invalid key provided for symmetric encryption, size: " + strconv.Itoa(len(key))) } block, err := aes.NewCipher(key) if err != nil { @@ -194,7 +191,7 @@ func (msg *sentMessage) encryptSymmetric(key []byte) (err error) { if err != nil { return err } - salt, err := generateSalt(aesgcm) + salt, err := generateSecureRandomData(aesNonceLength) // never use more than 2^32 random nonces with a given key if err != nil { return err } @@ -203,32 +200,35 @@ func (msg *sentMessage) encryptSymmetric(key []byte) (err error) { return nil } -func generateSalt(aesgcm cipher.AEAD) ([]byte, error) { - // never use more than 2^32 random nonces with a given key - sz := aesgcm.NonceSize() - x1 := make([]byte, sz) - x2 := make([]byte, sz) - salt := make([]byte, sz) +// generateSecureRandomData generates random data where extra security is required. +// The purpose of this function is to prevent some bugs in software or in hardware +// from delivering not-very-random data. This is especially useful for AES nonce, +// where true randomness does not really matter, but it is very important to have +// a unique nonce for every message. +func generateSecureRandomData(length int) ([]byte, error) { + x := make([]byte, length) + y := make([]byte, length) + res := make([]byte, length) - _, err := crand.Read(x1) + _, err := crand.Read(x) if err != nil { return nil, err - } else if !validateSymmetricKey(x1) { - return nil, errors.New("crypto/rand failed to generate salt") + } else if !validateRandomData(x, length) { + return nil, errors.New("crypto/rand failed to generate secure random data") } - _, err = mrand.Read(x2) + _, err = mrand.Read(y) if err != nil { return nil, err - } else if !validateSymmetricKey(x2) { - return nil, errors.New("math/rand failed to generate salt") + } else if !validateRandomData(y, length) { + return nil, errors.New("math/rand failed to generate secure random data") } - for i := 0; i < sz; i++ { - salt[i] = x1[i] ^ x2[i] + for i := 0; i < length; i++ { + res[i] = x[i] ^ y[i] } - if !validateSymmetricKey(salt) { - return nil, errors.New("failed to generate salt") + if !validateRandomData(res, length) { + return nil, errors.New("failed to generate secure random data") } - return salt, nil + return res, nil } // Wrap bundles the message into an Envelope to transmit over the network. @@ -263,10 +263,10 @@ func (msg *sentMessage) Wrap(options *MessageParams) (envelope *Envelope, err er // nonce size should be 12 bytes (see cipher.gcmStandardNonceSize). func (msg *ReceivedMessage) decryptSymmetric(key []byte) error { // symmetric messages are expected to contain the 12-byte nonce at the end of the payload - if len(msg.Raw) < AESNonceLength { + if len(msg.Raw) < aesNonceLength { return errors.New("missing salt or invalid payload in symmetric message") } - salt := msg.Raw[len(msg.Raw)-AESNonceLength:] + salt := msg.Raw[len(msg.Raw)-aesNonceLength:] block, err := aes.NewCipher(key) if err != nil { @@ -276,11 +276,7 @@ func (msg *ReceivedMessage) decryptSymmetric(key []byte) error { if err != nil { return err } - if len(salt) != aesgcm.NonceSize() { - log.Error("decrypting the message", "AES salt size", len(salt)) - return errors.New("wrong AES salt size") - } - decrypted, err := aesgcm.Open(nil, salt, msg.Raw[:len(msg.Raw)-AESNonceLength], nil) + decrypted, err := aesgcm.Open(nil, salt, msg.Raw[:len(msg.Raw)-aesNonceLength], nil) if err != nil { return err } diff --git a/whisper/whisperv6/message_test.go b/whisper/whisperv6/message_test.go index 81db58dedd..f8e61c31ab 100644 --- a/whisper/whisperv6/message_test.go +++ b/whisper/whisperv6/message_test.go @@ -18,9 +18,12 @@ package whisperv6 import ( "bytes" + "crypto/aes" + "crypto/cipher" mrand "math/rand" "testing" + "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/rlp" ) @@ -206,7 +209,7 @@ func TestEnvelopeOpen(t *testing.T) { InitSingleTest() var symmetric bool - for i := 0; i < 256; i++ { + for i := 0; i < 32; i++ { singleEnvelopeOpenTest(t, symmetric) symmetric = !symmetric } @@ -417,30 +420,6 @@ func TestPadding(t *testing.T) { } } -func TestPaddingAppendedToSymMessages(t *testing.T) { - params := &MessageParams{ - Payload: make([]byte, 246), - KeySym: make([]byte, aesKeyLength), - } - - // Simulate a message with a payload just under 256 so that - // payload + flag + aesnonce > 256. Check that the result - // is padded on the next 256 boundary. - msg := sentMessage{} - msg.Raw = make([]byte, 1+1+len(params.Payload)) - - err := msg.appendPadding(params) - - if err != nil { - t.Fatalf("Error appending padding to message %v", err) - return - } - - if len(msg.Raw) != 512-AESNonceLength { - t.Errorf("Invalid size %d != 512", len(msg.Raw)) - } -} - func TestPaddingAppendedToSymMessagesWithSignature(t *testing.T) { params := &MessageParams{ Payload: make([]byte, 246), @@ -456,7 +435,7 @@ func TestPaddingAppendedToSymMessagesWithSignature(t *testing.T) { params.Src = pSrc // Simulate a message with a payload just under 256 so that - // payload + flag + aesnonce > 256. Check that the result + // payload + flag + signature > 256. Check that the result // is padded on the next 256 boundary. msg := sentMessage{} msg.Raw = make([]byte, 1+1+len(params.Payload)) @@ -468,7 +447,24 @@ func TestPaddingAppendedToSymMessagesWithSignature(t *testing.T) { return } - if len(msg.Raw) != 512-AESNonceLength-signatureLength { + if len(msg.Raw) != 512-signatureLength { t.Errorf("Invalid size %d != 512", len(msg.Raw)) } } + +func TestAesNonce(t *testing.T) { + key := hexutil.MustDecode("0x03ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd31") + block, err := aes.NewCipher(key) + if err != nil { + t.Fatalf("NewCipher failed: %s", err) + } + aesgcm, err := cipher.NewGCM(block) + if err != nil { + t.Fatalf("NewGCM failed: %s", err) + } + // This is the most important single test in this package. + // If it fails, whisper will not be working. + if aesgcm.NonceSize() != aesNonceLength { + t.Fatalf("Nonce size is wrong. This is a critical error. Apparently AES nonce size have changed in the new version of AES GCM package. Whisper will not be working untill this problem is resolved.") + } +} diff --git a/whisper/whisperv6/peer_test.go b/whisper/whisperv6/peer_test.go index 8a65cb7143..29f1172968 100644 --- a/whisper/whisperv6/peer_test.go +++ b/whisper/whisperv6/peer_test.go @@ -27,6 +27,7 @@ import ( "time" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/common/hexutil" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/p2p" "github.com/ethereum/go-ethereum/p2p/discover" @@ -85,7 +86,7 @@ type TestNode struct { var result TestData var nodes [NumNodes]*TestNode -var sharedKey []byte = []byte("some arbitrary data here") +var sharedKey []byte = hexutil.MustDecode("0x03ca634cae0d49acb401d8a4c6b6fe8c55b70d115bf400769cc1400f3258cd31") var sharedTopic TopicType = TopicType{0xF, 0x1, 0x2, 0} var expectedMessage []byte = []byte("per rectum ad astra") var masterBloomFilter []byte diff --git a/whisper/whisperv6/whisper.go b/whisper/whisperv6/whisper.go index bc89aadccd..f7d71f612d 100644 --- a/whisper/whisperv6/whisper.go +++ b/whisper/whisperv6/whisper.go @@ -19,7 +19,6 @@ package whisperv6 import ( "bytes" "crypto/ecdsa" - crand "crypto/rand" "crypto/sha256" "fmt" "math" @@ -442,11 +441,10 @@ func (w *Whisper) GetPrivateKey(id string) (*ecdsa.PrivateKey, error) { // GenerateSymKey generates a random symmetric key and stores it under id, // which is then returned. Will be used in the future for session key exchange. func (w *Whisper) GenerateSymKey() (string, error) { - key := make([]byte, aesKeyLength) - _, err := crand.Read(key) + key, err := generateSecureRandomData(aesKeyLength) if err != nil { return "", err - } else if !validateSymmetricKey(key) { + } else if !validateRandomData(key, aesKeyLength) { return "", fmt.Errorf("error in GenerateSymKey: crypto/rand failed to generate random data") } @@ -983,9 +981,10 @@ func validatePrivateKey(k *ecdsa.PrivateKey) bool { return ValidatePublicKey(&k.PublicKey) } -// validateSymmetricKey returns false if the key contains all zeros -func validateSymmetricKey(k []byte) bool { - return len(k) > 0 && !containsOnlyZeros(k) +// validateSymmetricKey returns false if the key contains all zeros, +// which is a simplest and the most common bug. +func validateRandomData(k []byte, expectedSize int) bool { + return len(k) == expectedSize && !containsOnlyZeros(k) } // containsOnlyZeros checks if the data contain only zeros. @@ -1019,12 +1018,11 @@ func BytesToUintBigEndian(b []byte) (res uint64) { // GenerateRandomID generates a random string, which is then returned to be used as a key id func GenerateRandomID() (id string, err error) { - buf := make([]byte, keyIdSize) - _, err = crand.Read(buf) + buf, err := generateSecureRandomData(keyIdSize) if err != nil { return "", err } - if !validateSymmetricKey(buf) { + if !validateRandomData(buf, keyIdSize) { return "", fmt.Errorf("error in generateRandomID: crypto/rand failed to generate random data") } id = common.Bytes2Hex(buf) diff --git a/whisper/whisperv6/whisper_test.go b/whisper/whisperv6/whisper_test.go index fa14acb1b1..b10c59844e 100644 --- a/whisper/whisperv6/whisper_test.go +++ b/whisper/whisperv6/whisper_test.go @@ -81,7 +81,7 @@ func TestWhisperBasic(t *testing.T) { } derived := pbkdf2.Key([]byte(peerID), nil, 65356, aesKeyLength, sha256.New) - if !validateSymmetricKey(derived) { + if !validateRandomData(derived, aesKeyLength) { t.Fatalf("failed validateSymmetricKey with param = %v.", derived) } if containsOnlyZeros(derived) { @@ -448,24 +448,12 @@ func TestWhisperSymKeyManagement(t *testing.T) { if !w.HasSymKey(id2) { t.Fatalf("HasSymKey(id2) failed.") } - if k1 == nil { - t.Fatalf("k1 does not exist.") - } - if k2 == nil { - t.Fatalf("k2 does not exist.") + if !validateRandomData(k2, aesKeyLength) { + t.Fatalf("key validation failed.") } if !bytes.Equal(k1, k2) { t.Fatalf("k1 != k2.") } - if len(k1) != aesKeyLength { - t.Fatalf("wrong length of k1.") - } - if len(k2) != aesKeyLength { - t.Fatalf("wrong length of k2.") - } - if !validateSymmetricKey(k2) { - t.Fatalf("key validation failed.") - } } func TestExpiry(t *testing.T) {