CVE-2018-6347 (C++, published 2018-12-31T22:29Z)
Description: 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.
Severity: MEDIUM (CVSS v3 base score: 7.5)
Weakness: CWE-20 \u2014 Improper Input Validation
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.
Repository: https://github.com/facebook/proxygen
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 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


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::_));
    }
  }