Merge pull request #11 from SiaFoundation/flip-nonce

v2: Improve signature and nonce handling

mux.go

@@ -51,7 +51,7 @@ func (m *Mux) DialStream() *Stream {
 func Dial(conn net.Conn, theirKey ed25519.PublicKey) (*Mux, error) {
 	// exchange versions
 	var theirVersion [1]byte
-	if _, err := conn.Write([]byte{2}); err != nil {
+	if _, err := conn.Write([]byte{3}); err != nil {
 		return nil, fmt.Errorf("could not write our version: %w", err)
 	} else if _, err := io.ReadFull(conn, theirVersion[:]); err != nil {
 		return nil, fmt.Errorf("could not read peer version: %w", err)
@@ -62,7 +62,7 @@ func Dial(conn net.Conn, theirKey ed25519.PublicKey) (*Mux, error) {
 		m, err := muxv1.Dial(conn, theirKey)
 		return &Mux{m1: m}, err
 	}
-	m, err := muxv2.Dial(conn, theirKey)
+	m, err := muxv2.Dial(conn, theirKey, theirVersion[0])
 	return &Mux{m2: m}, err
 }
 
@@ -72,7 +72,7 @@ func Accept(conn net.Conn, ourKey ed25519.PrivateKey) (*Mux, error) {
 	var theirVersion [1]byte
 	if _, err := io.ReadFull(conn, theirVersion[:]); err != nil {
 		return nil, fmt.Errorf("could not read peer version: %w", err)
-	} else if _, err := conn.Write([]byte{2}); err != nil {
+	} else if _, err := conn.Write([]byte{3}); err != nil {
 		return nil, fmt.Errorf("could not write our version: %w", err)
 	} else if theirVersion[0] == 0 {
 		return nil, errors.New("peer sent invalid version")
@@ -81,7 +81,7 @@ func Accept(conn net.Conn, ourKey ed25519.PrivateKey) (*Mux, error) {
 		m, err := muxv1.Accept(conn, ourKey)
 		return &Mux{m1: m}, err
 	}
-	m, err := muxv2.Accept(conn, ourKey)
+	m, err := muxv2.Accept(conn, ourKey, theirVersion[0])
 	return &Mux{m2: m}, err
 }
 

spec_v2.md

@@ -23,7 +23,7 @@ when a "final" frame is sent, or (forcibly) when the connection is closed.
 The session is encrypted and authenticated: the dialer must know their peer's
 Ed25519 public key, which is used to sign the handshake and thereby derive a
 shared secret. This secret is then used to encrypt each frame with
-ChaCha20-Poly1305.
+ChaCha20-Poly1305, incrementing the nonce after each packet.
 
 All integers in this spec are little-endian.
 
@@ -33,24 +33,25 @@ The *dialing peer* generates an X25519 keypair and sends:
 
 | Length | Type   | Description   |
 |--------|--------|---------------|
-|   0    | uint8  | Version       |
+|   1    | uint8  | Version       |
 |   32   | []byte | X25519 pubkey |
 
-The current version is 2.
+The current version is 3.
 
-The *accepting* peer derives the shared X25519 secret, hashes it with BLAKE2b
-for use as a ChaCha20-Poly1305 key, hashes that key *again* to derive the
-initial nonce value, and responds with:
+The *accepting* peer generates an X25519 keypair, derives the shared X25519
+secret, and computes the ChaCha20-Poly1305 key as `BLAKE2b(secret | k1 | k2)`,
+where `k1` is `k2` are the dialing and accepting X25519 pubkeys. It initialies
+its nonce to `0`, and responds with:
 
 | Length | Type   | Description        |
 |--------|--------|--------------------|
-|   0    | uint8  | Version            |
+|   1    | uint8  | Version            |
 |   32   | []byte | X25519 pubkey      |
 |   64   | []byte | Ed25519 signature  |
 |   24   |        | Encrypted settings |
 
-Finally, the dialing peer derives the shared secret and responds with its own
-encrypted settings.
+Finally, the dialing peer derives the same ChaCha20-Poly1305 key, initializes
+its nonce to `1<<95`, and responds with its own encrypted settings.
 
 The settings are:
 
@@ -60,7 +61,7 @@ The settings are:
 |   4    | uint32 | Max timeout | 120000-7200000 |
 
 Settings are encrypted in the same manner as [Packets](#packets): a ciphertext
-(8 bytes in this case) followed by a 16-byte tag.
+(8 bytes in this case) followed by a 16-byte authentication tag.
 
 Peers agree upon settings by choosing the minimum of the two packet sizes and
 the maximum of the two timeouts. The timeout is an integer number of
@@ -116,8 +117,8 @@ must not be split across packet boundaries. (In other words, the maximum size of
 a frame's payload is `n - (4 + 2 + 2)`.)
 
 A separate nonce is tracked for both the dialing and accepting peer, incremented
-after each use. The initial value for both nonces is the first 12 bytes of
-BLAKE2b(BLAKE2b(shared secret)). To increment a nonce, interpret its leading 8
+after each use. The initial nonce value is `0` for the dialing peer and `1<<95`
+for the accepting peer. To increment a nonce, interpret its least-significant 8
 bytes as a 64-bit unsigned integer.
 
 ### Covert Frames

v2/handshake.go

@@ -44,34 +44,6 @@ func (c *seqCipher) decryptInPlace(buf []byte) ([]byte, error) {
 	return plaintext, err
 }
 
-func deriveSharedCipher(xsk, xpk [32]byte) (*seqCipher, error) {
-	// NOTE: an error is only possible here if xpk is a "low-order point."
-	// Basically, if the other party chooses one of these points as their public
-	// key, then the resulting "secret" can be derived by anyone who observes
-	// the handshake, effectively rendering the protocol unencrypted. This would
-	// be a strange thing to do; the other party can decrypt the messages
-	// anyway, so if they want to make the messages public, nothing can stop
-	// them from doing so. Consequently, some people (notably djb himself) will
-	// tell you not to bother checking for low-order points at all. But why
-	// would we want to talk to a peer that's behaving weirdly?
-	secret, err := curve25519.X25519(xsk[:], xpk[:])
-	if err != nil {
-		return nil, err
-	}
-	key := blake2b.Sum256(secret)
-	c, err := chacha20poly1305.New(key[:])
-	if err != nil {
-		return nil, err
-	}
-	// hash the key again to get the initial nonce value
-	nonce := blake2b.Sum256(key[:])
-	return &seqCipher{
-		aead:       c,
-		ourNonce:   *(*[chachaPoly1305NonceSize]byte)(nonce[:]),
-		theirNonce: *(*[chachaPoly1305NonceSize]byte)(nonce[:]),
-	}, nil
-}
-
 type connSettings struct {
 	PacketSize int
 	MaxTimeout time.Duration
@@ -128,7 +100,7 @@ func mergeSettings(ours, theirs connSettings) (connSettings, error) {
 	return merged, nil
 }
 
-func initiateHandshake(conn net.Conn, theirKey ed25519.PublicKey, ourSettings connSettings) (*seqCipher, connSettings, error) {
+func initiateHandshake(conn net.Conn, theirKey ed25519.PublicKey, theirVersion uint8, ourSettings connSettings) (*seqCipher, connSettings, error) {
 	xsk, xpk := generateX25519KeyPair()
 
 	// write pubkey
@@ -142,20 +114,47 @@ func initiateHandshake(conn net.Conn, theirKey ed25519.PublicKey, ourSettings co
 		return nil, connSettings{}, fmt.Errorf("could not read handshake response: %w", err)
 	}
 
-	// verify signature
+	// verify signature and derive shared cipher
 	var rxpk [32]byte
 	copy(rxpk[:], buf[:32])
+	msg := append(xpk[:], rxpk[:]...)
+	if theirVersion == 2 {
+		sigHash := blake2b.Sum256(msg)
+		msg = sigHash[:]
+	}
 	sig := buf[32:][:64]
-	sigHash := blake2b.Sum256(append(xpk[:], rxpk[:]...))
-	if !ed25519.Verify(theirKey, sigHash[:], sig) {
+	if !ed25519.Verify(theirKey, msg, sig) {
 		return nil, connSettings{}, errors.New("invalid signature")
 	}
 
 	// derive shared cipher
-	cipher, err := deriveSharedCipher(xsk, rxpk)
+	secret, err := curve25519.X25519(xsk[:], rxpk[:])
 	if err != nil {
+		// NOTE: an error is only possible here if xpk is a "low-order point."
+		// Basically, if the other party chooses one of these points as their public
+		// key, then the resulting "secret" can be derived by anyone who observes
+		// the handshake, effectively rendering the protocol unencrypted. This would
+		// be a strange thing to do; the other party can decrypt the messages
+		// anyway, so if they want to make the messages public, nothing can stop
+		// them from doing so. Consequently, some people (notably djb himself) will
+		// tell you not to bother checking for low-order points at all. But why
+		// would we want to talk to a peer that's behaving weirdly?
 		return nil, connSettings{}, fmt.Errorf("failed to derive shared cipher: %w", err)
 	}
+	var cipher *seqCipher
+	if theirVersion <= 2 {
+		key := blake2b.Sum256(secret)
+		aead, _ := chacha20poly1305.New(key[:]) // no error possible
+		cipher = &seqCipher{aead: aead}
+		nonce := blake2b.Sum256(key[:])
+		copy(cipher.ourNonce[:], nonce[:])
+		copy(cipher.theirNonce[:], nonce[:])
+	} else {
+		key := blake2b.Sum256(append(append(secret, xpk[:]...), rxpk[:]...))
+		aead, _ := chacha20poly1305.New(key[:]) // no error possible
+		cipher = &seqCipher{aead: aead}
+		cipher.theirNonce[len(cipher.theirNonce)-1] ^= 0x80
+	}
 
 	// decrypt settings
 	var mergedSettings connSettings
@@ -175,7 +174,7 @@ func initiateHandshake(conn net.Conn, theirKey ed25519.PublicKey, ourSettings co
 	return cipher, mergedSettings, nil
 }
 
-func acceptHandshake(conn net.Conn, ourKey ed25519.PrivateKey, ourSettings connSettings) (*seqCipher, connSettings, error) {
+func acceptHandshake(conn net.Conn, ourKey ed25519.PrivateKey, theirVersion uint8, ourSettings connSettings) (*seqCipher, connSettings, error) {
 	xsk, xpk := generateX25519KeyPair()
 
 	// read pubkey
@@ -187,14 +186,34 @@ func acceptHandshake(conn net.Conn, ourKey ed25519.PrivateKey, ourSettings connS
 	// derive shared cipher
 	var rxpk [32]byte
 	copy(rxpk[:], buf[:32])
-	cipher, err := deriveSharedCipher(xsk, rxpk)
+
+	// derive shared cipher
+	secret, err := curve25519.X25519(xsk[:], rxpk[:])
 	if err != nil {
 		return nil, connSettings{}, fmt.Errorf("failed to derive shared cipher: %w", err)
 	}
+	var cipher *seqCipher
+	if theirVersion <= 2 {
+		key := blake2b.Sum256(secret)
+		aead, _ := chacha20poly1305.New(key[:])
+		cipher = &seqCipher{aead: aead}
+		nonce := blake2b.Sum256(key[:])
+		copy(cipher.ourNonce[:], nonce[:])
+		copy(cipher.theirNonce[:], nonce[:])
+	} else {
+		key := blake2b.Sum256(append(append(secret, rxpk[:]...), xpk[:]...))
+		aead, _ := chacha20poly1305.New(key[:])
+		cipher = &seqCipher{aead: aead}
+		cipher.ourNonce[len(cipher.ourNonce)-1] ^= 0x80
+	}
 
 	// write pubkey, signature, and settings
-	sigHash := blake2b.Sum256(append(rxpk[:], xpk[:]...))
-	sig := ed25519.Sign(ourKey, sigHash[:])
+	msg := append(rxpk[:], xpk[:]...)
+	if theirVersion == 2 {
+		sigHash := blake2b.Sum256(msg)
+		msg = sigHash[:]
+	}
+	sig := ed25519.Sign(ourKey, msg)
 	copy(buf, xpk[:])
 	copy(buf[32:], sig)
 	encodeConnSettings(buf[32+64:], ourSettings)

v2/mux.go

@@ -383,17 +383,17 @@ func newMux(conn net.Conn, cipher *seqCipher, settings connSettings) *Mux {
 }
 
 // Dial initiates a mux protocol handshake on the provided conn.
-func Dial(conn net.Conn, theirKey ed25519.PublicKey) (*Mux, error) {
-	cipher, settings, err := initiateHandshake(conn, theirKey, defaultConnSettings)
+func Dial(conn net.Conn, theirKey ed25519.PublicKey, theirVersion uint8) (*Mux, error) {
+	cipher, settings, err := initiateHandshake(conn, theirKey, theirVersion, defaultConnSettings)
 	if err != nil {
 		return nil, fmt.Errorf("handshake failed: %w", err)
 	}
 	return newMux(conn, cipher, settings), nil
 }
 
 // Accept reciprocates a mux protocol handshake on the provided conn.
-func Accept(conn net.Conn, ourKey ed25519.PrivateKey) (*Mux, error) {
-	cipher, settings, err := acceptHandshake(conn, ourKey, defaultConnSettings)
+func Accept(conn net.Conn, ourKey ed25519.PrivateKey, theirVersion uint8) (*Mux, error) {
+	cipher, settings, err := acceptHandshake(conn, ourKey, theirVersion, defaultConnSettings)
 	if err != nil {
 		return nil, fmt.Errorf("handshake failed: %w", err)
 	}
@@ -408,12 +408,12 @@ var anonPubkey = anonPrivkey.Public().(ed25519.PublicKey)
 // DialAnonymous initiates a mux protocol handshake to a party without a
 // pre-established identity. The counterparty must reciprocate the handshake with
 // AcceptAnonymous.
-func DialAnonymous(conn net.Conn) (*Mux, error) { return Dial(conn, anonPubkey) }
+func DialAnonymous(conn net.Conn) (*Mux, error) { return Dial(conn, anonPubkey, 3) }
 
 // AcceptAnonymous reciprocates a mux protocol handshake without a
 // pre-established identity. The counterparty must initiate the handshake with
 // DialAnonymous.
-func AcceptAnonymous(conn net.Conn) (*Mux, error) { return Accept(conn, anonPrivkey) }
+func AcceptAnonymous(conn net.Conn) (*Mux, error) { return Accept(conn, anonPrivkey, 3) }
 
 // A Stream is a duplex connection multiplexed over a net.Conn. It implements
 // the net.Conn interface.

v2/mux_test.go

@@ -87,7 +87,7 @@ func TestMux(t *testing.T) {
 			if err != nil {
 				return err
 			}
-			m, err := Accept(conn, serverKey)
+			m, err := Accept(conn, serverKey, 3)
 			if err != nil {
 				return err
 			}
@@ -111,7 +111,7 @@ func TestMux(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	m, err := Dial(conn, serverKey.Public().(ed25519.PublicKey))
+	m, err := Dial(conn, serverKey.Public().(ed25519.PublicKey), 3)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -546,6 +546,67 @@ func TestCovertStream(t *testing.T) {
 	}
 }
 
+func TestCompatV2(t *testing.T) {
+	serverKey := ed25519.NewKeyFromSeed(frand.Bytes(ed25519.SeedSize))
+	l, err := net.Listen("tcp", ":0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	serverCh := make(chan error, 1)
+	go func() {
+		serverCh <- func() error {
+			conn, err := l.Accept()
+			if err != nil {
+				return err
+			}
+			m, err := Accept(conn, serverKey, 2)
+			if err != nil {
+				return err
+			}
+			defer m.Close()
+			s, err := m.AcceptStream()
+			if err != nil {
+				return err
+			}
+			defer s.Close()
+			buf := make([]byte, 100)
+			if n, err := s.Read(buf); err != nil {
+				return err
+			} else if _, err := fmt.Fprintf(s, "hello, %s!", buf[:n]); err != nil {
+				return err
+			}
+			return s.Close()
+		}()
+	}()
+
+	conn, err := net.Dial("tcp", l.Addr().String())
+	if err != nil {
+		t.Fatal(err)
+	}
+	m, err := Dial(conn, serverKey.Public().(ed25519.PublicKey), 2)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer m.Close()
+	s := m.DialStream()
+	defer s.Close()
+	buf := make([]byte, 100)
+	if _, err := s.Write([]byte("world")); err != nil {
+		t.Fatal(err)
+	} else if n, err := io.ReadFull(s, buf[:13]); err != nil {
+		t.Fatal(err)
+	} else if string(buf[:n]) != "hello, world!" {
+		t.Fatal("bad hello:", string(buf[:n]))
+	}
+	if err := s.Close(); err != nil && err != ErrPeerClosedConn {
+		t.Fatal(err)
+	}
+
+	if err := <-serverCh; err != nil && err != ErrPeerClosedStream {
+		t.Fatal(err)
+	}
+}
+
 func BenchmarkMux(b *testing.B) {
 	for _, numStreams := range []int{1, 2, 10, 100, 500, 1000} {
 		b.Run(fmt.Sprint(numStreams), func(b *testing.B) {