You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2019/07/24 08:49:06 UTC
[tomcat] 04/04: Fix
https://bz.apache.org/bugzilla/show_bug.cgi?id=63578
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 52c9e419cb6121df819fc181994eaa014491c89d
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Jul 23 19:07:19 2019 +0100
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63578
Various fixes to return 400 responses rather than 500 responses when the
provided request is invalid.
---
.../apache/catalina/connector/CoyoteAdapter.java | 11 +++-
java/org/apache/coyote/http11/Http11Processor.java | 35 ++++++++----
.../apache/coyote/http11/LocalStrings.properties | 4 +-
.../connector/TestCoyoteAdapterRequestFuzzing.java | 63 +++++++++++++++++-----
.../apache/catalina/startup/SimpleHttpClient.java | 12 ++++-
test/org/apache/coyote/http2/TestHttp2Limits.java | 9 ++--
test/org/apache/tomcat/unittest/TesterData.java | 37 +++++++++++++
webapps/docs/changelog.xml | 4 ++
8 files changed, 145 insertions(+), 30 deletions(-)
diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java b/java/org/apache/catalina/connector/CoyoteAdapter.java
index 869abd9..5274eee 100644
--- a/java/org/apache/catalina/connector/CoyoteAdapter.java
+++ b/java/org/apache/catalina/connector/CoyoteAdapter.java
@@ -727,7 +727,16 @@ public class CoyoteAdapter implements Adapter {
}
// Look for session ID in cookies and SSL session
- parseSessionCookiesId(request);
+ try {
+ parseSessionCookiesId(request);
+ } catch (IllegalArgumentException e) {
+ // Too many cookies
+ if (!response.isError()) {
+ response.setError();
+ response.sendError(400);
+ }
+ return true;
+ }
parseSessionSslId(request);
sessionID = request.getRequestedSessionId();
diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java
index f22f913..1658c95 100644
--- a/java/org/apache/coyote/http11/Http11Processor.java
+++ b/java/org/apache/coyote/http11/Http11Processor.java
@@ -739,8 +739,10 @@ public class Http11Processor extends AbstractProcessor {
Enumeration<String> connectionValues = request.getMimeHeaders().values("Connection");
boolean foundUpgrade = false;
while (connectionValues.hasMoreElements() && !foundUpgrade) {
- foundUpgrade = connectionValues.nextElement().toLowerCase(
- Locale.ENGLISH).contains("upgrade");
+ String connectionValue = connectionValues.nextElement();
+ if (connectionValue != null) {
+ foundUpgrade = connectionValue.toLowerCase(Locale.ENGLISH).contains("upgrade");
+ }
}
if (foundUpgrade) {
@@ -985,7 +987,7 @@ public class Http11Processor extends AbstractProcessor {
// Check connection header
MessageBytes connectionValueMB = headers.getValue(Constants.CONNECTION);
- if (connectionValueMB != null) {
+ if (connectionValueMB != null && !connectionValueMB.isNull()) {
ByteChunk connectionValueBC = connectionValueMB.getByteChunk();
if (findBytes(connectionValueBC, Constants.CLOSE_BYTES) != -1) {
keepAlive = false;
@@ -997,7 +999,7 @@ public class Http11Processor extends AbstractProcessor {
if (http11) {
MessageBytes expectMB = headers.getValue("expect");
- if (expectMB != null) {
+ if (expectMB != null && !expectMB.isNull()) {
if (expectMB.indexOfIgnoreCase("100-continue", 0) != -1) {
inputBuffer.setSwallowInput(false);
request.setExpectation(true);
@@ -1013,7 +1015,7 @@ public class Http11Processor extends AbstractProcessor {
MessageBytes userAgentValueMB = headers.getValue("user-agent");
// Check in the restricted list, and adjust the http11
// and keepAlive flags accordingly
- if(userAgentValueMB != null) {
+ if(userAgentValueMB != null && !userAgentValueMB.isNull()) {
String userAgentValue = userAgentValueMB.toString();
if (restrictedUserAgents != null &&
restrictedUserAgents.matcher(userAgentValue).matches()) {
@@ -1114,8 +1116,16 @@ public class Http11Processor extends AbstractProcessor {
} else {
// Not HTTP/1.1 - no Host header so generate one since
// Tomcat internals assume it is set
- hostValueMB = headers.setValue("host");
- hostValueMB.setBytes(uriB, uriBCStart + pos, slashPos - pos);
+ try {
+ hostValueMB = headers.setValue("host");
+ hostValueMB.setBytes(uriB, uriBCStart + pos, slashPos - pos);
+ } catch (IllegalStateException e) {
+ // Edge case
+ // If the request has too many headers it won't be
+ // possible to create the host header. Ignore this as
+ // processing won't reach the point where the Tomcat
+ // internals expect there to be a host header.
+ }
}
} else {
badRequest("http11processor.request.invalidScheme");
@@ -1137,7 +1147,7 @@ public class Http11Processor extends AbstractProcessor {
// Parse transfer-encoding header
if (http11) {
MessageBytes transferEncodingValueMB = headers.getValue("transfer-encoding");
- if (transferEncodingValueMB != null) {
+ if (transferEncodingValueMB != null && !transferEncodingValueMB.isNull()) {
String transferEncodingValue = transferEncodingValueMB.toString();
// Parse the comma separated list. "identity" codings are ignored
int startPos = 0;
@@ -1155,7 +1165,14 @@ public class Http11Processor extends AbstractProcessor {
}
// Parse content-length header
- long contentLength = request.getContentLengthLong();
+ long contentLength = -1;
+ try {
+ contentLength = request.getContentLengthLong();
+ } catch (NumberFormatException e) {
+ badRequest("http11processor.request.nonNumericContentLength");
+ } catch (IllegalArgumentException e) {
+ badRequest("http11processor.request.multipleContentLength");
+ }
if (contentLength >= 0) {
if (contentDelimitation) {
// contentDelimitation being true at this point indicates that
diff --git a/java/org/apache/coyote/http11/LocalStrings.properties b/java/org/apache/coyote/http11/LocalStrings.properties
index a1d3bcb..187b954 100644
--- a/java/org/apache/coyote/http11/LocalStrings.properties
+++ b/java/org/apache/coyote/http11/LocalStrings.properties
@@ -24,10 +24,12 @@ http11processor.neverused=This method should never be used
http11processor.request.finish=Error finishing request
http11processor.request.inconsistentHosts=The host specified in the request line is not consistent with the host header
http11processor.request.invalidScheme=The HTTP request contained an absolute URI with an invalid scheme
-http11processor.request.invalidUri==The HTTP request contained an invalid URI
+http11processor.request.invalidUri=The HTTP request contained an invalid URI
http11processor.request.invalidUserInfo=The HTTP request contained an absolute URI with an invalid userinfo
+http11processor.request.multipleContentLength=The request contained multiple content-length headers
http11processor.request.multipleHosts=The request contained multiple host headers
http11processor.request.noHostHeader=The HTTP/1.1 request did not provide a host header
+http11processor.request.nonNumericContentLength=The request contained a content-length header with a non-numeric value
http11processor.request.prepare=Error preparing request
http11processor.request.process=Error processing request
http11processor.response.finish=Error finishing response
diff --git a/test/org/apache/catalina/connector/TestCoyoteAdapterRequestFuzzing.java b/test/org/apache/catalina/connector/TestCoyoteAdapterRequestFuzzing.java
index 160ff6c..acd01a7 100644
--- a/test/org/apache/catalina/connector/TestCoyoteAdapterRequestFuzzing.java
+++ b/test/org/apache/catalina/connector/TestCoyoteAdapterRequestFuzzing.java
@@ -27,11 +27,13 @@ import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameter;
+import static org.apache.catalina.startup.SimpleHttpClient.CRLF;
import org.apache.catalina.Context;
import org.apache.catalina.servlets.DefaultServlet;
import org.apache.catalina.startup.SimpleHttpClient;
import org.apache.catalina.startup.Tomcat;
import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.unittest.TesterData;
/*
* Various requests, usually originating from fuzzing, that have triggered an
@@ -41,21 +43,61 @@ import org.apache.catalina.startup.TomcatBaseTest;
@RunWith(Parameterized.class)
public class TestCoyoteAdapterRequestFuzzing extends TomcatBaseTest {
- @Parameterized.Parameters(name = "{index}: uri[{0}], host[{1}], expected[{2}]")
+ private static final String VALUE_16K = TesterData.string('x', 16 * 1024);
+ // Default max header count is 100
+ private static final String HEADER_150 = TesterData.string("X-Tomcat-Test: a" + CRLF, 150);
+ // Default max header count is 200 (need to keep under maxHeaderCount as well)
+ private static final String COOKIE_250 = TesterData.string("Cookie: a=b;c=d;e=f;g=h" + CRLF, 75);
+
+ @Parameterized.Parameters(name = "{index}: requestline[{0}], expected[{2}]")
public static Collection<Object[]> parameters() {
List<Object[]> parameterSets = new ArrayList<>();
- parameterSets.add(new Object[] { "/", "lÿ#", "400" } );
- parameterSets.add(new Object[] { "*;", "", "400" } );
+ parameterSets.add(new Object[] { "GET /00 HTTP/1.1",
+ "Host: lÿ#" + CRLF,
+ "400" } );
+ parameterSets.add(new Object[] { "GET *; HTTP/1.1",
+ "Host: localhost" + CRLF,
+ "400" } );
+ parameterSets.add(new Object[] { "GET /02 HTTP/1.1",
+ "Host: localhost" + CRLF +
+ "Content-Length: \u00A0" + CRLF,
+ "400" } );
+ parameterSets.add(new Object[] { "GET /03 HTTP/1.1",
+ "Content-Length: 1" + CRLF +
+ "Content-Length: 1" + CRLF,
+ "400" } );
+ parameterSets.add(new Object[] { "GET /04 HTTP/1.1",
+ "Transfer-Encoding: " + VALUE_16K + CRLF,
+ "400" } );
+ parameterSets.add(new Object[] { "GET /05 HTTP/1.1",
+ "Expect: " + VALUE_16K + CRLF,
+ "400" } );
+ parameterSets.add(new Object[] { "GET /06 HTTP/1.1",
+ "Connection: " + VALUE_16K + CRLF,
+ "400" } );
+ parameterSets.add(new Object[] { "GET /07 HTTP/1.1",
+ "User-Agent: " + VALUE_16K + CRLF,
+ "400" } );
+ parameterSets.add(new Object[] { "GET /08 HTTP/1.1",
+ HEADER_150,
+ "400" } );
+ parameterSets.add(new Object[] { "GET http://host/09 HTTP/1.0",
+ HEADER_150,
+ "400" } );
+ parameterSets.add(new Object[] { "GET /10 HTTP/1.1",
+ "Host: localhost" + CRLF +
+ COOKIE_250,
+ "400" } );
return parameterSets;
}
@Parameter(0)
- public String uri;
+ public String requestLine;
@Parameter(1)
- public String host;
+ public String headers;
@Parameter(2)
public String expected;
@@ -64,6 +106,7 @@ public class TestCoyoteAdapterRequestFuzzing extends TomcatBaseTest {
@Test
public void doTest() throws Exception {
Tomcat tomcat = getTomcatInstance();
+ tomcat.getConnector().setAttribute("restrictedUserAgents", "value-not-important");
File appDir = new File("test/webapp");
Context ctxt = tomcat.addContext("", appDir.getAbsolutePath());
@@ -72,20 +115,15 @@ public class TestCoyoteAdapterRequestFuzzing extends TomcatBaseTest {
tomcat.start();
- String request =
- "GET " + uri + " HTTP/1.1" + SimpleHttpClient.CRLF +
- "Host: " + host + SimpleHttpClient.CRLF +
- SimpleHttpClient.CRLF;
-
Client client = new Client(tomcat.getConnector().getLocalPort());
- client.setRequest(new String[] {request});
+ client.setRequest(new String[] {requestLine + CRLF, headers + CRLF});
client.connect();
client.processRequest();
// Expected response
String line = client.getResponseLine();
- Assert.assertTrue(line, line.startsWith("HTTP/1.1 " + expected + " "));
+ Assert.assertTrue(line + CRLF + client.getResponseBody(), line.startsWith("HTTP/1.1 " + expected + " "));
}
@@ -93,6 +131,7 @@ public class TestCoyoteAdapterRequestFuzzing extends TomcatBaseTest {
public Client(int port) {
setPort(port);
+ setRequestPause(0);
}
@Override
diff --git a/test/org/apache/catalina/startup/SimpleHttpClient.java b/test/org/apache/catalina/startup/SimpleHttpClient.java
index ec0af7d..1d6e81e 100644
--- a/test/org/apache/catalina/startup/SimpleHttpClient.java
+++ b/test/org/apache/catalina/startup/SimpleHttpClient.java
@@ -28,6 +28,7 @@ import java.io.Writer;
import java.net.InetSocketAddress;
import java.net.Socket;
import java.net.SocketAddress;
+import java.net.SocketException;
import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.List;
@@ -307,8 +308,15 @@ public abstract class SimpleHttpClient {
else {
// not using content length, so just read it line by line
String line = null;
- while ((line = readLine()) != null) {
- builder.append(line);
+ try {
+ while ((line = readLine()) != null) {
+ builder.append(line);
+ }
+ } catch (SocketException e) {
+ // Ignore
+ // May see a SocketException if the request hasn't been
+ // fully read when the connection is closed as that may
+ // trigger a TCP reset.
}
}
}
diff --git a/test/org/apache/coyote/http2/TestHttp2Limits.java b/test/org/apache/coyote/http2/TestHttp2Limits.java
index 97f7b4e..2a9ba74 100644
--- a/test/org/apache/coyote/http2/TestHttp2Limits.java
+++ b/test/org/apache/coyote/http2/TestHttp2Limits.java
@@ -387,15 +387,14 @@ public class TestHttp2Limits extends Http2TestBase {
break;
}
case 1: {
- // Check status is 500
+ // Check status is 400
parser.readFrame(true);
Assert.assertTrue(output.getTrace(), output.getTrace().startsWith(
- "3-HeadersStart\n3-Header-[:status]-[500]"));
+ "3-HeadersStart\n3-Header-[:status]-[400]"));
output.clearTrace();
- // Check EOS followed by reset is next
+ // Check EOS followed by error page body
parser.readFrame(true);
- parser.readFrame(true);
- Assert.assertEquals("3-EndOfStream\n3-RST-[2]\n", output.getTrace());
+ Assert.assertTrue(output.getTrace(), output.getTrace().startsWith("3-EndOfStream\n3-Body-<!doctype"));
break;
}
default: {
diff --git a/test/org/apache/tomcat/unittest/TesterData.java b/test/org/apache/tomcat/unittest/TesterData.java
new file mode 100644
index 0000000..fbdc72a
--- /dev/null
+++ b/test/org/apache/tomcat/unittest/TesterData.java
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.tomcat.unittest;
+
+public class TesterData {
+
+ public static String string(char c, int count) {
+ StringBuilder sb = new StringBuilder(count);
+ for (int i = 0; i < count; i++) {
+ sb.append(c);
+ }
+ return sb.toString();
+ }
+
+
+ public static String string(String str, int count) {
+ StringBuilder sb = new StringBuilder(str.length() * count);
+ for (int i = 0; i < count; i++) {
+ sb.append(str);
+ }
+ return sb.toString();
+ }
+}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index ef559dc..624a4e0 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -89,6 +89,10 @@
128 to 255 and reject them with a 400 response rather than triggering an
internal error that results in a 500 response. (markt)
</fix>
+ <fix>
+ <bug>63578</bug>: Improve handling of invalid requests so that 400
+ responses are returned to the client rather than 500 responses. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Other">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org