From 0416168d0edfaa8f321e318bd4129df7b425ee79 Mon Sep 17 00:00:00 2001 From: Arjen Poutsma Date: Wed, 27 Oct 2021 15:07:15 +0200 Subject: [PATCH] Fix bug in max header calculation in DefaultPartHttpMessageReader This commit fixes a bug in the DefaultPartHttpMessageReader, in the check for exceeding the maximum header size. Before this commit, the entire buffer size was considered, thus triggering an exception even though the max header limit was not exceeded. After this commit, we only consider the size up until the end-of-header mark (CRLFCRLF). Furthermore, this commit increases the default maximum header size to 10k, the same default as Commons File upload. Closes gh-27612 --- .../DefaultPartHttpMessageReader.java | 2 +- .../http/codec/multipart/MultipartParser.java | 71 +++++++++++-------- .../DefaultPartHttpMessageReaderTests.java | 25 +++++++ .../http/codec/multipart/files.multipart | 2 - 4 files changed, 67 insertions(+), 33 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/codec/multipart/DefaultPartHttpMessageReader.java b/spring-web/src/main/java/org/springframework/http/codec/multipart/DefaultPartHttpMessageReader.java index d4248c8442d..6e94678cef1 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/multipart/DefaultPartHttpMessageReader.java +++ b/spring-web/src/main/java/org/springframework/http/codec/multipart/DefaultPartHttpMessageReader.java @@ -63,7 +63,7 @@ public class DefaultPartHttpMessageReader extends LoggingCodecSupport implements private int maxInMemorySize = 256 * 1024; - private int maxHeadersSize = 8 * 1024; + private int maxHeadersSize = 10 * 1024; private long maxDiskUsagePerPart = -1; diff --git a/spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartParser.java b/spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartParser.java index d2057f53d62..d797b99f4b1 100644 --- a/spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartParser.java +++ b/spring-web/src/main/java/org/springframework/http/codec/multipart/MultipartParser.java @@ -342,33 +342,23 @@ final class MultipartParser extends BaseSubscriber { /** * First checks whether the multipart boundary leading to this state - * was the final boundary, or whether {@link #maxHeadersSize} is - * exceeded. Then looks for the header-body boundary - * ({@code CR LF CR LF}) in the given buffer. If found, convert - * all buffers collected so far into a {@link HttpHeaders} object + * was the final boundary. Then looks for the header-body boundary + * ({@code CR LF CR LF}) in the given buffer. If found, checks whether + * the size of all header buffers does not exceed {@link #maxHeadersSize}, + * converts all buffers collected so far into a {@link HttpHeaders} object * and changes to {@link BodyState}, passing the remainder of the - * buffer. If the boundary is not found, the buffer is collected. + * buffer. If the boundary is not found, the buffer is collected if + * its size does not exceed {@link #maxHeadersSize}. */ @Override public void onNext(DataBuffer buf) { - long prevCount = this.byteCount.get(); - long count = this.byteCount.addAndGet(buf.readableByteCount()); - if (prevCount < 2 && count >= 2) { - if (isLastBoundary(buf)) { - if (logger.isTraceEnabled()) { - logger.trace("Last boundary found in " + buf); - } - - if (changeState(this, DisposedState.INSTANCE, buf)) { - emitComplete(); - } - return; + if (isLastBoundary(buf)) { + if (logger.isTraceEnabled()) { + logger.trace("Last boundary found in " + buf); } - } - else if (count > MultipartParser.this.maxHeadersSize) { + if (changeState(this, DisposedState.INSTANCE, buf)) { - emitError(new DataBufferLimitException("Part headers exceeded the memory usage limit of " + - MultipartParser.this.maxHeadersSize + " bytes")); + emitComplete(); } return; } @@ -377,17 +367,23 @@ final class MultipartParser extends BaseSubscriber { if (logger.isTraceEnabled()) { logger.trace("End of headers found @" + endIdx + " in " + buf); } - DataBuffer headerBuf = MultipartUtils.sliceTo(buf, endIdx); - this.buffers.add(headerBuf); - DataBuffer bodyBuf = MultipartUtils.sliceFrom(buf, endIdx); - DataBufferUtils.release(buf); - - emitHeaders(parseHeaders()); - changeState(this, new BodyState(), bodyBuf); + long count = this.byteCount.addAndGet(endIdx); + if (belowMaxHeaderSize(count)) { + DataBuffer headerBuf = MultipartUtils.sliceTo(buf, endIdx); + this.buffers.add(headerBuf); + DataBuffer bodyBuf = MultipartUtils.sliceFrom(buf, endIdx); + DataBufferUtils.release(buf); + + emitHeaders(parseHeaders()); + changeState(this, new BodyState(), bodyBuf); + } } else { - this.buffers.add(buf); - requestBuffer(); + long count = this.byteCount.addAndGet(buf.readableByteCount()); + if (belowMaxHeaderSize(count)) { + this.buffers.add(buf); + requestBuffer(); + } } } @@ -407,6 +403,21 @@ final class MultipartParser extends BaseSubscriber { buf.getByte(0) == HYPHEN); } + /** + * Checks whether the given {@code count} is below or equal to {@link #maxHeadersSize} + * and emits a {@link DataBufferLimitException} if not. + */ + private boolean belowMaxHeaderSize(long count) { + if (count <= MultipartParser.this.maxHeadersSize) { + return true; + } + else { + emitError(new DataBufferLimitException("Part headers exceeded the memory usage limit of " + + MultipartParser.this.maxHeadersSize + " bytes")); + return false; + } + } + /** * Parses the list of buffers into a {@link HttpHeaders} instance. * Converts the joined buffers into a string using ISO=8859-1, and parses diff --git a/spring-web/src/test/java/org/springframework/http/codec/multipart/DefaultPartHttpMessageReaderTests.java b/spring-web/src/test/java/org/springframework/http/codec/multipart/DefaultPartHttpMessageReaderTests.java index 8e812e720fb..9179a546820 100644 --- a/spring-web/src/test/java/org/springframework/http/codec/multipart/DefaultPartHttpMessageReaderTests.java +++ b/spring-web/src/test/java/org/springframework/http/codec/multipart/DefaultPartHttpMessageReaderTests.java @@ -270,6 +270,31 @@ public class DefaultPartHttpMessageReaderTests { latch.await(); } + // gh-27612 + @Test + public void exceedHeaderLimit() throws InterruptedException { + Flux body = DataBufferUtils + .readByteChannel((new ClassPathResource("files.multipart", getClass()))::readableChannel, bufferFactory, 282); + + MediaType contentType = new MediaType("multipart", "form-data", singletonMap("boundary", "----WebKitFormBoundaryG8fJ50opQOML0oGD")); + MockServerHttpRequest request = MockServerHttpRequest.post("/") + .contentType(contentType) + .body(body); + + DefaultPartHttpMessageReader reader = new DefaultPartHttpMessageReader(); + + reader.setMaxHeadersSize(230); + + Flux result = reader.read(forClass(Part.class), request, emptyMap()); + + CountDownLatch latch = new CountDownLatch(2); + StepVerifier.create(result) + .consumeNextWith(part -> testPart(part, null, LOREM_IPSUM, latch)) + .consumeNextWith(part -> testPart(part, null, MUSPI_MEROL, latch)) + .verifyComplete(); + + latch.await(); + } private void testBrowser(DefaultPartHttpMessageReader reader, Resource resource, String boundary) throws InterruptedException { diff --git a/spring-web/src/test/resources/org/springframework/http/codec/multipart/files.multipart b/spring-web/src/test/resources/org/springframework/http/codec/multipart/files.multipart index 03b41190647..bd10aea6b0a 100644 --- a/spring-web/src/test/resources/org/springframework/http/codec/multipart/files.multipart +++ b/spring-web/src/test/resources/org/springframework/http/codec/multipart/files.multipart @@ -3,11 +3,9 @@ Content-Disposition: form-data; name="file2"; filename="a.txt" Content-Type: text/plain Lorem ipsum dolor sit amet, consectetur adipiscing elit. Integer iaculis metus id vestibulum nullam. - ------WebKitFormBoundaryG8fJ50opQOML0oGD Content-Disposition: form-data; name="file2"; filename="b.txt" Content-Type: text/plain .mallun mulubitsev di sutem silucai regetnI .tile gnicsipida rutetcesnoc ,tema tis rolod muspi meroL - ------WebKitFormBoundaryG8fJ50opQOML0oGD--