CVE-2019-11938: Allocation of Resources Without Limits or Throttling (CWE-770)
value: Java 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 v2019.12.09.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: Java
Files changed: thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TBinaryProtocol.java, thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TCompactProtocol.java, thrift/lib/java/src/resources/testing_data_structures.thrift, thrift/lib/java/src/test/java/com/facebook/thrift/TruncatedFrameTest.java
Commit date: 2019-12-06 13:13:54 -0800
Commit message: Java: Check the size of the remaining frame before deserializing a string

Summary:
In order to avoid over-allocating memory for malformed or truncated frame, we
ensure that we have enough data in the current frame.

This is a partial fix for CVE-2019-11938.

Reviewed By: vitaut

Differential Revision: D14505601

fbshipit-source-id: c90f248828b067a3a5debcc8df6a3f4e9da6d195

Diff: diff --git a/thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TBinaryProtocol.java b/thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TBinaryProtocol.java
index 3f4740a76..3b9d60a51 100644
--- a/thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TBinaryProtocol.java
+++ b/thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TBinaryProtocol.java
@@ -341,18 +341,20 @@ public class TBinaryProtocol extends TProtocol {
 
     return readStringBody(size);
   }
 
   public String readStringBody(int size) throws TException {
+    ensureContainerHasEnough(size, TType.BYTE);
     checkReadLength(size);
     byte[] buf = new byte[size];
     trans_.readAll(buf, 0, size);
     return new String(buf, StandardCharsets.UTF_8);
   }
 
   public byte[] readBinary() throws TException {
     int size = readI32();
+    ensureContainerHasEnough(size, TType.BYTE);
     checkReadLength(size);
     byte[] buf = new byte[size];
     trans_.readAll(buf, 0, size);
     return buf;
   }
