From f36bd07c9b54e58a3cd4e8cadf183f665c0028f4 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sun, 6 Sep 2026 09:51:29 +0100 Subject: [PATCH 1/3] Bind the opening transcript to the traffic keys An on-path attacker could alter the ServerHello and the client completed the handshake anyway. That is an ACTIVE distinguisher and a cheap one: flip a byte, watch whether the connection survives. A genuine TLS 1.3 client aborts -- RFC 8446 4.1.3 requires legacy_session_id_echo to match, and the server Finished MACs the whole transcript -- so a peer that sails on is visibly not TLS. It needs no traffic analysis, works on the first connection, and costs a censor only the connection it probes. Measured before the fix, with a proxy flipping one bit in the ServerHello. SIX of seven fields were accepted: ServerHello.random accepted legacy_session_id_echo accepted cipher_suite accepted legacy_version accepted compression_method accepted ML-KEM half of key_share accepted X25519 half of key_share detected Only the last, and only incidentally: it breaks the ECDH, so the AEAD fails. Nothing else in the ServerHello was load-bearing, because 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. It never read the session_id echo, never compared the cipher suite on the wire, never used the ML-KEM half, and there is no Finished to verify. Fixed the way TLS does it, by binding rather than by adding field-by-field checks: DeriveSession now takes the opening transcript -- ClientHello, ServerHello, ChangeCipherSpec as they went on the wire -- and the transcript hash rides in the HKDF info beside the label, which is where TLS 1.3's Derive-Secret puts it. Any difference between what the server sent and what the client received now yields 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. Field-by-field checks would have been the smaller change and the wrong one: each field individually looks unimportant, which is exactly how this survived review. Binding covers the fields nobody thought to list, including the record length header and the ChangeCipherSpec. Transcript is variadic so that nothing which later joins the opening can be left out by forgetting to widen a signature. This CHANGES THE WIRE PROTOCOL: keys now depend on the transcript, so both ends must move together. That is consistent with what readTickets already documents -- this transport supports no mixed-version deployment, deliberately -- but it means lantern-box needs a bump before either end ships. Found by Codex during a review of the opening. Co-Authored-By: Claude Opus 5 --- conn.go | 57 +++++++++++++++- conn_race_test.go | 2 +- handshake.go | 12 +++- handshake_test.go | 8 +-- opening_test.go | 2 +- shaping_test.go | 2 +- tamper_test.go | 161 ++++++++++++++++++++++++++++++++++++++++++++++ 7 files changed, 231 insertions(+), 13 deletions(-) create mode 100644 tamper_test.go diff --git a/conn.go b/conn.go index cca80d8..01fa5ec 100644 --- a/conn.go +++ b/conn.go @@ -65,7 +65,53 @@ type Session struct { // 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 binds the opening to the keys, exactly as TLS 1.3's transcript +// hash does, and it is the reason a tampered ServerHello is detected at all. +// +// 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. +// It takes the opening records in order -- ClientHello, ServerHello, +// ChangeCipherSpec -- and is variadic so nothing that later joins the opening +// can be left out by forgetting to widen a signature. +func Transcript(records ...[]byte) []byte { + n := 0 + for _, r := range records { + n += len(r) + } + t := make([]byte, 0, n) + for _, r := range records { + t = append(t, r...) + } + return t +} + +// DeriveSession builds traffic keys from the pre-shared key, the ECDH shared +// secret, and the opening transcript. +// +// transcript must be the ClientHello and ServerHello RECORDS as they went on +// the wire, from Transcript. Passing nil derives keys bound to nothing, which +// is what the tampering above exploited; it is accepted only so tests can +// construct matched pairs without running a handshake. +func DeriveSession(psk, shared []byte, suite uint16, transcript []byte) (*Session, error) { var keyLen int switch suite { case TLS_AES_128_GCM_SHA256: @@ -86,12 +132,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 diff --git a/conn_race_test.go b/conn_race_test.go index 9405c48..ac1b719 100644 --- a/conn_race_test.go +++ b/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, nil) if err != nil { t.Fatal(err) } diff --git a/handshake.go b/handshake.go index 6a09b23..0dab5e5 100644 --- a/handshake.go +++ b/handshake.go @@ -187,7 +187,8 @@ 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 } @@ -195,7 +196,11 @@ func Client(raw net.Conn, cfg ClientConfig) (*Conn, *Credential, error) { 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 } diff --git a/handshake_test.go b/handshake_test.go index a9d897b..0d6eb19 100644 --- a/handshake_test.go +++ b/handshake_test.go @@ -406,7 +406,7 @@ 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, nil) if err != nil { t.Fatal(err) } @@ -414,7 +414,7 @@ func TestReadTicketsRejectsNonHandshakeRecords(t *testing.T) { 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, nil) 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, nil) 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, nil) if err != nil { t.Fatal(err) } diff --git a/opening_test.go b/opening_test.go index 99aea6c..cc3db06 100644 --- a/opening_test.go +++ b/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, nil) if err != nil { t.Fatal(err) } diff --git a/shaping_test.go b/shaping_test.go index 2f50f97..0580f4f 100644 --- a/shaping_test.go +++ b/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, nil) if err != nil { t.Fatal(err) } diff --git a/tamper_test.go b/tamper_test.go new file mode 100644 index 0000000..efba03e --- /dev/null +++ b/tamper_test.go @@ -0,0 +1,161 @@ +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")) + time.Sleep(300 * time.Millisecond) + } + }() + + 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") + } +} From 583921cffa16f9604dc745e5cf2ec5cdd7721bbf Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sun, 6 Sep 2026 10:03:12 +0100 Subject: [PATCH 2/3] Fix the transcript contract, and give DeriveSession its doc comment back From the review on #4. Three findings, all correct. Transcript is now FIXED ARITY. I made it variadic and justified that as letting a record which joins the opening later be added without widening a signature. That 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 PR exists to remove. Widening a signature is a compile error at every call site, which is the direction that gets looked at. The comment block introducing Transcript was pasted directly onto the end of DeriveSession's existing doc, so godoc showed prose about psk and Diffie-Hellman labour division as the documentation for Transcript. DeriveSession has its own comment back, Transcript has one that describes Transcript, and the transcript contract now says all three records rather than the two it said before the ChangeCipherSpec was added -- documentation that was already stale when it was written. Dropped a 300ms sleep from the tamper test. Conn.Write blocks until the record reaches the underlying conn, so it bought nothing; five consecutive runs pass without it, and the suite is four seconds shorter. The vulnerability is still caught: binding an empty transcript instead of the real one fails six ServerHello subtests and the ChangeCipherSpec one. Co-Authored-By: Claude Opus 5 --- conn.go | 46 +++++++++++++++++++++++----------------------- tamper_test.go | 1 - 2 files changed, 23 insertions(+), 24 deletions(-) diff --git a/conn.go b/conn.go index 01fa5ec..6416650 100644 --- a/conn.go +++ b/conn.go @@ -62,11 +62,9 @@ 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. -// Transcript binds the opening to the keys, exactly as TLS 1.3's transcript -// hash does, and it is the reason a tampered ServerHello is detected at all. +// 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 @@ -89,28 +87,30 @@ type Session struct { // 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. -// It takes the opening records in order -- ClientHello, ServerHello, -// ChangeCipherSpec -- and is variadic so nothing that later joins the opening -// can be left out by forgetting to widen a signature. -func Transcript(records ...[]byte) []byte { - n := 0 - for _, r := range records { - n += len(r) - } - t := make([]byte, 0, n) - for _, r := range records { - t = append(t, r...) - } - return t +// 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. +// 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 the ClientHello and ServerHello RECORDS as they went on -// the wire, from Transcript. Passing nil derives keys bound to nothing, which -// is what the tampering above exploited; it is accepted only so tests can -// construct matched pairs without running a handshake. +// transcript must be Transcript(ClientHello, ServerHello, ChangeCipherSpec) +// over the records as they went on the wire. Passing nil derives keys bound to +// nothing, which is precisely what the ServerHello tampering exploited; it is +// accepted only so tests can construct matched pairs without a handshake. func DeriveSession(psk, shared []byte, suite uint16, transcript []byte) (*Session, error) { var keyLen int switch suite { diff --git a/tamper_test.go b/tamper_test.go index efba03e..0c11c49 100644 --- a/tamper_test.go +++ b/tamper_test.go @@ -130,7 +130,6 @@ func tamperedOpening(t *testing.T, record, offset int) error { MaxAge: time.Hour, Replay: NewReplayCache(16, time.Hour), }); err == nil { sc.Write([]byte("payload")) - time.Sleep(300 * time.Millisecond) } }() From 24d00c5cd4ae6f5babb3770cfab70a1cd796b92b Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Sun, 6 Sep 2026 10:12:06 +0100 Subject: [PATCH 3/3] Refuse an unbound transcript instead of documenting that it is dangerous From the second review round on #4, and it catches an inconsistency in my own fix. I made Transcript fixed-arity so a caller could not silently drop one opening record, then left DeriveSession(psk, shared, suite, nil) fully legal -- which achieves the same silent weakening by a shorter route. Both compile, both produce working keys, and neither fails a test that only checks bytes move. DeriveSession now errors on an empty transcript. The comment that said nil "is accepted only so tests can construct matched pairs" was the whole problem: the only reason the unbound path existed was test convenience, and a documented footgun in a security-critical API is still a footgun. Tests that need a matched key pair now pass testTranscript(), a fixed stand-in, so no unbound derivation exists anywhere -- including in tests, where one would quietly become the example everyone copies. Mutation-tested: removing the guard fails TestDeriveSessionRefusesAnUnbound- Transcript, which also asserts the bound form still works so the check cannot be indiscriminate. Co-Authored-By: Claude Opus 5 --- conn.go | 14 +++++++++++--- conn_race_test.go | 2 +- conn_test.go | 7 +++++++ handshake_test.go | 8 ++++---- opening_test.go | 2 +- shaping_test.go | 2 +- tamper_test.go | 30 ++++++++++++++++++++++++++++++ 7 files changed, 55 insertions(+), 10 deletions(-) create mode 100644 conn_test.go diff --git a/conn.go b/conn.go index 6416650..f5bc53c 100644 --- a/conn.go +++ b/conn.go @@ -108,10 +108,18 @@ func Transcript(clientHello, serverHello, changeCipherSpec []byte) []byte { // that produced them. // // transcript must be Transcript(ClientHello, ServerHello, ChangeCipherSpec) -// over the records as they went on the wire. Passing nil derives keys bound to -// nothing, which is precisely what the ServerHello tampering exploited; it is -// accepted only so tests can construct matched pairs without a handshake. +// 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: diff --git a/conn_race_test.go b/conn_race_test.go index ac1b719..1d35c3a 100644 --- a/conn_race_test.go +++ b/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, nil) + s, err := DeriveSession(make([]byte, 32), make([]byte, 32), TLS_AES_128_GCM_SHA256, testTranscript()) if err != nil { t.Fatal(err) } diff --git a/conn_test.go b/conn_test.go new file mode 100644 index 0000000..c1880e3 --- /dev/null +++ b/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") } diff --git a/handshake_test.go b/handshake_test.go index 0d6eb19..fe25565 100644 --- a/handshake_test.go +++ b/handshake_test.go @@ -406,7 +406,7 @@ 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, nil) + sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, testTranscript()) if err != nil { t.Fatal(err) } @@ -414,7 +414,7 @@ func TestReadTicketsRejectsNonHandshakeRecords(t *testing.T) { defer server.Close() defer client.Close() - wSess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, nil) + 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, nil) + 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, nil) + sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, testTranscript()) if err != nil { t.Fatal(err) } diff --git a/opening_test.go b/opening_test.go index cc3db06..9b660e0 100644 --- a/opening_test.go +++ b/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, nil) + sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), cover.CipherSuite, testTranscript()) if err != nil { t.Fatal(err) } diff --git a/shaping_test.go b/shaping_test.go index 0580f4f..0a7ba61 100644 --- a/shaping_test.go +++ b/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, nil) + sess, err := DeriveSession(make([]byte, 32), make([]byte, 32), TLS_AES_128_GCM_SHA256, testTranscript()) if err != nil { t.Fatal(err) } diff --git a/tamper_test.go b/tamper_test.go index 0c11c49..c9cdd36 100644 --- a/tamper_test.go +++ b/tamper_test.go @@ -158,3 +158,33 @@ func TestChangeCipherSpecTamperingBreaksTheHandshake(t *testing.T) { 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) + } +}