From 361fd2d2b279ae6ab2d6992d37d11224380e9ae7 Mon Sep 17 00:00:00 2001 From: Tatiana Bradley Date: Fri, 2 Aug 2019 15:28:51 +0000 Subject: [PATCH] addressed reviewer comments --- client_test.go | 21 ++---------- integrationtests/self/mitm_test.go | 37 +++++++++----------- internal/testutils/testutils.go | 54 +++++++----------------------- session_test.go | 10 +++--- 4 files changed, 36 insertions(+), 86 deletions(-) diff --git a/client_test.go b/client_test.go index 6b2086f7..0bc1e2ef 100644 --- a/client_test.go +++ b/client_test.go @@ -601,6 +601,8 @@ var _ = Describe("Client", func() { Eventually(cl.versionNegotiated.Get).Should(BeTrue()) }) + // Illustrates that adversary that injects a version negotiation packet + // with no supported versions can break a connection. It("errors if no matching version is found", func() { sess := NewMockQuicSession(mockCtrl) done := make(chan struct{}) @@ -692,24 +694,5 @@ var _ = Describe("Client", func() { cl.handlePacket(composeVersionNegotiationPacket(connID, []protocol.VersionNumber{1234})) Expect(cl.version).To(Equal(ver)) }) - - // Illustrates that adversary that injects a version negotiation packet - // with no supported versions can break a connection. - It("connection fails if no supported versions are found in version negotation packet", func() { - // Copy of existing test "errors if no matching version is found" - sess := NewMockQuicSession(mockCtrl) - done := make(chan struct{}) - sess.EXPECT().destroy(gomock.Any()).Do(func(err error) { - defer GinkgoRecover() - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("No compatible QUIC version found.")) - close(done) - }) - cl.session = sess - cl.config = &Config{Versions: protocol.SupportedVersions} - cl.handlePacket(composeVersionNegotiationPacket(connID, []protocol.VersionNumber{1337})) - Eventually(done).Should(BeClosed()) - }) - }) }) diff --git a/integrationtests/self/mitm_test.go b/integrationtests/self/mitm_test.go index a8fe91f3..0290fd3e 100644 --- a/integrationtests/self/mitm_test.go +++ b/integrationtests/self/mitm_test.go @@ -287,7 +287,7 @@ var _ = Describe("MITM test", func() { // sendForgedVersionNegotiationPacket sends a fake VN packet with no supported versions // from serverConn to client's remoteAddr // expects hdr from an Initial packet intercepted from client - sendForgedVersionNegotationPacket := func(serverConn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) { + sendForgedVersionNegotationPacket := func(conn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) { defer GinkgoRecover() // Create fake version negotiation packet with no supported versions @@ -295,24 +295,22 @@ var _ = Describe("MITM test", func() { packet, _ := wire.ComposeVersionNegotiation(hdr.SrcConnectionID, hdr.DestConnectionID, versions) // Send the packet - if _, err := serverConn.WriteTo(packet, remoteAddr); err != nil { - return - } + _, err := conn.WriteTo(packet, remoteAddr) + Expect(err).ToNot(HaveOccurred()) } // sendForgedRetryPacket sends a fake Retry packet with a modified srcConnID // from serverConn to client's remoteAddr // expects hdr from an Initial packet intercepted from client - sendForgedRetryPacket := func(serverConn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) { + sendForgedRetryPacket := func(conn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) { defer GinkgoRecover() var x byte = 0x12 fakeSrcConnID := protocol.ConnectionID{x, x, x, x, x, x, x, x} retryPacket := testutils.ComposeRetryPacket(fakeSrcConnID, hdr.SrcConnectionID, hdr.DestConnectionID, []byte("token"), hdr.Version) - if _, err := serverConn.WriteTo(retryPacket, remoteAddr); err != nil { - return - } + _, err := conn.WriteTo(retryPacket, remoteAddr) + Expect(err).ToNot(HaveOccurred()) } // Send a forged Initial packet with no frames to client @@ -320,10 +318,9 @@ var _ = Describe("MITM test", func() { sendForgedInitialPacket := func(conn net.PacketConn, remoteAddr net.Addr, hdr *wire.Header) { defer GinkgoRecover() - initialPacket := testutils.ComposeInitialPacket(hdr.DestConnectionID, hdr.SrcConnectionID, hdr.Version, hdr.DestConnectionID, testutils.NoFrame) - if _, err := conn.WriteTo(initialPacket, remoteAddr); err != nil { - return - } + initialPacket := testutils.ComposeInitialPacket(hdr.DestConnectionID, hdr.SrcConnectionID, hdr.Version, hdr.DestConnectionID, nil) + _, err := conn.WriteTo(initialPacket, remoteAddr) + Expect(err).ToNot(HaveOccurred()) } // Send a forged Initial packet with ACK for random packet to client @@ -332,10 +329,10 @@ var _ = Describe("MITM test", func() { defer GinkgoRecover() // Fake Initial with ACK for packet 2 (unsent) - initialPacket := testutils.ComposeInitialPacket(hdr.DestConnectionID, hdr.SrcConnectionID, hdr.Version, hdr.DestConnectionID, testutils.AckFrame) - if _, err := conn.WriteTo(initialPacket, remoteAddr); err != nil { - return - } + ackFrame := testutils.ComposeAckFrame(2, 2) + initialPacket := testutils.ComposeInitialPacket(hdr.DestConnectionID, hdr.SrcConnectionID, hdr.Version, hdr.DestConnectionID, []wire.Frame{ackFrame}) + _, err := conn.WriteTo(initialPacket, remoteAddr) + Expect(err).ToNot(HaveOccurred()) } // runTestFail succeeds if an error occurs in dialing @@ -367,7 +364,7 @@ var _ = Describe("MITM test", func() { hdr, _, _, err := wire.ParsePacket(raw, connIDLen) Expect(err).ToNot(HaveOccurred()) - if !(hdr.Type == protocol.PacketTypeInitial) { + if hdr.Type != protocol.PacketTypeInitial { return 0 } @@ -383,7 +380,6 @@ var _ = Describe("MITM test", func() { // TODO: determine behavior when server does not send Retry packets initialPacketIntercepted := false It("fails when a forged retry packet with modified srcConnID is sent to client", func() { - // serverConfig.AcceptToken = func(net.Addr, *quic.Token) bool { return true } delayCb := func(dir quicproxy.Direction, raw []byte) time.Duration { if dir == quicproxy.DirectionIncoming && !initialPacketIntercepted { defer GinkgoRecover() @@ -407,7 +403,6 @@ var _ = Describe("MITM test", func() { // it has already accepted an initial. // TODO: determine behavior when server does not send Retry packets It("fails when a forged initial packet is sent to client", func() { - // serverConfig.AcceptToken = func(net.Addr, *quic.Token) bool { return true } delayCb := func(dir quicproxy.Direction, raw []byte) time.Duration { if dir == quicproxy.DirectionIncoming { defer GinkgoRecover() @@ -415,7 +410,7 @@ var _ = Describe("MITM test", func() { hdr, _, _, err := wire.ParsePacket(raw, connIDLen) Expect(err).ToNot(HaveOccurred()) - if !(hdr.Type == protocol.PacketTypeInitial) { + if hdr.Type != protocol.PacketTypeInitial { return 0 } @@ -435,7 +430,7 @@ var _ = Describe("MITM test", func() { hdr, _, _, err := wire.ParsePacket(raw, connIDLen) Expect(err).ToNot(HaveOccurred()) - if !(hdr.Type == protocol.PacketTypeInitial) { + if hdr.Type != protocol.PacketTypeInitial { return 0 } diff --git a/internal/testutils/testutils.go b/internal/testutils/testutils.go index 3cb8bdce..c6b711fc 100644 --- a/internal/testutils/testutils.go +++ b/internal/testutils/testutils.go @@ -11,29 +11,18 @@ import ( // Utilities for simulating packet injection and man-in-the-middle (MITM) attacker tests. // Do not use for non-testing purposes. -// CryptoFrameType types copied from unexported messageType in crypto_setup.go +// CryptoFrameType uses same types as messageType in crypto_setup.go type CryptoFrameType uint8 -const ( - //typeClientHello CryptoFrameType = 1 - typeServerHello CryptoFrameType = 2 - //typeNewSessionTicket CryptoFrameType = 4 - //typeEncryptedExtensions CryptoFrameType = 8 - //typeCertificate CryptoFrameType = 11 - //typeCertificateRequest CryptoFrameType = 13 - //typeCertificateVerify CryptoFrameType = 15 - //typeFinished CryptoFrameType = 20 -) - -// getRawPacket returns a new raw packet with the specified header and payload -func getRawPacket(hdr *wire.ExtendedHeader, data []byte) []byte { +// writePacket returns a new raw packet with the specified header and payload +func writePacket(hdr *wire.ExtendedHeader, data []byte) []byte { buf := &bytes.Buffer{} hdr.Write(buf, protocol.VersionTLS) return append(buf.Bytes(), data...) } // packRawPayload returns a new raw payload containing given frames -func packRawPayload(version protocol.VersionNumber, frames ...wire.Frame) []byte { +func packRawPayload(version protocol.VersionNumber, frames []wire.Frame) []byte { buf := new(bytes.Buffer) for _, cf := range frames { cf.Write(buf, version) @@ -61,7 +50,7 @@ func ComposeConnCloseFrame() *wire.ConnectionCloseFrame { } } -// ComposeAckFrame returns a new Ack Frame that ACKs packet 0 +// ComposeAckFrame returns a new Ack Frame that acknowledges all packets between smallest and largest func ComposeAckFrame(smallest protocol.PacketNumber, largest protocol.PacketNumber) *wire.AckFrame { ackRange := wire.AckRange{ Smallest: smallest, @@ -73,36 +62,17 @@ func ComposeAckFrame(smallest protocol.PacketNumber, largest protocol.PacketNumb } } -// InitialContents enumerates possible frames to include in forged Initial packets -type InitialContents int - -const ( - ServerHelloFrame InitialContents = iota + 1 - AckFrame - ConnectionCloseFrame - NoFrame -) - // ComposeInitialPacket returns an Initial packet encrypted under key -// (the original destination connection ID) -// contains frame of specified type -func ComposeInitialPacket(srcConnID protocol.ConnectionID, destConnID protocol.ConnectionID, version protocol.VersionNumber, key protocol.ConnectionID, frameType InitialContents) []byte { +// (the original destination connection ID) containing specified frames +func ComposeInitialPacket(srcConnID protocol.ConnectionID, destConnID protocol.ConnectionID, version protocol.VersionNumber, key protocol.ConnectionID, frames []wire.Frame) []byte { sealer, _, _ := handshake.NewInitialAEAD(key, protocol.PerspectiveServer) // compose payload var payload []byte - switch frameType { - case ServerHelloFrame: - cf := ComposeCryptoFrame(typeServerHello, 20) - payload = packRawPayload(version, cf) - case AckFrame: - ack := ComposeAckFrame(2, 2) // ack packet 2 - payload = packRawPayload(version, ack) - case ConnectionCloseFrame: - ccf := ComposeConnCloseFrame() - payload = packRawPayload(version, ccf) - case NoFrame: + if len(frames) == 0 { payload = make([]byte, protocol.MinInitialPacketSize) + } else { + payload = packRawPayload(version, frames) } // compose Initial header @@ -122,7 +92,7 @@ func ComposeInitialPacket(srcConnID protocol.ConnectionID, destConnID protocol.C PacketNumber: 0x0, } - raw := getRawPacket(hdr, payload) + raw := writePacket(hdr, payload) // encrypt payload and header payloadOffset := len(raw) - payloadSize @@ -152,5 +122,5 @@ func ComposeRetryPacket(srcConnID protocol.ConnectionID, destConnID protocol.Con Version: version, }, } - return getRawPacket(hdr, nil) + return writePacket(hdr, nil) } diff --git a/session_test.go b/session_test.go index 099ec3c7..7c34c2fd 100644 --- a/session_test.go +++ b/session_test.go @@ -1748,11 +1748,12 @@ var _ = Describe("Client Session", func() { Expect(sess.handlePacketImpl(getPacket(hdr2, nil))).To(BeFalse()) }) - // Illustrates that an injected Initial with an ACK frame for an unsent causes + // Illustrates that an injected Initial with an ACK frame for an unsent packet causes // the connection to immediately break down It("fails on Initial-level ACK for unsent packet", func() { sessionRunner.EXPECT().Retire(gomock.Any()) - initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, testutils.AckFrame) + ackFrame := testutils.ComposeAckFrame(0, 0) + initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, []wire.Frame{ackFrame}) Expect(sess.handlePacketImpl(wrapPacket(initialPacket))).To(BeFalse()) }) @@ -1760,7 +1761,8 @@ var _ = Describe("Client Session", func() { // the connection to immediately break down It("fails on Initial-level CONNECTION_CLOSE frame", func() { sessionRunner.EXPECT().Remove(gomock.Any()) - initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, testutils.ConnectionCloseFrame) + connCloseFrame := testutils.ComposeConnCloseFrame() + initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, []wire.Frame{connCloseFrame}) Expect(sess.handlePacketImpl(wrapPacket(initialPacket))).To(BeTrue()) }) @@ -1773,7 +1775,7 @@ var _ = Describe("Client Session", func() { packer.EXPECT().ChangeDestConnectionID(newSrcConnID) sess.handlePacketImpl(wrapPacket(testutils.ComposeRetryPacket(newSrcConnID, sess.destConnID, sess.destConnID, []byte("foobar"), sess.version))) - initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, testutils.NoFrame) + initialPacket := testutils.ComposeInitialPacket(sess.destConnID, sess.srcConnID, sess.version, sess.destConnID, nil) Expect(sess.handlePacketImpl(wrapPacket(initialPacket))).To(BeFalse()) })