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) {