CVE-2019-3563: Out-of-bounds Write (CWE-787)
value: Wangle's LineBasedFrameDecoder contains logic for identifying newlines which incorrectly advances a buffer, leading to a potential underflow. This affects versions of Wangle prior to v2019.04.22.00
CWE description: The product writes data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 9.8
Published: 2019-04-29T16:29Z
Repository: https://github.com/facebook/wangle
Language: C++
Files changed: wangle/codec/LineBasedFrameDecoder.cpp, wangle/codec/test/CodecTest.cpp
Commit date: 2019-04-16 09:25:25 -0700
Commit message: Peek for \n in LineBasedFrameDecoder.

Summary:
Previously this could underflow if there was not a following \n.

CVE-2019-3563

Reviewed By: siyengar

Differential Revision: D14935715

fbshipit-source-id: 25c3eecf373f89efa1232456aeeb092f13b7fa06

Diff: diff --git a/wangle/codec/LineBasedFrameDecoder.cpp b/wangle/codec/LineBasedFrameDecoder.cpp
index 37808660..be6f47f9 100644
--- a/wangle/codec/LineBasedFrameDecoder.cpp
+++ b/wangle/codec/LineBasedFrameDecoder.cpp
@@ -95,12 +95,13 @@ int64_t LineBasedFrameDecoder::findEndOfLine(IOBufQueue& buf) {
   Cursor c(buf.front());
   for (uint32_t i = 0; i < maxLength_ && i < buf.chainLength(); i++) {
     auto b = c.read<char>();
     if (b == '\n' && terminatorType_ != TerminatorType::CARRIAGENEWLINE) {
       return i;
-    } else if (terminatorType_ != TerminatorType::NEWLINE &&
-               b == '\r' && !c.isAtEnd() && c.read<char>() == '\n') {
+    } else if (
+        terminatorType_ != TerminatorType::NEWLINE && b == '\r' &&
+        !c.isAtEnd() && *c.peekBytes().data() == '\n') {
       return i;
     }
   }
 
   return -1;
diff --git a/wangle/codec/test/CodecTest.cpp b/wangle/codec/test/CodecTest.cpp
index c6ef6df5..4d348094 100644
--- a/wangle/codec/test/CodecTest.cpp
+++ b/wangle/codec/test/CodecTest.cpp
@@ -604,5 +604,39 @@ TEST(LineBasedFrameDecoder, CarriageNewLineOnly) {
 
   q.append(std::move(buf));
   pipeline->read(q);
   EXPECT_EQ(called, 1);
 }
+
+TEST(LineBasedFrameDecoder, CarriageOnly) {
+  auto pipeline = Pipeline<IOBufQueue&, std::unique_ptr<IOBuf>>::create();
+
+  (*pipeline)
+      .addBack(LineBasedFrameDecoder(
+          10, true, LineBasedFrameDecoder::TerminatorType::CARRIAGENEWLINE))
+      .addBack(test::FrameTester([&](std::unique_ptr<IOBuf>) { FAIL(); }))
+      .finalize();
+
+  IOBufQueue q(IOBufQueue::cacheChainLength());
+  q.append(IOBuf::copyBuffer("\raa"));
+  pipeline->read(q);
+}
+
+TEST(LineBasedFrameDecoder, DoubleCarriage) {
+  auto pipeline = Pipeline<IOBufQueue&, std::unique_ptr<IOBuf>>::create();
+  int called = 0;
+
+  (*pipeline)
+      .addBack(LineBasedFrameDecoder(
+          10, true, LineBasedFrameDecoder::TerminatorType::CARRIAGENEWLINE))
+      .addBack(test::FrameTester([&](std::unique_ptr<IOBuf> buf) {
+        auto sz = buf->computeChainDataLength();
+        called++;
+        EXPECT_EQ(sz, 1);
+      }))
+      .finalize();
+
+  IOBufQueue q(IOBufQueue::cacheChainLength());
+  q.append(IOBuf::copyBuffer("\r\r\na\r\n"));
+  pipeline->read(q);
+  EXPECT_EQ(called, 2);
+}

Vulnerable code:     } else if (terminatorType_ != TerminatorType::NEWLINE &&
               b == '\r' && !c.isAtEnd() && c.read<char>() == '\n') {
Fixed code:     } else if (
        terminatorType_ != TerminatorType::NEWLINE && b == '\r' &&
        !c.isAtEnd() && *c.peekBytes().data() == '\n') {

TEST(LineBasedFrameDecoder, CarriageOnly) {
  auto pipeline = Pipeline<IOBufQueue&, std::unique_ptr<IOBuf>>::create();

  (*pipeline)
      .addBack(LineBasedFrameDecoder(
          10, true, LineBasedFrameDecoder::TerminatorType::CARRIAGENEWLINE))
      .addBack(test::FrameTester([&](std::unique_ptr<IOBuf>) { FAIL(); }))
      .finalize();

  IOBufQueue q(IOBufQueue::cacheChainLength());
  q.append(IOBuf::copyBuffer("\raa"));
  pipeline->read(q);
}

TEST(LineBasedFrameDecoder, DoubleCarriage) {
  auto pipeline = Pipeline<IOBufQueue&, std::unique_ptr<IOBuf>>::create();
  int called = 0;

  (*pipeline)
      .addBack(LineBasedFrameDecoder(
          10, true, LineBasedFrameDecoder::TerminatorType::CARRIAGENEWLINE))
      .addBack(test::FrameTester([&](std::unique_ptr<IOBuf> buf) {
        auto sz = buf->computeChainDataLength();
        called++;
        EXPECT_EQ(sz, 1);
      }))
      .finalize();

  IOBufQueue q(IOBufQueue::cacheChainLength());
  q.append(IOBuf::copyBuffer("\r\r\na\r\n"));
  pipeline->read(q);
  EXPECT_EQ(called, 2);
}