[go] crypto/tls: omit PSK in ECH outer client hello
Roland Shoemaker (Gerrit)
Roland Shoemaker has uploaded the change for review![]( "Open in Gerrit")
Commit message
crypto/tls: omit PSK in ECH outer client hello
When using ECH, do not include the PSK extension in the outer hello.
Including the PSK extension allows for a degradation in privacy, as an
on-path attacker can harvest outer client hellos, and then construct new
hellos using the PSK extension and arbitrary guessed SNI values,
replaying them to the target server. If the server rejects the PSK, the
handshake will continue, but if the PSK is accepted, the binder check
will fail.
Thanks to Coia Prant (github.com/rbqvq) for
reporting this issue.
Fixes CVE-2026-42505
Fixes #79282
Change diff
diff --git a/src/crypto/tls/handshake_messages.go b/src/crypto/tls/handshake_messages.go
index aa0b7db..511e073 100644
--- a/src/crypto/tls/handshake_messages.go
+++ b/src/crypto/tls/handshake_messages.go
@@ -5,6 +5,7 @@
package tls
import (
+ "bytes"
"errors"
"fmt"
"slices"
@@ -317,7 +318,8 @@
})
})
}
- if len(m.pskIdentities) > 0 { // pre_shared_key must be the last extension
+ // pre_shared_key must be the last extension
+ if len(m.pskIdentities) > 0 && (echInner || len(m.encryptedClientHello) == 0 || bytes.Equal(m.encryptedClientHello, []byte{byte(innerECHExt)})) {
// RFC 8446, Section 4.2.11
exts.AddUint16(extensionPreSharedKey)
exts.AddUint16LengthPrefixed(func(exts *cryptobyte.Builder) {
diff --git a/src/crypto/tls/handshake_messages_test.go b/src/crypto/tls/handshake_messages_test.go
index fa81a72..6e6f503 100644
--- a/src/crypto/tls/handshake_messages_test.go
+++ b/src/crypto/tls/handshake_messages_test.go
@@ -215,7 +215,16 @@
case 2:
m.pskModes = []uint8{pskModeDHE, pskModePlain}
}
- for i := 0; i < rand.Intn(5); i++ {
+ // clientHelloMsg.marshal uses echInner == false. If m.encryptedClientHello > 0 and
+ // does not equal []byte{innerECHExt}, then the psk extension will be omitted,
+ // so either only emit empty encryptedClientHello and psk, encryptedClientHello with
+ // []byte{innerECHExt} and psk, or random encryptedClientHello and no psk.
+ if rand.Intn(10) > 5 {
+ m.encryptedClientHello = randomBytes(rand.Intn(50)+1, rand)
+ } else {
+ if rand.Intn(10) > 5 {
+ m.encryptedClientHello = []byte{byte(innerECHExt)}
+ }
var psk pskIdentity
psk.obfuscatedTicketAge = uint32(rand.Intn(500000))
psk.label = randomBytes(rand.Intn(500)+1, rand)
@@ -228,9 +237,6 @@
if rand.Intn(10) > 5 {
m.earlyData = true
}
- if rand.Intn(10) > 5 {
- m.encryptedClientHello = randomBytes(rand.Intn(50)+1, rand)
- }
return reflect.ValueOf(m)
}
diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index e2b1aa0..d076d31 100644
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -2379,6 +2379,7 @@
clientConfig.RootCAs.AddCert(secretCert)
clientConfig.RootCAs.AddCert(publicCert)
clientConfig.EncryptedClientHelloConfigList = echConfigList
+ clientConfig.ClientSessionCache = NewLRUClientSessionCache(2)
serverConfig.InsecureSkipVerify = false
serverConfig.Rand = rand.Reader
serverConfig.Time = nil
Change information
- M src/crypto/tls/handshake_messages.go
- M src/crypto/tls/handshake_messages_test.go
- M src/crypto/tls/tls_test.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Roland Shoemaker voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McCarney (Gerrit)
Daniel McCarney added 1 comment![]( "Open in Gerrit")
Did you consider using a GREASE'd PSK in the outer hello instead of removing it outright? E.g. the approach described in [RFC 9849 §6.1.2](https://www.rfc-editor.org/rfc/rfc9849.html#name-grease-psk). That's what we did in [rustls](https://github.com/rustls/rustls/blob/b44c09fbca5172b3f5e5ed6ba2ffe6fcd934e07a/rustls/src/client/ech.rs#L730-L760).
§6.1.1 also says "This is possible because the ClientHelloOuter's "pre_shared_key" extension is either omitted or uses a random binder" so I think omitting it entirely isn't _wrong_, but I also wonder if we should favor the GREASE'd approach instead (or at least mention why it wasn't used) since it does seem to blend in better at face value.
Separately, AFAICT there's no test coverage for the specific fix. Reverting `handshake_messages.go` and then re-running the `/crypto/tls/...` test suite still shows everything passing. I think it would be nice to take a crack at implementing a test that would fail without the relevant fix. Do you think that's workable?
The meat of the `handshake_messages.go` diff seems reasonable to me with the two topics above put to the side 👍
Related details
- Roland Shoemaker
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Roland Shoemaker uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by golang...@luci-project-accounts.iam.gserviceaccount.com
Related details
- Daniel McCarney
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Roland Shoemaker added 1 comment![]( "Open in Gerrit")
Did you consider using a GREASE'd PSK in the outer hello instead of removing it outright? E.g. the approach described in [RFC 9849 §6.1.2](https://www.rfc-editor.org/rfc/rfc9849.html#name-grease-psk). That's what we did in [rustls](https://github.com/rustls/rustls/blob/b44c09fbca5172b3f5e5ed6ba2ffe6fcd934e07a/rustls/src/client/ech.rs#L730-L760).
§6.1.1 also says "This is possible because the ClientHelloOuter's "pre_shared_key" extension is either omitted or uses a random binder" so I think omitting it entirely isn't _wrong_, but I also wonder if we should favor the GREASE'd approach instead (or at least mention why it wasn't used) since it does seem to blend in better at face value.
Separately, AFAICT there's no test coverage for the specific fix. Reverting `handshake_messages.go` and then re-running the `/crypto/tls/...` test suite still shows everything passing. I think it would be nice to take a crack at implementing a test that would fail without the relevant fix. Do you think that's workable?
The meat of the `handshake_messages.go` diff seems reasonable to me with the two topics above put to the side 👍
Discussed out of band last week (I think?). In short, we don't GREASE PSK because it doesn't seem particularly valuable, and is mainly another "make middlebox happy" measure. BoringSSL matches our behavior here as far as I know.
We could revisit that decision, but if so it should be a separate change.
Added a test to make sure the outer PSKs are removed.
Related details
- Daniel McCarney
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel McCarney (Gerrit)
Daniel McCarney voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel McCarney
| Code-Review | +2 |
Roland ShoemakerDid you consider using a GREASE'd PSK in the outer hello instead of removing it outright? E.g. the approach described in [RFC 9849 §6.1.2](https://www.rfc-editor.org/rfc/rfc9849.html#name-grease-psk). That's what we did in [rustls](https://github.com/rustls/rustls/blob/b44c09fbca5172b3f5e5ed6ba2ffe6fcd934e07a/rustls/src/client/ech.rs#L730-L760).
§6.1.1 also says "This is possible because the ClientHelloOuter's "pre_shared_key" extension is either omitted or uses a random binder" so I think omitting it entirely isn't _wrong_, but I also wonder if we should favor the GREASE'd approach instead (or at least mention why it wasn't used) since it does seem to blend in better at face value.
Separately, AFAICT there's no test coverage for the specific fix. Reverting `handshake_messages.go` and then re-running the `/crypto/tls/...` test suite still shows everything passing. I think it would be nice to take a crack at implementing a test that would fail without the relevant fix. Do you think that's workable?
The meat of the `handshake_messages.go` diff seems reasonable to me with the two topics above put to the side 👍
Discussed out of band last week (I think?). In short, we don't GREASE PSK because it doesn't seem particularly valuable, and is mainly another "make middlebox happy" measure. BoringSSL matches our behavior here as far as I know.
We could revisit that decision, but if so it should be a separate change.
Added a test to make sure the outer PSKs are removed.
Discussed out of band last week (I think?).
- Roland Shoemaker
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Roland Shoemaker voted and added 1 comment![]( "Open in Gerrit")
Votes added by Roland Shoemaker
| Auto-Submit | +1 |
| TryBot-Bypass | +1 |
1 comment
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Carlos Amedee (Gerrit)
Carlos Amedee voted Code-Review+1![]( "Open in Gerrit")
- Roland Shoemaker
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
Gopher Robot submitted the change![]( "Open in Gerrit")
Change information
crypto/tls: omit PSK in ECH outer client hello
When using ECH, do not include the PSK extension in the outer hello.
Including the PSK extension allows for a degradation in privacy, as an
on-path attacker can harvest outer client hellos, and then construct new
hellos using the PSK extension and arbitrary guessed SNI values,
replaying them to the target server. If the server rejects the PSK, the
handshake will continue, but if the PSK is accepted, the binder check
will fail.
Thanks to Coia Prant (github.com/rbqvq) for
reporting this issue.
Fixes CVE-2026-42505
Fixes #79282
- M src/crypto/tls/handshake_messages.go
- M src/crypto/tls/handshake_messages_test.go
- M src/crypto/tls/tls_test.go
Code-Review: +1 by Carlos Amedee, +2 by Daniel McCarney
TryBots-Pass: LUCI-TryBot-Result-1 by golang...@luci-project-accounts.iam.gserviceaccount.com, TryBot-Bypass+1 by Roland Shoemaker
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Roland Shoemaker has uploaded the change for review![]( "Open in Gerrit")
Commit message
[release-branch.go1.25] crypto/tls: omit PSK in ECH outer client hello
When using ECH, do not include the PSK extension in the outer hello.
Including the PSK extension allows for a degradation in privacy, as an
on-path attacker can harvest outer client hellos, and then construct new
hellos using the PSK extension and arbitrary guessed SNI values,
replaying them to the target server. If the server rejects the PSK, the
handshake will continue, but if the PSK is accepted, the binder check
will fail.
Thanks to Coia Prant (github.com/rbqvq) for
reporting this issue.
Fixes CVE-2026-42505
Updates #79282
Fixes #80174
Change-Id: Ib3a3c948106a57c1b07b9e61a58cbf757848be18
Reviewed-on: https://go-review.googlesource.com/c/go/+/775960
Auto-Submit: Roland Shoemaker <rol...@golang.org>
TryBot-Bypass: Roland Shoemaker <rol...@golang.org>
Reviewed-by: Daniel McCarney <dan...@binaryparadox.net>
Reviewed-by: Carlos Amedee <car...@golang.org>
(cherry picked from commit 137b8065ab5b485bbde0ed430dd89841c0602bb2)
Change diff
diff --git a/src/crypto/tls/handshake_messages.go b/src/crypto/tls/handshake_messages.go
index aa0b7db..511e073 100644
--- a/src/crypto/tls/handshake_messages.go
+++ b/src/crypto/tls/handshake_messages.go
@@ -5,6 +5,7 @@
package tls
import (
+ "bytes"
"errors"
"fmt"
"slices"
@@ -317,7 +318,8 @@
})
})
}
- if len(m.pskIdentities) > 0 { // pre_shared_key must be the last extension
+ // pre_shared_key must be the last extension
+ if len(m.pskIdentities) > 0 && (echInner || len(m.encryptedClientHello) == 0 || bytes.Equal(m.encryptedClientHello, []byte{byte(innerECHExt)})) {
// RFC 8446, Section 4.2.11
exts.AddUint16(extensionPreSharedKey)
exts.AddUint16LengthPrefixed(func(exts *cryptobyte.Builder) {
diff --git a/src/crypto/tls/handshake_messages_test.go b/src/crypto/tls/handshake_messages_test.go
index fa81a72..80a7e76 100644@@ -587,3 +593,88 @@
t.Fatal("Unmarshaled ServerHello with duplicate extensions")
}
}
+
+func TestECHRemoveOuterPSK(t *testing.T) {
+ r := rand.New(rand.NewSource(0))
+
+ for _, tc := range []struct {
+ name string
+ echInner bool
+ echExt []byte
+ expectRemoved bool
+ }{
+ {
+ name: "echInner true",
+ echInner: true,
+ expectRemoved: false,
+ },
+ {
+ name: "echInner true, no ech ext",
+ echInner: true,
+ expectRemoved: false,
+ },
+ {
+ name: "echInner true, ech ext present",
+ echInner: true,
+ echExt: []byte{254},
+ expectRemoved: false,
+ },
+ {
+ name: "echInner true, ech ext present, inner ech sentinel",
+ echInner: true,
+ echExt: []byte{byte(innerECHExt)},
+ expectRemoved: false,
+ },
+ {
+ name: "echInner false, no ech ext",
+ echInner: false,
+ expectRemoved: false,
+ },
+ {
+ name: "echInner false, ech ext present",
+ echInner: false,
+ echExt: []byte{254},
+ expectRemoved: true,
+ },
+ {
+ name: "echInner false, ech ext present, inner ech sentinel",
+ echInner: false,
+ echExt: []byte{byte(innerECHExt)},
+ expectRemoved: false,
+ },
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ ch := (&clientHelloMsg{}).Generate(r, 0).Interface().(*clientHelloMsg)
+
+ ch.pskBinders = [][]byte{[]byte("test")}
+ ch.pskIdentities = []pskIdentity{{label: []byte("test")}}
+ ch.encryptedClientHello = tc.echExt
+
+ b, err := ch.marshalMsg(tc.echInner)
+ if err != nil {
+ t.Fatal(err)
+ }
+ var rch clientHelloMsg
+ if !rch.unmarshal(b) {
+ t.Fatal("Failed to unmarshal ClientHello")
+ }
+
+ if tc.expectRemoved {
+ if rch.pskIdentities != nil {
+ t.Error("expected PSK identities to be removed")
+ }
+ if rch.pskBinders != nil {
+ t.Error("expected PSK binders to be removed")
+ }
+ } else {
+ if rch.pskIdentities == nil {
+ t.Error("expected PSK identities to be present")
+ }
+ if rch.pskBinders == nil {
+ t.Error("expected PSK binders to be present")
+ }
+ }
+ })
+ }
+
+}
diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index 40817a2..7ee34a2 100644
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -2328,6 +2328,7 @@
clientConfig.RootCAs.AddCert(secretCert)
clientConfig.RootCAs.AddCert(publicCert)
clientConfig.EncryptedClientHelloConfigList = echConfigList
+ clientConfig.ClientSessionCache = NewLRUClientSessionCache(2)
serverConfig.InsecureSkipVerify = false
serverConfig.Rand = rand.Reader
serverConfig.Time = nil
Change information
- M src/crypto/tls/handshake_messages.go
- M src/crypto/tls/handshake_messages_test.go
- M src/crypto/tls/tls_test.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Roland Shoemaker (Gerrit)
Roland Shoemaker has uploaded the change for review![]( "Open in Gerrit")
Commit message
[release-branch.go1.26] crypto/tls: omit PSK in ECH outer client hello
When using ECH, do not include the PSK extension in the outer hello.
Including the PSK extension allows for a degradation in privacy, as an
on-path attacker can harvest outer client hellos, and then construct new
hellos using the PSK extension and arbitrary guessed SNI values,
replaying them to the target server. If the server rejects the PSK, the
handshake will continue, but if the PSK is accepted, the binder check
will fail.
Thanks to Coia Prant (github.com/rbqvq) for
reporting this issue.
Fixes CVE-2026-42505
Updates #79282
Fixes #80175
Change diff
index 8632239..655f58b 100644
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -2379,6 +2379,7 @@