diff --git a/README.md b/README.md index 117416768..4e966eda0 100644 --- a/README.md +++ b/README.md @@ -30,6 +30,7 @@ Kryo maintenance and development is sponsored by the [Gecko fund](https://geckof - [IO](#io) * [Output](#output) * [Input](#input) + * [Limiting deserialized size](#limiting-deserialized-size) * [ByteBuffers](#bytebuffers) * [Unsafe buffers](#unsafe-buffers) * [Variable length encoding](#variable-length-encoding) @@ -226,6 +227,21 @@ If the Input `close` is called, the Input's InputStream is closed, if any. If no The zero argument Input constructor creates an uninitialized Input. Input `setBuffer` must be called before the Input can be used. +### Limiting deserialized size + +When reading an array, string, collection, or map, Kryo first reads a declared size and uses it to allocate before reading any elements. A corrupt or malicious message can declare a size of billions, triggering a large allocation from only a few bytes. + +For an Input backed by a byte array (no InputStream), this is guarded automatically: the declared size cannot exceed the bytes remaining, since every element occupies at least one byte, so an impossible size is rejected with a `KryoException` before allocating. No configuration is needed and valid input is never affected. + +An Input reading from an InputStream cannot be checked this way, because the buffered window is not the total size. For that case, `setMaxArraySize` bounds the declared size: + +```java +Input input = new Input(inputStream); +input.setMaxArraySize(1024 * 1024); // reject any declared array/string/collection/map size above 1M elements +``` + +The default is `Integer.MAX_VALUE`, ie no limit, so by default behavior is unchanged and Kryo's trusted-source assumption is preserved: a valid payload never declares more elements than the input can supply, so the limit never fires on valid input. A declared size above the limit throws a `KryoException` before allocating. Callers that decode untrusted input, especially from a stream, should set a limit suited to their application. + ### ByteBuffers The ByteBufferOutput and ByteBufferInput classes work exactly like Output and Input, except they use a ByteBuffer rather than a byte array. diff --git a/benchmarks/src/main/java/com/esotericsoftware/kryo/benchmarks/io/ArrayBenchmark.java b/benchmarks/src/main/java/com/esotericsoftware/kryo/benchmarks/io/ArrayBenchmark.java index 6649310c6..08ed6fa5d 100644 --- a/benchmarks/src/main/java/com/esotericsoftware/kryo/benchmarks/io/ArrayBenchmark.java +++ b/benchmarks/src/main/java/com/esotericsoftware/kryo/benchmarks/io/ArrayBenchmark.java @@ -77,6 +77,18 @@ public void readVarLongs (ReadLongsState state) { state.input.readLongs(state.longs.length, true); } + @Benchmark + public void writeDoubles (WriteDoublesState state) { + state.reset(); + state.output.writeDoubles(state.doubles, 0, state.doubles.length); + } + + @Benchmark + public void readDoubles (ReadDoublesState state) { + state.reset(); + state.input.readDoubles(state.doubles.length); + } + // @State(Scope.Thread) @@ -111,4 +123,18 @@ public void setup () { new ArrayBenchmark().writeLongs(this); } } + + @State(Scope.Thread) + static public class WriteDoublesState extends InputOutputState { + public double[] doubles = {0, 1, 2, 3, 4, 5, 63, 64, 65, 127, 128, 129, 4000, 5000, 6000, 16000, 32000, 256000, 1024000, + -1, -2, -3, -4, Double.MIN_VALUE, Double.MAX_VALUE, 0.5, -0.5, 3.14159}; + } + + @State(Scope.Thread) + static public class ReadDoublesState extends WriteDoublesState { + public void setup () { + super.setup(); + new ArrayBenchmark().writeDoubles(this); + } + } } diff --git a/src/com/esotericsoftware/kryo/io/ByteBufferInput.java b/src/com/esotericsoftware/kryo/io/ByteBufferInput.java index fc2c58f65..e0130b116 100644 --- a/src/com/esotericsoftware/kryo/io/ByteBufferInput.java +++ b/src/com/esotericsoftware/kryo/io/ByteBufferInput.java @@ -333,7 +333,7 @@ public int readByteUnsigned () throws KryoException { } public byte[] readBytes (int length) throws KryoException { - byte[] bytes = new byte[length]; + byte[] bytes = new byte[validateArrayLength(length)]; readBytes(bytes, 0, length); return bytes; } @@ -762,7 +762,7 @@ public StringBuilder readStringBuilder () { } private void readUtf8Chars (int charCount) { - if (chars.length < charCount) chars = new char[charCount]; + if (chars.length < charCount) chars = new char[validateArrayLength(charCount)]; char[] chars = this.chars; // Try to read 7 bit ASCII chars. ByteBuffer byteBuffer = this.byteBuffer; @@ -858,8 +858,8 @@ private String readAscii_slow (int charCount) { // Primitive arrays: public int[] readInts (int length) throws KryoException { - int[] array = new int[length]; - if (optional(length << 2) == length << 2) { + int[] array = new int[validateArrayLength(length, Integer.BYTES)]; + if (optional(length * Integer.BYTES) == length * Integer.BYTES) { ByteBuffer byteBuffer = this.byteBuffer; for (int i = 0; i < length; i++) { array[i] = byteBuffer.get() & 0xFF // @@ -876,8 +876,8 @@ public int[] readInts (int length) throws KryoException { } public long[] readLongs (int length) throws KryoException { - long[] array = new long[length]; - if (optional(length << 3) == length << 3) { + long[] array = new long[validateArrayLength(length, Long.BYTES)]; + if (optional(length * Long.BYTES) == length * Long.BYTES) { ByteBuffer byteBuffer = this.byteBuffer; for (int i = 0; i < length; i++) { array[i] = byteBuffer.get() & 0xFF// @@ -898,8 +898,8 @@ public long[] readLongs (int length) throws KryoException { } public float[] readFloats (int length) throws KryoException { - float[] array = new float[length]; - if (optional(length << 2) == length << 2) { + float[] array = new float[validateArrayLength(length, Float.BYTES)]; + if (optional(length * Float.BYTES) == length * Float.BYTES) { ByteBuffer byteBuffer = this.byteBuffer; for (int i = 0; i < length; i++) { array[i] = Float.intBitsToFloat(byteBuffer.get() & 0xFF // @@ -916,8 +916,8 @@ public float[] readFloats (int length) throws KryoException { } public double[] readDoubles (int length) throws KryoException { - double[] array = new double[length]; - if (optional(length << 3) == length << 3) { + double[] array = new double[validateArrayLength(length, Double.BYTES)]; + if (optional(length * Double.BYTES) == length * Double.BYTES) { ByteBuffer byteBuffer = this.byteBuffer; for (int i = 0; i < length; i++) { array[i] = Double.longBitsToDouble(byteBuffer.get() & 0xFF // @@ -938,8 +938,8 @@ public double[] readDoubles (int length) throws KryoException { } public short[] readShorts (int length) throws KryoException { - short[] array = new short[length]; - if (optional(length << 1) == length << 1) { + short[] array = new short[validateArrayLength(length, Short.BYTES)]; + if (optional(length * Short.BYTES) == length * Short.BYTES) { ByteBuffer byteBuffer = this.byteBuffer; for (int i = 0; i < length; i++) array[i] = (short)((byteBuffer.get() & 0xFF) | ((byteBuffer.get() & 0xFF) << 8)); @@ -952,8 +952,8 @@ public short[] readShorts (int length) throws KryoException { } public char[] readChars (int length) throws KryoException { - char[] array = new char[length]; - if (optional(length << 1) == length << 1) { + char[] array = new char[validateArrayLength(length, Character.BYTES)]; + if (optional(length * Character.BYTES) == length * Character.BYTES) { ByteBuffer byteBuffer = this.byteBuffer; for (int i = 0; i < length; i++) array[i] = (char)((byteBuffer.get() & 0xFF) | ((byteBuffer.get() & 0xFF) << 8)); @@ -966,7 +966,7 @@ public char[] readChars (int length) throws KryoException { } public boolean[] readBooleans (int length) throws KryoException { - boolean[] array = new boolean[length]; + boolean[] array = new boolean[validateArrayLength(length)]; if (optional(length) == length) { ByteBuffer byteBuffer = this.byteBuffer; for (int i = 0; i < length; i++) diff --git a/src/com/esotericsoftware/kryo/io/Input.java b/src/com/esotericsoftware/kryo/io/Input.java index 6b83334b2..8f50649f5 100644 --- a/src/com/esotericsoftware/kryo/io/Input.java +++ b/src/com/esotericsoftware/kryo/io/Input.java @@ -39,6 +39,7 @@ public class Input extends InputStream implements Poolable { protected char[] chars = new char[32]; protected InputStream inputStream; protected boolean varEncoding = true; + protected int maxArraySize = Integer.MAX_VALUE; /** Creates an uninitialized Input, {@link #setBuffer(byte[])} must be called before the Input is used. */ public Input () { @@ -126,6 +127,24 @@ public void setVariableLengthEncoding (boolean varEncoding) { this.varEncoding = varEncoding; } + /** Returns the maximum size, in elements, that a declared array, string, collection, or map size may have when reading. See + * {@link #setMaxArraySize(int)}. */ + public int getMaxArraySize () { + return maxArraySize; + } + + /** Sets the maximum size, in elements, that a declared array, string, collection, or map size may have when reading. A + * declared size larger than this throws {@link KryoException} before any allocation, bounding the memory a corrupt or + * malicious message can request. The default is {@link Integer#MAX_VALUE}, ie no limit, which preserves Kryo's trusted-source + * behavior: a valid payload never declares more elements than the input can supply, so the limit never fires on valid input. + * Callers that decode untrusted input, especially from an {@link InputStream} where the declared size cannot be checked + * against the buffered bytes, should set a limit suited to their application. + * @param maxArraySize must be >= 0. */ + public void setMaxArraySize (int maxArraySize) { + if (maxArraySize < 0) throw new IllegalArgumentException("maxArraySize must be >= 0: " + maxArraySize); + this.maxArraySize = maxArraySize; + } + /** Returns the total number of bytes read. */ public long total () { return total + position; @@ -348,7 +367,7 @@ public int readByteUnsigned () throws KryoException { /** Reads the specified number of bytes into a new byte[]. */ public byte[] readBytes (int length) throws KryoException { - byte[] bytes = new byte[length]; + byte[] bytes = new byte[validateArrayLength(length)]; readBytes(bytes, 0, length); return bytes; } @@ -848,7 +867,7 @@ public StringBuilder readStringBuilder () { } private void readUtf8Chars (int charCount) { - if (chars.length < charCount) chars = new char[charCount]; + if (chars.length < charCount) chars = new char[validateArrayLength(charCount)]; byte[] buffer = this.buffer; char[] chars = this.chars; // Try to read 7 bit ASCII chars. @@ -941,10 +960,50 @@ private String readAscii_slow (int charCount) { // Primitive arrays: + /** Validates a declared array length read from the input, treating each element as occupying at least one byte. Equivalent to + * {@link #validateArrayLength(int, int)} with a bytes per element of 1. */ + public int validateArrayLength (int length) { + return validateArrayLength(length, 1); + } + + /** Validates a declared array length read from the input before it is used to allocate. Throws if {@code length} exceeds + * {@link #setMaxArraySize(int) maxArraySize}, if the total size {@code length * bytesPerElement} would overflow an int, or, for + * a buffer-backed input (no {@link InputStream}), if it exceeds the bytes remaining. A buffer-backed input can never hold more + * elements than it has bytes remaining, since every element occupies at least {@code bytesPerElement} bytes, so a larger + * declared length is provably malformed and is rejected before allocating. Stream-backed input cannot be checked this way and + * is bounded only by {@code maxArraySize} and the overflow check. + * @param bytesPerElement the minimum number of bytes each element occupies in the input. */ + public int validateArrayLength (int length, int bytesPerElement) { + if (length > maxArraySize) + throw new KryoException("Declared size larger than maxArraySize: " + length + " > " + maxArraySize); + long bytes = (long)length * bytesPerElement; + if (inputStream == null) { + // A buffer-backed input cannot supply more bytes than it has remaining, so a larger declared size is malformed. + if (bytes > limit - position) throw new KryoBufferUnderflowException("Buffer underflow."); + } else if (bytes > Integer.MAX_VALUE) { + // Stream-backed input cannot be checked against the buffered window, but a size whose byte count overflows an int + // can never be read and would overflow the length * bytesPerElement computations in the bulk readers, so reject it. + throw new KryoException("Declared size is too large to read: " + length + " elements of " + bytesPerElement + " bytes"); + } + return length; + } + + /** Clamps a declared collection or map size before it is used as an initial capacity. Throws if {@code size} exceeds + * {@link #setMaxArraySize(int) maxArraySize}. Otherwise, for a buffer-backed input (no {@link InputStream}), caps it at the + * bytes remaining so a corrupt size cannot force a large pre-allocation; the collection or map still grows as elements are + * read. A stream-backed input is returned unchanged (the remaining size is unknown). Unlike + * {@link #validateArrayLength(int, int)} the size is only clamped, not rejected, because it is an initial-capacity hint rather + * than an exact element count. */ + public int clampSize (int size) { + if (size > maxArraySize) + throw new KryoException("Declared size larger than maxArraySize: " + size + " > " + maxArraySize); + return inputStream == null ? Math.min(size, limit - position) : size; + } + /** Reads an int array in bulk. This may be more efficient than reading them individually. */ public int[] readInts (int length) throws KryoException { - int[] array = new int[length]; - if (optional(length << 2) == length << 2) { + int[] array = new int[validateArrayLength(length, Integer.BYTES)]; + if (optional(length * Integer.BYTES) == length * Integer.BYTES) { byte[] buffer = this.buffer; int p = this.position; for (int i = 0; i < length; i++, p += 4) { @@ -965,7 +1024,7 @@ public int[] readInts (int length) throws KryoException { * {@link #setVariableLengthEncoding(boolean)}. This may be more efficient than reading them individually. */ public int[] readInts (int length, boolean optimizePositive) throws KryoException { if (varEncoding) { - int[] array = new int[length]; + int[] array = new int[validateArrayLength(length)]; for (int i = 0; i < length; i++) array[i] = readVarInt(optimizePositive); return array; @@ -975,8 +1034,8 @@ public int[] readInts (int length, boolean optimizePositive) throws KryoExceptio /** Reads a long array in bulk. This may be more efficient than reading them individually. */ public long[] readLongs (int length) throws KryoException { - long[] array = new long[length]; - if (optional(length << 3) == length << 3) { + long[] array = new long[validateArrayLength(length, Long.BYTES)]; + if (optional(length * Long.BYTES) == length * Long.BYTES) { byte[] buffer = this.buffer; int p = this.position; for (int i = 0; i < length; i++, p += 8) { @@ -1001,7 +1060,7 @@ public long[] readLongs (int length) throws KryoException { * {@link #setVariableLengthEncoding(boolean)}. This may be more efficient than reading them individually. */ public long[] readLongs (int length, boolean optimizePositive) throws KryoException { if (varEncoding) { - long[] array = new long[length]; + long[] array = new long[validateArrayLength(length)]; for (int i = 0; i < length; i++) array[i] = readVarLong(optimizePositive); return array; @@ -1011,8 +1070,8 @@ public long[] readLongs (int length, boolean optimizePositive) throws KryoExcept /** Reads a float array in bulk. This may be more efficient than reading them individually. */ public float[] readFloats (int length) throws KryoException { - float[] array = new float[length]; - if (optional(length << 2) == length << 2) { + float[] array = new float[validateArrayLength(length, Float.BYTES)]; + if (optional(length * Float.BYTES) == length * Float.BYTES) { byte[] buffer = this.buffer; int p = this.position; for (int i = 0; i < length; i++, p += 4) { @@ -1031,8 +1090,8 @@ public float[] readFloats (int length) throws KryoException { /** Reads a double array in bulk. This may be more efficient than reading them individually. */ public double[] readDoubles (int length) throws KryoException { - double[] array = new double[length]; - if (optional(length << 3) == length << 3) { + double[] array = new double[validateArrayLength(length, Double.BYTES)]; + if (optional(length * Double.BYTES) == length * Double.BYTES) { byte[] buffer = this.buffer; int p = this.position; for (int i = 0; i < length; i++, p += 8) { @@ -1055,8 +1114,8 @@ public double[] readDoubles (int length) throws KryoException { /** Reads a short array in bulk. This may be more efficient than reading them individually. */ public short[] readShorts (int length) throws KryoException { - short[] array = new short[length]; - if (optional(length << 1) == length << 1) { + short[] array = new short[validateArrayLength(length, Short.BYTES)]; + if (optional(length * Short.BYTES) == length * Short.BYTES) { byte[] buffer = this.buffer; int p = this.position; for (int i = 0; i < length; i++, p += 2) @@ -1071,8 +1130,8 @@ public short[] readShorts (int length) throws KryoException { /** Reads a char array in bulk. This may be more efficient than reading them individually. */ public char[] readChars (int length) throws KryoException { - char[] array = new char[length]; - if (optional(length << 1) == length << 1) { + char[] array = new char[validateArrayLength(length, Character.BYTES)]; + if (optional(length * Character.BYTES) == length * Character.BYTES) { byte[] buffer = this.buffer; int p = this.position; for (int i = 0; i < length; i++, p += 2) @@ -1087,7 +1146,7 @@ public char[] readChars (int length) throws KryoException { /** Reads a boolean array in bulk. This may be more efficient than reading them individually. */ public boolean[] readBooleans (int length) throws KryoException { - boolean[] array = new boolean[length]; + boolean[] array = new boolean[validateArrayLength(length)]; if (optional(length) == length) { byte[] buffer = this.buffer; int p = this.position; diff --git a/src/com/esotericsoftware/kryo/serializers/ClosureSerializer.java b/src/com/esotericsoftware/kryo/serializers/ClosureSerializer.java index 5061e64a8..aa2282f1a 100644 --- a/src/com/esotericsoftware/kryo/serializers/ClosureSerializer.java +++ b/src/com/esotericsoftware/kryo/serializers/ClosureSerializer.java @@ -98,7 +98,7 @@ public void write (Kryo kryo, Output output, Object object) { public Object read (Kryo kryo, Input input, Class type) { int count = input.readVarInt(true); - Object[] capturedArgs = new Object[count]; + Object[] capturedArgs = new Object[input.validateArrayLength(count)]; for (int i = 0; i < count; i++) capturedArgs[i] = kryo.readClassAndObject(input); Class capturingClass = kryo.readClass(input).getType(); diff --git a/src/com/esotericsoftware/kryo/serializers/CollectionSerializer.java b/src/com/esotericsoftware/kryo/serializers/CollectionSerializer.java index 299a72b2c..17e8cd3e8 100644 --- a/src/com/esotericsoftware/kryo/serializers/CollectionSerializer.java +++ b/src/com/esotericsoftware/kryo/serializers/CollectionSerializer.java @@ -203,7 +203,7 @@ public T read (Kryo kryo, Input input, Class type) { if (length == 0) return null; length--; - collection = create(kryo, input, type, length); + collection = create(kryo, input, type, input.clampSize(length)); kryo.reference(collection); if (length == 0) return collection; @@ -213,7 +213,7 @@ public T read (Kryo kryo, Input input, Class type) { if (length == 0) return null; length--; - collection = create(kryo, input, type, length); + collection = create(kryo, input, type, input.clampSize(length)); kryo.reference(collection); if (length == 0) return collection; diff --git a/src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java b/src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java index 75874321d..c3708c36c 100644 --- a/src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java +++ b/src/com/esotericsoftware/kryo/serializers/CompatibleFieldSerializer.java @@ -193,7 +193,7 @@ public T read (Kryo kryo, Input input, Class type) { private CachedField[] readFields (Kryo kryo, Input input) { if (TRACE) trace("kryo", "Read fields for class: " + type.getName()); - int length = input.readVarInt(true); + int length = input.validateArrayLength(input.readVarInt(true)); String[] names = new String[length]; for (int i = 0; i < length; i++) { names[i] = input.readString(); diff --git a/src/com/esotericsoftware/kryo/serializers/DefaultArraySerializers.java b/src/com/esotericsoftware/kryo/serializers/DefaultArraySerializers.java index fd3c261af..97b88137b 100644 --- a/src/com/esotericsoftware/kryo/serializers/DefaultArraySerializers.java +++ b/src/com/esotericsoftware/kryo/serializers/DefaultArraySerializers.java @@ -277,7 +277,7 @@ public void write (Kryo kryo, Output output, String[] object) { public String[] read (Kryo kryo, Input input, Class type) { int length = input.readVarInt(true); if (length == NULL) return null; - String[] array = new String[--length]; + String[] array = new String[input.validateArrayLength(--length)]; if (kryo.getReferences() && kryo.getReferenceResolver().useReferences(String.class)) { Serializer serializer = kryo.getSerializer(String.class); for (int i = 0; i < length; i++) diff --git a/src/com/esotericsoftware/kryo/serializers/MapSerializer.java b/src/com/esotericsoftware/kryo/serializers/MapSerializer.java index 2a0edf843..bc5085347 100644 --- a/src/com/esotericsoftware/kryo/serializers/MapSerializer.java +++ b/src/com/esotericsoftware/kryo/serializers/MapSerializer.java @@ -188,7 +188,7 @@ public T read (Kryo kryo, Input input, Class type) { if (length == 0) return null; length--; - T map = create(kryo, input, type, length); + T map = create(kryo, input, type, input.clampSize(length)); kryo.reference(map); if (length == 0) return map; diff --git a/src/com/esotericsoftware/kryo/unsafe/UnsafeByteBufferInput.java b/src/com/esotericsoftware/kryo/unsafe/UnsafeByteBufferInput.java index 2e5945694..6eb78eea9 100644 --- a/src/com/esotericsoftware/kryo/unsafe/UnsafeByteBufferInput.java +++ b/src/com/esotericsoftware/kryo/unsafe/UnsafeByteBufferInput.java @@ -185,43 +185,43 @@ public boolean readBoolean () throws KryoException { } public int[] readInts (int length) throws KryoException { - int[] array = new int[length]; - readBytes(array, intArrayBaseOffset, length << 2); + int[] array = new int[validateArrayLength(length, Integer.BYTES)]; + readBytes(array, intArrayBaseOffset, length * Integer.BYTES); return array; } public long[] readLongs (int length) throws KryoException { - long[] array = new long[length]; - readBytes(array, longArrayBaseOffset, length << 3); + long[] array = new long[validateArrayLength(length, Long.BYTES)]; + readBytes(array, longArrayBaseOffset, length * Long.BYTES); return array; } public float[] readFloats (int length) throws KryoException { - float[] array = new float[length]; - readBytes(array, floatArrayBaseOffset, length << 2); + float[] array = new float[validateArrayLength(length, Float.BYTES)]; + readBytes(array, floatArrayBaseOffset, length * Float.BYTES); return array; } public double[] readDoubles (int length) throws KryoException { - double[] array = new double[length]; - readBytes(array, doubleArrayBaseOffset, length << 3); + double[] array = new double[validateArrayLength(length, Double.BYTES)]; + readBytes(array, doubleArrayBaseOffset, length * Double.BYTES); return array; } public short[] readShorts (int length) throws KryoException { - short[] array = new short[length]; - readBytes(array, shortArrayBaseOffset, length << 1); + short[] array = new short[validateArrayLength(length, Short.BYTES)]; + readBytes(array, shortArrayBaseOffset, length * Short.BYTES); return array; } public char[] readChars (int length) throws KryoException { - char[] array = new char[length]; - readBytes(array, charArrayBaseOffset, length << 1); + char[] array = new char[validateArrayLength(length, Character.BYTES)]; + readBytes(array, charArrayBaseOffset, length * Character.BYTES); return array; } public boolean[] readBooleans (int length) throws KryoException { - boolean[] array = new boolean[length]; + boolean[] array = new boolean[validateArrayLength(length)]; readBytes(array, booleanArrayBaseOffset, length); return array; } diff --git a/src/com/esotericsoftware/kryo/unsafe/UnsafeInput.java b/src/com/esotericsoftware/kryo/unsafe/UnsafeInput.java index 79f673b45..d98ca680c 100644 --- a/src/com/esotericsoftware/kryo/unsafe/UnsafeInput.java +++ b/src/com/esotericsoftware/kryo/unsafe/UnsafeInput.java @@ -136,43 +136,43 @@ public boolean readBoolean () throws KryoException { } public int[] readInts (int length) throws KryoException { - int[] array = new int[length]; - readBytes(array, intArrayBaseOffset, length << 2); + int[] array = new int[validateArrayLength(length, Integer.BYTES)]; + readBytes(array, intArrayBaseOffset, length * Integer.BYTES); return array; } public long[] readLongs (int length) throws KryoException { - long[] array = new long[length]; - readBytes(array, longArrayBaseOffset, length << 3); + long[] array = new long[validateArrayLength(length, Long.BYTES)]; + readBytes(array, longArrayBaseOffset, length * Long.BYTES); return array; } public float[] readFloats (int length) throws KryoException { - float[] array = new float[length]; - readBytes(array, floatArrayBaseOffset, length << 2); + float[] array = new float[validateArrayLength(length, Float.BYTES)]; + readBytes(array, floatArrayBaseOffset, length * Float.BYTES); return array; } public double[] readDoubles (int length) throws KryoException { - double[] array = new double[length]; - readBytes(array, doubleArrayBaseOffset, length << 3); + double[] array = new double[validateArrayLength(length, Double.BYTES)]; + readBytes(array, doubleArrayBaseOffset, length * Double.BYTES); return array; } public short[] readShorts (int length) throws KryoException { - short[] array = new short[length]; - readBytes(array, shortArrayBaseOffset, length << 1); + short[] array = new short[validateArrayLength(length, Short.BYTES)]; + readBytes(array, shortArrayBaseOffset, length * Short.BYTES); return array; } public char[] readChars (int length) throws KryoException { - char[] array = new char[length]; - readBytes(array, charArrayBaseOffset, length << 1); + char[] array = new char[validateArrayLength(length, Character.BYTES)]; + readBytes(array, charArrayBaseOffset, length * Character.BYTES); return array; } public boolean[] readBooleans (int length) throws KryoException { - boolean[] array = new boolean[length]; + boolean[] array = new boolean[validateArrayLength(length)]; readBytes(array, booleanArrayBaseOffset, length); return array; } diff --git a/test/com/esotericsoftware/kryo/io/InputOutputTest.java b/test/com/esotericsoftware/kryo/io/InputOutputTest.java index b0e5a19f3..9540fab16 100644 --- a/test/com/esotericsoftware/kryo/io/InputOutputTest.java +++ b/test/com/esotericsoftware/kryo/io/InputOutputTest.java @@ -23,6 +23,7 @@ import static org.junit.jupiter.api.Assertions.*; import com.esotericsoftware.kryo.Kryo; +import com.esotericsoftware.kryo.KryoException; import com.esotericsoftware.kryo.KryoTestCase; import com.esotericsoftware.kryo.Registration; import com.esotericsoftware.kryo.io.KryoBufferUnderflowException; @@ -139,6 +140,98 @@ void testUnderflow () throws IOException { assertTrue(thrown.getMessage().equals("Buffer underflow.")); } + @Test + void testArrayLengthValidation () throws IOException { + int hugeLength = 2000000000; + + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readInts(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readInts(hugeLength, true)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readLongs(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readLongs(hugeLength, true)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readFloats(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readDoubles(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readShorts(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readChars(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readBooleans(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[5]).readBytes(hugeLength)); + + assertThrows(KryoBufferUnderflowException.class, () -> new ByteBufferInput(new byte[5]).readInts(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new ByteBufferInput(new byte[5]).readDoubles(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new ByteBufferInput(new byte[5]).readBytes(hugeLength)); + + int[] ints = {1, 2, 3, 4, 5}; + Output output = new Output(64); + output.writeInts(ints, 0, ints.length, false); + output.flush(); + assertArrayEquals(ints, new Input(output.toBytes()).readInts(ints.length, false)); + } + + @Test + void testMaliciousArrayLength () throws IOException { + kryo.register(int[].class); + + Output declared = new Output(8); + declared.writeVarInt(2000000001, true); + declared.flush(); + assertThrows(KryoException.class, () -> kryo.readObject(new Input(declared.toBytes()), int[].class)); + + int[] valid = {1, 2, 3, 4, 5}; + Output output = new Output(64); + kryo.writeObject(output, valid); + output.flush(); + assertArrayEquals(valid, kryo.readObject(new Input(output.toBytes()), int[].class)); + } + + @Test + void testMaliciousStringLength () throws IOException { + Output declared = new Output(8); + declared.writeVarIntFlag(true, 2000000001, true); + declared.flush(); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(declared.toBytes()).readString()); + assertThrows(KryoBufferUnderflowException.class, () -> new ByteBufferInput(declared.toBytes()).readString()); + + String value = "kryo \u00e9\u00fc\u1234 unicode string"; + Output output = new Output(64); + output.writeString(value); + output.flush(); + assertEquals(value, new Input(output.toBytes()).readString()); + } + + @Test + void testMaxArraySize () throws IOException { + Input stream = new Input(new ByteArrayInputStream(new byte[16])); + stream.setMaxArraySize(1024); + assertEquals(1024, stream.getMaxArraySize()); + assertThrows(KryoException.class, () -> stream.readInts(2000000)); + assertThrows(KryoException.class, () -> stream.readBytes(2000000)); + assertThrows(KryoException.class, () -> stream.readLongs(2000000, true)); + } + + @Test + void testArrayLengthByteWidthValidation () throws IOException { + // A declared element count that fits the buffer by count but not once element width is counted is rejected up front, + // before the (potentially much larger) array is allocated. Eight ints need 32 bytes, not 8. + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[8]).readInts(8)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[8]).readLongs(8)); + assertThrows(KryoBufferUnderflowException.class, () -> new Input(new byte[8]).readDoubles(8)); + assertThrows(KryoBufferUnderflowException.class, () -> new ByteBufferInput(new byte[8]).readInts(8)); + + // Stream-backed: a declared size whose total byte count overflows an int can never be read and previously overflowed the + // length << n shifts (risking ArrayIndexOutOfBoundsException or a huge allocation). It is now rejected cleanly. + assertThrows(KryoException.class, () -> new Input(new ByteArrayInputStream(new byte[16])).readInts(600000000)); + assertThrows(KryoException.class, () -> new Input(new ByteArrayInputStream(new byte[16])).readLongs(300000000)); + + // setMaxArraySize rejects negative limits. + assertThrows(IllegalArgumentException.class, () -> new Input(new byte[1]).setMaxArraySize(-1)); + + // Valid fixed-width arrays still round-trip through the guard. + long[] longs = {1, 2, 3, 4, 5}; + Output output = new Output(64); + output.writeLongs(longs, 0, longs.length, false); + output.flush(); + assertArrayEquals(longs, new Input(output.toBytes()).readLongs(longs.length, false)); + } + @Test void testStrings () throws IOException { runStringTest(new Output(4096)); diff --git a/test/com/esotericsoftware/kryo/io/UnsafeInputOutputTest.java b/test/com/esotericsoftware/kryo/io/UnsafeInputOutputTest.java index 2d6256d33..8236c2716 100644 --- a/test/com/esotericsoftware/kryo/io/UnsafeInputOutputTest.java +++ b/test/com/esotericsoftware/kryo/io/UnsafeInputOutputTest.java @@ -23,6 +23,8 @@ import static org.junit.jupiter.api.Assertions.*; import com.esotericsoftware.kryo.Unsafe; +import com.esotericsoftware.kryo.io.KryoBufferUnderflowException; +import com.esotericsoftware.kryo.unsafe.UnsafeByteBufferInput; import com.esotericsoftware.kryo.unsafe.UnsafeInput; import com.esotericsoftware.kryo.unsafe.UnsafeOutput; @@ -35,6 +37,18 @@ /** @author Nathan Sweet */ @Unsafe class UnsafeInputOutputTest { + @Test + void testArrayLengthValidation () { + int hugeLength = 2000000000; + + assertThrows(KryoBufferUnderflowException.class, () -> new UnsafeInput(new byte[5]).readInts(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new UnsafeInput(new byte[5]).readLongs(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new UnsafeInput(new byte[5]).readDoubles(hugeLength)); + + assertThrows(KryoBufferUnderflowException.class, () -> new UnsafeByteBufferInput(new byte[5]).readInts(hugeLength)); + assertThrows(KryoBufferUnderflowException.class, () -> new UnsafeByteBufferInput(new byte[5]).readDoubles(hugeLength)); + } + @Test void testOutputStream () { ByteArrayOutputStream buffer = new ByteArrayOutputStream(); diff --git a/test/com/esotericsoftware/kryo/serializers/CollectionSerializerTest.java b/test/com/esotericsoftware/kryo/serializers/CollectionSerializerTest.java index a98650948..9c6615308 100644 --- a/test/com/esotericsoftware/kryo/serializers/CollectionSerializerTest.java +++ b/test/com/esotericsoftware/kryo/serializers/CollectionSerializerTest.java @@ -22,11 +22,15 @@ import static org.junit.jupiter.api.Assertions.*; import com.esotericsoftware.kryo.Kryo; +import com.esotericsoftware.kryo.KryoException; import com.esotericsoftware.kryo.KryoTestCase; +import com.esotericsoftware.kryo.io.Input; +import com.esotericsoftware.kryo.io.Output; import com.esotericsoftware.kryo.serializers.DefaultSerializers.StringSerializer; import com.esotericsoftware.kryo.serializers.MapSerializerTest.KeyComparator; import com.esotericsoftware.kryo.serializers.MapSerializerTest.KeyThatIsntComparable; +import java.io.ByteArrayInputStream; import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; @@ -43,6 +47,30 @@ class CollectionSerializerTest extends KryoTestCase { supportsCopy = true; } + @Test + void testMaliciousCollectionSize () { + kryo.setReferences(false); + kryo.register(ArrayList.class); + Output declared = new Output(8); + kryo.writeClass(declared, ArrayList.class); + declared.writeVarIntFlag(false, 2000000001, true); + declared.flush(); + assertThrows(KryoException.class, () -> kryo.readClassAndObject(new Input(declared.toBytes()))); + } + + @Test + void testMaxCollectionSize () { + kryo.setReferences(false); + kryo.register(ArrayList.class); + Output declared = new Output(8); + kryo.writeClass(declared, ArrayList.class); + declared.writeVarIntFlag(false, 2000000001, true); + declared.flush(); + Input stream = new Input(new ByteArrayInputStream(declared.toBytes())); + stream.setMaxArraySize(1024); + assertThrows(KryoException.class, () -> kryo.readClassAndObject(stream)); + } + @Test void testCollections () { kryo.register(ArrayList.class); diff --git a/test/com/esotericsoftware/kryo/serializers/MapSerializerTest.java b/test/com/esotericsoftware/kryo/serializers/MapSerializerTest.java index 648785559..6faa71e82 100644 --- a/test/com/esotericsoftware/kryo/serializers/MapSerializerTest.java +++ b/test/com/esotericsoftware/kryo/serializers/MapSerializerTest.java @@ -22,6 +22,7 @@ import static org.junit.jupiter.api.Assertions.*; import com.esotericsoftware.kryo.Kryo; +import com.esotericsoftware.kryo.KryoException; import com.esotericsoftware.kryo.KryoTestCase; import com.esotericsoftware.kryo.io.Input; import com.esotericsoftware.kryo.io.Output; @@ -50,6 +51,30 @@ class MapSerializerTest extends KryoTestCase { supportsCopy = true; } + @Test + void testMaliciousMapSize () { + kryo.setReferences(false); + kryo.register(HashMap.class); + Output declared = new Output(8); + kryo.writeClass(declared, HashMap.class); + declared.writeVarInt(2000000001, true); + declared.flush(); + assertThrows(KryoException.class, () -> kryo.readClassAndObject(new Input(declared.toBytes()))); + } + + @Test + void testMaxMapSize () { + kryo.setReferences(false); + kryo.register(HashMap.class); + Output declared = new Output(8); + kryo.writeClass(declared, HashMap.class); + declared.writeVarInt(2000000001, true); + declared.flush(); + Input stream = new Input(new ByteArrayInputStream(declared.toBytes())); + stream.setMaxArraySize(1024); + assertThrows(KryoException.class, () -> kryo.readClassAndObject(stream)); + } + @Test void testMaps () { kryo.register(HashMap.class);