Thrift-1643:Denial of Service attack in TBinaryProtocol.readString

Client: java
Patch: Niraj Tolia

In readString, if the string field's size is greater than the number of bytes remaining in the byte array to deserialize, libthrift will happily allocate a byte array of that size in readStringBody, filling the heap.



git-svn-id: https://svn.apache.org/repos/asf/thrift/branches/0.9.x@1397398 13f79535-47bb-0310-9956-ffa450edef68

3 files changed, 47 insertions, 8 deletions

diff --git a/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java b/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
index 3db256def..7b273c5e0 100644
--- a/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
+++ b/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
@@ -639,15 +639,12 @@ public class TCompactProtocol extends TProtocol {
    */
   public String readString() throws TException {
     int length = readVarint32();
+    checkReadLength(length);
 
     if (length == 0) {
       return "";
     }
 
-    if (maxNetworkBytes_ != -1 && length > maxNetworkBytes_) {
-      throw new TException("Read size greater than max allowed.");
-    }
-
     try {
       if (trans_.getBytesRemainingInBuffer() >= length) {
         String str = new String(trans_.getBuffer(), trans_.getBufferPosition(), length, "UTF-8");
@@ -666,12 +663,9 @@ public class TCompactProtocol extends TProtocol {
    */
   public ByteBuffer readBinary() throws TException {
     int length = readVarint32();
+    checkReadLength(length);
     if (length == 0) return ByteBuffer.wrap(new byte[0]);
 
-    if (maxNetworkBytes_ != -1 && length > maxNetworkBytes_) {
-      throw new TException("Read size greater than max allowed.");
-    }
-
     byte[] buf = new byte[length];
     trans_.readAll(buf, 0, length);
     return ByteBuffer.wrap(buf);
@@ -688,6 +682,15 @@ public class TCompactProtocol extends TProtocol {
     return buf;
   }
 
+  private void checkReadLength(int length) throws TProtocolException {
+    if (length < 0) {
+      throw new TProtocolException("Negative length: " + length);
+    }
+    if (maxNetworkBytes_ != -1 && length > maxNetworkBytes_) {
+      throw new TProtocolException("Length exceeded max allowed: " + length);
+    }
+  }
+
   //
   // These methods are here for the struct to call, but don't have any wire 
   // encoding.
diff --git a/lib/java/test/org/apache/thrift/protocol/TestTBinaryProtocol.java b/lib/java/test/org/apache/thrift/protocol/TestTBinaryProtocol.java
index 0cfd9377a..a2f79d79f 100644
--- a/lib/java/test/org/apache/thrift/protocol/TestTBinaryProtocol.java
+++ b/lib/java/test/org/apache/thrift/protocol/TestTBinaryProtocol.java
@@ -19,6 +19,11 @@
 
 package org.apache.thrift.protocol;
 
+import org.apache.thrift.TDeserializer;
+import org.apache.thrift.TException;
+
+import thrift.test.Bonk;
+
 public class TestTBinaryProtocol extends ProtocolTestBase {
   @Override
   protected TProtocolFactory getFactory() {
@@ -29,4 +34,17 @@ public class TestTBinaryProtocol extends ProtocolTestBase {
   protected boolean canBeUsedNaked() {
     return true;
   }
+
+  public void testOOMDenialOfService() throws Exception {
+    TDeserializer deser = new TDeserializer(new TBinaryProtocol
+					    .Factory(false, false, 1000));
+    Bonk bonk = new Bonk();
+    try {
+      // Invalid read length specified here. Would cause an OOM
+      // without the limit on the read length
+      deser.deserialize(bonk, new byte[]{11, 0, 1, 127, -1, -1, -1});
+    } catch (TException e) {
+      // Ignore as we are only checking for OOM in the failure case
+    }
+  }
 }
diff --git a/lib/java/test/org/apache/thrift/protocol/TestTCompactProtocol.java b/lib/java/test/org/apache/thrift/protocol/TestTCompactProtocol.java
index f0d78955e..b4c0888a4 100644
--- a/lib/java/test/org/apache/thrift/protocol/TestTCompactProtocol.java
+++ b/lib/java/test/org/apache/thrift/protocol/TestTCompactProtocol.java
@@ -20,6 +20,10 @@
 
 package org.apache.thrift.protocol;
 
+import org.apache.thrift.TDeserializer;
+import org.apache.thrift.TException;
+
+import thrift.test.Bonk;
 
 public class TestTCompactProtocol extends ProtocolTestBase {
   @Override
@@ -32,6 +36,20 @@ public class TestTCompactProtocol extends ProtocolTestBase {
     return true;
   }
 
+  public void testOOMDenialOfService() throws Exception {
+    // Struct header, Integer.MAX_VALUE length, and only one real
+    // byte of data
+    byte [] bytes = {24, -1, -1, -1, -17, 49};
+    TDeserializer deser = new TDeserializer(new TCompactProtocol
+					    .Factory(1000));
+    Bonk bonk = new Bonk();
+    try {
+	deser.deserialize(bonk, bytes);
+    } catch (TException e) {
+	// Ignore as we are only checking for OOM in the failure case
+    }
+  }
+
   public static void main(String args[]) throws Exception {
     new TestTCompactProtocol().benchmark();
   }