CVE-2019-3553: Allocation of Resources Without Limits or Throttling (CWE-770)
value: C++ Facebook Thrift servers would not error upon receiving messages declaring containers of sizes larger than the payload. As a result, malicious clients could send short messages which would result in a large memory allocation, potentially leading to denial of service. This issue affects Facebook Thrift prior to v2020.02.03.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: 2020-03-10T21:15Z
Repository: https://github.com/facebook/fbthrift
Language: C
Files changed: thrift/lib/cpp2/protocol/BinaryProtocol-inl.h, thrift/lib/cpp2/protocol/CompactProtocol-inl.h, thrift/test/ProtocolTruncatedData.thrift, thrift/test/ProtocolTruncatedDataTest.cpp
Commit date: 2020-01-28 08:32:03 -0800
Commit message: Better handling of truncated data when reading strings

Summary:
Currently we read string size and blindly pre-allocate it. This allows malicious attacker to send a few bytes message and cause server to allocate huge amount of memory (>1GB).

This diff changes the logic to check if we have enough data in the buffer before allocating the string.

This is a second part of a fix for CVE-2019-3553.

Reviewed By: vitaut

Differential Revision: D14393393

fbshipit-source-id: e2046d2f5b087d3abc9a9d2c6c107cf088673057

Diff: diff --git a/thrift/lib/cpp2/protocol/BinaryProtocol-inl.h b/thrift/lib/cpp2/protocol/BinaryProtocol-inl.h
index 19bf780438..21eb793720 100644
--- a/thrift/lib/cpp2/protocol/BinaryProtocol-inl.h
+++ b/thrift/lib/cpp2/protocol/BinaryProtocol-inl.h
@@ -551,10 +551,13 @@ void BinaryProtocolReader::readStringBody(StrType& str, int32_t size) {
     str.clear();
     return;
   }
 
   if (static_cast<int32_t>(in_.length()) < size) {
+    if (!in_.canAdvance(size)) {
+      protocol::TProtocolException::throwTruncatedData();
+    }
     str.reserve(size); // only reserve for multi iter case below
   }
   str.clear();
   size_t size_left = size;
   while (size_left > 0) {
diff --git a/thrift/lib/cpp2/protocol/CompactProtocol-inl.h b/thrift/lib/cpp2/protocol/CompactProtocol-inl.h
index 0ff536d948..c6782abf6a 100644
--- a/thrift/lib/cpp2/protocol/CompactProtocol-inl.h
+++ b/thrift/lib/cpp2/protocol/CompactProtocol-inl.h
@@ -670,10 +670,13 @@ void CompactProtocolReader::readStringSize(int32_t& size) {
 }
 
 template <typename StrType>
 void CompactProtocolReader::readStringBody(StrType& str, int32_t size) {
   if (static_cast<int32_t>(in_.length()) < size) {
+    if (!in_.canAdvance(size)) {
+      protocol::TProtocolException::throwTruncatedData();
+    }
     str.reserve(size); // only reserve for multi iter case below
   }
   str.clear();
   size_t size_left = size;
   while (size_left > 0) {
diff --git a/thrift/test/ProtocolTruncatedData.thrift b/thrift/test/ProtocolTruncatedData.thrift
index bd1f6a2722..69a14ce663 100644
--- a/thrift/test/ProtocolTruncatedData.thrift
+++ b/thrift/test/ProtocolTruncatedData.thrift
@@ -18,6 +18,7 @@ namespace cpp2 thrift.test
 
 struct TestStruct {
   1: optional list<i64> i64_list,
   2: optional set<i32> i32_set,
   3: optional map<i32,i16> i32_i16_map,
+  4: optional string a_string,
 }
diff --git a/thrift/test/ProtocolTruncatedDataTest.cpp b/thrift/test/ProtocolTruncatedDataTest.cpp
index 890355729c..59f86eccf5 100644
--- a/thrift/test/ProtocolTruncatedDataTest.cpp
+++ b/thrift/test/ProtocolTruncatedDataTest.cpp
@@ -75,5 +75,21 @@ TEST(ProtocolTruncatedDataTest, TruncatedMap) {
   }
 
   testPartialDataHandling<CompactSerializer>(
       s, 3 /* headers */ + 30 * 2 /* 2b / kv pair */);
 }
+
+TEST(ProtocolTruncatedDataTest, TuncatedString_Compact) {
+  TestStruct s;
+  s.a_string_ref() = "foobarbazstring";
+
+  testPartialDataHandling<CompactSerializer>(
+      s, 2 /* field & length header */ + s.a_string_ref()->size());
+}
+
+TEST(ProtocolTruncatedDataTest, TuncatedString_Binary) {
+  TestStruct s;
+  s.a_string_ref() = "foobarbazstring";
+
+  testPartialDataHandling<BinarySerializer>(
+      s, 7 /* field & length header */ + s.a_string_ref()->size());
+}

Fixed code:     if (!in_.canAdvance(size)) {
      protocol::TProtocolException::throwTruncatedData();
    }
    if (!in_.canAdvance(size)) {
      protocol::TProtocolException::throwTruncatedData();
    }
  4: optional string a_string,

TEST(ProtocolTruncatedDataTest, TuncatedString_Compact) {
  TestStruct s;
  s.a_string_ref() = "foobarbazstring";

  testPartialDataHandling<CompactSerializer>(
      s, 2 /* field & length header */ + s.a_string_ref()->size());
}

TEST(ProtocolTruncatedDataTest, TuncatedString_Binary) {
  TestStruct s;
  s.a_string_ref() = "foobarbazstring";

  testPartialDataHandling<BinarySerializer>(
      s, 7 /* field & length header */ + s.a_string_ref()->size());
}