From 3c771971524168eecc00e25ea94399c1eddb55b7 Mon Sep 17 00:00:00 2001 From: fabifont Date: Tue, 20 Oct 2026 17:08:11 +0200 Subject: [PATCH] fix(keyshare): keep the private key of every classical key share ApplyPreset generated a key for every classical key share a spec offered but kept only the first as KeyShareKeys.Ecdhe, and the client's key exchange used that key for whichever group the server selected. Firefox offers X25519 and P-256 shares, and the Firefox 105, 120 and 148 parrots follow it, so a server selecting P-256 made the client derive the shared secret with an X25519 key and the handshake failed with "invalid server key share" on the client and a bad record MAC on the server, with or without ECH. Keep every classical share's key by group in KeyShareKeys.Ecdhes, Ecdhe staying the first one, and let the ECDHE key exchange use the key for its own group when there is one. Keys generated by the Go path or by a HelloRetryRequest carry no map and behave as before. --- key_schedule.go | 7 +++++ u_key_shares_test.go | 69 ++++++++++++++++++++++++++++++++++++++++++++ u_parrots.go | 28 +++++++++++------- u_public.go | 6 ++++ 4 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 u_key_shares_test.go diff --git a/key_schedule.go b/key_schedule.go index e423484afc..a15deafcf4 100644 --- a/key_schedule.go +++ b/key_schedule.go @@ -57,6 +57,11 @@ type keySharePrivateKeys struct { mlkem crypto.Decapsulator mlkemEcdhe *ecdh.PrivateKey // [uTLS] seperate ecdhe key for pq keyshare in line with Chrome, instead of reusing ecdhe key like stdlib + + // ecdhes holds the key of every classical share a custom ClientHello + // offered, by group, when it offered more than one; ecdhe is the first. + // Firefox offers X25519 and P-256, and the server may select either. [uTLS] + ecdhes map[CurveID]*ecdh.PrivateKey } // A keyExchange implements a TLS 1.3 KEM. @@ -196,6 +201,8 @@ func (ke *ecdhKeyExchange) clientSharedSecretChooseEcdhe(priv *keySharePrivateKe return nil, errors.New("tls: missing separate hybrid ECDHE private key") } ecdhe = priv.mlkemEcdhe + } else if key := priv.ecdhes[ke.id]; key != nil { + ecdhe = key } if ecdhe == nil { return nil, errors.New("tls: missing ECDHE private key") diff --git a/u_key_shares_test.go b/u_key_shares_test.go new file mode 100644 index 0000000000..896b88baa5 --- /dev/null +++ b/u_key_shares_test.go @@ -0,0 +1,69 @@ +package tls + +import ( + "fmt" + "testing" +) + +// classicalKeyShareGroups lists the groups of a spec's classical key shares, +// the ones a server may select without a HelloRetryRequest. +func classicalKeyShareGroups(spec *ClientHelloSpec) []CurveID { + var groups []CurveID + for _, ext := range spec.Extensions { + if ks, ok := ext.(*KeyShareExtension); ok { + for _, share := range ks.KeyShares { + switch share.Group { + case X25519, CurveP256, CurveP384, CurveP521: + groups = append(groups, share.Group) + } + } + } + } + return groups +} + +// TestUTLSEveryClassicalKeyShareIsUsable checks that a server may select any +// of the classical key shares a spec offers, not only the first: Firefox +// offers X25519 and P-256 shares, and a server preferring P-256 completes the +// handshake with it, without a HelloRetryRequest. +func TestUTLSEveryClassicalKeyShareIsUsable(t *testing.T) { + specs := map[string]*ClientHelloSpec{} + for _, id := range []ClientHelloID{HelloFirefox_105, HelloFirefox_120, HelloFirefox_148} { + spec, err := utlsIdToSpec(id) + if err != nil { + t.Fatal(err) + } + specs[id.Str()] = &spec + } + specs["three classical shares"] = &ClientHelloSpec{ + CipherSuites: []uint16{TLS_AES_128_GCM_SHA256}, + Extensions: []TLSExtension{ + &SNIExtension{}, + &SupportedVersionsExtension{Versions: []uint16{VersionTLS13}}, + &SupportedCurvesExtension{Curves: []CurveID{X25519, CurveP256, CurveP384}}, + &SignatureAlgorithmsExtension{SupportedSignatureAlgorithms: []SignatureScheme{ECDSAWithP256AndSHA256}}, + &KeyShareExtension{KeyShares: []KeyShare{{Group: X25519}, {Group: CurveP256}, {Group: CurveP384}}}, + }, + } + + for name, spec := range specs { + groups := classicalKeyShareGroups(spec) + if len(groups) < 2 { + t.Fatalf("%s: offers %v classical key shares, want at least two", name, groups) + } + for _, group := range groups { + t.Run(fmt.Sprintf("%s/%v", name, group), func(t *testing.T) { + clientConfig := testConfigClient.Clone() + serverConfig := testConfigServer.Clone() + serverConfig.CurvePreferences = []CurveID{group} + _, state, err := testUtlsHandshake(t, clientConfig, serverConfig, spec) + if err != nil { + t.Fatalf("server selecting %v: %v", group, err) + } + if state.CurveID != group || state.HelloRetryRequest { + t.Fatalf("negotiated %v with HelloRetryRequest=%v, want %v without a retry", state.CurveID, state.HelloRetryRequest, group) + } + }) + } + } +} diff --git a/u_parrots.go b/u_parrots.go index fe17382ac7..4780525aa7 100644 --- a/u_parrots.go +++ b/u_parrots.go @@ -3084,6 +3084,21 @@ func (uconn *UConn) applyPresetByID(id ClientHelloID) (err error) { // ApplyPreset should only be used in conjunction with HelloCustom to apply custom specs. // The provided ClientHelloSpec is cloned before per-connection handshake state is applied. +// keepClassicalKeyShareKey records the private key of a classical key share +// the ClientHello offers: the first one as KeyShareKeys.Ecdhe, as before, and +// every one by group in KeyShareKeys.Ecdhes, so that the server may select +// any of them. +func (uconn *UConn) keepClassicalKeyShareKey(group CurveID, key *ecdh.PrivateKey) { + keys := uconn.HandshakeState.State13.KeyShareKeys + if keys.Ecdhe == nil { + keys.Ecdhe = key + } + if keys.Ecdhes == nil { + keys.Ecdhes = make(map[CurveID]*ecdh.PrivateKey) + } + keys.Ecdhes[group] = key +} + func (uconn *UConn) ApplyPreset(p *ClientHelloSpec) error { var err error p = p.clone() @@ -3216,7 +3231,6 @@ func (uconn *UConn) ApplyPreset(p *ClientHelloSpec) error { } } case *KeyShareExtension: - preferredCurveIsSet := false reusableClassicalKeys := make(map[CurveID][]*ecdh.PrivateKey) for i := range ext.KeyShares { curveID := ext.KeyShares[i].Group @@ -3293,11 +3307,7 @@ func (uconn *UConn) ApplyPreset(p *ClientHelloSpec) error { reusableClassicalKeys[curveID] = keysForCurve[1:] ext.KeyShares[i].Data = reusedKey.PublicKey().Bytes() - if !preferredCurveIsSet { - // only do this once for the first non-grease curve - uconn.HandshakeState.State13.KeyShareKeys.Ecdhe = reusedKey - preferredCurveIsSet = true - } + uconn.keepClassicalKeyShareKey(curveID, reusedKey) continue } @@ -3308,11 +3318,7 @@ func (uconn *UConn) ApplyPreset(p *ClientHelloSpec) error { } ext.KeyShares[i].Data = ecdheKey.PublicKey().Bytes() - if !preferredCurveIsSet { - // only do this once for the first non-grease curve - uconn.HandshakeState.State13.KeyShareKeys.Ecdhe = ecdheKey - preferredCurveIsSet = true - } + uconn.keepClassicalKeyShareKey(curveID, ecdheKey) } } case *SupportedVersionsExtension: diff --git a/u_public.go b/u_public.go index f19cf91ed1..d51c6a24a5 100644 --- a/u_public.go +++ b/u_public.go @@ -928,6 +928,10 @@ type KeySharePrivateKeys struct { Ecdhe *ecdh.PrivateKey Mlkem crypto.Decapsulator MlkemEcdhe *ecdh.PrivateKey + // Ecdhes holds the key of every classical share the ClientHello offers, + // by group, so that the server may select any of them; Ecdhe is the + // first one. + Ecdhes map[CurveID]*ecdh.PrivateKey } func (ksp *KeySharePrivateKeys) ToPrivate() *keySharePrivateKeys { @@ -939,6 +943,7 @@ func (ksp *KeySharePrivateKeys) ToPrivate() *keySharePrivateKeys { ecdhe: ksp.Ecdhe, mlkem: ksp.Mlkem, mlkemEcdhe: ksp.MlkemEcdhe, + ecdhes: ksp.Ecdhes, } } @@ -951,5 +956,6 @@ func (ksp *keySharePrivateKeys) ToPublic() *KeySharePrivateKeys { Ecdhe: ksp.ecdhe, Mlkem: ksp.mlkem, MlkemEcdhe: ksp.mlkemEcdhe, + Ecdhes: ksp.ecdhes, } }