[security][CVE-2018-6335] Fix potential crash in HTTP2 padding handling

fredemmott · fredemmott · commit 4cb57dd753a3 · 2018-05-03T12:31:15.000-07:00
CVE-2018-6335
diff --git a/patches/proxygen-CVE-2018-6335.patch b/patches/proxygen-CVE-2018-6335.patch
@@ -0,0 +1,100 @@
+diff --git a/third-party/proxygen/src/proxygen/lib/http/codec/HTTP2Framer.cpp b/third-party/proxygen/src/proxygen/lib/http/codec/HTTP2Framer.cpp
+index 068fea4b..9dc0c994 100644
+--- a/third-party/proxygen/src/proxygen/lib/http/codec/HTTP2Framer.cpp
++++ b/third-party/proxygen/src/proxygen/lib/http/codec/HTTP2Framer.cpp
+@@ -187,6 +187,11 @@ parsePadding(Cursor& cursor,
+   } else {
+     padding = 0;
+   }
++
++  if (header.length < padding) {
++    return ErrorCode::PROTOCOL_ERROR;
++  }
++  header.length -= padding;
+   return ErrorCode::NO_ERROR;
+ }
+ 
+@@ -259,7 +264,7 @@ parseData(Cursor& cursor,
+   // outPadding is the total number of flow-controlled pad bytes, which
+   // includes the length byte, if present.
+   outPadding = padding + ((frameHasPadding(header)) ? 1 : 0);
+-  cursor.clone(outBuf, header.length - padding);
++  cursor.clone(outBuf, header.length);
+   return skipPadding(cursor, padding, kStrictPadding);
+ }
+ 
+@@ -313,7 +318,7 @@ parseHeaders(Cursor& cursor,
+   if (header.length < padding) {
+     return ErrorCode::PROTOCOL_ERROR;
+   }
+-  cursor.clone(outBuf, header.length - padding);
++  cursor.clone(outBuf, header.length);
+   return skipPadding(cursor, padding, kStrictPadding);
+ }
+ 
+@@ -398,7 +403,7 @@ parsePushPromise(Cursor& cursor,
+   if (header.length < padding) {
+     return ErrorCode::PROTOCOL_ERROR;
+   }
+-  cursor.clone(outBuf, header.length - padding);
++  cursor.clone(outBuf, header.length);
+   return skipPadding(cursor, padding, kStrictPadding);
+ }
+ 
+@@ -470,7 +475,7 @@ parseContinuation(Cursor& cursor,
+   if (header.length < padding) {
+     return ErrorCode::PROTOCOL_ERROR;
+   }
+-  cursor.clone(outBuf, header.length - padding);
++  cursor.clone(outBuf, header.length);
+   return skipPadding(cursor, padding, kStrictPadding);
+ }
+ 
+diff --git a/third-party/proxygen/src/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp b/third-party/proxygen/src/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
+index 97008421..a859861c 100644
+--- a/third-party/proxygen/src/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
++++ b/third-party/proxygen/src/proxygen/lib/http/codec/test/HTTP2CodecTest.cpp
+@@ -638,15 +638,17 @@ TEST_F(HTTP2CodecTest, MalformedPaddingLength) {
+   output_.append(badInput, sizeof(badInput));
+   EXPECT_EQ(output_.chainLength(), sizeof(badInput));
+ 
+-  bool caughtException = false;
+-  bool parseResult = true;
+-  try {
+-    parseResult = parse();
+-  } catch (const std::exception &e) {
+-    caughtException = true;
+-  }
+-  EXPECT_FALSE(caughtException);
+-  EXPECT_FALSE(parseResult);
++  EXPECT_FALSE(parse());
++}
++
++TEST_F(HTTP2CodecTest, MalformedPadding) {
++  const uint8_t badInput[] = {
++    0x00, 0x00, 0x0d, 0x01, 0xbe, 0x63, 0x0d, 0x0a, 0x0d, 0x0a, 0x00, 0x73,
++    0x00, 0x00, 0x06, 0x08, 0x72, 0x00, 0x24, 0x00, 0xfa, 0x4d, 0x0d
++  };
++  output_.append(badInput, sizeof(badInput));
++
++  EXPECT_FALSE(parse());
+ }
+ 
+ TEST_F(HTTP2CodecTest, NoAppByte) {
+@@ -659,15 +661,7 @@ TEST_F(HTTP2CodecTest, NoAppByte) {
+   output_.append(noAppByte, sizeof(noAppByte));
+   EXPECT_EQ(output_.chainLength(), sizeof(noAppByte));
+ 
+-  bool caughtException = false;
+-  bool parseResult = false;
+-  try {
+-    parseResult = parse();
+-  } catch (const std::exception &e) {
+-    caughtException = true;
+-  }
+-  EXPECT_FALSE(caughtException);
+-  EXPECT_TRUE(parseResult);
++  EXPECT_TRUE(parse());
+   EXPECT_EQ(callbacks_.messageBegin, 0);
+   EXPECT_EQ(callbacks_.headersComplete, 0);
+   EXPECT_EQ(callbacks_.messageComplete, 0);
