From a15a4b0a23ef8e96db8048baeb58d650572c0fb8 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Wed, 29 Nov 2023 09:17:04 +0100 Subject: [PATCH] Fixes #10919 - EE10 multipart parsing may include '\r' at the front under certain conditions (#10921) Setting MultiPart.Parser.crContent=false when the boundary is matched. This is necessary when a read chunk ends with \r\n, and then matches the boundary on the next read chunk, and avoids to emit the CR as part content. Fixed case of a chunk ending with \r. Fixed case where a CR in a previous chunk is followed by a boundary at index 0 in the next chunk. The CR should not be emitted as content, and crContent reset to false. Signed-off-by: Simone Bordet --- .../org/eclipse/jetty/http/MultiPart.java | 33 +-- .../handler/MultiPartFormDataHandlerTest.java | 192 +++++++++++++++++- 2 files changed, 210 insertions(+), 15 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java index a6758b5b076..bf990249237 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPart.java @@ -1326,6 +1326,7 @@ public class MultiPart // The boundary was fully matched. buffer.position(buffer.position() + boundaryMatch - partialBoundaryMatch); partialBoundaryMatch = 0; + crContent = false; notifyPartContent(Content.Chunk.EOF); notifyPartEnd(); return true; @@ -1349,14 +1350,8 @@ public class MultiPart return false; } - // Must output as content the previous partial match. - if (crContent) - { - crContent = false; - Content.Chunk partContentChunk = Content.Chunk.from(CR.slice(), false); - notifyPartContent(partContentChunk); - partContentChunk.release(); - } + // Output as content the previous partial match. + notifyCRContent(); ByteBuffer content = ByteBuffer.wrap(boundaryFinder.getPattern(), 0, partialBoundaryMatch); partialBoundaryMatch = 0; Content.Chunk partContentChunk = Content.Chunk.from(content, false); @@ -1368,8 +1363,12 @@ public class MultiPart // Search for a full boundary. int boundaryOffset = boundaryFinder.match(buffer); + if (boundaryOffset == 0) + crContent = false; if (boundaryOffset >= 0) { + // Output as content the previous partial match, if any. + notifyCRContent(); int position = buffer.position(); int length = boundaryOffset; // BoundaryFinder is configured to search for '\n--Boundary'; @@ -1406,11 +1405,9 @@ public class MultiPart } // There is normal content with no boundary. - if (crContent) - { - crContent = false; - notifyPartContent(Content.Chunk.from(CR.slice(), false)); - } + + // Output as content the previous partial match, if any. + notifyCRContent(); // If '\r' is found at the end of the buffer, it may // not be content but the beginning of a '\r\n--Boundary'; // remember it in case it is truly normal content. @@ -1428,6 +1425,16 @@ public class MultiPart return false; } + private void notifyCRContent() + { + if (!crContent) + return; + crContent = false; + Content.Chunk partContentChunk = Content.Chunk.from(CR.slice(), false); + notifyPartContent(partContentChunk); + partContentChunk.release(); + } + private Content.Chunk asSlice(Content.Chunk chunk, int position, int length, boolean last) { if (chunk.isLast() && !chunk.hasRemaining()) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java index 97df17d5b3e..375642169d8 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/handler/MultiPartFormDataHandlerTest.java @@ -16,7 +16,9 @@ package org.eclipse.jetty.server.handler; import java.net.InetSocketAddress; import java.nio.ByteBuffer; import java.nio.channels.SocketChannel; +import java.nio.charset.StandardCharsets; import java.nio.file.Path; +import java.util.concurrent.TimeUnit; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; @@ -204,7 +206,7 @@ public class MultiPartFormDataHandlerTest { try { - // Allow method process(...) to return. + // Allow method handle(...) to return. Thread.sleep(1000); // Add another part and close the multipart content. @@ -214,7 +216,7 @@ public class MultiPartFormDataHandlerTest } catch (InterruptedException x) { - x.printStackTrace(); + source.fail(x); } finally { @@ -262,4 +264,190 @@ public class MultiPartFormDataHandlerTest } } } + + @Test + public void testMultiPartWithTransferEncodingChunked() throws Exception + { + start(new Handler.Abstract.NonBlocking() + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception + { + String boundary = MultiPart.extractBoundary(request.getHeaders().get(HttpHeader.CONTENT_TYPE)); + MultiPartFormData.Parser parser = new MultiPartFormData.Parser(boundary); + parser.setMaxMemoryFileSize(-1); + MultiPartFormData.Parts parts = parser.parse(request).get(5, TimeUnit.SECONDS); + + assertEquals(2, parts.size()); + MultiPart.Part part0 = parts.get(0); + String part0Content = part0.getContentAsString(StandardCharsets.ISO_8859_1); + assertEquals("upload_file", part0Content); + MultiPart.Part part1 = parts.get(1); + String part1Content = part1.getContentAsString(StandardCharsets.US_ASCII); + assertEquals("abcde", part1Content); + callback.succeeded(); + return true; + } + }); + + try (SocketChannel client = SocketChannel.open(new InetSocketAddress("localhost", connector.getLocalPort()))) + { + String request = """ + POST / HTTP/1.1\r + Host: localhost\r + Content-Type: multipart/form-data; boundary=908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r + Transfer-Encoding: chunked\r + \r + 90\r + --908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r + Content-Disposition: form-data; name="az"\r + Content-Type: text/plain; charset=ISO-8859-1\r + \r + upload_file\r + \r + 94\r + --908d442b-2c7d-401a-ab46-7c6ec6f89fe6\r + Content-Disposition: form-data; name="file_upload"; filename="testUpload.test"\r + Content-Type: text/plain\r + \r + \r + 5\r + abcde\r + 2\r + \r + \r + 28\r + --908d442b-2c7d-401a-ab46-7c6ec6f89fe6--\r + 0\r + \r + """; + + client.write(UTF_8.encode(request)); + + HttpTester.Response response = HttpTester.parseResponse(HttpTester.from(client)); + assertNotNull(response); + assertEquals(HttpStatus.OK_200, response.getStatus()); + } + } + + @Test + public void testMultiPartWithTransferEncodingChunkedContentEndingWithCR() throws Exception + { + start(new Handler.Abstract.NonBlocking() + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception + { + String boundary = MultiPart.extractBoundary(request.getHeaders().get(HttpHeader.CONTENT_TYPE)); + MultiPartFormData.Parser parser = new MultiPartFormData.Parser(boundary); + parser.setMaxMemoryFileSize(-1); + MultiPartFormData.Parts parts = parser.parse(request).get(5, TimeUnit.SECONDS); + + assertEquals(2, parts.size()); + MultiPart.Part part0 = parts.get(0); + String part0Content = part0.getContentAsString(StandardCharsets.ISO_8859_1); + assertEquals("text_one\r", part0Content); + MultiPart.Part part1 = parts.get(1); + String part1Content = part1.getContentAsString(StandardCharsets.US_ASCII); + assertEquals("text_two", part1Content); + callback.succeeded(); + return true; + } + }); + + try (SocketChannel client = SocketChannel.open(new InetSocketAddress("localhost", connector.getLocalPort()))) + { + String request = """ + POST / HTTP/1.1\r + Host: localhost\r + Content-Type: multipart/form-data; boundary=A1B2C3\r + Transfer-Encoding: chunked\r + \r + 6A\r + --A1B2C3\r + Content-Disposition: form-data; name="one"\r + Content-Type: text/plain; charset=UTF-8\r + \r + text_one\r\r + 61\r + \r + --A1B2C3\r + Content-Disposition: form-data; name="two"\r + Content-Type: text/plain; charset=UTF-8\r + \r + C\r + \r + text_two\r + \r + A\r + --A1B2C3--\r + 0\r + \r + """; + + client.write(UTF_8.encode(request)); + + HttpTester.Response response = HttpTester.parseResponse(HttpTester.from(client)); + assertNotNull(response); + assertEquals(HttpStatus.OK_200, response.getStatus()); + } + } + + @Test + public void testMultiPartWithTransferEncodingChunkedBoundaryOnNewChunk() throws Exception + { + start(new Handler.Abstract.NonBlocking() + { + @Override + public boolean handle(Request request, Response response, Callback callback) throws Exception + { + String boundary = MultiPart.extractBoundary(request.getHeaders().get(HttpHeader.CONTENT_TYPE)); + MultiPartFormData.Parser parser = new MultiPartFormData.Parser(boundary); + parser.setMaxMemoryFileSize(-1); + MultiPartFormData.Parts parts = parser.parse(request).get(5, TimeUnit.SECONDS); + + assertEquals(2, parts.size()); + MultiPart.Part part0 = parts.get(0); + String part0Content = part0.getContentAsString(StandardCharsets.ISO_8859_1); + assertEquals("text_one", part0Content); + MultiPart.Part part1 = parts.get(1); + String part1Content = part1.getContentAsString(StandardCharsets.US_ASCII); + assertEquals("text_two", part1Content); + callback.succeeded(); + return true; + } + }); + + try (SocketChannel client = SocketChannel.open(new InetSocketAddress("localhost", connector.getLocalPort()))) + { + String request = """ + POST / HTTP/1.1\r + Host: localhost\r + Content-Type: multipart/form-data; boundary=A1B2C3\r + Transfer-Encoding: chunked\r + \r + 6A\r + --A1B2C3\r + Content-Disposition: form-data; name="one"\r + Content-Type: text/plain; charset=UTF-8\r + \r + text_one\r\r + 76\r + \n--A1B2C3\r + Content-Disposition: form-data; name="two"\r + Content-Type: text/plain; charset=UTF-8\r + \r + text_two\r + --A1B2C3--\r + 0\r + \r + """; + + client.write(UTF_8.encode(request)); + + HttpTester.Response response = HttpTester.parseResponse(HttpTester.from(client)); + assertNotNull(response); + assertEquals(HttpStatus.OK_200, response.getStatus()); + } + } }