Bind the opening transcript to the traffic keys

conn.go

@@ -62,10 +62,64 @@ type Session struct {
 	Suite          uint16
 }
 
-// DeriveSession builds traffic keys from the pre-shared key and the ECDH shared
-// secret. Authentication comes from the psk and forward secrecy from the DH --
-// the same division of labour as TLS 1.3 psk_dhe_ke.
-func DeriveSession(psk, shared []byte, suite uint16) (*Session, error) {
+// Transcript is the opening as it went on the wire, in order, and binding it to
+// the traffic keys is what makes a tampered ServerHello detectable at all --
+// exactly as TLS 1.3's transcript hash does.
+//
+// Without it the client checked almost nothing about the ServerHello: it pulled
+// the X25519 half out of the key_share, did the ECDH, and derived keys from the
+// psk and its OWN configured cipher suite. So an on-path attacker could flip a
+// byte in legacy_session_id_echo, cipher_suite, legacy_version,
+// compression_method, ServerHello.random or the ML-KEM half of the share and
+// the client completed the handshake regardless -- while a genuine TLS 1.3
+// client aborts, because RFC 8446 4.1.3 requires the session_id echo to match
+// and the server Finished MACs the whole transcript.
+//
+// That gap was an ACTIVE DISTINGUISHER, and a cheap one: flip one byte in a
+// ServerHello and watch whether the connection survives. Real TLS dies, we did
+// not. It needs no traffic analysis, works on the first connection, and the
+// only cost to a censor is breaking the connection it probes. Six of seven
+// mutated fields were accepted before this; only the X25519 half was caught,
+// and only incidentally, because it breaks the ECDH.
+//
+// Feeding the transcript into the derivation closes it the way TLS does rather
+// than by adding field-by-field checks: ANY difference between what the server
+// sent and what the client received produces different keys, so the first
+// encrypted record fails to decrypt. Under theater that is the right analogue
+// of an alert, since this transport never sends one.
+// The arity is FIXED at the three opening records rather than variadic. The
+// first version was variadic, reasoning that a record joining the opening later
+// could then be added without widening a signature -- which had it backwards. A
+// variadic call site can silently drop a record and still compile, weakening the
+// binding with nothing to notice, and silent weakening is the exact class of bug
+// this change exists to remove. Widening a signature is a compile error at every
+// call site, which is the direction that gets looked at.
+func Transcript(clientHello, serverHello, changeCipherSpec []byte) []byte {
+	t := make([]byte, 0, len(clientHello)+len(serverHello)+len(changeCipherSpec))
+	t = append(t, clientHello...)
+	t = append(t, serverHello...)
+	return append(t, changeCipherSpec...)
+}
+
+// DeriveSession builds traffic keys from the pre-shared key, the ECDH shared
+// secret, and the opening transcript. Authentication comes from the psk and
+// forward secrecy from the DH -- the same division of labour as TLS 1.3
+// psk_dhe_ke -- while the transcript is what binds those keys to the opening
+// that produced them.
+//
+// transcript must be Transcript(ClientHello, ServerHello, ChangeCipherSpec)
+// over the records as they went on the wire, and an empty one is REFUSED.
+//
+// Refusing it matters for the same reason Transcript has fixed arity: an
+// unbound derivation still compiles and still produces working keys, so a call
+// site that lost its transcript would keep passing tests while silently
+// reinstating the tampering this exists to stop. There is no legitimate unbound
+// caller -- tests that only need a matched key pair supply a fixed transcript
+// of their own -- so the case is removed rather than documented.
+func DeriveSession(psk, shared []byte, suite uint16, transcript []byte) (*Session, error) {
+	if len(transcript) == 0 {
+		return nil, errors.New("twiddle: empty transcript; keys must be bound to the opening")
+	}
 	var keyLen int
 	switch suite {
 	case TLS_AES_128_GCM_SHA256:
@@ -86,12 +140,17 @@ func DeriveSession(psk, shared []byte, suite uint16) (*Session, error) {
 		// psk is the salt and the ECDH secret the input keying material, so both
 		// must be present to derive traffic keys: authentication from the
 		// pre-shared key, forward secrecy from the Diffie-Hellman.
+		// The transcript hash rides in the HKDF info alongside the label,
+		// which is where TLS 1.3 puts it too: Derive-Secret binds each secret
+		// to the handshake messages that produced it.
 		var out []byte
 		var err error
 		if suite == TLS_AES_256_GCM_SHA384 {
-			out, err = hkdf.Key(sha512.New384, shared, psk, d.label, keyLen+12)
+			sum := sha512.Sum384(transcript)
+			out, err = hkdf.Key(sha512.New384, shared, psk, d.label+string(sum[:]), keyLen+12)
 		} else {
-			out, err = hkdf.Key(sha256.New, shared, psk, d.label, keyLen+12)
+			sum := sha256.Sum256(transcript)
+			out, err = hkdf.Key(sha256.New, shared, psk, d.label+string(sum[:]), keyLen+12)
 		}
 		if err != nil {
 			return nil, err

conn_race_test.go

@@ -68,7 +68,7 @@ func (c *replayConn) Close() error               { return nil }
 
 func testSession(t *testing.T) *Session {
 	t.Helper()
-	s, err := DeriveSession(make([]byte, 32), make([]byte, 32), TLS_AES_128_GCM_SHA256)
+	s, err := DeriveSession(make([]byte, 32), make([]byte, 32), TLS_AES_128_GCM_SHA256, testTranscript())
 	if err != nil {
 		t.Fatal(err)
 	}

conn_test.go

@@ -0,0 +1,7 @@
+package twiddle
+
+// testTranscript is a fixed stand-in for the opening, for tests that only need
+// two Sessions whose keys agree. DeriveSession refuses an empty transcript, so
+// there is no unbound derivation anywhere -- including in tests, where one
+// would quietly become the example everyone copies.
+func testTranscript() []byte { return []byte("twiddle test transcript") }

handshake.go

@@ -187,15 +187,20 @@ func Client(raw net.Conn, cfg ClientConfig) (*Conn, *Credential, error) {
 	if err != nil {
 		return nil, nil, err
 	}
-	if _, err := readRecord(raw); err != nil { // ChangeCipherSpec
+	ccs, err := readRecord(raw) // ChangeCipherSpec
+	if err != nil {
 		return nil, nil, err
 	}
 
 	shared, err := eph.ECDH(serverEph)
 	if err != nil {
 		return nil, nil, err
 	}
-	sess, err := DeriveSession(cfg.Credential.PSK[:], shared, cfg.Cover.CipherSuite)
+	// Binds the opening to the keys. wire is what we sent and sh is what came
+	// back, so a ServerHello altered in flight yields different keys here than
+	// the server derived, and the first encrypted record fails to decrypt.
+	sess, err := DeriveSession(cfg.Credential.PSK[:], shared, cfg.Cover.CipherSuite,
+		Transcript(wire, sh, ccs))
 	if err != nil {
 		return nil, nil, err
 	}
@@ -316,7 +321,8 @@ func Server(raw net.Conn, cfg ServerConfig) (*Conn, error) {
 	if err != nil {
 		return nil, err
 	}
-	sess, err := DeriveSession(res.PSK[:], shared, cfg.Cover.CipherSuite)
+	sess, err := DeriveSession(res.PSK[:], shared, cfg.Cover.CipherSuite,
+		Transcript(rec, sh, ChangeCipherSpec()))
 	if err != nil {
 		return nil, err
 	}

handshake_test.go

@@ -406,15 +406,15 @@ func serverHelloExtOrder(t *testing.T, sh []byte) []uint16 {
 // would parse the first of them as a ticket instead of failing.
 func TestReadTicketsRejectsNonHandshakeRecords(t *testing.T) {
 	cover := mustCover(t, "www.microsoft.com")
-	sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite)
+	sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, testTranscript())
 	if err != nil {
 		t.Fatal(err)
 	}
 	server, client := net.Pipe()
 	defer server.Close()
 	defer client.Close()
 
-	wSess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite)
+	wSess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, testTranscript())
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -1100,7 +1100,7 @@ func TestClientReportsANilConnectionRatherThanPanicking(t *testing.T) {
 func TestReadTicketsRejectsALooseButInexactBody(t *testing.T) {
 	cover := mustCover(t, "www.microsoft.com")
 	newSess := func() *Session {
-		sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite)
+		sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, testTranscript())
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -1162,7 +1162,7 @@ func TestReadTicketsRejectsALooseButInexactBody(t *testing.T) {
 func TestReadTicketsRejectsAnOversizedCompanionBody(t *testing.T) {
 	cover := mustCover(t, "www.microsoft.com")
 	newSess := func() *Session {
-		sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite)
+		sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, testTranscript())
 		if err != nil {
 			t.Fatal(err)
 		}

opening_test.go

@@ -351,7 +351,7 @@ func TestCloseSendsCloseNotify(t *testing.T) {
 	for _, host := range MeasuredCovers() {
 		t.Run(host, func(t *testing.T) {
 			cover := mustCover(t, host)
-			sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite)
+			sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, testTranscript())
 			if err != nil {
 				t.Fatal(err)
 			}

shaping_test.go

@@ -197,7 +197,7 @@ func TestCoalescingMergesConcurrentWrites(t *testing.T) {
 	a, b := net.Pipe()
 	defer a.Close()
 	defer b.Close()
-	sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), TLS_AES_128_GCM_SHA256)
+	sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), TLS_AES_128_GCM_SHA256, testTranscript())
 	if err != nil {
 		t.Fatal(err)
 	}

tamper_test.go

@@ -0,0 +1,190 @@
+package twiddle
+
+import (
+	"encoding/binary"
+	"io"
+	"net"
+	"testing"
+	"time"
+)
+
+// tamperingProxy relays between client and server, flipping one bit in the Nth
+// record the SERVER sends. Record 0 is the ServerHello, record 1 the
+// ChangeCipherSpec.
+func tamperingProxy(t *testing.T, upstream string, record, offset int) net.Listener {
+	t.Helper()
+	ln, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	go func() {
+		c, err := ln.Accept()
+		if err != nil {
+			return
+		}
+		defer c.Close()
+		up, err := net.Dial("tcp", upstream)
+		if err != nil {
+			return
+		}
+		defer up.Close()
+		go io.Copy(up, c)
+
+		for i := 0; ; i++ {
+			var hdr [recordHeaderLen]byte
+			if _, err := io.ReadFull(up, hdr[:]); err != nil {
+				return
+			}
+			body := make([]byte, int(binary.BigEndian.Uint16(hdr[3:5])))
+			if _, err := io.ReadFull(up, body); err != nil {
+				return
+			}
+			rec := append(hdr[:], body...)
+			if i == record && offset >= 0 && offset < len(rec) {
+				rec[offset] ^= 0x01
+			}
+			if _, err := c.Write(rec); err != nil {
+				return
+			}
+			if i >= record {
+				io.Copy(c, up)
+				return
+			}
+		}
+	}()
+	return ln
+}
+
+// An on-path attacker must not be able to alter the opening and have the
+// handshake still succeed. This is an ACTIVE distinguisher, not a passive one:
+// a censor flips one byte in a ServerHello and watches whether the connection
+// survives. A genuine TLS 1.3 client aborts -- RFC 8446 4.1.3 requires the
+// legacy_session_id_echo to match, and the server Finished MACs the whole
+// transcript -- so a peer that sails on is visibly not TLS.
+//
+// Before DeriveSession took a transcript, SIX of the seven fields below were
+// accepted. The client pulled the X25519 half out of the key_share, did the
+// ECDH, and derived keys from the psk and its OWN configured cipher suite, so
+// nothing else in the ServerHello was load-bearing. Only the X25519 half was
+// caught, and only because it breaks the ECDH.
+func TestOnPathServerHelloTamperingBreaksTheHandshake(t *testing.T) {
+	// Offsets into the ServerHello record, from SynthesizeServerHello:
+	//   0 type | 1-2 ver | 3-4 len | 5 hs type | 6-8 hs len | 9-10 legacy_ver
+	//   11-42 random | 43 sid len | 44-75 session_id echo | 76-77 cipher
+	//   78 compression | 79-80 ext len | ...
+	cases := []struct {
+		name   string
+		offset int
+	}{
+		{"ServerHello.random", 20},
+		{"legacy_session_id_echo", 50},
+		{"cipher_suite", 76},
+		{"legacy_version", 10},
+		{"compression_method", 78},
+		{"ML-KEM half of key_share", 200},
+		{"X25519 half of key_share", ServerHelloResumedLen - 16},
+		{"record length header", 4},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if err := tamperedOpening(t, 0, tc.offset); err == nil {
+				t.Errorf("the client completed a handshake whose ServerHello was altered in %s; "+
+					"a real TLS client aborts, so surviving this is an active distinguisher", tc.name)
+			}
+		})
+	}
+}
+
+// The control: the same harness with nothing altered must succeed, or every
+// case above would "pass" for the wrong reason.
+func TestUntamperedOpeningThroughTheProxySucceeds(t *testing.T) {
+	if err := tamperedOpening(t, 0, -1); err != nil {
+		t.Fatalf("an untampered opening failed through the proxy: %v", err)
+	}
+}
+
+// tamperedOpening runs one client/server opening through the proxy and returns
+// the client's error.
+func tamperedOpening(t *testing.T, record, offset int) error {
+	t.Helper()
+	k := ticketKey(t)
+	cover := mustCover(t, "www.microsoft.com")
+	cred, err := k.Issue(1, cover.TicketLen)
+	if err != nil {
+		t.Fatal(err)
+	}
+	srv, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer srv.Close()
+	go func() {
+		c, err := srv.Accept()
+		if err != nil {
+			return
+		}
+		defer c.Close()
+		c.SetDeadline(time.Now().Add(5 * time.Second))
+		if sc, err := Server(c, ServerConfig{
+			TicketKey: k, Cover: cover,
+			MaxAge: time.Hour, Replay: NewReplayCache(16, time.Hour),
+		}); err == nil {
+			sc.Write([]byte("payload"))
+		}
+	}()
+
+	proxy := tamperingProxy(t, srv.Addr().String(), record, offset)
+	defer proxy.Close()
+	raw, err := net.Dial("tcp", proxy.Addr().String())
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer raw.Close()
+	raw.SetDeadline(time.Now().Add(4 * time.Second))
+
+	_, _, err = Client(raw, ClientConfig{Pool: pool(t), Cover: cover, Credential: cred})
+	return err
+}
+
+// The ChangeCipherSpec is in the transcript too.
+//
+// On its own it is a far weaker distinguisher than the ServerHello was -- a
+// fixed 6-byte legacy record with one meaningful byte, so there is almost
+// nothing for a censor to vary. It is covered anyway because covering it costs
+// one argument, and leaving a known hole open because it is small is how the
+// ServerHello hole survived: each individual field looked unimportant.
+func TestChangeCipherSpecTamperingBreaksTheHandshake(t *testing.T) {
+	if err := tamperedOpening(t, 1, 5); err == nil {
+		t.Error("the client completed a handshake whose ChangeCipherSpec was altered in flight")
+	}
+}
+
+// An unbound derivation must not be possible at all, not merely discouraged.
+//
+// Fixed-arity Transcript stops a caller dropping one record; this stops a
+// caller dropping the whole transcript, which is the same silent weakening by a
+// shorter route. Both compile, both produce working keys, and neither fails a
+// test that only checks bytes move -- which is why the check has to be in
+// DeriveSession rather than in a comment.
+func TestDeriveSessionRefusesAnUnboundTranscript(t *testing.T) {
+	for _, tc := range []struct {
+		name       string
+		transcript []byte
+	}{
+		{"nil", nil},
+		{"empty", []byte{}},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := DeriveSession(make([]byte, 32), make([]byte, 32),
+				TLS_AES_128_GCM_SHA256, tc.transcript)
+			if err == nil {
+				t.Error("DeriveSession produced keys bound to nothing; ServerHello tampering would go undetected")
+			}
+		})
+	}
+	// And the bound form still works, or the check would be indiscriminate.
+	if _, err := DeriveSession(make([]byte, 32), make([]byte, 32),
+		TLS_AES_128_GCM_SHA256, testTranscript()); err != nil {
+		t.Errorf("a bound derivation was refused: %v", err)
+	}
+}