Skip to content
Permalink
Browse files Browse the repository at this point in the history
Java: Check the size of the remaining frame before deserializing coll…
…ection

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

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

Reviewed By: vitaut

Differential Revision: D14500775

fbshipit-source-id: ca8b38965514d6319addcb72c8999a6854a94a88
  • Loading branch information
stevegury authored and facebook-github-bot committed Dec 6, 2019
1 parent 0076bf7 commit 71c97ff
Show file tree
Hide file tree
Showing 7 changed files with 414 additions and 9 deletions.
Expand Up @@ -225,19 +225,29 @@ public TField readFieldBegin() throws TException {
public void readFieldEnd() {}

public TMap readMapBegin() throws TException {
return new TMap(readByte(), readByte(), readI32());
byte keyType = readByte();
byte valueType = readByte();
int size = readI32();
ensureMapHasEnough(size, keyType, valueType);
return new TMap(keyType, valueType, size);
}

public void readMapEnd() {}

public TList readListBegin() throws TException {
return new TList(readByte(), readI32());
byte type = readByte();
int size = readI32();
ensureContainerHasEnough(size, type);
return new TList(type, size);
}

public void readListEnd() {}

public TSet readSetBegin() throws TException {
return new TSet(readByte(), readI32());
byte type = readByte();
int size = readI32();
ensureContainerHasEnough(size, type);
return new TSet(type, size);
}

public void readSetEnd() {}
Expand Down Expand Up @@ -368,4 +378,35 @@ protected void checkReadLength(int length) throws TException {
}
}
}

@Override
protected int typeMinimumSize(byte type) {
switch (type & 0x0f) {
case TType.BOOL:
case TType.BYTE:
return 1;
case TType.I16:
return 2;
case TType.I32:
case TType.FLOAT:
return 4;
case TType.DOUBLE:
case TType.I64:
return 8;
case TType.STRING:
return 4;
case TType.LIST:
case TType.SET:
// type (1 byte) + size (4 bytes)
return 1 + 4;
case TType.MAP:
// key type (1 byte) + value type (1 byte) + size (4 bytes)
return 1 + 1 + 4;
case TType.STRUCT:
return 1;
default:
throw new TProtocolException(
TProtocolException.INVALID_DATA, "Unexpected data type " + (byte) (type & 0x0f));
}
}
}
Expand Up @@ -513,8 +513,12 @@ public TField readFieldBegin() throws TException {
public TMap readMapBegin() throws TException {
int size = readVarint32();
byte keyAndValueType = size == 0 ? 0 : readByte();
return new TMap(
getTType((byte) (keyAndValueType >> 4)), getTType((byte) (keyAndValueType & 0xf)), size);
byte keyType = getTType((byte) (keyAndValueType >> 4));
byte valueType = getTType((byte) (keyAndValueType & 0xf));
if (size > 0) {
ensureMapHasEnough(size, keyType, valueType);
}
return new TMap(keyType, valueType, size);
}

/**
Expand All @@ -529,6 +533,7 @@ public TList readListBegin() throws TException {
size = readVarint32();
}
byte type = getTType(size_and_type);
ensureContainerHasEnough(size, type);
return new TList(type, size);
}

Expand Down Expand Up @@ -829,4 +834,30 @@ private byte getTType(byte type) throws TProtocolException {
private byte getCompactType(byte ttype) {
return ttypeToCompactType[ttype];
}

@Override
protected int typeMinimumSize(byte type) {
switch (type & 0x0f) {
case TType.BOOL:
case TType.BYTE:
case TType.I16: // because of variable length encoding
case TType.I32: // because of variable length encoding
case TType.I64: // because of variable length encoding
return 1;
case TType.FLOAT:
return 4;
case TType.DOUBLE:
return 8;
case TType.STRING:
case TType.STRUCT:
case TType.MAP:
case TType.SET:
case TType.LIST:
case TType.ENUM:
return 1;
default:
throw new TProtocolException(
TProtocolException.INVALID_DATA, "Unexpected data type " + (byte) (type & 0x0f));
}
}
}
Expand Up @@ -179,4 +179,31 @@ public void reset() {}
public Class<? extends IScheme> getScheme() {
return StandardScheme.class;
}

/** Return the minimum size of a type */
protected int typeMinimumSize(byte type) {
return 1;
}

protected void ensureContainerHasEnough(int size, byte type) {
int minimumExpected = size * typeMinimumSize(type);
ensureHasEnoughBytes(minimumExpected);
}

protected void ensureMapHasEnough(int size, byte keyType, byte valueType) {
int minimumExpected = size * (typeMinimumSize(keyType) + typeMinimumSize(valueType));
ensureHasEnoughBytes(minimumExpected);
}

private void ensureHasEnoughBytes(int minimumExpected) {
int remaining = trans_.getBytesRemainingInBuffer();
if (remaining < 0) {
return; // Some transport are not buffered
}
if (remaining < minimumExpected) {
throw new TProtocolException(
TProtocolException.INVALID_DATA,
"Not enough bytes to read the entire message, the data appears to be truncated");
}
}
}
Expand Up @@ -217,4 +217,9 @@ public String readString() throws TException {
public byte[] readBinary() throws TException {
return concreteProtocol.readBinary();
}

@Override
protected int typeMinimumSize(byte type) {
return concreteProtocol.typeMinimumSize(type);
}
}
15 changes: 14 additions & 1 deletion thrift/lib/java/src/resources/testing_data_structures.thrift
Expand Up @@ -14,7 +14,8 @@
* limitations under the License.
*/

namespace java com.facebook.thrift.test
namespace java com.facebook.thrift.java.test
namespace java.swift com.facebook.thrift.javaswift.test

struct MySimpleStruct {
1: i64 id,
Expand Down Expand Up @@ -49,3 +50,15 @@ enum BigEnum {
NINETEEN = 19,
TWENTY = 20,
}

struct MyListStruct {
1: list<i64> ids
}

struct MySetStruct {
1: set<i64> ids
}

struct MyMapStruct {
1: map<i64, string> mapping
}
Expand Up @@ -21,9 +21,9 @@
import static org.hamcrest.core.Is.is;
import static org.junit.Assert.assertThat;

import com.facebook.thrift.test.BigEnum;
import com.facebook.thrift.test.MySimpleStruct;
import com.facebook.thrift.test.SmallEnum;
import com.facebook.thrift.java.test.BigEnum;
import com.facebook.thrift.java.test.MySimpleStruct;
import com.facebook.thrift.java.test.SmallEnum;
import junit.framework.TestCase;
import org.junit.Test;

Expand Down

0 comments on commit 71c97ff

Please sign in to comment.