fix a race condition in CryptoSetup

CryptoSetup's AEADs were changed between calls to
DiversificationNonce() and Seal()

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 {

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 {

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(),