CVE-2019-3560: Loop with Unreachable Exit Condition ('Infinite Loop') (CWE-835)
value: An improperly performed length calculation on a buffer in PlaintextRecordLayer could lead to an infinite loop and denial-of-service based on user input. This issue affected versions of fizz prior to v2019.03.04.00.
CWE description: The product contains an iteration or loop with an exit condition that cannot be reached, i.e., an infinite loop.
CVSS v3 base score: 7.5
Published: 2019-04-29T16:29Z
Repository: https://github.com/facebookincubator/fizz
Language: C++
Files changed: fizz/record/PlaintextRecordLayer.cpp, fizz/record/test/PlaintextRecordTest.cpp
Commit date: 2019-02-25 17:12:24 -0800
Commit message: Avoid arithmetic operation on uint16 read from the wire.

Summary:
This could overflow previously.

CVE-2019-3560

Reviewed By: yfeldblum

Differential Revision: D14152362

fbshipit-source-id: c0ebb3fc59b49c7c23e6bcb90458c19cd891be65

Diff: diff --git a/fizz/record/PlaintextRecordLayer.cpp b/fizz/record/PlaintextRecordLayer.cpp
index e33ef9e..958c412 100644
--- a/fizz/record/PlaintextRecordLayer.cpp
+++ b/fizz/record/PlaintextRecordLayer.cpp
@@ -37,13 +37,11 @@ folly::Optional<TLSMessage> PlaintextReadRecordLayer::read(
         cursor.skip(sizeof(ProtocolVersion));
         auto length = cursor.readBE<uint16_t>();
         if (buf.chainLength() < (cursor - buf.front()) + length) {
           return folly::none;
         }
-        length +=
-            sizeof(ContentType) + sizeof(ProtocolVersion) + sizeof(uint16_t);
-        buf.trimStart(length);
+        buf.trimStart(static_cast<size_t>(kPlaintextHeaderSize) + length);
         continue;
       } else if (msg.type != ContentType::change_cipher_spec) {
         skipEncryptedRecords_ = false;
       }
     }
diff --git a/fizz/record/test/PlaintextRecordTest.cpp b/fizz/record/test/PlaintextRecordTest.cpp
index 2184228..1f65e71 100644
--- a/fizz/record/test/PlaintextRecordTest.cpp
+++ b/fizz/record/test/PlaintextRecordTest.cpp
@@ -113,10 +113,20 @@ TEST_F(PlaintextRecordTest, TestSkipAndWait) {
   addToQueue("17030100050123456789");
   EXPECT_FALSE(read_.read(queue_).hasValue());
   EXPECT_TRUE(queue_.empty());
 }
 
+TEST_F(PlaintextRecordTest, TestSkipOversizedRecord) {
+  read_.setSkipEncryptedRecords(true);
+  addToQueue("170301fffb");
+  auto longBuf = IOBuf::create(0xfffb);
+  longBuf->append(0xfffb);
+  queue_.append(std::move(longBuf));
+  EXPECT_FALSE(read_.read(queue_).hasValue());
+  EXPECT_TRUE(queue_.empty());
+}
+
 TEST_F(PlaintextRecordTest, TestWaitBeforeSkip) {
   read_.setSkipEncryptedRecords(true);
   addToQueue("170301000501234567");
   EXPECT_FALSE(read_.read(queue_).hasValue());
   expectSame(queue_.move(), "170301000501234567");

Vulnerable code:         length +=
            sizeof(ContentType) + sizeof(ProtocolVersion) + sizeof(uint16_t);
        buf.trimStart(length);
Fixed code:         buf.trimStart(static_cast<size_t>(kPlaintextHeaderSize) + length);
TEST_F(PlaintextRecordTest, TestSkipOversizedRecord) {
  read_.setSkipEncryptedRecords(true);
  addToQueue("170301fffb");
  auto longBuf = IOBuf::create(0xfffb);
  longBuf->append(0xfffb);
  queue_.append(std::move(longBuf));
  EXPECT_FALSE(read_.read(queue_).hasValue());
  EXPECT_TRUE(queue_.empty());
}