From 783bc87396fc1a81d7ab8385c052b76ddf55454f Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Mon, 18 Sep 2017 16:13:48 +0200 Subject: [PATCH 1/3] Allow `InputStreamStreamInput` array size validation where applicable Today we can't validate the array length in `InputStreamStreamInput` since we can't rely on `InputStream.available` yet in some situations we know the size of the stream and can apply additional validation. --- .../io/stream/InputStreamStreamInput.java | 19 ++++++++++++++++++- .../common/io/stream/StreamInput.java | 2 +- .../index/translog/TranslogReader.java | 3 ++- .../common/io/stream/StreamTests.java | 18 ++++++++++++++++++ .../percolator/PercolateQueryBuilder.java | 3 ++- 5 files changed, 41 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/io/stream/InputStreamStreamInput.java b/core/src/main/java/org/elasticsearch/common/io/stream/InputStreamStreamInput.java index 6d952b01a21e3..6699dc64821d5 100644 --- a/core/src/main/java/org/elasticsearch/common/io/stream/InputStreamStreamInput.java +++ b/core/src/main/java/org/elasticsearch/common/io/stream/InputStreamStreamInput.java @@ -28,9 +28,24 @@ public class InputStreamStreamInput extends StreamInput { private final InputStream is; + private final long sizeLimit; public InputStreamStreamInput(InputStream is) { + this(is, Long.MAX_VALUE); + } + + /** + * Creates a new InputStreamStreamInput with a size limit + * @param is the input stream to wrap + * @param sizeLimit a hard limit of the number of bytes in the given input stream. This is used for internal input validation + */ + public InputStreamStreamInput(InputStream is, long sizeLimit) { this.is = is; + if (sizeLimit < 0) { + throw new IllegalArgumentException("size limit must be positive"); + } + this.sizeLimit = sizeLimit; + } @Override @@ -98,6 +113,8 @@ public long skip(long n) throws IOException { @Override protected void ensureCanReadBytes(int length) throws EOFException { - // TODO what can we do here? + if (length > sizeLimit) { + throw new EOFException("tried to read: " + length + " bytes but this stream is limited to: " + sizeLimit); + } } } diff --git a/core/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java b/core/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java index ac627cfd95d7f..31f53874f1949 100644 --- a/core/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java +++ b/core/src/main/java/org/elasticsearch/common/io/stream/StreamInput.java @@ -928,7 +928,7 @@ public static StreamInput wrap(byte[] bytes) { } public static StreamInput wrap(byte[] bytes, int offset, int length) { - return new InputStreamStreamInput(new ByteArrayInputStream(bytes, offset, length)); + return new InputStreamStreamInput(new ByteArrayInputStream(bytes, offset, length), length); } /** diff --git a/core/src/main/java/org/elasticsearch/index/translog/TranslogReader.java b/core/src/main/java/org/elasticsearch/index/translog/TranslogReader.java index 46439afead10a..b88037c32fd59 100644 --- a/core/src/main/java/org/elasticsearch/index/translog/TranslogReader.java +++ b/core/src/main/java/org/elasticsearch/index/translog/TranslogReader.java @@ -79,7 +79,8 @@ public static TranslogReader open( final FileChannel channel, final Path path, final Checkpoint checkpoint, final String translogUUID) throws IOException { try { - InputStreamStreamInput headerStream = new InputStreamStreamInput(java.nio.channels.Channels.newInputStream(channel)); // don't close + InputStreamStreamInput headerStream = new InputStreamStreamInput(java.nio.channels.Channels.newInputStream(channel), + channel.size()); // don't close // Lucene's CodecUtil writes a magic number of 0x3FD76C17 with the // header, in binary this looks like: // diff --git a/core/src/test/java/org/elasticsearch/common/io/stream/StreamTests.java b/core/src/test/java/org/elasticsearch/common/io/stream/StreamTests.java index 9d885fe131c7a..59eda8030e60c 100644 --- a/core/src/test/java/org/elasticsearch/common/io/stream/StreamTests.java +++ b/core/src/test/java/org/elasticsearch/common/io/stream/StreamTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.io.stream; +import com.sun.xml.internal.messaging.saaj.util.ByteOutputStream; import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; @@ -26,6 +27,7 @@ import org.elasticsearch.test.ESTestCase; import java.io.ByteArrayInputStream; +import java.io.EOFException; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; @@ -192,6 +194,22 @@ public void testInputStreamStreamInputDelegatesAvailable() throws IOException { assertEquals(streamInput.available(), length - bytesToRead); } + public void testReadArraySize() throws IOException { + BytesStreamOutput stream = new BytesStreamOutput(); + byte[] array = new byte[randomIntBetween(1, 10)]; + for (int i = 0; i < array.length; i++) { + array[i] = randomByte(); + } + stream.writeByteArray(array); + InputStreamStreamInput streamInput = new InputStreamStreamInput(StreamInput.wrap(BytesReference.toBytes(stream.bytes())), array + .length-1); + expectThrows(EOFException.class, streamInput::readByteArray); + streamInput = new InputStreamStreamInput(StreamInput.wrap(BytesReference.toBytes(stream.bytes())), BytesReference.toBytes(stream + .bytes()).length); + + assertArrayEquals(array, streamInput.readByteArray()); + } + public void testWritableArrays() throws IOException { final String[] strings = generateRandomStringArray(10, 10, false, true); diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java index 337b7ee2f36b5..f0f4de771f4e0 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java @@ -705,7 +705,8 @@ static PercolateQuery.QueryStore createStore(MappedFieldType queryBuilderFieldTy if (binaryDocValues.advanceExact(docId)) { BytesRef qbSource = binaryDocValues.binaryValue(); try (InputStream in = new ByteArrayInputStream(qbSource.bytes, qbSource.offset, qbSource.length)) { - try (StreamInput input = new NamedWriteableAwareStreamInput(new InputStreamStreamInput(in), registry)) { + try (StreamInput input = new NamedWriteableAwareStreamInput(new InputStreamStreamInput(in, qbSource.length), + registry)) { input.setVersion(indexVersion); // Query builder's content is stored via BinaryFieldMapper, which has a custom encoding // to encode multiple binary values into a single binary doc values field. From f0d3479eac9022756a4ef62bfb5654d362d2cc84 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Mon, 18 Sep 2017 16:19:52 +0200 Subject: [PATCH 2/3] fix imports --- .../java/org/elasticsearch/common/io/stream/StreamTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/test/java/org/elasticsearch/common/io/stream/StreamTests.java b/core/src/test/java/org/elasticsearch/common/io/stream/StreamTests.java index 59eda8030e60c..d64dece7867aa 100644 --- a/core/src/test/java/org/elasticsearch/common/io/stream/StreamTests.java +++ b/core/src/test/java/org/elasticsearch/common/io/stream/StreamTests.java @@ -19,7 +19,6 @@ package org.elasticsearch.common.io.stream; -import com.sun.xml.internal.messaging.saaj.util.ByteOutputStream; import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; From 82ed9c1d42f6ecf4b19a1201650f22e81c5621f7 Mon Sep 17 00:00:00 2001 From: Simon Willnauer Date: Mon, 18 Sep 2017 17:20:59 +0200 Subject: [PATCH 3/3] address commetns --- .../common/io/stream/InputStreamStreamInput.java | 4 ++++ .../org/elasticsearch/percolator/PercolateQueryBuilder.java | 4 ++-- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/io/stream/InputStreamStreamInput.java b/core/src/main/java/org/elasticsearch/common/io/stream/InputStreamStreamInput.java index 6699dc64821d5..5999427e1a206 100644 --- a/core/src/main/java/org/elasticsearch/common/io/stream/InputStreamStreamInput.java +++ b/core/src/main/java/org/elasticsearch/common/io/stream/InputStreamStreamInput.java @@ -30,6 +30,10 @@ public class InputStreamStreamInput extends StreamInput { private final InputStream is; private final long sizeLimit; + /** + * Creates a new InputStreamStreamInput with unlimited size + * @param is the input stream to wrap + */ public InputStreamStreamInput(InputStream is) { this(is, Long.MAX_VALUE); } diff --git a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java index f0f4de771f4e0..db1b444dcd28e 100644 --- a/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java +++ b/modules/percolator/src/main/java/org/elasticsearch/percolator/PercolateQueryBuilder.java @@ -705,8 +705,8 @@ static PercolateQuery.QueryStore createStore(MappedFieldType queryBuilderFieldTy if (binaryDocValues.advanceExact(docId)) { BytesRef qbSource = binaryDocValues.binaryValue(); try (InputStream in = new ByteArrayInputStream(qbSource.bytes, qbSource.offset, qbSource.length)) { - try (StreamInput input = new NamedWriteableAwareStreamInput(new InputStreamStreamInput(in, qbSource.length), - registry)) { + try (StreamInput input = new NamedWriteableAwareStreamInput( + new InputStreamStreamInput(in, qbSource.length), registry)) { input.setVersion(indexVersion); // Query builder's content is stored via BinaryFieldMapper, which has a custom encoding // to encode multiple binary values into a single binary doc values field.