From 2093f3822e336c2ec50f61acb6c4af6cd5f057c4 Mon Sep 17 00:00:00 2001 From: Lachlan Date: Thu, 12 May 2022 14:31:21 +1000 Subject: [PATCH] Add TRANSFER_ENCODING violation for MultiPart RFC7578 parser. (#7976) * Add TRANSFER_ENCODING violation for MultiPart RFC7578 parser. * Ignore TRANSFER_ENCODING violation for 8bit and binary. Signed-off-by: Lachlan Roberts --- .../server/MultiPartFormInputStream.java | 33 +++++- .../org/eclipse/jetty/server/Request.java | 18 ++++ .../server/MultiPartFormInputStreamTest.java | 101 ++++++++++++++++++ .../org/eclipse/jetty/server/RequestTest.java | 5 +- 4 files changed, 155 insertions(+), 2 deletions(-) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java index 59afd2f26bde..763ee57036b5 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/MultiPartFormInputStream.java @@ -30,6 +30,7 @@ import java.nio.file.StandardOpenOption; import java.util.Collection; import java.util.Collections; +import java.util.EnumSet; import java.util.List; import java.util.stream.Collectors; import javax.servlet.MultipartConfigElement; @@ -91,6 +92,7 @@ private enum State private final AutoLock _lock = new AutoLock(); private final MultiMap _parts = new MultiMap<>(); + private final EnumSet _nonComplianceWarnings = EnumSet.noneOf(NonCompliance.class); private final InputStream _in; private final MultipartConfigElement _config; private final File _contextTmpDir; @@ -102,6 +104,31 @@ private enum State private volatile int _bufferSize = 16 * 1024; private State state = State.UNPARSED; + public enum NonCompliance + { + TRANSFER_ENCODING("https://tools.ietf.org/html/rfc7578#section-4.7"); + + final String _rfcRef; + + NonCompliance(String rfcRef) + { + _rfcRef = rfcRef; + } + + public String getURL() + { + return _rfcRef; + } + } + + /** + * @return an EnumSet of non compliances with the RFC that were accepted by this parser + */ + public EnumSet getNonComplianceWarnings() + { + return _nonComplianceWarnings; + } + public class MultiPart implements Part { protected String _name; @@ -671,7 +698,11 @@ else if (key.equalsIgnoreCase("content-type")) // Transfer encoding is not longer considers as it is deprecated as per // https://tools.ietf.org/html/rfc7578#section-4.7 - + if (key.equalsIgnoreCase("content-transfer-encoding")) + { + if (!"8bit".equalsIgnoreCase(value) && !"binary".equalsIgnoreCase(value)) + _nonComplianceWarnings.add(NonCompliance.TRANSFER_ENCODING); + } } @Override 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 777eb1394884..126766c4c0c9 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 @@ -66,6 +66,7 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HostPortHttpField; +import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpCookie.SetCookieHttpField; import org.eclipse.jetty.http.HttpField; @@ -2295,6 +2296,7 @@ private Collection getParts(MultiMap params) throws IOException _multiParts = newMultiParts(config); Collection parts = _multiParts.getParts(); + setNonComplianceViolationsOnRequest(); String formCharset = null; Part charsetPart = _multiParts.getPart("_charset_"); @@ -2355,6 +2357,22 @@ else if (getCharacterEncoding() != null) return _multiParts.getParts(); } + private void setNonComplianceViolationsOnRequest() + { + @SuppressWarnings("unchecked") + List violations = (List)getAttribute(HttpCompliance.VIOLATIONS_ATTR); + if (violations != null) + return; + + EnumSet nonComplianceWarnings = _multiParts.getNonComplianceWarnings(); + violations = new ArrayList<>(); + for (MultiPartFormInputStream.NonCompliance nc : nonComplianceWarnings) + { + violations.add(nc.name() + ": " + nc.getURL()); + } + setAttribute(HttpCompliance.VIOLATIONS_ATTR, violations); + } + private MultiPartFormInputStream newMultiParts(MultipartConfigElement config) throws IOException { return new MultiPartFormInputStream(getInputStream(), getContentType(), config, diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java index 5fbd19ed1166..4b067c8682e2 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/MultiPartFormInputStreamTest.java @@ -18,10 +18,12 @@ import java.io.File; import java.io.IOException; import java.io.InputStream; +import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.nio.file.Path; import java.util.Base64; import java.util.Collection; +import java.util.EnumSet; import java.util.concurrent.CompletableFuture; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -32,13 +34,17 @@ import javax.servlet.http.Part; import org.eclipse.jetty.server.MultiPartFormInputStream.MultiPart; +import org.eclipse.jetty.server.MultiPartFormInputStream.NonCompliance; import org.eclipse.jetty.toolchain.test.MavenTestingUtils; +import org.eclipse.jetty.util.BlockingArrayQueue; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.IO; import org.junit.jupiter.api.Test; import static java.nio.charset.StandardCharsets.ISO_8859_1; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.notNullValue; @@ -1024,6 +1030,99 @@ public void testBase64EncodedContent() throws Exception baos = new ByteArrayOutputStream(); IO.copy(p3.getInputStream(), baos); assertEquals("the end", baos.toString(StandardCharsets.US_ASCII)); + + assertThat(mpis.getNonComplianceWarnings(), equalTo(EnumSet.of(NonCompliance.TRANSFER_ENCODING))); + } + + @Test + public void testFragmentation() throws IOException + { + String contentType = "multipart/form-data, boundary=----WebKitFormBoundaryhXfFAMfUnUKhmqT8"; + String payload1 = + "------WebKitFormBoundaryhXfFAMfUnUKhmqT8\r\n" + + "Content-Disposition: form-data; name=\"field1\"\r\n\r\n" + + "value1" + + "\r\n--"; + String payload2 = "----WebKitFormBoundaryhXfFAMfUnUKhmqT8\r\n" + + "Content-Disposition: form-data; name=\"field2\"\r\n\r\n" + + "value2" + + "\r\n------WebKitFormBoundaryhXfFAMfUnUKhmqT8--\r\n"; + + // Split the content into separate reads, with the content broken up on the boundary string. + AppendableInputStream stream = new AppendableInputStream(); + stream.append(payload1); + stream.append(""); + stream.append(payload2); + stream.endOfContent(); + + MultipartConfigElement config = new MultipartConfigElement(_dirname); + MultiPartFormInputStream mpis = new MultiPartFormInputStream(stream, contentType, config, _tmpDir); + mpis.setDeleteOnExit(true); + + // Check size. + Collection parts = mpis.getParts(); + assertThat(parts.size(), is(2)); + + // Check part content. + assertThat(IO.toString(mpis.getPart("field1").getInputStream()), is("value1")); + assertThat(IO.toString(mpis.getPart("field2").getInputStream()), is("value2")); + } + + static class AppendableInputStream extends InputStream + { + private static final ByteBuffer EOF = ByteBuffer.allocate(0); + private final BlockingArrayQueue buffers = new BlockingArrayQueue<>(); + private ByteBuffer current; + + public void append(String data) + { + append(data.getBytes(StandardCharsets.US_ASCII)); + } + + public void append(byte[] data) + { + buffers.add(BufferUtil.toBuffer(data)); + } + + public void endOfContent() + { + buffers.add(EOF); + } + + @Override + public int read() throws IOException + { + byte[] buf = new byte[1]; + while (true) + { + int len = read(buf, 0, 1); + if (len < 0) + return -1; + if (len > 0) + return buf[0]; + } + } + + @Override + public int read(byte[] b, int off, int len) throws IOException + { + if (current == null) + current = buffers.poll(); + if (current == EOF) + return -1; + if (BufferUtil.isEmpty(current)) + { + current = null; + return 0; + } + + ByteBuffer buffer = ByteBuffer.wrap(b, off, len); + buffer.flip(); + int read = BufferUtil.append(buffer, current); + if (BufferUtil.isEmpty(current)) + current = buffers.poll(); + return read; + } } @Test @@ -1062,6 +1161,8 @@ public void testQuotedPrintableEncoding() throws Exception baos = new ByteArrayOutputStream(); IO.copy(p2.getInputStream(), baos); assertEquals("truth=3Dbeauty", baos.toString(StandardCharsets.US_ASCII)); + + assertThat(mpis.getNonComplianceWarnings(), equalTo(EnumSet.of(NonCompliance.TRANSFER_ENCODING))); } @Test diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java index 99e3dca600fb..91669cf873ff 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/RequestTest.java @@ -501,6 +501,7 @@ public void testMultiPart() throws Exception "--AaB03x\r\n" + "content-disposition: form-data; name=\"stuff\"; filename=\"foo.upload\"\r\n" + "Content-Type: text/plain;charset=ISO-8859-1\r\n" + + "Content-Transfer-Encoding: something\r\n" + "\r\n" + "000000000000000000000000000000000000000000000000000\r\n" + "--AaB03x--\r\n"; @@ -514,7 +515,9 @@ public void testMultiPart() throws Exception LocalEndPoint endPoint = _connector.connect(); endPoint.addInput(request); - assertTrue(endPoint.getResponse().startsWith("HTTP/1.1 200")); + String response = endPoint.getResponse(); + assertTrue(response.startsWith("HTTP/1.1 200")); + assertThat(response, containsString("Violation: TRANSFER_ENCODING")); // We know the previous request has completed if another request can be processed on the same connection. String cleanupRequest = "GET /foo/cleanup HTTP/1.1\r\n" +