From 0c92828864118517f8d8a23a2801da82852cf36c Mon Sep 17 00:00:00 2001 From: Dominic Evans Date: Fri, 18 Mar 2016 11:20:23 +0000 Subject: [PATCH] PROTON-1249: Safeguard type initialisations. In #readValue() for ArrayType, BinaryType, ListType and MapType decoding, if the 'count' specified is very large then it is likely to trigger an OutOfMemoryException. As these can come from an external data source, during the SASL init for example, there is a potential for a denial of service. The fix is to throw an IllegalArgumentException if the count value is larger than the amount of data available in the received bytes. --- .../org/apache/qpid/proton/codec/ArrayType.java | 17 ++++++++++++++--- .../apache/qpid/proton/codec/BinaryType.java | 9 +++++++-- .../qpid/proton/codec/ByteBufferDecoder.java | 2 ++ .../apache/qpid/proton/codec/DecoderImpl.java | 4 ++++ .../org/apache/qpid/proton/codec/ListType.java | 5 +++++ .../org/apache/qpid/proton/codec/MapType.java | 4 ++++ 6 files changed, 36 insertions(+), 5 deletions(-) diff --git a/proton-j/src/main/java/org/apache/qpid/proton/codec/ArrayType.java b/proton-j/src/main/java/org/apache/qpid/proton/codec/ArrayType.java index 11a1dc966f..45b8dd5e30 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/codec/ArrayType.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/codec/ArrayType.java @@ -970,12 +970,18 @@ private CharacterType.CharacterEncoding getUnderlyingEncoding(final char[] a) private static Object[] decodeArray(final DecoderImpl decoder, final int count) { TypeConstructor constructor = decoder.readConstructor(); - return decodeNonPrimitive(constructor, count); + return decodeNonPrimitive(decoder, constructor, count); } - private static Object[] decodeNonPrimitive(final TypeConstructor constructor, + private static Object[] decodeNonPrimitive(final DecoderImpl decoder, + final TypeConstructor constructor, final int count) { + if (count > decoder.getByteBufferRemaining()) { + throw new IllegalArgumentException("Array element count "+count+" is specified to be greater than the amount of data available ("+ + decoder.getByteBufferRemaining()+")"); + } + if(constructor instanceof ArrayEncoding) { ArrayEncoding arrayEncoding = (ArrayEncoding) constructor; @@ -1006,6 +1012,11 @@ private static Object decodeArrayAsObject(final DecoderImpl decoder, final int c TypeConstructor constructor = decoder.readConstructor(); if(constructor.encodesJavaPrimitive()) { + if (count > decoder.getByteBufferRemaining()) { + throw new IllegalArgumentException("Array element count "+count+" is specified to be greater than the amount of data available ("+ + decoder.getByteBufferRemaining()+")"); + } + if(constructor instanceof BooleanType.BooleanEncoding) { return decodeBooleanArray((BooleanType.BooleanEncoding) constructor, count); @@ -1042,7 +1053,7 @@ else if(constructor instanceof DoubleType.DoubleEncoding) } else { - return decodeNonPrimitive(constructor, count); + return decodeNonPrimitive(decoder, constructor, count); } } diff --git a/proton-j/src/main/java/org/apache/qpid/proton/codec/BinaryType.java b/proton-j/src/main/java/org/apache/qpid/proton/codec/BinaryType.java index 6b463e42ea..88c204faf9 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/codec/BinaryType.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/codec/BinaryType.java @@ -105,9 +105,14 @@ public boolean encodesSuperset(final TypeEncoding encoding) public Binary readValue() { - int size = getDecoder().readRawInt(); + final DecoderImpl decoder = getDecoder(); + int size = decoder.readRawInt(); + if (size > decoder.getByteBufferRemaining()) { + throw new IllegalArgumentException("Binary data size "+size+" is specified to be greater than the amount of data available ("+ + decoder.getByteBufferRemaining()+")"); + } byte[] data = new byte[size]; - getDecoder().readRaw(data, 0, size); + decoder.readRaw(data, 0, size); return new Binary(data); } } diff --git a/proton-j/src/main/java/org/apache/qpid/proton/codec/ByteBufferDecoder.java b/proton-j/src/main/java/org/apache/qpid/proton/codec/ByteBufferDecoder.java index 4a10d76f91..39718b76ba 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/codec/ByteBufferDecoder.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/codec/ByteBufferDecoder.java @@ -25,4 +25,6 @@ public interface ByteBufferDecoder extends Decoder { public void setByteBuffer(ByteBuffer buffer); + + public int getByteBufferRemaining(); } diff --git a/proton-j/src/main/java/org/apache/qpid/proton/codec/DecoderImpl.java b/proton-j/src/main/java/org/apache/qpid/proton/codec/DecoderImpl.java index 450ad0e24a..dd68f6ad81 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/codec/DecoderImpl.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/codec/DecoderImpl.java @@ -992,4 +992,8 @@ public boolean equals(Object obj) } } + + public int getByteBufferRemaining() { + return _buffer.remaining(); + } } diff --git a/proton-j/src/main/java/org/apache/qpid/proton/codec/ListType.java b/proton-j/src/main/java/org/apache/qpid/proton/codec/ListType.java index 0c726b2299..185373f9bf 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/codec/ListType.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/codec/ListType.java @@ -155,6 +155,11 @@ public List readValue() int size = decoder.readRawInt(); // todo - limit the decoder with size int count = decoder.readRawInt(); + // Ensure we do not allocate an array of size greater then the available data, otherwise there is a risk for an OOM error + if (count > decoder.getByteBufferRemaining()) { + throw new IllegalArgumentException("List element count "+count+" is specified to be greater than the amount of data available ("+ + decoder.getByteBufferRemaining()+")"); + } List list = new ArrayList(count); for(int i = 0; i < count; i++) { diff --git a/proton-j/src/main/java/org/apache/qpid/proton/codec/MapType.java b/proton-j/src/main/java/org/apache/qpid/proton/codec/MapType.java index 53aac4f377..5c8a7c7cbb 100644 --- a/proton-j/src/main/java/org/apache/qpid/proton/codec/MapType.java +++ b/proton-j/src/main/java/org/apache/qpid/proton/codec/MapType.java @@ -150,6 +150,10 @@ public Map readValue() int size = decoder.readRawInt(); // todo - limit the decoder with size int count = decoder.readRawInt(); + if (count > decoder.getByteBufferRemaining()) { + throw new IllegalArgumentException("Map element count "+count+" is specified to be greater than the amount of data available ("+ + decoder.getByteBufferRemaining()+")"); + } Map map = new LinkedHashMap(count); for(int i = 0; i < count; i++) {