From 1042cdb8a54e86c14e593ce6f69072a88bcac393 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 13 Feb 2023 11:56:06 +1100 Subject: [PATCH 1/5] cleanups and extra testing for multipart Signed-off-by: Lachlan Roberts --- .../jetty/http/MultiPartFormInputStream.java | 24 ++- .../org/eclipse/jetty/server/MultiParts.java | 14 +- .../org/eclipse/jetty/server/Request.java | 27 +++- .../jetty/servlet/MultiPartServletTest.java | 142 ++++++++++++++++++ .../util/MultiPartInputStreamParser.java | 23 ++- 5 files changed, 220 insertions(+), 10 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java index 22533461a531..27af3f380602 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java @@ -61,9 +61,12 @@ public class MultiPartFormInputStream { private static final Logger LOG = Log.getLogger(MultiPartFormInputStream.class); + private static final int DEFAULT_MAX_FORM_KEYS = 1000; private static final MultiMap EMPTY_MAP = new MultiMap<>(Collections.emptyMap()); - private final MultiMap _parts; private final EnumSet _nonComplianceWarnings = EnumSet.noneOf(NonCompliance.class); + private final MultiMap _parts; + private final int _maxParts; + private int _numParts = 0; private InputStream _in; private MultipartConfigElement _config; private String _contentType; @@ -350,18 +353,30 @@ public String getContentDispositionFilename() * @param contextTmpDir javax.servlet.context.tempdir */ public MultiPartFormInputStream(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir) + { + this(in, contentType, config, contextTmpDir, DEFAULT_MAX_FORM_KEYS); + } + + /** + * @param in Request input stream + * @param contentType Content-Type header + * @param config MultipartConfigElement + * @param contextTmpDir javax.servlet.context.tempdir + * @param maxParts the maximum number of parts that can be parsed from the multipart content (0 for no parts allowed, -1 for unlimited parts). + */ + public MultiPartFormInputStream(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, int maxParts) { _contentType = contentType; _config = config; _contextTmpDir = contextTmpDir; + _maxParts = maxParts; if (_contextTmpDir == null) _contextTmpDir = new File(System.getProperty("java.io.tmpdir")); if (_config == null) _config = new MultipartConfigElement(_contextTmpDir.getAbsolutePath()); - MultiMap parts = new MultiMap(); - + MultiMap parts = new MultiMap<>(); if (in instanceof ServletInputStream) { if (((ServletInputStream)in).isFinished()) @@ -752,6 +767,9 @@ public boolean content(ByteBuffer buffer, boolean last) public void startPart() { reset(); + _numParts++; + if (_maxParts >= 0 && _numParts > _maxParts) + throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", _numParts, _maxParts)); } @Override diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java index 5bb283dd6177..8ae73f7b7cc0 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiParts.java @@ -58,7 +58,12 @@ class MultiPartsHttpParser implements MultiParts public MultiPartsHttpParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request) throws IOException { - _httpParser = new MultiPartFormInputStream(in, contentType, config, contextTmpDir); + this(in, contentType, config, contextTmpDir, request, ContextHandler.DEFAULT_MAX_FORM_KEYS); + } + + public MultiPartsHttpParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request, int maxParts) throws IOException + { + _httpParser = new MultiPartFormInputStream(in, contentType, config, contextTmpDir, maxParts); _context = request.getContext(); _request = request; } @@ -123,7 +128,12 @@ class MultiPartsUtilParser implements MultiParts public MultiPartsUtilParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request) throws IOException { - _utilParser = new MultiPartInputStreamParser(in, contentType, config, contextTmpDir); + this(in, contentType, config, contextTmpDir, request, ContextHandler.DEFAULT_MAX_FORM_KEYS); + } + + public MultiPartsUtilParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, Request request, int maxParts) throws IOException + { + _utilParser = new MultiPartInputStreamParser(in, contentType, config, contextTmpDir, maxParts); _context = request.getContext(); _request = request; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 4d66f75eb8f7..2f60fb7d308c 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -2431,7 +2431,21 @@ private Collection getParts(MultiMap params) throws IOException if (config == null) throw new IllegalStateException("No multipart config for servlet"); - _multiParts = newMultiParts(config); + int maxFormContentSize = ContextHandler.DEFAULT_MAX_FORM_CONTENT_SIZE; + int maxFormKeys = ContextHandler.DEFAULT_MAX_FORM_KEYS; + if (_context != null) + { + ContextHandler contextHandler = _context.getContextHandler(); + maxFormContentSize = contextHandler.getMaxFormContentSize(); + maxFormKeys = contextHandler.getMaxFormKeys(); + } + else + { + maxFormContentSize = lookupServerAttribute(ContextHandler.MAX_FORM_CONTENT_SIZE_KEY, maxFormContentSize); + maxFormKeys = lookupServerAttribute(ContextHandler.MAX_FORM_KEYS_KEY, maxFormKeys); + } + + _multiParts = newMultiParts(config, maxFormKeys); setAttribute(MULTIPARTS, _multiParts); Collection parts = _multiParts.getParts(); @@ -2465,11 +2479,16 @@ else if (getCharacterEncoding() != null) else defaultCharset = StandardCharsets.UTF_8; + long formContentSize = 0; ByteArrayOutputStream os = null; for (Part p : parts) { if (p.getSubmittedFileName() == null) { + formContentSize = Math.addExact(formContentSize, p.getSize()); + if (maxFormContentSize >= 0 && formContentSize > maxFormContentSize) + throw new IllegalStateException("Form is larger than max length " + maxFormContentSize); + // Servlet Spec 3.0 pg 23, parts without filename must be put into params. String charset = null; if (p.getContentType() != null) @@ -2494,7 +2513,7 @@ else if (getCharacterEncoding() != null) return _multiParts.getParts(); } - private MultiParts newMultiParts(MultipartConfigElement config) throws IOException + private MultiParts newMultiParts(MultipartConfigElement config, int maxParts) throws IOException { MultiPartFormDataCompliance compliance = getHttpChannel().getHttpConfiguration().getMultipartFormDataCompliance(); if (LOG.isDebugEnabled()) @@ -2504,12 +2523,12 @@ private MultiParts newMultiParts(MultipartConfigElement config) throws IOExcepti { case RFC7578: return new MultiParts.MultiPartsHttpParser(getInputStream(), getContentType(), config, - (_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this); + (_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this, maxParts); case LEGACY: default: return new MultiParts.MultiPartsUtilParser(getInputStream(), getContentType(), config, - (_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this); + (_context != null ? (File)_context.getAttribute("javax.servlet.context.tempdir") : null), this, maxParts); } } diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java index b87e63c90785..85fb39afc62c 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java @@ -41,13 +41,16 @@ import org.eclipse.jetty.client.util.BytesContentProvider; import org.eclipse.jetty.client.util.InputStreamResponseListener; import org.eclipse.jetty.client.util.MultiPartContentProvider; +import org.eclipse.jetty.client.util.OutputStreamContentProvider; import org.eclipse.jetty.client.util.StringContentProvider; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpScheme; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.MultiPartFormInputStream; +import org.eclipse.jetty.io.EofException; import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.MultiPartFormDataCompliance; @@ -67,6 +70,8 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.startsWith; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -82,6 +87,7 @@ public class MultiPartServletTest private Path tmpDir; private static final int MAX_FILE_SIZE = 512 * 1024; + private static final int MAX_REQUEST_SIZE = 1024 * 1024 * 8; private static final int LARGE_MESSAGE_SIZE = 1024 * 1024; public static Stream data() @@ -89,6 +95,19 @@ public static Stream data() return Arrays.asList(MultiPartFormDataCompliance.values()).stream().map(Arguments::of); } + public static class RequestParameterServlet extends HttpServlet + { + @Override + protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException + { + req.getParameterMap(); + req.getParts(); + resp.setStatus(200); + resp.getWriter().print("success"); + resp.getWriter().close(); + } + } + public static class MultiPartServlet extends HttpServlet { @Override @@ -130,6 +149,7 @@ protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws S public void start() throws Exception { tmpDir = Files.createTempDirectory(MultiPartServletTest.class.getSimpleName()); + Files.deleteIfExists(tmpDir); assertNotNull(tmpDir); server = new Server(); @@ -138,11 +158,19 @@ public void start() throws Exception MultipartConfigElement config = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), MAX_FILE_SIZE, -1, 1); + MultipartConfigElement requestSizedConfig = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), + -1, MAX_REQUEST_SIZE, 1); + MultipartConfigElement defaultConfig = new MultipartConfigElement(tmpDir.toAbsolutePath().toString(), + -1, -1, 1); ServletContextHandler contextHandler = new ServletContextHandler(ServletContextHandler.SESSIONS); contextHandler.setContextPath("/"); ServletHolder servletHolder = contextHandler.addServlet(MultiPartServlet.class, "/"); servletHolder.getRegistration().setMultipartConfig(config); + servletHolder = contextHandler.addServlet(RequestParameterServlet.class, "/defaultConfig"); + servletHolder.getRegistration().setMultipartConfig(defaultConfig); + servletHolder = contextHandler.addServlet(RequestParameterServlet.class, "/requestSizeLimit"); + servletHolder.getRegistration().setMultipartConfig(requestSizedConfig); servletHolder = contextHandler.addServlet(MultiPartEchoServlet.class, "/echo"); servletHolder.getRegistration().setMultipartConfig(config); @@ -169,6 +197,120 @@ public void stop() throws Exception IO.delete(tmpDir.toFile()); } + + @ParameterizedTest + @MethodSource("data") + public void testLargePart(MultiPartFormDataCompliance compliance) throws Exception + { + connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration() + .setMultiPartFormDataCompliance(compliance); + + OutputStreamContentProvider content = new OutputStreamContentProvider(); + MultiPartContentProvider multiPart = new MultiPartContentProvider(); + multiPart.addFieldPart("param", content, null); + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .content(multiPart) + .send(listener); + + // Write large amount of content to the part. + byte[] byteArray = new byte[1024 * 1024]; + Arrays.fill(byteArray, (byte)1); + for (int i = 0; i < 128 * 2; i++) + { + content.getOutputStream().write(byteArray); + } + content.close(); + + Response response = listener.get(2, TimeUnit.MINUTES); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + String responseContent = IO.toString(listener.getInputStream()); + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Form is larger than max length")); + } + + @ParameterizedTest + @MethodSource("data") + public void testManyParts(MultiPartFormDataCompliance compliance) throws Exception + { + connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration() + .setMultiPartFormDataCompliance(compliance); + + byte[] byteArray = new byte[1024]; + Arrays.fill(byteArray, (byte)1); + + MultiPartContentProvider multiPart = new MultiPartContentProvider(); + for (int i = 0; i < 1024 * 1024; i++) + { + BytesContentProvider content = new BytesContentProvider(byteArray); + multiPart.addFieldPart("part" + i, content, null); + } + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/defaultConfig") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .content(multiPart) + .send(listener); + + Response response = listener.get(30, TimeUnit.SECONDS); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + String responseContent = IO.toString(listener.getInputStream()); + assertThat(responseContent, containsString("Unable to parse form content")); + assertThat(responseContent, containsString("Form with too many keys")); + } + + @ParameterizedTest + @MethodSource("data") + public void testMaxRequestSize(MultiPartFormDataCompliance compliance) throws Exception + { + connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration() + .setMultiPartFormDataCompliance(compliance); + + OutputStreamContentProvider content = new OutputStreamContentProvider(); + MultiPartContentProvider multiPart = new MultiPartContentProvider(); + multiPart.addFieldPart("param", content, null); + multiPart.close(); + + InputStreamResponseListener listener = new InputStreamResponseListener(); + client.newRequest("localhost", connector.getLocalPort()) + .path("/requestSizeLimit") + .scheme(HttpScheme.HTTP.asString()) + .method(HttpMethod.POST) + .content(multiPart) + .send(listener); + + Throwable writeError = null; + try + { + // Write large amount of content to the part. + byte[] byteArray = new byte[1024 * 1024]; + Arrays.fill(byteArray, (byte)1); + for (int i = 0; i < 512; i++) + { + content.getOutputStream().write(byteArray); + } + } + catch (Exception e) + { + writeError = e; + } + + if (writeError != null) + assertThat(writeError, instanceOf(EofException.class)); + + // We should get 400 response. + Response response = listener.get(30, TimeUnit.SECONDS); + assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); + } + @ParameterizedTest @MethodSource("data") public void testTempFilesDeletedOnError(MultiPartFormDataCompliance compliance) throws Exception diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java index bb242483e175..1e954f86ef52 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java @@ -64,8 +64,11 @@ public class MultiPartInputStreamParser { private static final Logger LOG = Log.getLogger(MultiPartInputStreamParser.class); + private static final int DEFAULT_MAX_FORM_KEYS = 1000; public static final MultipartConfigElement __DEFAULT_MULTIPART_CONFIG = new MultipartConfigElement(System.getProperty("java.io.tmpdir")); - public static final MultiMap EMPTY_MAP = new MultiMap(Collections.emptyMap()); + public static final MultiMap EMPTY_MAP = new MultiMap<>(Collections.emptyMap()); + private final int _maxParts; + private int _numParts; protected InputStream _in; protected MultipartConfigElement _config; protected String _contentType; @@ -394,10 +397,23 @@ public String getContentDispositionFilename() * @param contextTmpDir javax.servlet.context.tempdir */ public MultiPartInputStreamParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir) + { + this(in, contentType, config, contextTmpDir, DEFAULT_MAX_FORM_KEYS); + } + + /** + * @param in Request input stream + * @param contentType Content-Type header + * @param config MultipartConfigElement + * @param contextTmpDir javax.servlet.context.tempdir + * @param maxParts the maximum number of parts that can be parsed from the multipart content (0 for no parts allowed, -1 for unlimited parts). + */ + public MultiPartInputStreamParser(InputStream in, String contentType, MultipartConfigElement config, File contextTmpDir, int maxParts) { _contentType = contentType; _config = config; _contextTmpDir = contextTmpDir; + _maxParts = maxParts; if (_contextTmpDir == null) _contextTmpDir = new File(System.getProperty("java.io.tmpdir")); @@ -693,6 +709,11 @@ else if (tl.startsWith("filename=")) continue; } + // Check if we can create a new part. + _numParts++; + if (_maxParts >= 0 && _numParts > _maxParts) + throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", _numParts, _maxParts)); + //Have a new Part MultiPart part = new MultiPart(name, filename); part.setHeaders(headers); From 940c92a0fccf53f2d9a163ec98ba598c58113413 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Mon, 13 Feb 2023 21:47:37 +1100 Subject: [PATCH 2/5] fix checkstyle error Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/servlet/MultiPartServletTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java index 85fb39afc62c..91ccb31ddd98 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java @@ -197,7 +197,6 @@ public void stop() throws Exception IO.delete(tmpDir.toFile()); } - @ParameterizedTest @MethodSource("data") public void testLargePart(MultiPartFormDataCompliance compliance) throws Exception From be61306bf4ac25d998cc8f21291db4aca75bf880 Mon Sep 17 00:00:00 2001 From: Lachlan Date: Tue, 14 Feb 2023 09:34:26 +1100 Subject: [PATCH 3/5] #9345 changes from review Co-authored-by: Simone Bordet --- .../java/org/eclipse/jetty/http/MultiPartFormInputStream.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java index 27af3f380602..4f1811f948bd 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormInputStream.java @@ -769,7 +769,7 @@ public void startPart() reset(); _numParts++; if (_maxParts >= 0 && _numParts > _maxParts) - throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", _numParts, _maxParts)); + throw new IllegalStateException(String.format("Form with too many parts [%d > %d]", _numParts, _maxParts)); } @Override From d18082c9531615f5d9fb6c29287934aa58d28564 Mon Sep 17 00:00:00 2001 From: Lachlan Date: Tue, 14 Feb 2023 09:34:36 +1100 Subject: [PATCH 4/5] #9345 changes from review Co-authored-by: Simone Bordet --- .../java/org/eclipse/jetty/util/MultiPartInputStreamParser.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java index 1e954f86ef52..359af1cdbdab 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/MultiPartInputStreamParser.java @@ -712,7 +712,7 @@ else if (tl.startsWith("filename=")) // Check if we can create a new part. _numParts++; if (_maxParts >= 0 && _numParts > _maxParts) - throw new IllegalStateException(String.format("Form with too many keys [%d > %d]", _numParts, _maxParts)); + throw new IllegalStateException(String.format("Form with too many parts [%d > %d]", _numParts, _maxParts)); //Have a new Part MultiPart part = new MultiPart(name, filename); From c874c00440d2aacfc1795f864d1ca7e15531ad12 Mon Sep 17 00:00:00 2001 From: Lachlan Roberts Date: Tue, 14 Feb 2023 10:26:19 +1100 Subject: [PATCH 5/5] fix test expectation Signed-off-by: Lachlan Roberts --- .../java/org/eclipse/jetty/servlet/MultiPartServletTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java index 91ccb31ddd98..e0089218dd07 100644 --- a/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java +++ b/jetty-servlet/src/test/java/org/eclipse/jetty/servlet/MultiPartServletTest.java @@ -263,7 +263,7 @@ public void testManyParts(MultiPartFormDataCompliance compliance) throws Excepti assertThat(response.getStatus(), equalTo(HttpStatus.BAD_REQUEST_400)); String responseContent = IO.toString(listener.getInputStream()); assertThat(responseContent, containsString("Unable to parse form content")); - assertThat(responseContent, containsString("Form with too many keys")); + assertThat(responseContent, containsString("Form with too many parts")); } @ParameterizedTest