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 ++++++++++++++++++++++++++++++++++++++++++++++++ diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go index 9c662ef8f67c0ee7ec264d7917c8bdbaa6fe8570..cfadec68f195b85c4db574e6238c41a5a17576c3 100644 --- a/src/crypto/tls/conn.go +++ b/src/crypto/tls/conn.go @@ -1363,7 +1363,7 @@ c.setWriteTrafficSecret(cipherSuite, QUICEncryptionLevelInitial, newSecret) } 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 @@ // 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 77a24b4a78d8fcfb4f67d58bb183e966e45b9f4b..65177767a05b1f5d3461aa9236a3a7fb394403d1 100644 --- a/src/crypto/tls/handshake_client_tls13.go +++ b/src/crypto/tls/handshake_client_tls13.go @@ -492,7 +492,7 @@ 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 @@ // Derive secrets that take context through the server Finished. 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 bce94ed2d8c4369e43fb612931aff9229190933b..b45d7cbc537ffea4157357a596e8540fad225d94 100644 --- a/src/crypto/tls/handshake_server_tls13.go +++ b/src/crypto/tls/handshake_server_tls13.go @@ -752,7 +752,7 @@ 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 @@ c.sendAlert(alertDecryptError) 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 9cea8182d0415474197bcb716f332bdcc7c9b253..6e00b4348e60386eee808f975ea914e8bbb6c0e4 100644 --- a/src/crypto/tls/handshake_test.go +++ b/src/crypto/tls/handshake_test.go @@ -778,3 +778,51 @@ outBuf[4] = byte(m) 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) + } + }) + } +}