From 5ed0182b6756919067517b519925859a36924e48 Mon Sep 17 00:00:00 2001 From: Lucas Clemente Date: Thu, 2 Jun 2016 16:13:06 +0200 Subject: [PATCH] fix a race condition in CryptoSetup CryptoSetup's AEADs were changed between calls to DiversificationNonce() and Seal() --- handshake/crypto_setup.go | 17 ++++++++++++----- packet_packer.go | 16 ++++++++++------ packet_packer_test.go | 4 +--- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/handshake/crypto_setup.go b/handshake/crypto_setup.go index 00752b13..a68c461f 100644 --- a/handshake/crypto_setup.go +++ b/handshake/crypto_setup.go @@ -169,11 +169,8 @@ func (h *CryptoSetup) Open(packetNumber protocol.PacketNumber, associatedData [] return (&crypto.NullAEAD{}).Open(packetNumber, associatedData, ciphertext) } -// Seal a message +// Seal a message, call LockForSealing() before! func (h *CryptoSetup) Seal(packetNumber protocol.PacketNumber, associatedData []byte, plaintext []byte) []byte { - h.mutex.RLock() - defer h.mutex.RUnlock() - if h.receivedForwardSecurePacket { return h.forwardSecureAEAD.Seal(packetNumber, associatedData, plaintext) } else if h.secureAEAD != nil { @@ -308,7 +305,7 @@ func (h *CryptoSetup) handleCHLO(sni string, data []byte, cryptoData map[Tag][]b return reply.Bytes(), nil } -// DiversificationNonce returns a diversification nonce if required in the next packet to be Seal'ed +// DiversificationNonce returns a diversification nonce if required in the next packet to be Seal'ed. See LockForSealing()! func (h *CryptoSetup) DiversificationNonce() []byte { if h.version < protocol.VersionNumber(33) { return nil @@ -319,6 +316,16 @@ func (h *CryptoSetup) DiversificationNonce() []byte { return h.diversificationNonce } +// LockForSealing should be called before Seal(). It is needed so that diversification nonces can be obtained before packets are sealed, and the AEADs are not changed in the meantime. +func (h *CryptoSetup) LockForSealing() { + h.mutex.RLock() +} + +// UnlockForSealing should be called after Seal() is complete, see LockForSealing(). +func (h *CryptoSetup) UnlockForSealing() { + h.mutex.RUnlock() +} + func (h *CryptoSetup) verifyOrCreateSTK(token []byte) ([]byte, error) { err := h.scfg.stkSource.VerifyToken(h.ip, token) if err != nil { diff --git a/packet_packer.go b/packet_packer.go index b18610a7..cd0df09e 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -6,7 +6,6 @@ import ( "sync/atomic" "github.com/lucas-clemente/quic-go/ackhandler" - "github.com/lucas-clemente/quic-go/crypto" "github.com/lucas-clemente/quic-go/frames" "github.com/lucas-clemente/quic-go/handshake" "github.com/lucas-clemente/quic-go/protocol" @@ -23,7 +22,7 @@ type packedPacket struct { type packetPacker struct { connectionID protocol.ConnectionID version protocol.VersionNumber - aead crypto.AEAD + cryptoSetup *handshake.CryptoSetup sentPacketHandler ackhandler.SentPacketHandler connectionParametersManager *handshake.ConnectionParametersManager @@ -35,9 +34,9 @@ type packetPacker struct { lastPacketNumber protocol.PacketNumber } -func newPacketPacker(connectionID protocol.ConnectionID, aead crypto.AEAD, sentPacketHandler ackhandler.SentPacketHandler, connectionParametersHandler *handshake.ConnectionParametersManager, blockedManager *blockedManager, version protocol.VersionNumber) *packetPacker { +func newPacketPacker(connectionID protocol.ConnectionID, cryptoSetup *handshake.CryptoSetup, sentPacketHandler ackhandler.SentPacketHandler, connectionParametersHandler *handshake.ConnectionParametersManager, blockedManager *blockedManager, version protocol.VersionNumber) *packetPacker { return &packetPacker{ - aead: aead, + cryptoSetup: cryptoSetup, connectionID: connectionID, connectionParametersManager: connectionParametersHandler, version: version, @@ -89,13 +88,18 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, con 1, )) + // cryptoSetup needs to be locked here, so that the AEADs are not changed between + // calling DiversificationNonce() and Seal(). + p.cryptoSetup.LockForSealing() + defer p.cryptoSetup.UnlockForSealing() + packetNumberLen := protocol.GetPacketNumberLengthForPublicHeader(currentPacketNumber, p.sentPacketHandler.GetLargestObserved()) responsePublicHeader := &publicHeader{ ConnectionID: p.connectionID, PacketNumber: currentPacketNumber, PacketNumberLen: packetNumberLen, TruncateConnectionID: p.connectionParametersManager.TruncateConnectionID(), - DiversificationNonce: p.aead.DiversificationNonce(), + DiversificationNonce: p.cryptoSetup.DiversificationNonce(), } publicHeaderLength, err := responsePublicHeader.GetLength() @@ -136,7 +140,7 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, con return nil, err } - ciphertext := p.aead.Seal(currentPacketNumber, raw.Bytes(), payload) + ciphertext := p.cryptoSetup.Seal(currentPacketNumber, raw.Bytes(), payload) raw.Write(ciphertext) if protocol.ByteCount(raw.Len()) > protocol.MaxPacketSize { diff --git a/packet_packer_test.go b/packet_packer_test.go index 6243e8ff..0877d19f 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -5,7 +5,6 @@ import ( "time" "github.com/lucas-clemente/quic-go/ackhandler" - "github.com/lucas-clemente/quic-go/crypto" "github.com/lucas-clemente/quic-go/frames" "github.com/lucas-clemente/quic-go/handshake" "github.com/lucas-clemente/quic-go/protocol" @@ -36,9 +35,8 @@ var _ = Describe("Packet packer", func() { ) BeforeEach(func() { - aead := &crypto.NullAEAD{} packer = &packetPacker{ - aead: aead, + cryptoSetup: &handshake.CryptoSetup{}, connectionParametersManager: handshake.NewConnectionParamatersManager(), sentPacketHandler: newMockSentPacketHandler(), blockedManager: newBlockedManager(),