CVE-2018-6346: 7PK - Errors (CWE-388)
value: A potential denial-of-service issue in the Proxygen handling of invalid HTTP2 priority settings (specifically a circular dependency). This affects Proxygen prior to v2018.12.31.00.
CWE description: This category represents one of the phyla in the Seven Pernicious Kingdoms vulnerability classification. It includes weaknesses that occur when an application does not properly handle errors that occur during processing. According to the authors of the Seven Pernicious Kingdoms, "Errors and error handling represent a class of API. Errors related to error handling are so common that they deserve a special kingdom of their own. As with 'API Abuse,' there are two ways to introduce an error-related security vulnerability: the most common one is handling errors poorly (or not at all). The second is producing errors that either give out too much information (to possible attackers) or are difficult to handle."
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
Commit date: 2018-12-27 12:49:37 -0800
Commit message: Fix h2 codec state after bad priority header.

Summary:
It's possible for the http2 codec to enter an invalid state after processing a http2 header with invalid priorities.
CVE-2018-6346

Reviewed By: maxgeorg

Differential Revision: D13510025

fbshipit-source-id: 7c4e42daf1cd2b912454d13a66ab8488d1863263

Diff: diff --git a/proxygen/lib/http/codec/HTTP2Codec.cpp b/proxygen/lib/http/codec/HTTP2Codec.cpp
index 5ce0fe3f..db9a8273 100644
--- a/proxygen/lib/http/codec/HTTP2Codec.cpp
+++ b/proxygen/lib/http/codec/HTTP2Codec.cpp
@@ -512,25 +512,27 @@ folly::Optional<ErrorCode> HTTP2Codec::parseHeadersDecodeFrames(
     isReq = isRequest(curHeader_.stream);
   } else {
     isReq = transportDirection_ == TransportDirection::DOWNSTREAM;
   }
 
+  // Validate circular dependencies.
+  if (priority && (curHeader_.stream == priority->streamDependency)) {
+    streamError(
+        folly::to<string>("Circular dependency for txn=", curHeader_.stream),
+        ErrorCode::PROTOCOL_ERROR,
+        curHeader_.type == http2::FrameType::HEADERS);
+    return ErrorCode::NO_ERROR;
+  }
+
   decodeInfo_.init(isReq, parsingDownstreamTrailers_);
   if (priority) {
-    if (curHeader_.stream == priority->streamDependency) {
-      streamError(folly::to<string>("Circular dependency for txn=",
-                                    curHeader_.stream),
-                  ErrorCode::PROTOCOL_ERROR,
-                  curHeader_.type == http2::FrameType::HEADERS);
-      return ErrorCode::NO_ERROR;
-    }
-
     decodeInfo_.msg->setHTTP2Priority(
         std::make_tuple(priority->streamDependency,
                         priority->exclusive,
                         priority->weight));
   }
+
   headerCodec_.decodeStreaming(
       headerCursor, curHeaderBlock_.chainLength(), this);
   msg = std::move(decodeInfo_.msg);
   // Saving this in case we need to log it on error
   auto g = folly::makeGuard([this] { curHeaderBlock_.move(); });
diff --git a/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp b/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
index 7c818dd1..22471b13 100644
--- a/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
+++ b/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
@@ -1375,10 +1375,34 @@ TEST_F(HTTP2CodecTest, BadHeaderPriority) {
 
   EXPECT_EQ(callbacks_.streamErrors, 1);
   EXPECT_EQ(callbacks_.sessionErrors, 0);
 }
 
+TEST_F(HTTP2CodecTest, DuplicateBadHeaderPriority) {
+  // Sent an initial header with a circular dependency
+  HTTPMessage req = getGetRequest();
+  req.setHTTP2Priority(HTTPMessage::HTTPPriority(0, false, 7));
+  upstreamCodec_.generateHeader(output_, 1, req, true /* eom */);
+
+  // Hack ingress with circular dependency.
+  EXPECT_TRUE(parse([&](IOBuf* ingress) {
+    folly::io::RWPrivateCursor c(ingress);
+    c.skip(http2::kFrameHeaderSize + http2::kConnectionPreface.length());
+    c.writeBE<uint32_t>(1);
+  }));
+
+  EXPECT_EQ(callbacks_.streamErrors, 1);
+  EXPECT_EQ(callbacks_.sessionErrors, 0);
+
+  // On the same stream, send another request.
+  HTTPMessage nextRequest = getGetRequest();
+  upstreamCodec_.generateHeader(output_, 1, nextRequest, true /* eom */);
+  parse();
+  EXPECT_EQ(callbacks_.streamErrors, 2);
+  EXPECT_EQ(callbacks_.sessionErrors, 0);
+}
+
 TEST_F(HTTP2CodecTest, BadPriority) {
   auto pri = HTTPMessage::HTTPPriority(0, true, 1);
   upstreamCodec_.generatePriority(output_, 1, pri);
 
   // hack ingress with cirular dep

Vulnerable code:     if (curHeader_.stream == priority->streamDependency) {
      streamError(folly::to<string>("Circular dependency for txn=",
                                    curHeader_.stream),
                  ErrorCode::PROTOCOL_ERROR,
                  curHeader_.type == http2::FrameType::HEADERS);
      return ErrorCode::NO_ERROR;
    }

Fixed code:   // Validate circular dependencies.
  if (priority && (curHeader_.stream == priority->streamDependency)) {
    streamError(
        folly::to<string>("Circular dependency for txn=", curHeader_.stream),
        ErrorCode::PROTOCOL_ERROR,
        curHeader_.type == http2::FrameType::HEADERS);
    return ErrorCode::NO_ERROR;
  }


TEST_F(HTTP2CodecTest, DuplicateBadHeaderPriority) {
  // Sent an initial header with a circular dependency
  HTTPMessage req = getGetRequest();
  req.setHTTP2Priority(HTTPMessage::HTTPPriority(0, false, 7));
  upstreamCodec_.generateHeader(output_, 1, req, true /* eom */);

  // Hack ingress with circular dependency.
  EXPECT_TRUE(parse([&](IOBuf* ingress) {
    folly::io::RWPrivateCursor c(ingress);
    c.skip(http2::kFrameHeaderSize + http2::kConnectionPreface.length());
    c.writeBE<uint32_t>(1);
  }));

  EXPECT_EQ(callbacks_.streamErrors, 1);
  EXPECT_EQ(callbacks_.sessionErrors, 0);

  // On the same stream, send another request.
  HTTPMessage nextRequest = getGetRequest();
  upstreamCodec_.generateHeader(output_, 1, nextRequest, true /* eom */);
  parse();
  EXPECT_EQ(callbacks_.streamErrors, 2);
  EXPECT_EQ(callbacks_.sessionErrors, 0);
}