add correct authentication with HMAC

p2p/crypto.go

@@ -150,7 +150,7 @@ func (self *cryptoId) NewSession(r io.Reader, w io.Writer, remotePubKeyS []byte,
 		}
 		clogger.Debugf("receiver handshake (sent to %v):\n%v", hexkey(remotePubKeyS), hexkey(response))
 	}
-	return self.newSession(r, w, initNonce, recNonce, auth, randomPrivKey, remoteRandomPubKey)
+	return self.newSession(r, w, initiator, initNonce, recNonce, auth, randomPrivKey, remoteRandomPubKey)
 }
 
 /*
@@ -357,7 +357,7 @@ func (self *cryptoId) completeHandshake(auth []byte) (respNonce []byte, remoteRa
 /*
 newSession is called after the handshake is completed. The arguments are values negotiated in the handshake and the return value is a new session : a new session Token to be remembered for the next time we connect with this peer. And a MsgReadWriter that implements an encrypted and authenticated connection with key material obtained from the crypto handshake key exchange
 */
-func (self *cryptoId) newSession(r io.Reader, w io.Writer, initNonce, respNonce, auth []byte, privKey *ecdsa.PrivateKey, remoteRandomPubKey *ecdsa.PublicKey) (sessionToken []byte, rw MsgReadWriter, err error) {
+func (self *cryptoId) newSession(r io.Reader, w io.Writer, initiator bool, initNonce, respNonce, auth []byte, privKey *ecdsa.PrivateKey, remoteRandomPubKey *ecdsa.PublicKey) (sessionToken []byte, rw MsgReadWriter, err error) {
 	// 3) Now we can trust ecdhe-random-pubk to derive new keys
 	//ecdhe-shared-secret = ecdh.agree(ecdhe-random, remote-ecdhe-random-pubk)
 	var dhSharedSecret []byte
@@ -375,13 +375,14 @@ func (self *cryptoId) newSession(r io.Reader, w io.Writer, initNonce, respNonce,
 	// mac-secret = crypto.Sha3(ecdhe-shared-secret || aes-secret)
 	var macSecret = crypto.Sha3(append(dhSharedSecret, aesSecret...))
 	// # destroy ecdhe-shared-secret
-	// egress-mac = crypto.Sha3(mac-secret^nonce || auth)
-	var egressMac = crypto.Sha3(append(Xor(macSecret, respNonce), auth...))
-	// # destroy nonce
-	// ingress-mac = crypto.Sha3(mac-secret^initiator-nonce || auth),
-	var ingressMac = crypto.Sha3(append(Xor(macSecret, initNonce), auth...))
-	// # destroy remote-nonce
-
+	var egressMac, ingressMac []byte
+	if initiator {
+		egressMac = Xor(macSecret, respNonce)
+		ingressMac = Xor(macSecret, initNonce)
+	} else {
+		egressMac = Xor(macSecret, initNonce)
+		ingressMac = Xor(macSecret, respNonce)
+	}
 	clogger.Debugf("aes-secret: %v", hexkey(aesSecret))
 	clogger.Debugf("mac-secret: %v", hexkey(macSecret))
 	clogger.Debugf("egress-mac: %v", hexkey(egressMac))

p2p/crypto_test.go

@@ -109,12 +109,12 @@ func TestCryptoHandshake(t *testing.T) {
 	conn0, conn1 := net.Pipe()
 
 	// now both parties should have the same session parameters
-	initSessionToken, initRW, err := initiator.newSession(bufio.NewReader(conn0), conn0, initNonce, recNonce, auth, randomPrivKey, remoteRandomPubKey)
+	initSessionToken, initRW, err := initiator.newSession(bufio.NewReader(conn0), conn0, true, initNonce, recNonce, auth, randomPrivKey, remoteRandomPubKey)
 	if err != nil {
 		t.Errorf("%v", err)
 	}
 
-	recSessionToken, recRW, err := receiver.newSession(bufio.NewReader(conn1), conn1, remoteInitNonce, remoteRecNonce, auth, remoteRandomPrivKey, remoteInitRandomPubKey)
+	recSessionToken, recRW, err := receiver.newSession(bufio.NewReader(conn1), conn1, false, remoteInitNonce, remoteRecNonce, auth, remoteRandomPrivKey, remoteInitRandomPubKey)
 	if err != nil {
 		t.Errorf("%v", err)
 	}
@@ -149,11 +149,11 @@ func TestCryptoHandshake(t *testing.T) {
 	if !bytes.Equal(initSecretRW.macSecret, recSecretRW.macSecret) {
 		t.Errorf("macSecrets do not match")
 	}
-	if !bytes.Equal(initSecretRW.egressMac, recSecretRW.egressMac) {
-		t.Errorf("egressMacs do not match")
+	if !bytes.Equal(initSecretRW.egressMac, recSecretRW.ingressMac) {
+		t.Errorf("initiator's egressMac do not match receiver's ingressMac")
 	}
-	if !bytes.Equal(initSecretRW.ingressMac, recSecretRW.ingressMac) {
-		t.Errorf("ingressMacs do not match")
+	if !bytes.Equal(initSecretRW.ingressMac, recSecretRW.egressMac) {
+		t.Errorf("initiator's inressMac do not match receiver's egressMac")
 	}
 
 }

p2p/encryption.go

@@ -7,11 +7,9 @@ import (
 	"crypto/hmac"
 	"crypto/sha256"
 	"encoding/binary"
-	"fmt"
 	"hash"
 	"io"
 
-	// "github.com/ethereum/go-ethereum/crypto/sha256"
 	"github.com/ethereum/go-ethereum/ethutil"
 	"github.com/ethereum/go-ethereum/rlp"
 )
@@ -82,11 +80,11 @@ func (self *CryptoMsgRW) WriteMsg(msg Msg) (err error) {
 	copy(plaintext[listhdrLen:], code)
 	msg.Payload.Read(plaintext[listhdrLen+codeLen:])
 	self.Encrypt(ciphertext, plaintext)
-	fmt.Printf("ENCRYPT:\npt: %v\nct: %v\n", hexkey(plaintext), hexkey(ciphertext))
 	if _, err = self.w.Write(ciphertext); err != nil {
 		return
 	}
-	if _, err = self.w.Write(self.egress.Sum(nil)); err != nil {
+	mac := self.egress.Sum(nil)
+	if _, err = self.w.Write(mac); err != nil {
 		return
 	}
 
@@ -130,18 +128,16 @@ func (self *CryptoMsgRW) readPayload(size uint32) (r rlp.ByteReader, err error)
 	ciphertext := make([]byte, size)
 	self.r.Read(ciphertext)
 	self.Decrypt(plaintext, ciphertext)
-	fmt.Printf("DECRYPT:\npt: %v\nct: %v\n", hexkey(plaintext), hexkey(ciphertext))
 	mac := make([]byte, 32)
 	if _, err = self.r.Read(mac); err != nil {
 		err = newPeerError(errRead, "%v", err)
 		return
 	}
-	// var expectedMac = self.ingress.Sum(nil)
-	// if !hmac.Equal(expectedMac, mac) {
-	// 	err = newPeerError(errAuthentication, "ingress incorrect")
-	// 	return
-	// }
+	var expectedMac = self.ingress.Sum(nil)
+	if !hmac.Equal(expectedMac, mac) {
+		err = newPeerError(errAuthentication, "ingress incorrect:\nexp %v\ngot %v\n", hexkey(expectedMac), hexkey(mac))
+		return
+	}
 	r = bytes.NewReader(plaintext)
-	// r = io.LimitReader(bytes.NewReader(plaintext), int64(size))
 	return
 }

p2p/encryption_test.go

@@ -4,7 +4,6 @@ import (
 	"bufio"
 	"bytes"
 	"crypto/rand"
-	// "fmt"
 	"io"
 	"net"
 	"testing"
@@ -47,7 +46,8 @@ func TestEncryption(t *testing.T) {
 	}
 	messenger0 := NewMessenger(rw0)
 
-	rw1, err := NewCryptoMsgRW(bufio.NewReader(conn1), conn1, args[0], args[1], args[2], args[3])
+	// note that args 3/2 swapped! ingress <-> egress MAC should reverse
+	rw1, err := NewCryptoMsgRW(bufio.NewReader(conn1), conn1, args[0], args[1], args[3], args[2])
 	if err != nil {
 		return
 	}

p2p/peer.go

@@ -321,6 +321,8 @@ func (p *Peer) handleCryptoHandshake() (err error) {
 		if crw, err = NewMsgRW(bufio.NewReader(p.conn), p.conn); err != nil {
 			return
 		}
+		p.Infof("insecure connection using no encryption/authentication")
+
 	case EthCrypto:
 		// cryptoId is just created for the lifecycle of the handshake
 		// it is survived by an encrypted readwriter
@@ -350,6 +352,7 @@ func (p *Peer) handleCryptoHandshake() (err error) {
 		p.Errorf("%v", err)
 	}
 	p.crw = NewMessenger(crw)
+	p.Infof("secure connection using %v", p.CryptoType)
 	return
 }