From 7103a0cf0783cb4c7f025b1ef58244f9b555aafa Mon Sep 17 00:00:00 2001 From: yuhan6665 <1588741+yuhan6665@users.noreply.github.com> Date: Sun, 10 May 2026 20:14:34 -0400 Subject: [PATCH] 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 Change-Id: If6ba8ad16f48d5cd5db5574824062ad4244a5b52 Reviewed-on: https://go-review.googlesource.com/c/go/+/724120 LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Knyszek Reviewed-by: Daniel McCarney Reviewed-by: Coia Prant --- conn.go | 39 ++++++++++++++++++++++++++++++++++----- handshake_client_tls13.go | 22 +++++++++++----------- handshake_server_tls13.go | 37 +++++++++++++++++++++++-------------- quic.go | 11 ++++++++++- 4 files changed, 78 insertions(+), 31 deletions(-) diff --git a/conn.go b/conn.go index 0b4fedd..39e299a 100644 --- a/conn.go +++ b/conn.go @@ -232,6 +232,9 @@ func (hc *halfConn) changeCipherSpec() error { return nil } +// setTrafficSecret sets the traffic secret for the given encryption level. setTrafficSecret +// should not be called directly, but rather through the Conn setWriteTrafficSecret and +// setReadTrafficSecret wrapper methods. func (hc *halfConn) setTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) { hc.trafficSecret = secret hc.level = level @@ -1406,9 +1409,6 @@ func (c *Conn) handleKeyUpdate(keyUpdate *keyUpdateMsg) error { return c.in.setErrorLocked(c.sendAlert(alertInternalError)) } - newSecret := cipherSuite.nextTrafficSecret(c.in.trafficSecret) - c.in.setTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret) - if keyUpdate.updateRequested { c.out.Lock() defer c.out.Unlock() @@ -1426,7 +1426,12 @@ func (c *Conn) handleKeyUpdate(keyUpdate *keyUpdateMsg) error { } newSecret := cipherSuite.nextTrafficSecret(c.out.trafficSecret) - c.out.setTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret) + c.setWriteTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret) + } + + newSecret := cipherSuite.nextTrafficSecret(c.in.trafficSecret) + if err := c.setReadTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret); err != nil { + return err } return nil @@ -1643,7 +1648,9 @@ func (c *Conn) handshakeContext(ctx context.Context) (ret error) { // Provide the 1-RTT read secret now that the handshake is complete. // The QUIC layer MUST NOT decrypt 1-RTT packets prior to completing // the handshake (RFC 9001, Section 5.7). - c.quicSetReadSecret(QUICEncryptionLevelApplication, c.cipherSuite, c.in.trafficSecret) + if err := c.quicSetReadSecret(QUICEncryptionLevelApplication, c.cipherSuite, c.in.trafficSecret); err != nil { + return err + } } else { c.out.Lock() a, ok := errors.AsType[alert](c.out.err) @@ -1737,3 +1744,25 @@ func (c *Conn) VerifyHostname(host string) error { } return c.peerCertificates[0].VerifyHostname(host) } + +// setReadTrafficSecret sets the read traffic secret for the given encryption level. If +// being called at the same time as setWriteTrafficSecret, the caller must ensure the call +// to setWriteTrafficSecret happens first so any alerts are sent at the write level. +func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) error { + // Ensure that there are no buffered handshake messages before changing the + // read keys, since that can cause messages to be parsed that were encrypted + // using old keys which are no longer appropriate. + if c.hand.Len() != 0 { + c.sendAlert(alertUnexpectedMessage) + return errors.New("tls: handshake buffer not empty before setting read traffic secret") + } + c.in.setTrafficSecret(suite, level, secret) + return nil +} + +// setWriteTrafficSecret sets the write traffic secret for the given encryption level. If +// being called at the same time as setReadTrafficSecret, the caller must ensure the call +// to setWriteTrafficSecret happens first so any alerts are sent at the write level. +func (c *Conn) setWriteTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte) { + c.out.setTrafficSecret(suite, level, secret) +} diff --git a/handshake_client_tls13.go b/handshake_client_tls13.go index 801a731..bb37e86 100644 --- a/handshake_client_tls13.go +++ b/handshake_client_tls13.go @@ -490,16 +490,17 @@ func (hs *clientHandshakeStateTLS13) establishHandshakeKeys() error { handshakeSecret := earlySecret.HandshakeSecret(sharedKey) clientSecret := handshakeSecret.ClientHandshakeTrafficSecret(hs.transcript) - c.out.setTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, clientSecret) + c.setWriteTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, clientSecret) serverSecret := handshakeSecret.ServerHandshakeTrafficSecret(hs.transcript) - c.in.setTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, serverSecret) + if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, serverSecret); err != nil { + return err + } if c.quic != nil { - if c.hand.Len() != 0 { - c.sendAlert(alertUnexpectedMessage) - } c.quicSetWriteSecret(QUICEncryptionLevelHandshake, hs.suite.id, clientSecret) - c.quicSetReadSecret(QUICEncryptionLevelHandshake, hs.suite.id, serverSecret) + if err := c.quicSetReadSecret(QUICEncryptionLevelHandshake, hs.suite.id, serverSecret); err != nil { + return err + } } err = c.config.writeKeyLog(keyLogLabelClientHandshake, hs.hello.random, clientSecret) @@ -710,7 +711,9 @@ func (hs *clientHandshakeStateTLS13) readServerFinished() error { hs.trafficSecret = hs.masterSecret.ClientApplicationTrafficSecret(hs.transcript) serverSecret := hs.masterSecret.ServerApplicationTrafficSecret(hs.transcript) - c.in.setTrafficSecret(hs.suite, QUICEncryptionLevelApplication, serverSecret) + if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelApplication, serverSecret); err != nil { + return err + } err = c.config.writeKeyLog(keyLogLabelClientTraffic, hs.hello.random, hs.trafficSecret) if err != nil { @@ -813,16 +816,13 @@ func (hs *clientHandshakeStateTLS13) sendClientFinished() error { return err } - c.out.setTrafficSecret(hs.suite, QUICEncryptionLevelApplication, hs.trafficSecret) + c.setWriteTrafficSecret(hs.suite, QUICEncryptionLevelApplication, hs.trafficSecret) if !c.config.SessionTicketsDisabled && c.config.ClientSessionCache != nil { c.resumptionSecret = hs.masterSecret.ResumptionMasterSecret(hs.transcript) } if c.quic != nil { - if c.hand.Len() != 0 { - c.sendAlert(alertUnexpectedMessage) - } c.quicSetWriteSecret(QUICEncryptionLevelApplication, hs.suite.id, hs.trafficSecret) } diff --git a/handshake_server_tls13.go b/handshake_server_tls13.go index 2df5e0f..ff28fe2 100644 --- a/handshake_server_tls13.go +++ b/handshake_server_tls13.go @@ -505,7 +505,9 @@ func (hs *serverHandshakeStateTLS13) checkForResumption() error { return err } earlyTrafficSecret := hs.earlySecret.ClientEarlyTrafficSecret(transcript) - c.quicSetReadSecret(QUICEncryptionLevelEarly, hs.suite.id, earlyTrafficSecret) + if err := c.quicSetReadSecret(QUICEncryptionLevelEarly, hs.suite.id, earlyTrafficSecret); err != nil { + return err + } } c.didResume = true @@ -609,6 +611,14 @@ func (hs *serverHandshakeStateTLS13) sendDummyChangeCipherSpec() error { func (hs *serverHandshakeStateTLS13) doHelloRetryRequest(selectedGroup CurveID) (*keyShare, error) { c := hs.c + // Make sure the client didn't send extra handshake messages alongside + // their initial client_hello. If they sent two client_hello messages, + // we will consume the second before they respond to the server_hello. + if c.hand.Len() != 0 { + c.sendAlert(alertUnexpectedMessage) + return nil, errors.New("tls: handshake buffer not empty before HelloRetryRequest") + } + // The first ClientHello gets double-hashed into the transcript upon a // HelloRetryRequest. See RFC 8446, Section 4.4.1. if err := transcriptMsg(hs.clientHello, hs.transcript); err != nil { @@ -836,17 +846,18 @@ func (hs *serverHandshakeStateTLS13) sendServerParameters() error { } hs.handshakeSecret = earlySecret.HandshakeSecret(hs.sharedKey) - clientSecret := hs.handshakeSecret.ClientHandshakeTrafficSecret(hs.transcript) - c.in.setTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, clientSecret) serverSecret := hs.handshakeSecret.ServerHandshakeTrafficSecret(hs.transcript) - c.out.setTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, serverSecret) + c.setWriteTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, serverSecret) + clientSecret := hs.handshakeSecret.ClientHandshakeTrafficSecret(hs.transcript) + if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, clientSecret); err != nil { + return err + } if c.quic != nil { - if c.hand.Len() != 0 { - c.sendAlert(alertUnexpectedMessage) - } c.quicSetWriteSecret(QUICEncryptionLevelHandshake, hs.suite.id, serverSecret) - c.quicSetReadSecret(QUICEncryptionLevelHandshake, hs.suite.id, clientSecret) + if err := c.quicSetReadSecret(QUICEncryptionLevelHandshake, hs.suite.id, clientSecret); err != nil { + return err + } } err := c.config.writeKeyLog(keyLogLabelClientHandshake, hs.clientHello.random, clientSecret) @@ -990,13 +1001,9 @@ func (hs *serverHandshakeStateTLS13) sendServerFinished() error { hs.trafficSecret = hs.masterSecret.ClientApplicationTrafficSecret(hs.transcript) serverSecret := hs.masterSecret.ServerApplicationTrafficSecret(hs.transcript) - c.out.setTrafficSecret(hs.suite, QUICEncryptionLevelApplication, serverSecret) + c.setWriteTrafficSecret(hs.suite, QUICEncryptionLevelApplication, serverSecret) if c.quic != nil { - if c.hand.Len() != 0 { - // TODO: Handle this in setTrafficSecret? - c.sendAlert(alertUnexpectedMessage) - } c.quicSetWriteSecret(QUICEncryptionLevelApplication, hs.suite.id, serverSecret) } @@ -1226,7 +1233,9 @@ func (hs *serverHandshakeStateTLS13) readClientFinished() error { return errors.New("tls: invalid client finished hash") } - c.in.setTrafficSecret(hs.suite, QUICEncryptionLevelApplication, hs.trafficSecret) + if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelApplication, hs.trafficSecret); err != nil { + return err + } return nil } diff --git a/quic.go b/quic.go index 3f8d6a7..83a5feb 100644 --- a/quic.go +++ b/quic.go @@ -403,13 +403,22 @@ func (c *Conn) quicReadHandshakeBytes(n int) error { return nil } -func (c *Conn) quicSetReadSecret(level QUICEncryptionLevel, suite uint16, secret []byte) { +func (c *Conn) quicSetReadSecret(level QUICEncryptionLevel, suite uint16, secret []byte) error { + // Ensure that there are no buffered handshake messages before changing the + // read keys, since that can cause messages to be parsed that were encrypted + // using old keys which are no longer appropriate. + // TODO(roland): we should merge this check with the similar one in setReadTrafficSecret. + if c.hand.Len() != 0 { + c.sendAlert(alertUnexpectedMessage) + return errors.New("tls: handshake buffer not empty before setting read traffic secret") + } c.quic.events = append(c.quic.events, QUICEvent{ Kind: QUICSetReadSecret, Level: level, Suite: suite, Data: secret, }) + return nil } func (c *Conn) quicSetWriteSecret(level QUICEncryptionLevel, suite uint16, secret []byte) {