CVE-2018-6347: Improper Input Validation (CWE-20)
value: An issue in the Proxygen handling of HTTP2 parsing of headers/trailers can lead to a denial-of-service attack. This affects Proxygen prior to v2018.12.31.00.
CWE description: The product receives input or data, but it does
        not validate or incorrectly validates that the input has the
        properties that are required to process the data safely and
        correctly.
CVSS v3 base score: 7.5
Published: 2018-12-31T22:29Z
Repository: https://github.com/facebook/proxygen
Language: C++
Files changed: proxygen/lib/http/codec/HTTP2Codec.cpp, proxygen/lib/http/codec/test/HTTP2CodecTest.cpp, proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp, proxygen/lib/http/session/test/HTTPSessionMocks.h
Commit date: 2018-12-27 16:14:42 -0800
Commit message: Don't lower lastStreamID_

Summary:
This could throw off the logic about what is headers/trailers
CVE-2018-6347

Reviewed By: avasylev, bolekk

Differential Revision: D13521704

fbshipit-source-id: ed436ff13f191bf04764494973fcb73b35ed1256

Diff: diff --git a/proxygen/lib/http/codec/HTTP2Codec.cpp b/proxygen/lib/http/codec/HTTP2Codec.cpp
index db9a82731..d3095d626 100644
--- a/proxygen/lib/http/codec/HTTP2Codec.cpp
+++ b/proxygen/lib/http/codec/HTTP2Codec.cpp
@@ -985,11 +985,11 @@ ErrorCode HTTP2Codec::checkNewStream(uint32_t streamId, bool trailersAllowed) {
   parsingDownstreamTrailers_ = trailersAllowed && (streamId <= lastStreamID_);
   if (parsingDownstreamTrailers_) {
     VLOG(4) << "Parsing downstream trailers streamId=" << streamId;
   }
 
-  if (sessionClosing_ != ClosingState::CLOSED) {
+  if (sessionClosing_ != ClosingState::CLOSED && streamId > lastStreamID_) {
     lastStreamID_ = streamId;
   }
 
   if (isInitiatedStream(streamId)) {
     // this stream should be initiated by us, not by peer
@@ -1311,10 +1311,11 @@ size_t HTTP2Codec::generateChunkTerminator(folly::IOBufQueue& /*writeBuf*/,
 }
 
 size_t HTTP2Codec::generateTrailers(folly::IOBufQueue& writeBuf,
                                     StreamID stream,
                                     const HTTPHeaders& trailers) {
+  VLOG(4) << "generating TRAILERS for stream=" << stream;
   std::vector<compress::Header> allHeaders;
   CodecUtil::appendHeaders(trailers, allHeaders, HTTP_HEADER_NONE);
 
   HTTPHeaderSize size;
   auto out = encodeHeaders(trailers, allHeaders, &size);
diff --git a/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp b/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
index 22471b13e..c123e0d3a 100644
--- a/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
+++ b/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
@@ -2143,5 +2143,30 @@ TEST_F(HTTP2CodecTest, TrailersReplyMissingContinuation) {
   EXPECT_EQ(callbacks_.sessionErrors, 1);
 #ifndef NDEBUG
   EXPECT_EQ(upstreamCodec_.getReceivedFrameCount(), 4);
 #endif
 }
+
+TEST_F(HTTP2CodecTest, TrailersNotLatest) {
+  HTTPMessage req = getGetRequest("/guacamole");
+  req.getHeaders().add(HTTP_HEADER_USER_AGENT, "coolio");
+  upstreamCodec_.generateHeader(output_, 1, req);
+  upstreamCodec_.generateHeader(output_, 3, req);
+
+  HTTPHeaders trailers;
+  trailers.add("x-trailer-1", "pico-de-gallo");
+  upstreamCodec_.generateTrailers(output_, 1, trailers);
+  upstreamCodec_.generateHeader(output_, 3, req);
+
+  parse();
+
+  EXPECT_EQ(callbacks_.messageBegin, 2);
+  EXPECT_EQ(callbacks_.headersComplete, 2);
+  EXPECT_EQ(callbacks_.bodyCalls, 0);
+  EXPECT_EQ(callbacks_.trailers, 1);
+  EXPECT_NE(nullptr, callbacks_.msg->getTrailers());
+  EXPECT_EQ("pico-de-gallo",
+            callbacks_.msg->getTrailers()->getSingleOrEmpty("x-trailer-1"));
+  EXPECT_EQ(callbacks_.messageComplete, 1);
+  EXPECT_EQ(callbacks_.streamErrors, 1);
+  EXPECT_EQ(callbacks_.sessionErrors, 0);
+}
diff --git a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp
index a54088d95..f3478c52c 100644
--- a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp
+++ b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp
@@ -3566,5 +3566,37 @@ TEST_F(HTTP2DownstreamSessionTest, TestSetEgressSettings) {
   }
 
   flushRequestsAndLoop();
   gracefulShutdown();
 }
+
+TEST_F(HTTP2DownstreamSessionTest, TestDuplicateRequestStream) {
+  // Send the following:
+  // HEADERS id=1
+  // HEADERS id=2
+  // HEADERS id=1 (trailers)
+  // HEADERS id=2 -> contains pseudo-headers after EOM so ignored
+  auto handler2 = addSimpleStrictHandler();
+  auto handler1 = addSimpleStrictHandler();
+  auto streamID1 = sendRequest("/withtrailers", 0, false);
+  auto streamID2 = sendRequest();
+  HTTPHeaders trailers;
+  trailers.add("Foo", "Bar");
+  clientCodec_->generateTrailers(requests_, streamID1, trailers);
+  clientCodec_->generateEOM(requests_, streamID1);
+
+  clientCodec_->generateHeader(requests_, streamID2, getGetRequest(), false);
+  handler1->expectHeaders();
+  handler2->expectHeaders();
+  handler2->expectEOM();
+  handler1->expectTrailers();
+  handler1->expectEOM([&] {
+      handler1->sendReplyWithBody(200, 100);
+      // 2 got an error after EOM, which gets ignored - need a response to
+      // cleanly terminate it
+      handler2->sendReplyWithBody(200, 100);
+    });
+  handler1->expectDetachTransaction();
+  handler2->expectDetachTransaction();
+  flushRequestsAndLoop();
+  gracefulShutdown();
+}
diff --git a/proxygen/lib/http/session/test/HTTPSessionMocks.h b/proxygen/lib/http/session/test/HTTPSessionMocks.h
index 9af0ea7a2..9b3f81ae0 100644
--- a/proxygen/lib/http/session/test/HTTPSessionMocks.h
+++ b/proxygen/lib/http/session/test/HTTPSessionMocks.h
@@ -219,10 +219,21 @@ class MockHTTPHandler
     EXPECT_CALL(*this, onHeadersComplete(testing::_))
         .WillOnce(testing::Invoke(cb))
         .RetiresOnSaturation();
   }
 
+  void expectTrailers(
+      std::function<void()> callback = std::function<void()>()) {
+    if (callback) {
+      EXPECT_CALL(*this, onTrailers(testing::_))
+          .WillOnce(testing::InvokeWithoutArgs(callback))
+          .RetiresOnSaturation();
+    } else {
+      EXPECT_CALL(*this, onTrailers(testing::_));
+    }
+  }
+
   void expectTrailers(
       std::function<void(std::shared_ptr<HTTPHeaders> trailers)> cb) {
     EXPECT_CALL(*this, onTrailers(testing::_))
         .WillOnce(testing::Invoke(cb))
         .RetiresOnSaturation();

Vulnerable code:   if (sessionClosing_ != ClosingState::CLOSED) {
Fixed code:   if (sessionClosing_ != ClosingState::CLOSED && streamId > lastStreamID_) {
  VLOG(4) << "generating TRAILERS for stream=" << stream;

TEST_F(HTTP2CodecTest, TrailersNotLatest) {
  HTTPMessage req = getGetRequest("/guacamole");
  req.getHeaders().add(HTTP_HEADER_USER_AGENT, "coolio");
  upstreamCodec_.generateHeader(output_, 1, req);
  upstreamCodec_.generateHeader(output_, 3, req);

  HTTPHeaders trailers;
  trailers.add("x-trailer-1", "pico-de-gallo");
  upstreamCodec_.generateTrailers(output_, 1, trailers);
  upstreamCodec_.generateHeader(output_, 3, req);

  parse();

  EXPECT_EQ(callbacks_.messageBegin, 2);
  EXPECT_EQ(callbacks_.headersComplete, 2);
  EXPECT_EQ(callbacks_.bodyCalls, 0);
  EXPECT_EQ(callbacks_.trailers, 1);
  EXPECT_NE(nullptr, callbacks_.msg->getTrailers());
  EXPECT_EQ("pico-de-gallo",
            callbacks_.msg->getTrailers()->getSingleOrEmpty("x-trailer-1"));
  EXPECT_EQ(callbacks_.messageComplete, 1);
  EXPECT_EQ(callbacks_.streamErrors, 1);
  EXPECT_EQ(callbacks_.sessionErrors, 0);
}

TEST_F(HTTP2DownstreamSessionTest, TestDuplicateRequestStream) {
  // Send the following:
  // HEADERS id=1
  // HEADERS id=2
  // HEADERS id=1 (trailers)
  // HEADERS id=2 -> contains pseudo-headers after EOM so ignored
  auto handler2 = addSimpleStrictHandler();
  auto handler1 = addSimpleStrictHandler();
  auto streamID1 = sendRequest("/withtrailers", 0, false);
  auto streamID2 = sendRequest();
  HTTPHeaders trailers;
  trailers.add("Foo", "Bar");
  clientCodec_->generateTrailers(requests_, streamID1, trailers);
  clientCodec_->generateEOM(requests_, streamID1);

  clientCodec_->generateHeader(requests_, streamID2, getGetRequest(), false);
  handler1->expectHeaders();
  handler2->expectHeaders();
  handler2->expectEOM();
  handler1->expectTrailers();
  handler1->expectEOM([&] {
      handler1->sendReplyWithBody(200, 100);
      // 2 got an error after EOM, which gets ignored - need a response to
      // cleanly terminate it
      handler2->sendReplyWithBody(200, 100);
    });
  handler1->expectDetachTransaction();
  handler2->expectDetachTransaction();
  flushRequestsAndLoop();
  gracefulShutdown();
}
  void expectTrailers(
      std::function<void()> callback = std::function<void()>()) {
    if (callback) {
      EXPECT_CALL(*this, onTrailers(testing::_))
          .WillOnce(testing::InvokeWithoutArgs(callback))
          .RetiresOnSaturation();
    } else {
      EXPECT_CALL(*this, onTrailers(testing::_));
    }
  }