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
diff --git a/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java b/lib/java/src/org/apache/thrift/protocol/TCompactProtocol.java
index 3db256d..7b273c5 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 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 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 @@
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 0cfd937..a2f79d7 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 @@
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 f0d7895..b4c0888 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 @@
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();
}