This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 7.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 3de983433578765d52179e4b1c69ad04019673b2
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 +++-
.../coyote/http11/AbstractHttp11Processor.java | 46 ++++++++++------
.../apache/coyote/http11/LocalStrings.properties | 4 +-
.../connector/TestCoyoteAdapterRequestFuzzing.java | 63 +++++++++++++++++-----
.../apache/catalina/startup/SimpleHttpClient.java | 12 ++++-
test/org/apache/tomcat/unittest/TesterData.java | 37 +++++++++++++
webapps/docs/changelog.xml | 8 +++
7 files changed, 149 insertions(+), 32 deletions(-)
diff --git a/java/org/apache/catalina/connector/CoyoteAdapter.java
b/java/org/apache/catalina/connector/CoyoteAdapter.java
index 151fb48..3813073 100644
--- a/java/org/apache/catalina/connector/CoyoteAdapter.java
+++ b/java/org/apache/catalina/connector/CoyoteAdapter.java
@@ -816,7 +816,16 @@ public class CoyoteAdapter implements Adapter {
}
// Look for session ID in cookies and SSL session
- parseSessionCookiesId(req, request);
+ try {
+ parseSessionCookiesId(req, request);
+ } catch (IllegalArgumentException e) {
+ // Too many cookies
+ if (!response.isError()) {
+ response.setError();
+ response.sendError(400);
+ }
+ return false;
+ }
parseSessionSslId(request);
sessionID = request.getRequestedSessionId();
diff --git a/java/org/apache/coyote/http11/AbstractHttp11Processor.java
b/java/org/apache/coyote/http11/AbstractHttp11Processor.java
index ae0bb2f..1bd0adb 100644
--- a/java/org/apache/coyote/http11/AbstractHttp11Processor.java
+++ b/java/org/apache/coyote/http11/AbstractHttp11Processor.java
@@ -1313,7 +1313,7 @@ public abstract class AbstractHttp11Processor<S> extends
AbstractProcessor<S> {
// 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;
@@ -1323,17 +1323,16 @@ public abstract class AbstractHttp11Processor<S>
extends AbstractProcessor<S> {
}
}
- MessageBytes expectMB = null;
if (http11) {
- expectMB = headers.getValue("expect");
- }
- if (expectMB != null) {
- if (expectMB.indexOfIgnoreCase("100-continue", 0) != -1) {
- getInputBuffer().setSwallowInput(false);
- expectation = true;
- } else {
- response.setStatus(HttpServletResponse.SC_EXPECTATION_FAILED);
- setErrorState(ErrorState.CLOSE_CLEAN, null);
+ MessageBytes expectMB = headers.getValue("expect");
+ if (expectMB != null && !expectMB.isNull()) {
+ if (expectMB.indexOfIgnoreCase("100-continue", 0) != -1) {
+ getInputBuffer().setSwallowInput(false);
+ expectation = true;
+ } else {
+
response.setStatus(HttpServletResponse.SC_EXPECTATION_FAILED);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ }
}
}
@@ -1342,7 +1341,7 @@ public abstract class AbstractHttp11Processor<S> extends
AbstractProcessor<S> {
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.matcher(userAgentValue).matches()) {
http11 = false;
@@ -1442,8 +1441,16 @@ public abstract class AbstractHttp11Processor<S> extends
AbstractProcessor<S> {
} 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");
@@ -1467,7 +1474,7 @@ public abstract class AbstractHttp11Processor<S> extends
AbstractProcessor<S> {
if (http11) {
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;
@@ -1484,7 +1491,14 @@ public abstract class AbstractHttp11Processor<S> extends
AbstractProcessor<S> {
}
// 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 e41e9d2..035bf71 100644
--- a/java/org/apache/coyote/http11/LocalStrings.properties
+++ b/java/org/apache/coyote/http11/LocalStrings.properties
@@ -26,10 +26,12 @@ http11processor.regexp.error=Error parsing regular
expression {0}
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 453e04a..a34adc5 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<Object[]>();
- 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 7e33c76..dd02566 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;
@@ -305,8 +306,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/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 948b825..a262706 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -60,6 +60,14 @@
issues do not "pop up" wrt. others).
-->
<section name="Tomcat 7.0.97 (violetagg)">
+ <subsection name="Coyote">
+ <changelog>
+ <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>
</section>
<section name="Tomcat 7.0.96 (violetagg)">
<subsection name="Catalina">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
