diff --git a/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/classic/ClassicIntegrationTest.java b/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/classic/ClassicIntegrationTest.java index b9c2f7f5b..a12e8be1a 100644 --- a/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/classic/ClassicIntegrationTest.java +++ b/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; diff --git a/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/nio/Http1IntegrationTest.java b/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/nio/Http1IntegrationTest.java index 33fc8afb8..a8a4ce418 100644 --- a/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/nio/Http1IntegrationTest.java +++ b/httpcore5-testing/src/test/java/org/apache/hc/core5/testing/nio/Http1IntegrationTest.java @@ -29,7 +29,10 @@ 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; @@ -37,6 +40,9 @@ 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()); + } + } diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/DefaultContentLengthStrategy.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/DefaultContentLengthStrategy.java index 3cbd17f1f..65df001cc 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/DefaultContentLengthStrategy.java +++ b/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. *

* 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

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 codingRef = new AtomicReference<>(); while (it.hasNext()) { MessageSupport.parseTokens(it.next(), e -> { diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/AbstractHttp1StreamDuplexer.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/AbstractHttp1StreamDuplexer.java index 83864e41d..d0eff513c 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/AbstractHttp1StreamDuplexer.java +++ b/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 { diff --git a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ServerHttp1StreamDuplexer.java b/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ServerHttp1StreamDuplexer.java index e4b1b6d6e..178ef6e60 100644 --- a/httpcore5/src/main/java/org/apache/hc/core5/http/impl/nio/ServerHttp1StreamDuplexer.java +++ b/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; diff --git a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/TestDefaultContentLengthStrategy.java b/httpcore5/src/test/java/org/apache/hc/core5/http/impl/TestDefaultContentLengthStrategy.java index fa7b51cae..482912698 100644 --- a/httpcore5/src/test/java/org/apache/hc/core5/http/impl/TestDefaultContentLengthStrategy.java +++ b/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();