From 7cafb4140d5616f1a0316a194a977b0794cd7d7c Mon Sep 17 00:00:00 2001 From: Roland Shoemaker Date: Mon, 23 Mar 2026 11:54:41 -0700 Subject: [release-branch.go1.26] crypto/tls: prevent deadlock when client sends multiple key update messages When we made setReadTrafficSecret send an alert when there are pending handshake messages, we introduced a deadlock when the client sends multiple key update messages that request a response, as handleKeyUpdate will lock the mutex, and defer the unlocking until the end of the function, but setReadTrafficSecret called sendAlert in the failure case, which also tries to lock the mutex. Add an argument to setReadTrafficSecret which lets the caller indicate if the mutex is already locked, and if so, call sendAlertLocked instead of sendAlert. Thanks to Jakub Ciolek for reporting this issue. Fixes #78334 Fixes CVE-2026-32283 Change-Id: Id8e56974233c910e0d66ba96eafbd2ea57832610 Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/3881 Reviewed-by: Damien Neil Reviewed-by: Nicholas Husin Reviewed-on: https://go-internal-review.googlesource.com/c/go/+/4004 Reviewed-on: https://go-review.googlesource.com/c/go/+/763548 Auto-Submit: Gopher Robot Reviewed-by: Junyang Shao TryBot-Bypass: Gopher Robot Reviewed-by: David Chase --- src/crypto/tls/conn.go | 10 +++++-- src/crypto/tls/handshake_client_tls13.go | 4 +-- src/crypto/tls/handshake_server_tls13.go | 4 +-- src/crypto/tls/handshake_test.go | 48 ++++++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 7 deletions(-) diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go index 9c662ef8f6..cfadec68f1 100644 --- a/src/crypto/tls/conn.go +++ b/src/crypto/tls/conn.go @@ -1363,7 +1363,7 @@ func (c *Conn) handleKeyUpdate(keyUpdate *keyUpdateMsg) error { } newSecret := cipherSuite.nextTrafficSecret(c.in.trafficSecret) - if err := c.setReadTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret); err != nil { + if err := c.setReadTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret, keyUpdate.updateRequested); err != nil { return err } @@ -1683,12 +1683,16 @@ func (c *Conn) VerifyHostname(host string) error { // 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 { +func (c *Conn) setReadTrafficSecret(suite *cipherSuiteTLS13, level QUICEncryptionLevel, secret []byte, locked bool) 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) + if locked { + c.sendAlertLocked(alertUnexpectedMessage) + } else { + c.sendAlert(alertUnexpectedMessage) + } return errors.New("tls: handshake buffer not empty before setting read traffic secret") } c.in.setTrafficSecret(suite, level, secret) diff --git a/src/crypto/tls/handshake_client_tls13.go b/src/crypto/tls/handshake_client_tls13.go index 77a24b4a78..65177767a0 100644 --- a/src/crypto/tls/handshake_client_tls13.go +++ b/src/crypto/tls/handshake_client_tls13.go @@ -492,7 +492,7 @@ func (hs *clientHandshakeStateTLS13) establishHandshakeKeys() error { clientSecret := handshakeSecret.ClientHandshakeTrafficSecret(hs.transcript) c.setWriteTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, clientSecret) serverSecret := handshakeSecret.ServerHandshakeTrafficSecret(hs.transcript) - if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, serverSecret); err != nil { + if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, serverSecret, false); err != nil { return err } @@ -711,7 +711,7 @@ func (hs *clientHandshakeStateTLS13) readServerFinished() error { hs.trafficSecret = hs.masterSecret.ClientApplicationTrafficSecret(hs.transcript) serverSecret := hs.masterSecret.ServerApplicationTrafficSecret(hs.transcript) - if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelApplication, serverSecret); err != nil { + if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelApplication, serverSecret, false); err != nil { return err } diff --git a/src/crypto/tls/handshake_server_tls13.go b/src/crypto/tls/handshake_server_tls13.go index bce94ed2d8..b45d7cbc53 100644 --- a/src/crypto/tls/handshake_server_tls13.go +++ b/src/crypto/tls/handshake_server_tls13.go @@ -752,7 +752,7 @@ func (hs *serverHandshakeStateTLS13) sendServerParameters() error { serverSecret := hs.handshakeSecret.ServerHandshakeTrafficSecret(hs.transcript) c.setWriteTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, serverSecret) clientSecret := hs.handshakeSecret.ClientHandshakeTrafficSecret(hs.transcript) - if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, clientSecret); err != nil { + if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelHandshake, clientSecret, false); err != nil { return err } @@ -1136,7 +1136,7 @@ func (hs *serverHandshakeStateTLS13) readClientFinished() error { return errors.New("tls: invalid client finished hash") } - if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelApplication, hs.trafficSecret); err != nil { + if err := c.setReadTrafficSecret(hs.suite, QUICEncryptionLevelApplication, hs.trafficSecret, false); err != nil { return err } diff --git a/src/crypto/tls/handshake_test.go b/src/crypto/tls/handshake_test.go index 9cea8182d0..6e00b4348e 100644 --- a/src/crypto/tls/handshake_test.go +++ b/src/crypto/tls/handshake_test.go @@ -778,3 +778,51 @@ func concatHandshakeMessages(msgs ...handshakeMessage) ([]byte, error) { outBuf = append(outBuf, marshalled...) return outBuf, nil } + +func TestMultipleKeyUpdate(t *testing.T) { + for _, requestUpdate := range []bool{true, false} { + t.Run(fmt.Sprintf("requestUpdate=%t", requestUpdate), func(t *testing.T) { + + c, s := localPipe(t) + cfg := testConfig.Clone() + cfg.MinVersion = VersionTLS13 + cfg.MaxVersion = VersionTLS13 + client := Client(c, testConfig) + server := Server(s, testConfig) + + clientHandshakeDone := make(chan struct{}) + go func() { + if err := client.Handshake(); err != nil { + } + close(clientHandshakeDone) + io.Copy(io.Discard, server) + }() + + if err := server.Handshake(); err != nil { + t.Fatalf("server handshake failed: %v\n", err) + } + <-clientHandshakeDone + + c.SetReadDeadline(time.Now().Add(1 * time.Second)) + s.SetReadDeadline(time.Now().Add(1 * time.Second)) + + kuMsg, err := (&keyUpdateMsg{updateRequested: requestUpdate}).marshal() + if err != nil { + t.Fatalf("failed to marshal key update message: %v", err) + } + + client.out.Lock() + if _, err := client.writeRecordLocked(recordTypeHandshake, append(kuMsg, kuMsg...)); err != nil { + t.Fatalf("failed to write key update messages: %v", err) + } + client.out.Unlock() + + _, err = io.Copy(io.Discard, client) + if err == nil { + t.Fatal("expected multiple key update messages to cause an error, got nil") + } else if !strings.HasSuffix(err.Error(), "tls: unexpected message") { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} -- cgit v1.3