CVE-2019-11924: Allocation of Resources Without Limits or Throttling (CWE-770)
value: A peer could send empty handshake fragments containing only padding which would be kept in memory until a full handshake was received, resulting in memory exhaustion. This issue affects versions v2019.01.28.00 and above of fizz, until v2019.08.05.00.
CWE description: The product allocates a reusable resource or group of resources on behalf of an actor without imposing any restrictions on the size or number of resources that can be allocated, in violation of the intended security policy for that actor.
CVSS v3 base score: 7.5
Published: 2019-08-20T20:15Z
Repository: https://github.com/facebookincubator/fizz
Language: C++
Files changed: fizz/record/EncryptedRecordLayer.cpp, fizz/record/RecordLayer.cpp
Commit date: 2019-08-08 11:43:20 -0700
Commit message: Coalesce handshake buffers

Summary:
It is possible that a peer might send us records in a manner such
that there is a 16KB record and only 1 byte of handshake message in
each record. Since we normally just trim the IOBuf, we would end up
holding 16K of data per actual byte of data. To prevent this we allocate a contiguous
buffer to copy over these bytes for handshake messages for now.

This is a partial fix for CVE-2019-11924

Reviewed By: ngoyal

Differential Revision: D16478044

fbshipit-source-id: 464bc68eaefda065d9a327818100427377293fbd

Diff: diff --git a/fizz/record/EncryptedRecordLayer.cpp b/fizz/record/EncryptedRecordLayer.cpp
index a56b762..377ae5b 100644
--- a/fizz/record/EncryptedRecordLayer.cpp
+++ b/fizz/record/EncryptedRecordLayer.cpp
@@ -111,11 +111,11 @@ folly::Optional<TLSMessage> EncryptedReadRecordLayer::read(
   auto decryptedBuf = getDecryptedBuf(buf);
   if (!decryptedBuf) {
     return folly::none;
   }
 
-  TLSMessage msg;
+  TLSMessage msg{};
   // Iterate over the buffers while trying to find
   // the first non-zero octet. This is much faster than
   // first iterating and then trimming.
   auto currentBuf = decryptedBuf->get();
   bool nonZeroFound = false;
diff --git a/fizz/record/RecordLayer.cpp b/fizz/record/RecordLayer.cpp
index 0e8136c..f608d90 100644
--- a/fizz/record/RecordLayer.cpp
+++ b/fizz/record/RecordLayer.cpp
@@ -47,11 +47,35 @@ folly::Optional<Param> ReadRecordLayer::readEvent(
         } else {
           return Param(std::move(alert));
         }
       }
       case ContentType::handshake: {
-        unparsedHandshakeData_.append(std::move(message->fragment));
+        std::unique_ptr<folly::IOBuf> handshakeMessage =
+            unparsedHandshakeData_.move();
+        // It is possible that a peer might send us records in a manner such
+        // that there is a 16KB record and only 1 byte of handshake message in
+        // each record. Since we normally just trim the IOBuf, we would end up
+        // holding 16K of data. To prevent this we allocate a contiguous
+        // buffer to copy over these bytes. We supply kExtraAlloc bytes in
+        // order to avoid needing to re-allocate a lot of times if we receive
+        // a lot of small messages. There might be more optimal reallocation
+        // policies, but this should be fine.
+        message->fragment->coalesce();
+        constexpr size_t kExtraAlloc = 1024;
+        if (!handshakeMessage) {
+          handshakeMessage =
+              folly::IOBuf::create(message->fragment->length() + kExtraAlloc);
+        } else if (handshakeMessage->tailroom() < message->fragment->length()) {
+          handshakeMessage->reserve(
+              0, message->fragment->length() + kExtraAlloc);
+        }
+        memcpy(
+            handshakeMessage->writableTail(),
+            message->fragment->data(),
+            message->fragment->length());
+        handshakeMessage->append(message->fragment->length());
+        unparsedHandshakeData_.append(std::move(handshakeMessage));
         auto param = decodeHandshakeMessage(unparsedHandshakeData_);
         if (param) {
           VLOG(8) << "Received handshake message "
                   << toString(boost::apply_visitor(EventVisitor(), *param));
           return param;

Vulnerable code:   TLSMessage msg;
        unparsedHandshakeData_.append(std::move(message->fragment));
Fixed code:   TLSMessage msg{};
        std::unique_ptr<folly::IOBuf> handshakeMessage =
            unparsedHandshakeData_.move();
        // It is possible that a peer might send us records in a manner such
        // that there is a 16KB record and only 1 byte of handshake message in
        // each record. Since we normally just trim the IOBuf, we would end up
        // holding 16K of data. To prevent this we allocate a contiguous
        // buffer to copy over these bytes. We supply kExtraAlloc bytes in
        // order to avoid needing to re-allocate a lot of times if we receive
        // a lot of small messages. There might be more optimal reallocation
        // policies, but this should be fine.
        message->fragment->coalesce();
        constexpr size_t kExtraAlloc = 1024;
        if (!handshakeMessage) {
          handshakeMessage =
              folly::IOBuf::create(message->fragment->length() + kExtraAlloc);
        } else if (handshakeMessage->tailroom() < message->fragment->length()) {
          handshakeMessage->reserve(
              0, message->fragment->length() + kExtraAlloc);
        }
        memcpy(
            handshakeMessage->writableTail(),
            message->fragment->data(),
            message->fragment->length());
        handshakeMessage->append(message->fragment->length());
        unparsedHandshakeData_.append(std::move(handshakeMessage));