[go] crypto/tls: reject trailing messages after client/server hello
Roland Shoemaker (Gerrit)
Roland Shoemaker has uploaded the change for review![]( "Open in Gerrit")
Commit message
crypto/tls: reject trailing messages after client/server hello
For TLS 1.3, after procesesing the server/client hello, if there isn't a
CCS message, reject the trailing messages which were appended to the
hello messages. This prevents an on-path attacker from injecting
plaintext messages into the handshake.
Fixes #76443
Fixes CVE-2025-61730
Change diff
diff --git a/src/crypto/tls/handshake_client_tls13.go b/src/crypto/tls/handshake_client_tls13.go
index 7018bb2..3458f42 100644
--- a/src/crypto/tls/handshake_client_tls13.go
+++ b/src/crypto/tls/handshake_client_tls13.go
@@ -507,6 +507,11 @@
}
c.curveID = hs.serverHello.serverShare.group
+ if c.hand.Len() != 0 {
+ c.sendAlert(alertUnexpectedMessage)
+ return errors.New("tls: handshake buffer not empty before setting traffic keys")
+ }
+
earlySecret := hs.earlySecret
if !hs.usingPSK {
earlySecret = tls13.NewEarlySecret(hs.suite.hash.New, nil)
diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go
index c5b0552..fe887a8 100644
--- a/src/crypto/tls/handshake_server_tls13.go
+++ b/src/crypto/tls/handshake_server_tls13.go
@@ -767,6 +767,11 @@
return err
}
+ if c.hand.Len() != 0 {
+ c.sendAlert(alertUnexpectedMessage)
+ return errors.New("tls: handshake buffer not empty before setting traffic keys")
+ }
+
earlySecret := hs.earlySecret
if earlySecret == nil {
earlySecret = tls13.NewEarlySecret(hs.suite.hash.New, nil)
diff --git a/src/crypto/tls/handshake_test.go b/src/crypto/tls/handshake_test.go
index 3e2c566..0c0cbb3 100644
--- a/src/crypto/tls/handshake_test.go
+++ b/src/crypto/tls/handshake_test.go
@@ -7,6 +7,7 @@
import (
"bufio"
"bytes"
+ "context"
"crypto/ed25519"
"crypto/x509"
"encoding/hex"
@@ -634,3 +635,110 @@
-----BEGIN TESTING KEY-----
MC4CAQAwBQYDK2VwBCIEINifzf07d9qx3d44e0FSbV4mC/xQxT644RRbpgNpin7I
-----END TESTING KEY-----`)
+
+func TestServerHelloTrailingMessage(t *testing.T) {
+ c, s := localPipe(t)
+ go func() {
+ ctx := context.Background()
+ srv := Server(s, testConfig)
+ clientHello, _, err := srv.readClientHello(ctx)
+ if err != nil {
+ testFatal(t, err)
+ }
+
+ hs := serverHandshakeStateTLS13{
+ c: srv,
+ ctx: ctx,
+ clientHello: clientHello,
+ }
+ if err := hs.processClientHello(); err != nil {
+ testFatal(t, err)
+ }
+ if err := transcriptMsg(hs.clientHello, hs.transcript); err != nil {
+ testFatal(t, err)
+ }
+
+ data, err := hs.hello.marshal()
+ if err != nil {
+ testFatal(t, err)
+ }
+ hs.transcript.Write(data)
+
+ eem := encryptedExtensionsMsg{alpnProtocol: "h2"}
+ eemData, err := eem.marshal()
+ if err != nil {
+ testFatal(t, err)
+ }
+
+ m := len(data) + len(eemData)
+ outBuf := make([]byte, recordHeaderLen)
+ outBuf[0] = byte(recordTypeHandshake)
+ vers := VersionTLS12
+ outBuf[1] = byte(vers >> 8)
+ outBuf[2] = byte(vers)
+ outBuf[3] = byte(m >> 8)
+ outBuf[4] = byte(m)
+ outBuf = append(outBuf, data...)
+ outBuf = append(outBuf, eemData...)
+
+ if _, err := s.Write(outBuf); err != nil {
+ testFatal(t, err)
+ }
+ srv.Close()
+ }()
+
+ cli := Client(c, testConfig)
+ expectedErr := "tls: handshake buffer not empty before setting traffic keys"
+ if err := cli.Handshake(); err == nil {
+ t.Fatal("expected error from incomplete handshake, got nil")
+ } else if err.Error() != expectedErr {
+ t.Fatalf("expected error %q, got %q", expectedErr, err.Error())
+ }
+}
+
+func TestClientHelloTrailingMessage(t *testing.T) {
+ c, s := localPipe(t)
+ go func() {
+ cli := Client(c, testConfig)
+
+ hello, _, _, err := cli.makeClientHello()
+ if err != nil {
+ testFatal(t, err)
+ }
+
+ data, err := hello.marshal()
+ if err != nil {
+ testFatal(t, err)
+ }
+
+ csm := &certificateMsgTLS13{}
+ csmData, err := csm.marshal()
+ if err != nil {
+ testFatal(t, err)
+ }
+
+ m := len(data) + len(csmData)
+ outBuf := make([]byte, recordHeaderLen)
+ outBuf[0] = byte(recordTypeHandshake)
+ vers := VersionTLS12
+ outBuf[1] = byte(vers >> 8)
+ outBuf[2] = byte(vers)
+ outBuf[3] = byte(m >> 8)
+ outBuf[4] = byte(m)
+ outBuf = append(outBuf, data...)
+ outBuf = append(outBuf, csmData...)
+
+ if _, err := c.Write(outBuf); err != nil {
+ testFatal(t, err)
+ }
+ cli.Close()
+ }()
+
+ srv := Server(s, testConfig)
+ expectedErr := "tls: handshake buffer not empty before setting traffic keys"
+ if err := srv.Handshake(); err == nil {
+ t.Fatal("expected error from incomplete handshake, got nil")
+ } else if err.Error() != expectedErr {
+ t.Fatalf("expected error %q, got %q", expectedErr, err.Error())
+ }
+}
Change information
- M src/crypto/tls/handshake_client_tls13.go
- M src/crypto/tls/handshake_server_tls13.go
- M src/crypto/tls/handshake_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. |
Roland Shoemaker (Gerrit)
Roland Shoemaker uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Filippo Valsorda
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")
Related details
- Filippo Valsorda
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")
| Commit-Queue | +1 |
Related details
- Filippo Valsorda
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error {I have some concerns about the current fix.
`setReadTrafficSecret` is invoked before `setWriteTrafficKeys`, which means any alert generated during this window may be sent under older traffic keys (or even plaintext).
Also, adding checks during early traffic is unnecessary, since no handshake messages have been processed yet.
A more reliable approach would be:
1. Add a guard at the very beginning of the HRR handling path to ensure we never process a second ClientHello that arrived as leftover fragments from a previous handshake.
2. Perform the unexpected-message check only after `setWriteTrafficKeys`, ensuring any alert is always encrypted under the correct traffic keys.
This avoids sending plaintext alerts and ensures correctness across both HRR and main handshake flows.
Related details
- Filippo Valsorda
- 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. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error {I have some concerns about the current fix.
`setReadTrafficSecret` is invoked before `setWriteTrafficKeys`, which means any alert generated during this window may be sent under older traffic keys (or even plaintext).
Also, adding checks during early traffic is unnecessary, since no handshake messages have been processed yet.A more reliable approach would be:
1. Add a guard at the very beginning of the HRR handling path to ensure we never process a second ClientHello that arrived as leftover fragments from a previous handshake.
2. Perform the unexpected-message check only after `setWriteTrafficKeys`, ensuring any alert is always encrypted under the correct traffic keys.
This avoids sending plaintext alerts and ensures correctness across both HRR and main handshake flows.
And, since ServerHello has been sent/received and the client or server has changed the key at this time, sending a plaintext Alert will cause the peer to throw a `bad_record_mac` alert (decrypt failed)
Roland Shoemaker (Gerrit)
Roland Shoemaker added 1 comment![]( "Open in Gerrit")
func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error {Coia PrantI have some concerns about the current fix.
`setReadTrafficSecret` is invoked before `setWriteTrafficKeys`, which means any alert generated during this window may be sent under older traffic keys (or even plaintext).
Also, adding checks during early traffic is unnecessary, since no handshake messages have been processed yet.A more reliable approach would be:
1. Add a guard at the very beginning of the HRR handling path to ensure we never process a second ClientHello that arrived as leftover fragments from a previous handshake.
2. Perform the unexpected-message check only after `setWriteTrafficKeys`, ensuring any alert is always encrypted under the correct traffic keys.
This avoids sending plaintext alerts and ensures correctness across both HRR and main handshake flows.
And, since ServerHello has been sent/received and the client or server has changed the key at this time, sending a plaintext Alert will cause the peer to throw a `bad_record_mac` alert (decrypt failed)
Good catch on the alert, I think just inverting the read/write secret calls fixes this issue.
I don't think there is a downside to doing this at all of the read secret callsite changes though. While this has the most impact when switching from plaintext to encrypted records, we really shouldn't be consuming messages at that were encrypted under one secret after we have switched to another and are expecting subsequent messages to be using that secret. While there isn't as explicit abuse that can be used, it is still an unexpected behavior that violates the protocol expectations.
Related details
- Coia Prant
- Filippo Valsorda
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error {Coia PrantI have some concerns about the current fix.
`setReadTrafficSecret` is invoked before `setWriteTrafficKeys`, which means any alert generated during this window may be sent under older traffic keys (or even plaintext).
Also, adding checks during early traffic is unnecessary, since no handshake messages have been processed yet.A more reliable approach would be:
1. Add a guard at the very beginning of the HRR handling path to ensure we never process a second ClientHello that arrived as leftover fragments from a previous handshake.
2. Perform the unexpected-message check only after `setWriteTrafficKeys`, ensuring any alert is always encrypted under the correct traffic keys.
This avoids sending plaintext alerts and ensures correctness across both HRR and main handshake flows.
Roland ShoemakerAnd, since ServerHello has been sent/received and the client or server has changed the key at this time, sending a plaintext Alert will cause the peer to throw a `bad_record_mac` alert (decrypt failed)
Good catch on the alert, I think just inverting the read/write secret calls fixes this issue.
I don't think there is a downside to doing this at all of the read secret callsite changes though. While this has the most impact when switching from plaintext to encrypted records, we really shouldn't be consuming messages at that were encrypted under one secret after we have switched to another and are expecting subsequent messages to be using that secret. While there isn't as explicit abuse that can be used, it is still an unexpected behavior that violates the protocol expectations.
Yes, exactly.
During the HRR path we never switch the read-traffic secret, so it is crucial that we do not accidentally consume leftover fragments from the previous handshake as if they belonged to the second ClientHello. Any such fragments must be rejected before entering the HRR processing path.
Please take special care to ensure that no pre-HRR residual data is interpreted as part of the new ClientHello.
Related details
- Filippo Valsorda
- 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. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
if c.hand.Len() != 0 {This check is performed in `setReadTrafficSecret`, which is unnecessary here; please remove it.
Filippo Valsorda (Gerrit)
Filippo Valsorda added 7 comments![]( "Open in Gerrit")
func (hc *halfConn) setTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) {Add a comment saying this must not be called directly, but only through the Conn wrappers.
func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error {Add a comment saying this must always be called after setWriteTrafficSecret, if they are both being called.
func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error {Coia PrantI have some concerns about the current fix.
`setReadTrafficSecret` is invoked before `setWriteTrafficKeys`, which means any alert generated during this window may be sent under older traffic keys (or even plaintext).
Also, adding checks during early traffic is unnecessary, since no handshake messages have been processed yet.A more reliable approach would be:
1. Add a guard at the very beginning of the HRR handling path to ensure we never process a second ClientHello that arrived as leftover fragments from a previous handshake.
2. Perform the unexpected-message check only after `setWriteTrafficKeys`, ensuring any alert is always encrypted under the correct traffic keys.
This avoids sending plaintext alerts and ensures correctness across both HRR and main handshake flows.
Roland ShoemakerAnd, since ServerHello has been sent/received and the client or server has changed the key at this time, sending a plaintext Alert will cause the peer to throw a `bad_record_mac` alert (decrypt failed)
Coia PrantGood catch on the alert, I think just inverting the read/write secret calls fixes this issue.
I don't think there is a downside to doing this at all of the read secret callsite changes though. While this has the most impact when switching from plaintext to encrypted records, we really shouldn't be consuming messages at that were encrypted under one secret after we have switched to another and are expecting subsequent messages to be using that secret. While there isn't as explicit abuse that can be used, it is still an unexpected behavior that violates the protocol expectations.
Yes, exactly.
During the HRR path we never switch the read-traffic secret, so it is crucial that we do not accidentally consume leftover fragments from the previous handshake as if they belonged to the second ClientHello. Any such fragments must be rejected before entering the HRR processing path.
Please take special care to ensure that no pre-HRR residual data is interpreted as part of the new ClientHello.
I am not sure I understand why the pre-HRR check matters. Since there is no encryption either before or after, any attacker can reorder
Client -> Server: [CH, EXTRA_MSG]
Server -> Client: [HRR]
Client -> Server: [CH]
to
Client -> Server: [CH]
Server -> Client: [HRR]
Client -> Server: [EXTRA_MSG, CH]
to get the exact same server behavior, even if the server checks there are no extra messages in the first flight, right?
(We can still add the check, but I would like to make sure I understand the attack correctly. Also, I'd like to understand if the check in setReadTrafficSecret is sufficient for security.)
}setWriteTrafficSecret doesn't do the check, so this check is being removed.
I don't actually get why this check is here, since we are changing write secrets, but we should be sure we don't need it / know why it was there.
}This is also redundant with setReadTrafficSecret.
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. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
If the client sends the [CH, CH2] fragments inside the same record:
Client → Server: [CH, CH2]
Server → Client: [HRR] [SH] …
then having two ClientHello messages in a single record is a protocol
violation. Without the check, the server will process CH2 and perform
unnecessary ECDHE/key schedule work (a minor DoS and a state-machine
violation).
However, an active attacker can still do:
Client → Server: [CH], [CH2]
Server → Client: [HRR] [SH] …
and this cannot be prevented under the current plaintext architecture.
So the pre-HRR check is not cryptographic protection; it only enforces
state-machine correctness and avoids unnecessary work.
This applies to the client side as well.
Alternatively, adding a cookie to HRR would also address both issues at
once.
For your assessment.
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. |
Coia Prant (Gerrit)
The HRR cookie is indeed designed to prevent the client from “predicting”
the HRR and sending a second ClientHello (CH2) before actually receiving it.
However, an active attacker can simply avoid triggering HRR entirely by sending a supported key share, which bypasses the cookie mechanism altogether.
So whether we add this pre-HRR check or rely on the HRR cookie, both options only address specific malformed-flight cases.
These remarks are only about state-machine correctness and minor DoS hardening.
Both comments are for your consideration.
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. |
Coia Prant (Gerrit)
Coia Prant added 2 comments![]( "Open in Gerrit")
setWriteTrafficSecret doesn't do the check, so this check is being removed.
I don't actually get why this check is here, since we are changing write secrets, but we should be sure we don't need it / know why it was there.
I also have doubts about this check.
Even though it exists, it lacked return, so the flow continues as if nothing happened.
For QUIC, `sendAlert` only calls `c.out.setErrorLocked`, and this error is
checked only in `(*Conn).Write`, not in `writeRecordLocked`.
In other words,
there is no actual alert transmission or handshake abort at this point.
So this check never prevented any behavior in practice.
It looked like a state validation, but without a return (or a QUIC-level abort), it had no real effect.
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. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
Coia PrantsetWriteTrafficSecret doesn't do the check, so this check is being removed.
I don't actually get why this check is here, since we are changing write secrets, but we should be sure we don't need it / know why it was there.
I also have doubts about this check.
Even though it exists, it lacked return, so the flow continues as if nothing happened.For QUIC, `sendAlert` only calls `c.out.setErrorLocked`, and this error is
checked only in `(*Conn).Write`, not in `writeRecordLocked`.
In other words,
there is no actual alert transmission or handshake abort at this point.So this check never prevented any behavior in practice.
It looked like a state validation, but without a return (or a QUIC-level abort), it had no real effect.
This check is still present in other places where it was removed, and the same lack of return does not interrupt the handshake process.
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. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
if !c.buffering {I don't understand why this branch exists.
`c.flush` will attempt to flush the out buffer into `c.conn`.
But for QUIC:
- we never use the record-layer buffer, and
- `c.conn` is always nil, if `c.flush` calling `c.conn.Write` would panic.
This path should be unreachable for QUIC, and its presence is confusing.
I think we probably need a separate CL to clean this up.
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. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
Coia PrantsetWriteTrafficSecret doesn't do the check, so this check is being removed.
I don't actually get why this check is here, since we are changing write secrets, but we should be sure we don't need it / know why it was there.
Coia PrantI also have doubts about this check.
Even though it exists, it lacked return, so the flow continues as if nothing happened.For QUIC, `sendAlert` only calls `c.out.setErrorLocked`, and this error is
checked only in `(*Conn).Write`, not in `writeRecordLocked`.
In other words,
there is no actual alert transmission or handshake abort at this point.So this check never prevented any behavior in practice.
It looked like a state validation, but without a return (or a QUIC-level abort), it had no real effect.
This check is still present in other places where it was removed, and the same lack of return does not interrupt the handshake process.
According to the QUIC specification, QUIC removes legacy TLS 1.3 compatibility layers (for middlebox),
such as explicit session_id, Alert, and ChangeCipherSpec messages.
This check aligns with what we are trying to do, but unfortunately, because it lacks `return`, it has no effect in practice.
Therefore, removing this check is safe and does not affect correctness or security.
I think this might answer your question.
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. |
Coia Prant (Gerrit)
Coia Prant added 1 comment![]( "Open in Gerrit")
Coia PrantsetWriteTrafficSecret doesn't do the check, so this check is being removed.
I don't actually get why this check is here, since we are changing write secrets, but we should be sure we don't need it / know why it was there.
Coia PrantI also have doubts about this check.
Even though it exists, it lacked return, so the flow continues as if nothing happened.For QUIC, `sendAlert` only calls `c.out.setErrorLocked`, and this error is
checked only in `(*Conn).Write`, not in `writeRecordLocked`.
In other words,
there is no actual alert transmission or handshake abort at this point.So this check never prevented any behavior in practice.
It looked like a state validation, but without a return (or a QUIC-level abort), it had no real effect.
Coia PrantThis check is still present in other places where it was removed, and the same lack of return does not interrupt the handshake process.
According to the QUIC specification, QUIC removes legacy TLS 1.3 compatibility layers (for middlebox),
such as explicit session_id, Alert, and ChangeCipherSpec messages.This check aligns with what we are trying to do, but unfortunately, because it lacks `return`, it has no effect in practice.
Therefore, removing this check is safe and does not affect correctness or security.
I think this might answer your question.
After reviewing the original QUIC CL (https://go.dev/cl/493655):
QUIC has two issues that were already raised in the current CL, and I don’t see additional problems.
However, I should clarify one detail:
`c.out.err` is checked when `c.handshakeErr` is non-nil on QUIC, but because the check lacked return, the handshake flow would continue executing and `c.handshakeErr` will not be set.
Unless another error is returned later, and the new error will overwrite `c.out.err`.
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 Go LUCI
Related details
- Coia Prant
- Filippo Valsorda
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 9 comments![]( "Open in Gerrit")
func (hc *halfConn) setTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) {Add a comment saying this must not be called directly, but only through the Conn wrappers.
Done
I don't understand why this branch exists.
`c.flush` will attempt to flush the out buffer into `c.conn`.
But for QUIC:
- we never use the record-layer buffer, and
- `c.conn` is always nil, if `c.flush` calling `c.conn.Write` would panic.
This path should be unreachable for QUIC, and its presence is confusing.
I think we probably need a separate CL to clean this up.
Lets address this in a follow-up.
func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error {Add a comment saying this must always be called after setWriteTrafficSecret, if they are both being called.
Done
func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error {Seems fine to add this additional check.
This check is performed in `setReadTrafficSecret`, which is unnecessary here; please remove it.
Done
I think it depends on how the higher-level QUIC implementation handled alerts. In the experimental x/net QUIC impl, seeing an alert is fatal, so the return was not strictly necessary because the handshake would immediately terminate anyway and the connection would be torn down.
That said, I think we are all in agreement that this didn't really make sense to begin with, so removing it is fine.
This is also redundant with setReadTrafficSecret.
Done
Related details
- Coia Prant
- Filippo Valsorda
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")
| Commit-Queue | +1 |
Coia Prant (Gerrit)
Coia Prant added 2 comments![]( "Open in Gerrit")
This is true for received alerts.
But for the sent alert, after `handshakeFn` exits at handshakeContext, if the `c.handshakeErr != nil` will report a wrapped error (`c.out.err`) to the quic layer.
That is, if the return is missing here, the `c.handshakeErr` is not set and no handshake error is thrown
I think you can force an error to be thrown here without return to see if `quic-go` works.
However, this code has been removed, and there seems to be no need to worry about it.
My suggestion: For future work, all sendAlert should be followed by a return to avoid this "empty check" problem.
Related details
- Filippo Valsorda
- 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")
Related details
- Filippo Valsorda
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 9 comments![]( "Open in Gerrit")
Votes added by Daniel McCarney
| Code-Review | +2 |
9 comments
c.in.setTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret)I think moving this to L1365 is to address the case called out in the comment on `setWriteTrafficSecret` and in earlier reviews w.r.t alerts. If that's accurate, then this it feels like an independent bug fix separate from the trailing message content and probably deserves some explicit mention in the commit message. In an ideal world it might even be fixed ahead of this CR in a separate commit.
A similar reordering happens for the `c.out` vs `c.in` traffic secret assignment in `handshake_server_tls13.go`.
if err := c.quicSetReadSecret(QUICEncryptionLevelApplication, c.cipherSuite, c.in.trafficSecret); err != nil {nit: this is pre-existing, but it seems a little awkward that `Conn.quicSet[Read|Write]Secret` and `halfConn.setTrafficSecret`/`Conn.set[Read|Write]TrafficSecret` all take the same three args (a ciphersuite, a quic enc level, and a secret) but do it in different order; the QUIC version puts the the QUIC enc level first, and the halfConn/Conn functions put it second.
I don't know that it's worth fixing here/now, but occurred to me while looking at the diff.
// read keys, since that can cause messags to be parsed that were encryptedtypo: messags -> messages
if c.hand.Len() != 0 {WDYT about adding unit test coverage for this new HRR check as well?
// TODO: Handle this in setTrafficSecret?Prophetic :-)
m := len(data) + len(eemData)Maybe worth a comment here, or on the `func TestServerHelloTrailingMessage(...)` def, to describe that this is the key part of the test: concatting a single record that contains both the expected server hello and a trailing unencrypted encrypted extensions message, and that `eemData` will be left in the conn buffer after the client processes the expected server hello.
Alternatively, it might be clearer (and reduce some duplication between the client/server tests) if you pulled out a helper to do the `outBuf` construction for two input messages and gave it a name like `conjoinedRecord`?
func TestClientHelloTrailingMessage(t *testing.T) {ditto for this test: maybe worth a comment summarizing the overall approach being used slash switching to a helper for the triggering record construction.
// read keys, since that can cause messags to be parsed that were encryptedtypo: messags -> messages
// TODO(roland): we should merge this check with the similar one in setReadTrafficSecret.Should this TODO be addressed before merge? It seems like a small change.
Related details
- Filippo Valsorda
- 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 Go LUCI, TryBot-Bypass+1 by Roland Shoemaker
Related details
- Filippo Valsorda
- 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 added 8 comments![]( "Open in Gerrit")
c.in.setTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret)I think moving this to L1365 is to address the case called out in the comment on `setWriteTrafficSecret` and in earlier reviews w.r.t alerts. If that's accurate, then this it feels like an independent bug fix separate from the trailing message content and probably deserves some explicit mention in the commit message. In an ideal world it might even be fixed ahead of this CR in a separate commit.
A similar reordering happens for the `c.out` vs `c.in` traffic secret assignment in `handshake_server_tls13.go`.
I think its fine to do these together, since its tightly coupled. I'll add a note to the commit noting this change.
if err := c.quicSetReadSecret(QUICEncryptionLevelApplication, c.cipherSuite, c.in.trafficSecret); err != nil {nit: this is pre-existing, but it seems a little awkward that `Conn.quicSet[Read|Write]Secret` and `halfConn.setTrafficSecret`/`Conn.set[Read|Write]TrafficSecret` all take the same three args (a ciphersuite, a quic enc level, and a secret) but do it in different order; the QUIC version puts the the QUIC enc level first, and the halfConn/Conn functions put it second.
I don't know that it's worth fixing here/now, but occurred to me while looking at the diff.
I agree its weird. I think if we collapse `quicSet[Read|Write]Secret` and `set[Read|Write]TrafficSecret` we can resolve this, but I think we should do this as a follow up.
// read keys, since that can cause messags to be parsed that were encryptedRoland Shoemakertypo: messags -> messages
Done
WDYT about adding unit test coverage for this new HRR check as well?
Done
Maybe worth a comment here, or on the `func TestServerHelloTrailingMessage(...)` def, to describe that this is the key part of the test: concatting a single record that contains both the expected server hello and a trailing unencrypted encrypted extensions message, and that `eemData` will be left in the conn buffer after the client processes the expected server hello.
Alternatively, it might be clearer (and reduce some duplication between the client/server tests) if you pulled out a helper to do the `outBuf` construction for two input messages and gave it a name like `conjoinedRecord`?
Done, I don't think these tests can be reasonably collapsed, since we need to do some confusing setup in a couple of the cases which makes making it generic a bit complicated.
ditto for this test: maybe worth a comment summarizing the overall approach being used slash switching to a helper for the triggering record construction.
Done
// read keys, since that can cause messags to be parsed that were encryptedRoland Shoemakertypo: messags -> messages
Done
// TODO(roland): we should merge this check with the similar one in setReadTrafficSecret.Should this TODO be addressed before merge? It seems like a small change.
I think it's actually a little more complicated to do than it seems, since we don't always do the quic versions in the same places as the pure-TLS versions. We should definitely collapse these, but I don't think it's urgent enough to do now.
Related details
- Filippo Valsorda
- 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![]( "Open in Gerrit")
| Auto-Submit | +1 |
| Commit-Queue | +1 |
Related details
- Filippo Valsorda
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 Go LUCI
Related details
- Filippo Valsorda
- 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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Filippo Valsorda
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Coia Prant (Gerrit)
Coia Prant voted and added 1 comment![]( "Open in Gerrit")
Votes added by Coia Prant
| Code-Review | +1 |
1 comment
func (hs *clientHandshakeStateTLS13) processHelloRetryRequest() error {Perhaps we should add HRR checks to the client as well, but I'm unsure if this will lead to patch fingerprinting.
Let's leave it as it is for now.
Perhaps we should determine BoringSSL's actual behavior on this for reference.
Related details
- Filippo Valsorda
- 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 Go LUCI
Related details
- Filippo Valsorda
- 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 Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Filippo Valsorda
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek voted Code-Review+1![]( "Open in Gerrit")
- Filippo Valsorda
- 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 submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/crypto/tls/handshake_test.go
Insertions: 71, Deletions: 39.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/crypto/tls/quic.go
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/crypto/tls/conn.go
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
Change information
crypto/tls: reject trailing messages after client/server hello
For TLS 1.3, after procesesing the server/client hello, if there isn't a
CCS message, reject the trailing messages which were appended to the
hello messages. This prevents an on-path attacker from injecting
plaintext messages into the handshake.
Additionally, check that we don't have any buffered messages before we
switch the read traffic secret regardless, since any buffered messages
would have been under an old key which is no longer appropriate.
We also invert the ordering of setting the read/write secrets so that if
we fail when changing the read secret we send the alert using the
correct write secret.
Fixes #76443
Fixes CVE-2025-61730
- M src/crypto/tls/conn.go
- M src/crypto/tls/handshake_client_tls13.go
- M src/crypto/tls/handshake_server_tls13.go
- M src/crypto/tls/handshake_test.go
- M src/crypto/tls/quic.go
Code-Review: +2 by Daniel McCarney, +1 by Coia Prant, +1 by Michael Knyszek
TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |