RFC 9112: reject messages with both Transfer-Encoding and Content-Length

httpcore5-testing/src/test/java/org/apache/hc/core5/testing/classic/ClassicIntegrationTest.java

@@ -41,6 +41,7 @@
 import java.util.List;
 import java.util.Random;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.function.Consumer;
 
 import javax.net.ssl.SSLSocket;
@@ -892,6 +893,86 @@ void testHeaderTooLargePost() throws Exception {
         }
     }
 
+    @Test
+    void testRequestWithTransferEncodingAndContentLengthRejected() throws Exception {
+        final ClassicTestServer server = testResources.server();
+        final AtomicInteger handled = new AtomicInteger();
+        server.register("*", (request, response, context) -> {
+            handled.incrementAndGet();
+            response.setEntity(new StringEntity("ok", ContentType.TEXT_PLAIN));
+        });
+        server.start();
+
+        final HttpHost host = new HttpHost(scheme.id, "localhost", server.getPort());
+        final String raw = "POST / HTTP/1.1\r\n"
+                + "Host: localhost\r\n"
+                + "Content-Length: 6\r\n"
+                + "Transfer-Encoding: chunked\r\n"
+                + "\r\n"
+                + "5\r\nAAAAA\r\n0\r\n\r\n"
+                + "GET /smuggled HTTP/1.1\r\n"
+                + "Host: localhost\r\n"
+                + "\r\n";
+        final StringBuilder reply = new StringBuilder();
+        runWithSocket(host, 5000, socket -> {
+            try {
+                final OutputStream out = socket.getOutputStream();
+                out.write(raw.getBytes(StandardCharsets.ISO_8859_1));
+                out.flush();
+                final InputStream in = socket.getInputStream();
+                final byte[] buf = new byte[1024];
+                int n;
+                // The read times out unless the server closes the connection
+                while ((n = in.read(buf)) != -1) {
+                    reply.append(new String(buf, 0, n, StandardCharsets.ISO_8859_1));
+                }
+            } catch (final IOException ex) {
+                Assertions.fail("Connection was not closed by the server: " + ex);
+            }
+        });
+        Assertions.assertTrue(reply.toString().startsWith("HTTP/1.1 400 "), reply.toString());
+        Assertions.assertEquals(1, reply.toString().split("HTTP/1\\.1 ", -1).length - 1, reply.toString());
+        Assertions.assertEquals(0, handled.get());
+    }
+
+    @Test
+    void testRequestWithUnsupportedTransferEncodingRejected() throws Exception {
+        final ClassicTestServer server = testResources.server();
+        final AtomicInteger handled = new AtomicInteger();
+        server.register("*", (request, response, context) -> {
+            handled.incrementAndGet();
+            response.setEntity(new StringEntity("ok", ContentType.TEXT_PLAIN));
+        });
+        server.start();
+
+        final HttpHost host = new HttpHost(scheme.id, "localhost", server.getPort());
+        final String raw = "POST / HTTP/1.1\r\n"
+                + "Host: localhost\r\n"
+                + "Transfer-Encoding: gzip\r\n"
+                + "\r\n"
+                + "xxxx";
+        final StringBuilder reply = new StringBuilder();
+        runWithSocket(host, 5000, socket -> {
+            try {
+                final OutputStream out = socket.getOutputStream();
+                out.write(raw.getBytes(StandardCharsets.ISO_8859_1));
+                out.flush();
+                final InputStream in = socket.getInputStream();
+                final byte[] buf = new byte[1024];
+                int n;
+                // The read times out unless the server closes the connection
+                while ((n = in.read(buf)) != -1) {
+                    reply.append(new String(buf, 0, n, StandardCharsets.ISO_8859_1));
+                }
+            } catch (final IOException ex) {
+                Assertions.fail("Connection was not closed by the server: " + ex);
+            }
+        });
+        Assertions.assertTrue(reply.toString().startsWith("HTTP/1.1 501 "), reply.toString());
+        Assertions.assertEquals(1, reply.toString().split("HTTP/1\\.1 ", -1).length - 1, reply.toString());
+        Assertions.assertEquals(0, handled.get());
+    }
+
     @Test
     void testImmediateCloseUponSocketTimeout() throws Exception {
         final int socketTimeoutMillis = 1000;

httpcore5-testing/src/test/java/org/apache/hc/core5/testing/nio/Http1IntegrationTest.java

@@ -29,14 +29,20 @@
 
 
 import java.io.IOException;
+import java.io.InputStream;
+import java.io.OutputStream;
 import java.net.InetSocketAddress;
+import java.net.Socket;
 import java.nio.ByteBuffer;
 import java.nio.channels.WritableByteChannel;
 import java.nio.charset.StandardCharsets;
 import java.util.List;
 import java.util.concurrent.CancellationException;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import javax.net.ssl.SSLSocket;
 
 import org.apache.hc.core5.function.Callback;
 import org.apache.hc.core5.http.ConnectionClosedException;
@@ -962,4 +968,96 @@ protected void writeHeadLine(final HttpRequest message, final CharArrayBuffer li
         Assertions.assertEquals(505, response.getCode());
     }
 
+    @Test
+    void testRequestWithTransferEncodingAndContentLengthRejected() throws Exception {
+        final Http1TestServer server = resources.server();
+        final AtomicInteger handled = new AtomicInteger();
+        server.register("/hello", () -> {
+            handled.incrementAndGet();
+            return new SingleLineResponseHandler("Hi there");
+        });
+        final InetSocketAddress serverEndpoint = server.start();
+
+        final String raw = "POST /hello HTTP/1.1\r\n"
+                + "Host: localhost\r\n"
+                + "Content-Length: 6\r\n"
+                + "Transfer-Encoding: chunked\r\n"
+                + "\r\n"
+                + "5\r\nAAAAA\r\n0\r\n\r\n"
+                + "GET /hello HTTP/1.1\r\n"
+                + "Host: localhost\r\n"
+                + "\r\n";
+        final StringBuilder reply = new StringBuilder();
+        try (final Socket plain = new Socket("localhost", serverEndpoint.getPort())) {
+            plain.setSoTimeout(5000);
+            final Socket socket;
+            if (scheme == URIScheme.HTTPS) {
+                final SSLSocket sslSocket = (SSLSocket) SSLTestContexts.createClientSSLContext()
+                        .getSocketFactory()
+                        .createSocket(plain, "localhost", plain.getPort(), true);
+                sslSocket.startHandshake();
+                socket = sslSocket;
+            } else {
+                socket = plain;
+            }
+            final OutputStream out = socket.getOutputStream();
+            out.write(raw.getBytes(StandardCharsets.ISO_8859_1));
+            out.flush();
+            final InputStream in = socket.getInputStream();
+            final byte[] buf = new byte[1024];
+            int n;
+            // The read times out unless the server closes the connection
+            while ((n = in.read(buf)) != -1) {
+                reply.append(new String(buf, 0, n, StandardCharsets.ISO_8859_1));
+            }
+        }
+        Assertions.assertTrue(reply.toString().startsWith("HTTP/1.1 400 "), reply.toString());
+        Assertions.assertEquals(1, reply.toString().split("HTTP/1\\.1 ", -1).length - 1, reply.toString());
+        Assertions.assertEquals(0, handled.get());
+    }
+
+    @Test
+    void testRequestWithUnsupportedTransferEncodingRejected() throws Exception {
+        final Http1TestServer server = resources.server();
+        final AtomicInteger handled = new AtomicInteger();
+        server.register("/hello", () -> {
+            handled.incrementAndGet();
+            return new SingleLineResponseHandler("Hi there");
+        });
+        final InetSocketAddress serverEndpoint = server.start();
+
+        final String raw = "POST /hello HTTP/1.1\r\n"
+                + "Host: localhost\r\n"
+                + "Transfer-Encoding: gzip\r\n"
+                + "\r\n"
+                + "xxxx";
+        final StringBuilder reply = new StringBuilder();
+        try (final Socket plain = new Socket("localhost", serverEndpoint.getPort())) {
+            plain.setSoTimeout(5000);
+            final Socket socket;
+            if (scheme == URIScheme.HTTPS) {
+                final SSLSocket sslSocket = (SSLSocket) SSLTestContexts.createClientSSLContext()
+                        .getSocketFactory()
+                        .createSocket(plain, "localhost", plain.getPort(), true);
+                sslSocket.startHandshake();
+                socket = sslSocket;
+            } else {
+                socket = plain;
+            }
+            final OutputStream out = socket.getOutputStream();
+            out.write(raw.getBytes(StandardCharsets.ISO_8859_1));
+            out.flush();
+            final InputStream in = socket.getInputStream();
+            final byte[] buf = new byte[1024];
+            int n;
+            // The read times out unless the server closes the connection
+            while ((n = in.read(buf)) != -1) {
+                reply.append(new String(buf, 0, n, StandardCharsets.ISO_8859_1));
+            }
+        }
+        Assertions.assertTrue(reply.toString().startsWith("HTTP/1.1 501 "), reply.toString());
+        Assertions.assertEquals(1, reply.toString().split("HTTP/1\\.1 ", -1).length - 1, reply.toString());
+        Assertions.assertEquals(0, handled.get());
+    }
+
 }

httpcore5/src/main/java/org/apache/hc/core5/http/impl/DefaultContentLengthStrategy.java

@@ -48,7 +48,9 @@
  * The default implementation of the content length strategy. This class
  * will throw {@link ProtocolException} if it encounters an unsupported
  * transfer encoding, multiple {@code Content-Length} header
- * values or a malformed {@code Content-Length} header value.
+ * values, a malformed {@code Content-Length} header value or a message
+ * that carries both {@code Transfer-Encoding} and {@code Content-Length}
+ * headers.
  * <p>
  * This class recognizes "chunked" transfer-coding only.
  *
@@ -73,6 +75,10 @@ public long determineLength(final HttpMessage message) throws HttpException {
         Args.notNull(message, "HTTP message");
         final Iterator<Header> it = message.headerIterator(HttpHeaders.TRANSFER_ENCODING);
         if (it.hasNext()) {
+            // Both headers may indicate request smuggling or response splitting
+            if (message.containsHeader(HttpHeaders.CONTENT_LENGTH)) {
+                throw new ProtocolException("Transfer-Encoding and Content-Length headers are both present");
+            }
             final AtomicReference<Coding> codingRef = new AtomicReference<>();
             while (it.hasNext()) {
                 MessageSupport.parseTokens(it.next(), e -> {

httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/AbstractHttp1StreamDuplexer.java

@@ -261,6 +261,16 @@ public final void onConnect() throws HttpException, IOException {
         }
     }
 
+    /**
+     * Gives a subclass the chance to answer a message whose framing cannot be determined.
+     *
+     * @return {@code true} if the message has been handled, {@code false} if
+     *   the exception should be propagated.
+     */
+    boolean rejectIncomingMessage(final HttpException ex) throws HttpException, IOException {
+        return false;
+    }
+
     IncomingMessage parseMessageHead(final boolean endOfStream) throws IOException, HttpException {
         final IncomingMessage messageHead = incomingMessageParser.parse(inbuf, endOfStream);
         if (messageHead != null) {
@@ -299,7 +309,15 @@ public final void onInput(final ByteBuffer src) throws HttpException, IOExceptio
                     updateInputMetrics(messageHead, connMetrics);
                     final ContentDecoder contentDecoder;
                     if (handleIncomingMessage(messageHead)) {
-                        final long len = incomingContentStrategy.determineLength(messageHead);
+                        final long len;
+                        try {
+                            len = incomingContentStrategy.determineLength(messageHead);
+                        } catch (final HttpException ex) {
+                            if (rejectIncomingMessage(ex)) {
+                                break;
+                            }
+                            throw ex;
+                        }
                         contentDecoder = createContentDecoder(len, ioSession, inbuf, inTransportMetrics);
                         consumeHeader(messageHead, contentDecoder != null ? new IncomingEntityDetails(messageHead, len) : null);
                     } else {

httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ServerHttp1StreamDuplexer.java

@@ -320,6 +320,12 @@ HttpRequest parseMessageHead(final boolean endOfStream) throws IOException, Http
         }
     }
 
+    @Override
+    boolean rejectIncomingMessage(final HttpException ex) throws HttpException, IOException {
+        terminateExchange(ex);
+        return true;
+    }
+
     void terminateExchange(final HttpException ex) throws HttpException, IOException {
         suspendSessionInput();
         final ServerHttp1StreamHandler streamHandler;

httpcore5/src/test/java/org/apache/hc/core5/http/impl/TestDefaultContentLengthStrategy.java

@@ -162,6 +162,26 @@ void testEntityWithPlusSignedContentLength() {
                 lenStrategy.determineLength(message));
     }
 
+    @Test
+    void testEntityWithChunkTransferEncodingAndContentLength() {
+        final ContentLengthStrategy lenStrategy = new DefaultContentLengthStrategy();
+        final HttpMessage message = new TestHttpMessage();
+        message.addHeader("Content-Length", "6");
+        message.addHeader("Transfer-Encoding", "chunked");
+        Assertions.assertThrows(ProtocolException.class, () ->
+                lenStrategy.determineLength(message));
+    }
+
+    @Test
+    void testEntityWithUnknownTransferEncodingAndContentLength() {
+        final ContentLengthStrategy lenStrategy = new DefaultContentLengthStrategy();
+        final HttpMessage message = new TestHttpMessage();
+        message.addHeader("Content-Length", "6");
+        message.addHeader("Transfer-Encoding", "blah");
+        Assertions.assertThrows(ProtocolException.class, () ->
+                lenStrategy.determineLength(message));
+    }
+
     @Test
     void testEntityNoContentDelimiter() throws Exception {
         final ContentLengthStrategy lenStrategy = new DefaultContentLengthStrategy();