From 88561ff8c7c08cdd0b3f99656d544eae48785afe Mon Sep 17 00:00:00 2001 From: Marten Seemann Date: Sat, 25 Feb 2017 10:13:34 +0700 Subject: [PATCH] prevent sending of unencrypted stream data on data streams fixes #446 --- packet_packer.go | 9 ++++++ packet_packer_test.go | 53 ++++++++++++++++++++++++++++++++++- session_test.go | 64 ++++++++++++++++++++++++------------------- 3 files changed, 97 insertions(+), 29 deletions(-) diff --git a/packet_packer.go b/packet_packer.go index 43499379..fafb70be 100644 --- a/packet_packer.go +++ b/packet_packer.go @@ -8,6 +8,7 @@ import ( "github.com/lucas-clemente/quic-go/frames" "github.com/lucas-clemente/quic-go/handshake" "github.com/lucas-clemente/quic-go/protocol" + "github.com/lucas-clemente/quic-go/qerr" ) type packedPacket struct { @@ -127,7 +128,11 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, lea payloadStartIndex := buffer.Len() + var hasNonCryptoStreamData bool // does this frame contain any stream frame on a stream > 1 for _, frame := range payloadFrames { + if sf, ok := frame.(*frames.StreamFrame); ok && sf.StreamID != 1 { + hasNonCryptoStreamData = true + } err := frame.Write(buffer, p.version) if err != nil { return nil, err @@ -142,6 +147,10 @@ func (p *packetPacker) packPacket(stopWaitingFrame *frames.StopWaitingFrame, lea _, encryptionLevel := p.cryptoSetup.Seal(raw[payloadStartIndex:payloadStartIndex], raw[payloadStartIndex:], currentPacketNumber, raw[:payloadStartIndex]) raw = raw[0 : buffer.Len()+12] + if hasNonCryptoStreamData && encryptionLevel <= protocol.EncryptionUnencrypted { + return nil, qerr.AttemptToSendUnencryptedStreamData + } + num := p.packetNumberGenerator.Pop() if num != currentPacketNumber { return nil, errors.New("PacketPacker BUG: Peeked and Popped packet numbers do not match.") diff --git a/packet_packer_test.go b/packet_packer_test.go index f2c0f836..b76918a5 100644 --- a/packet_packer_test.go +++ b/packet_packer_test.go @@ -5,6 +5,7 @@ import ( "github.com/lucas-clemente/quic-go/frames" "github.com/lucas-clemente/quic-go/protocol" + "github.com/lucas-clemente/quic-go/qerr" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) @@ -46,7 +47,7 @@ var _ = Describe("Packet packer", func() { streamFramer = newStreamFramer(newStreamsMap(nil, protocol.PerspectiveServer, cpm), fcm) packer = &packetPacker{ - cryptoSetup: &mockCryptoSetup{}, + cryptoSetup: &mockCryptoSetup{encLevelSeal: protocol.EncryptionForwardSecure}, connectionParameters: cpm, packetNumberGenerator: newPacketNumberGenerator(protocol.SkipPacketAveragePeriodLength), streamFramer: streamFramer, @@ -429,6 +430,56 @@ var _ = Describe("Packet packer", func() { Expect(err).ToNot(HaveOccurred()) Expect(payloadFrames).To(HaveLen(1)) }) + + It("refuses to send unencrypted stream data on a data stream", func() { + packer.cryptoSetup.(*mockCryptoSetup).encLevelSeal = protocol.EncryptionUnencrypted + f := &frames.StreamFrame{ + StreamID: 3, + Data: []byte("foobar"), + } + streamFramer.AddFrameForRetransmission(f) + _, err := packer.PackPacket(nil, nil, 0) + Expect(err).To(MatchError(qerr.AttemptToSendUnencryptedStreamData)) + }) + + It("sends encrypted, non forward-secure, stream data on a data stream", func() { + packer.cryptoSetup.(*mockCryptoSetup).encLevelSeal = protocol.EncryptionSecure + f := &frames.StreamFrame{ + StreamID: 5, + Data: []byte("foobar"), + } + streamFramer.AddFrameForRetransmission(f) + p, err := packer.PackPacket(nil, nil, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(p.encryptionLevel).To(Equal(protocol.EncryptionSecure)) + Expect(p.frames[0]).To(Equal(f)) + }) + + It("sends unencrypted stream data on the crypto stream", func() { + packer.cryptoSetup.(*mockCryptoSetup).encLevelSeal = protocol.EncryptionUnencrypted + f := &frames.StreamFrame{ + StreamID: 1, + Data: []byte("foobar"), + } + streamFramer.AddFrameForRetransmission(f) + p, err := packer.PackPacket(nil, nil, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(p.encryptionLevel).To(Equal(protocol.EncryptionUnencrypted)) + Expect(p.frames[0]).To(Equal(f)) + }) + + It("sends encrypted stream data on the crypto stream", func() { + packer.cryptoSetup.(*mockCryptoSetup).encLevelSeal = protocol.EncryptionSecure + f := &frames.StreamFrame{ + StreamID: 1, + Data: []byte("foobar"), + } + streamFramer.AddFrameForRetransmission(f) + p, err := packer.PackPacket(nil, nil, 0) + Expect(err).ToNot(HaveOccurred()) + Expect(p.encryptionLevel).To(Equal(protocol.EncryptionSecure)) + Expect(p.frames[0]).To(Equal(f)) + }) }) Context("Blocked frames", func() { diff --git a/session_test.go b/session_test.go index 96f6c195..a98bc019 100644 --- a/session_test.go +++ b/session_test.go @@ -803,6 +803,10 @@ var _ = Describe("Session", func() { }) Context("retransmissions", func() { + BeforeEach(func() { + sess.packer.cryptoSetup = &mockCryptoSetup{encLevelSeal: protocol.EncryptionForwardSecure} + }) + It("sends a StreamFrame from a packet queued for retransmission", func() { // a StopWaitingFrame is added, so make sure the packet number of the new package is higher than the packet number of the retransmitted packet sess.packer.packetNumberGenerator.next = 0x1337 + 9 @@ -954,9 +958,41 @@ var _ = Describe("Session", func() { sentPackets := sph.(*mockSentPacketHandler).sentPackets Expect(sentPackets).To(BeEmpty()) }) + + It("retransmits RTO packets", func() { + // We simulate consistently low RTTs, so that the test works faster + n := protocol.PacketNumber(10) + for p := protocol.PacketNumber(1); p < n; p++ { + err := sess.sentPacketHandler.SentPacket(&ackhandler.Packet{PacketNumber: p, Length: 1}) + Expect(err).NotTo(HaveOccurred()) + time.Sleep(time.Microsecond) + ack := &frames.AckFrame{} + ack.LargestAcked = p + err = sess.sentPacketHandler.ReceivedAck(ack, p, time.Now()) + Expect(err).NotTo(HaveOccurred()) + } + sess.packer.packetNumberGenerator.next = n + 1 + // Now, we send a single packet, and expect that it was retransmitted later + err := sess.sentPacketHandler.SentPacket(&ackhandler.Packet{ + PacketNumber: n, + Length: 1, + Frames: []frames.Frame{&frames.StreamFrame{ + Data: []byte("foobar"), + }}, + }) + Expect(err).NotTo(HaveOccurred()) + go sess.run() + sess.scheduleSending() + Eventually(func() [][]byte { return mconn.written }).ShouldNot(BeEmpty()) + Expect(mconn.written[0]).To(ContainSubstring("foobar")) + }) }) Context("scheduling sending", func() { + BeforeEach(func() { + sess.packer.cryptoSetup = &mockCryptoSetup{encLevelSeal: protocol.EncryptionForwardSecure} + }) + It("sends after writing to a stream", func(done Done) { Expect(sess.sendingScheduled).NotTo(Receive()) s, err := sess.GetOrOpenStream(3) @@ -1199,34 +1235,6 @@ var _ = Describe("Session", func() { close(done) }, 0.5) - It("retransmits RTO packets", func() { - // We simulate consistently low RTTs, so that the test works faster - n := protocol.PacketNumber(10) - for p := protocol.PacketNumber(1); p < n; p++ { - err := sess.sentPacketHandler.SentPacket(&ackhandler.Packet{PacketNumber: p, Length: 1}) - Expect(err).NotTo(HaveOccurred()) - time.Sleep(time.Microsecond) - ack := &frames.AckFrame{} - ack.LargestAcked = p - err = sess.sentPacketHandler.ReceivedAck(ack, p, time.Now()) - Expect(err).NotTo(HaveOccurred()) - } - sess.packer.packetNumberGenerator.next = n + 1 - // Now, we send a single packet, and expect that it was retransmitted later - err := sess.sentPacketHandler.SentPacket(&ackhandler.Packet{ - PacketNumber: n, - Length: 1, - Frames: []frames.Frame{&frames.StreamFrame{ - Data: []byte("foobar"), - }}, - }) - Expect(err).NotTo(HaveOccurred()) - go sess.run() - sess.scheduleSending() - Eventually(func() [][]byte { return mconn.written }).ShouldNot(BeEmpty()) - Expect(mconn.written[0]).To(ContainSubstring("foobar")) - }) - Context("getting streams", func() { It("returns a new stream", func() { str, err := sess.GetOrOpenStream(11)