diff --git a/thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TCompactProtocol.java b/thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TCompactProtocol.java
index 1a33ddc78..b25141faa 100755
--- a/thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TCompactProtocol.java
+++ b/thrift/lib/java/src/main/java/com/facebook/thrift/protocol/TCompactProtocol.java
@@ -636,10 +636,11 @@ public class TCompactProtocol extends TProtocol {
   private byte[] readBinary(int length) throws TException {
     if (length == 0) {
       return new byte[0];
     }
 
+    ensureContainerHasEnough(length, TType.BYTE);
     byte[] buf = new byte[length];
     trans_.readAll(buf, 0, length);
     return buf;
   }
 
diff --git a/thrift/lib/java/src/resources/testing_data_structures.thrift b/thrift/lib/java/src/resources/testing_data_structures.thrift
index 1c3d738c5..844cb6325 100644
--- a/thrift/lib/java/src/resources/testing_data_structures.thrift
+++ b/thrift/lib/java/src/resources/testing_data_structures.thrift
@@ -60,5 +60,9 @@ struct MySetStruct {
 }
 
 struct MyMapStruct {
   1: map<i64, string> mapping
 }
+
+struct MyStringStruct {
+  1: string aLongString
+}
diff --git a/thrift/lib/java/src/test/java/com/facebook/thrift/TruncatedFrameTest.java b/thrift/lib/java/src/test/java/com/facebook/thrift/TruncatedFrameTest.java
index 7bd4670e1..2a4cc583b 100644
--- a/thrift/lib/java/src/test/java/com/facebook/thrift/TruncatedFrameTest.java
+++ b/thrift/lib/java/src/test/java/com/facebook/thrift/TruncatedFrameTest.java
@@ -17,10 +17,11 @@
 package com.facebook.thrift;
 
 import com.facebook.thrift.java.test.MyListStruct;
 import com.facebook.thrift.java.test.MyMapStruct;
 import com.facebook.thrift.java.test.MySetStruct;
+import com.facebook.thrift.java.test.MyStringStruct;
 import com.facebook.thrift.protocol.TBinaryProtocol;
 import com.facebook.thrift.protocol.TCompactProtocol;
 import com.facebook.thrift.protocol.TProtocol;
 import com.facebook.thrift.protocol.TProtocolException;
 import com.facebook.thrift.protocol.TType;
@@ -50,19 +51,19 @@ public class TruncatedFrameTest extends junit.framework.TestCase {
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
-    (byte) 0x01, // value = 2L
+    (byte) 0x02, // value = 2L
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
-    (byte) 0x01, // value = 3L
+    (byte) 0x03, // value = 3L
     (byte) 0x00, // Stop
   };
 
   private static final byte[] kCompactListEncoding = {
     (byte) 0b00011001, // field id delta (0001) + type (1001) = List
@@ -137,19 +138,19 @@ public class TruncatedFrameTest extends junit.framework.TestCase {
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
-    (byte) 0x01, // value = 2L
+    (byte) 0x02, // value = 2L
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
     (byte) 0x00,
-    (byte) 0x01, // value = 3L
+    (byte) 0x03, // value = 3L
     (byte) 0x00, // Stop
   };
 
   private static final byte[] kCompactSetEncoding = {
     (byte) 0b00011010, // field id delta (0001) + type (1010) = Set
@@ -271,18 +272,63 @@ public class TruncatedFrameTest extends junit.framework.TestCase {
     TMemoryInputTransport buf = new TMemoryInputTransport(kCompactMapEncoding);
     TProtocol iprot = new TCompactProtocol(buf);
     testTruncated(new MyMapStruct(), iprot);
   }
 
-  private static final char[] hexArray = "0123456789ABCDEF".toCharArray();
+  private static final byte[] kBinaryStringEncoding = {
+    TType.STRING, // Field Type = string
+    (byte) 0x00,
+    (byte) 0x01, // Field id = 1
+    (byte) 0x00,
+    (byte) 0x00,
+    (byte) 0x00,
+    (byte) 0xFF, // string length (255!)
+    (byte) 0x48,
+    (byte) 0x65,
+    (byte) 0x6C,
+    (byte) 0x6C,
+    (byte) 0x6F,
+    (byte) 0x2C,
+    (byte) 0x20,
+    (byte) 0x57,
+    (byte) 0x6F,
+    (byte) 0x72,
+    (byte) 0x6C,
+    (byte) 0x64,
+    (byte) 0x21, // string chars: "Hello, World!"
+    (byte) 0x00, // Stop
+  };
 
-  private static String bytesToHex(byte[] bytes, int length) {
-    String out = "";
-    for (int j = 0; j < length; j++) {
-      int v = bytes[j] & 0xFF;
-      out += hexArray[v >>> 4];
-      out += hexArray[v & 0x0F];
-      out += " ";
-    }
-    return out;
+  private static final byte[] kCompactStringEncoding = {
+    (byte) 0b00011000, // field id delta (0001) + type (1000) = Binary
+    (byte) 0xFF,
+    (byte) 0x0F, // string size (varint) = 0x0FFF (4095)
+    (byte) 0x48,
+    (byte) 0x65,
+    (byte) 0x6C,
+    (byte) 0x6C,
+    (byte) 0x6F,
+    (byte) 0x2C,
+    (byte) 0x20,
+    (byte) 0x57,
+    (byte) 0x6F,
+    (byte) 0x72,
+    (byte) 0x6C,
+    (byte) 0x64,
+    (byte) 0x21, // string chars: "Hello, World!"
+    (byte) 0x00, // Stop
+  };
+
+  @Test
+  public void testStringBinary() throws Exception {
+    TMemoryInputTransport buf = new TMemoryInputTransport(kBinaryStringEncoding);
+    TProtocol iprot = new TBinaryProtocol(buf);
+    testTruncated(new MyStringStruct(), iprot);
+  }
+
+  @Test
+  public void testStringCompact() throws Exception {
+    TMemoryInputTransport buf = new TMemoryInputTransport(kCompactStringEncoding);
+    TProtocol iprot = new TCompactProtocol(buf);
+    testTruncated(new MyStringStruct(), iprot);
   }
 }

Vulnerable code:     (byte) 0x01, // value = 2L
    (byte) 0x01, // value = 3L
    (byte) 0x01, // value = 2L
    (byte) 0x01, // value = 3L
  private static final char[] hexArray = "0123456789ABCDEF".toCharArray();
  private static String bytesToHex(byte[] bytes, int length) {
    String out = "";
    for (int j = 0; j < length; j++) {
      int v = bytes[j] & 0xFF;
      out += hexArray[v >>> 4];
      out += hexArray[v & 0x0F];
      out += " ";
    }
    return out;
Fixed code:     ensureContainerHasEnough(size, TType.BYTE);
    ensureContainerHasEnough(size, TType.BYTE);
    ensureContainerHasEnough(length, TType.BYTE);

struct MyStringStruct {
  1: string aLongString
}
import com.facebook.thrift.java.test.MyStringStruct;
    (byte) 0x02, // value = 2L
    (byte) 0x03, // value = 3L
    (byte) 0x02, // value = 2L
    (byte) 0x03, // value = 3L
  private static final byte[] kBinaryStringEncoding = {
    TType.STRING, // Field Type = string
    (byte) 0x00,
    (byte) 0x01, // Field id = 1
    (byte) 0x00,
    (byte) 0x00,
    (byte) 0x00,
    (byte) 0xFF, // string length (255!)
    (byte) 0x48,
    (byte) 0x65,
    (byte) 0x6C,
    (byte) 0x6C,
    (byte) 0x6F,
    (byte) 0x2C,
    (byte) 0x20,
    (byte) 0x57,
    (byte) 0x6F,
    (byte) 0x72,
    (byte) 0x6C,
    (byte) 0x64,
    (byte) 0x21, // string chars: "Hello, World!"
    (byte) 0x00, // Stop
  };
  private static final byte[] kCompactStringEncoding = {
    (byte) 0b00011000, // field id delta (0001) + type (1000) = Binary
    (byte) 0xFF,
    (byte) 0x0F, // string size (varint) = 0x0FFF (4095)
    (byte) 0x48,
    (byte) 0x65,
    (byte) 0x6C,
    (byte) 0x6C,
    (byte) 0x6F,
    (byte) 0x2C,
    (byte) 0x20,
    (byte) 0x57,
    (byte) 0x6F,
    (byte) 0x72,
    (byte) 0x6C,
    (byte) 0x64,
    (byte) 0x21, // string chars: "Hello, World!"
    (byte) 0x00, // Stop
  };

  @Test
  public void testStringBinary() throws Exception {
    TMemoryInputTransport buf = new TMemoryInputTransport(kBinaryStringEncoding);
    TProtocol iprot = new TBinaryProtocol(buf);
    testTruncated(new MyStringStruct(), iprot);
  }

  @Test
  public void testStringCompact() throws Exception {
    TMemoryInputTransport buf = new TMemoryInputTransport(kCompactStringEncoding);
    TProtocol iprot = new TCompactProtocol(buf);
    testTruncated(new MyStringStruct(), iprot);