This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 9.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 27a0c116e02ba9cd66873ded0e64b8c0fec2bc19
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Tue Mar 10 17:02:55 2020 +0000
Fix BZ 64210. Correct parsing of CRLF across packet boundaries,
Also improve request line parsing including HTTP/0.9.
---
.../apache/coyote/http11/Http11InputBuffer.java | 72 +++++-----
java/org/apache/coyote/http11/Http11Processor.java | 37 +++--
.../coyote/http11/TestHttp11InputBuffer.java | 100 +++++++++++--
.../coyote/http11/TestHttp11InputBufferCRLF.java | 156 +++++++++++++++++++++
webapps/docs/changelog.xml | 17 ++-
5 files changed, 324 insertions(+), 58 deletions(-)
diff --git a/java/org/apache/coyote/http11/Http11InputBuffer.java
b/java/org/apache/coyote/http11/Http11InputBuffer.java
index 166835e..fd054d7 100644
--- a/java/org/apache/coyote/http11/Http11InputBuffer.java
+++ b/java/org/apache/coyote/http11/Http11InputBuffer.java
@@ -128,6 +128,8 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
* Parsing state - used for non blocking parsing so that
* when more data arrives, we can pick up where we left off.
*/
+ private byte prevChr = 0;
+ private byte chr = 0;
private boolean parsingRequestLine;
private int parsingRequestLinePhase = 0;
private boolean parsingRequestLineEol = false;
@@ -341,9 +343,7 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
// Skipping blank lines
//
if (parsingRequestLinePhase < 2) {
- byte chr = 0;
do {
-
// Read new bytes if needed
if (byteBuffer.position() >= byteBuffer.limit()) {
if (keptAlive) {
@@ -404,7 +404,7 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
// Spec says method name is a token followed by a single SP but
// also be tolerant of multiple SP and/or HT.
int pos = byteBuffer.position();
- byte chr = byteBuffer.get();
+ chr = byteBuffer.get();
if (chr == Constants.SP || chr == Constants.HT) {
space = true;
request.method().setBytes(byteBuffer.array(),
parsingRequestLineStart,
@@ -427,7 +427,7 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
if (!fill(false)) // request line parsing
return false;
}
- byte chr = byteBuffer.get();
+ chr = byteBuffer.get();
if (!(chr == Constants.SP || chr == Constants.HT)) {
space = false;
byteBuffer.position(byteBuffer.position() - 1);
@@ -451,15 +451,32 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
return false;
}
int pos = byteBuffer.position();
- byte chr = byteBuffer.get();
+ prevChr = chr;
+ chr = byteBuffer.get();
+ if (prevChr == Constants.CR && chr != Constants.LF) {
+ // CR not followed by LF so not an HTTP/0.9 request and
+ // therefore invalid. Trigger error handling.
+ // Avoid unknown protocol triggering an additional error
+ request.protocol().setString(Constants.HTTP_11);
+ throw new
IllegalArgumentException(sm.getString("iib.invalidRequestTarget"));
+ }
if (chr == Constants.SP || chr == Constants.HT) {
space = true;
end = pos;
- } else if (chr == Constants.CR || chr == Constants.LF) {
+ } else if (chr == Constants.CR) {
+ // HTTP/0.9 style request. CR is optional. LF is not.
+ } else if (chr == Constants.LF) {
// HTTP/0.9 style request
- parsingRequestLineEol = true;
+ // Stop this processing loop
space = true;
- end = pos;
+ // Skip the protocol processing
+ parsingRequestLinePhase = 6;
+ parsingRequestLineEol = true;
+ if (prevChr == Constants.CR) {
+ end = pos - 1;
+ } else {
+ end = pos;
+ }
} else if (chr == Constants.QUESTION && parsingRequestLineQPos
== -1) {
parsingRequestLineQPos = pos;
} else if (parsingRequestLineQPos != -1 &&
!httpParser.isQueryRelaxed(chr)) {
@@ -485,7 +502,9 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
request.requestURI().setBytes(byteBuffer.array(),
parsingRequestLineStart,
end - parsingRequestLineStart);
}
- parsingRequestLinePhase = 5;
+ if (!parsingRequestLineEol) {
+ parsingRequestLinePhase = 5;
+ }
}
if (parsingRequestLinePhase == 5) {
// Spec says single SP but also be tolerant of multiple and/or HT
@@ -521,13 +540,12 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
}
int pos = byteBuffer.position();
- byte chr = byteBuffer.get();
+ prevChr = chr;
+ chr = byteBuffer.get();
if (chr == Constants.CR) {
- end = pos;
- } else if (chr == Constants.LF) {
- if (end == 0) {
- end = pos;
- }
+ // Possible end of request line. Need LF next.
+ } else if (prevChr == Constants.CR && chr == Constants.LF) {
+ end = pos - 1;
parsingRequestLineEol = true;
} else if (!HttpParser.isHttpProtocol(chr)) {
throw new
IllegalArgumentException(sm.getString("iib.invalidHttpProtocol"));
@@ -756,13 +774,6 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
*/
private HeaderParseStatus parseHeader() throws IOException {
- //
- // Check for blank line
- //
-
- byte chr = 0;
- byte prevChr = 0;
-
while (headerParsePos == HeaderParsePosition.HEADER_START) {
// Read new bytes if needed
@@ -781,12 +792,12 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
} else if (prevChr == Constants.CR && chr == Constants.LF) {
return HeaderParseStatus.DONE;
} else {
- if (prevChr == 0) {
- // Must have only read one byte
- byteBuffer.position(byteBuffer.position() - 1);
- } else {
+ if (prevChr == Constants.CR) {
// Must have read two bytes (first was CR, second was not
LF)
byteBuffer.position(byteBuffer.position() - 2);
+ } else {
+ // Must have only read one byte
+ byteBuffer.position(byteBuffer.position() - 1);
}
break;
}
@@ -927,15 +938,15 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
}
}
- chr = byteBuffer.get(byteBuffer.position());
+ byte peek = byteBuffer.get(byteBuffer.position());
if (headerParsePos == HeaderParsePosition.HEADER_MULTI_LINE) {
- if ((chr != Constants.SP) && (chr != Constants.HT)) {
+ if ((peek != Constants.SP) && (peek != Constants.HT)) {
headerParsePos = HeaderParsePosition.HEADER_START;
break;
} else {
// Copying one extra space in the buffer (since there must
// be at least one space inserted between the lines)
- byteBuffer.put(headerData.realPos, chr);
+ byteBuffer.put(headerData.realPos, peek);
headerData.realPos++;
headerParsePos = HeaderParsePosition.HEADER_VALUE_START;
}
@@ -953,9 +964,6 @@ public class Http11InputBuffer implements InputBuffer,
ApplicationBufferHandler
headerParsePos = HeaderParsePosition.HEADER_SKIPLINE;
boolean eol = false;
- byte chr = 0;
- byte prevChr = 0;
-
// Reading bytes until the end of the line
while (!eol) {
diff --git a/java/org/apache/coyote/http11/Http11Processor.java
b/java/org/apache/coyote/http11/Http11Processor.java
index 15ff7ed..aa1569c 100644
--- a/java/org/apache/coyote/http11/Http11Processor.java
+++ b/java/org/apache/coyote/http11/Http11Processor.java
@@ -266,6 +266,11 @@ public class Http11Processor extends AbstractProcessor {
}
}
+ // Process the Protocol component of the request line
+ // Need to know if this is an HTTP 0.9 request before trying to
+ // parse headers.
+ prepareRequestProtocol();
+
if (protocol.isPaused()) {
// 503 - Service unavailable
response.setStatus(503);
@@ -274,7 +279,8 @@ public class Http11Processor extends AbstractProcessor {
keptAlive = true;
// Set this every time in case limit has been changed via
JMX
request.getMimeHeaders().setLimit(protocol.getMaxHeaderCount());
- if (!inputBuffer.parseHeaders()) {
+ // Don't parse headers for HTTP/0.9
+ if (!http09 && !inputBuffer.parseHeaders()) {
// We've read part of the request, don't recycle it
// instead associate it with the socket
openSocket = true;
@@ -521,22 +527,15 @@ public class Http11Processor extends AbstractProcessor {
}
- /**
- * After reading the request headers, we have to setup the request filters.
- */
- private void prepareRequest() throws IOException {
-
- http11 = true;
- http09 = false;
- contentDelimitation = false;
+ private void prepareRequestProtocol() {
- if (protocol.isSSLEnabled()) {
- request.scheme().setString("https");
- }
MessageBytes protocolMB = request.protocol();
if (protocolMB.equals(Constants.HTTP_11)) {
+ http09 = false;
+ http11 = true;
protocolMB.setString(Constants.HTTP_11);
} else if (protocolMB.equals(Constants.HTTP_10)) {
+ http09 = false;
http11 = false;
keepAlive = false;
protocolMB.setString(Constants.HTTP_10);
@@ -547,6 +546,7 @@ public class Http11Processor extends AbstractProcessor {
keepAlive = false;
} else {
// Unsupported protocol
+ http09 = false;
http11 = false;
// Send 505; Unsupported HTTP version
response.setStatus(505);
@@ -556,6 +556,19 @@ public class Http11Processor extends AbstractProcessor {
" Unsupported HTTP version \""+protocolMB+"\"");
}
}
+ }
+
+
+ /**
+ * After reading the request headers, we have to setup the request filters.
+ */
+ private void prepareRequest() throws IOException {
+
+ contentDelimitation = false;
+
+ if (protocol.isSSLEnabled()) {
+ request.scheme().setString("https");
+ }
MimeHeaders headers = request.getMimeHeaders();
diff --git a/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
b/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
index 73c7b03..94b9a17 100644
--- a/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
+++ b/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
@@ -38,6 +38,10 @@ import org.apache.catalina.startup.TomcatBaseTest;
public class TestHttp11InputBuffer extends TomcatBaseTest {
+ private static final String CR = "\r";
+ private static final String LF = "\n";
+ private static final String CRLF = CR + LF;
+
/**
* Test case for https://bz.apache.org/bugzilla/show_bug.cgi?id=48839
*/
@@ -636,7 +640,83 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
@Test
public void testInvalidMethod() {
- InvalidMethodClient client = new InvalidMethodClient();
+ String[] request = new String[1];
+ request[0] =
+ "GET" + (char) 0 + " /test HTTP/1.1" + CRLF +
+ "Host: localhost:8080" + CRLF +
+ "Connection: close" + CRLF +
+ CRLF;
+
+ InvalidClient client = new InvalidClient(request);
+
+ client.doRequest();
+ Assert.assertTrue(client.getResponseLine(), client.isResponse400());
+ Assert.assertTrue(client.isResponseBodyOK());
+ }
+
+
+ @Test
+ public void testInvalidHttp09() {
+
+ String[] request = new String[1];
+ request[0] = "GET /test" + CR + " " + LF;
+
+ InvalidClient client = new InvalidClient(request);
+
+ client.doRequest();
+ Assert.assertTrue(client.getResponseLine(), client.isResponse400());
+ Assert.assertTrue(client.isResponseBodyOK());
+ }
+
+
+ @Test
+ public void testInvalidEndOfRequestLine01() {
+
+ String[] request = new String[1];
+ request[0] =
+ "GET /test HTTP/1.1" + CR +
+ "Host: localhost:8080" + CRLF +
+ "Connection: close" + CRLF +
+ CRLF;
+
+ InvalidClient client = new InvalidClient(request);
+
+ client.doRequest();
+ Assert.assertTrue(client.getResponseLine(), client.isResponse400());
+ Assert.assertTrue(client.isResponseBodyOK());
+ }
+
+
+ @Test
+ public void testInvalidEndOfRequestLine02() {
+
+ String[] request = new String[1];
+ request[0] =
+ "GET /test HTTP/1.1" + LF +
+ "Host: localhost:8080" + CRLF +
+ "Connection: close" + CRLF +
+ CRLF;
+
+ InvalidClient client = new InvalidClient(request);
+
+ client.doRequest();
+ Assert.assertTrue(client.getResponseLine(), client.isResponse400());
+ Assert.assertTrue(client.isResponseBodyOK());
+ }
+
+
+ @Test
+ public void testInvalidHeader01() {
+
+ String[] request = new String[1];
+ request[0] =
+ "GET /test HTTP/1.1" + CRLF +
+ "Host: localhost:8080" + CRLF +
+ CR + "X-Header: xxx" + CRLF +
+ "Connection: close" + CRLF +
+ CRLF;
+
+ InvalidClient client = new InvalidClient(request);
client.doRequest();
Assert.assertTrue(client.getResponseLine(), client.isResponse400());
@@ -645,9 +725,15 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
/**
- * Bug 48839 test client.
+ * Invalid request test client.
*/
- private class InvalidMethodClient extends SimpleHttpClient {
+ private class InvalidClient extends SimpleHttpClient {
+
+ private final String[] request;
+
+ public InvalidClient(String[] request) {
+ this.request = request;
+ }
private Exception doRequest() {
@@ -661,14 +747,6 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
// Open connection
connect();
-
- String[] request = new String[1];
- request[0] =
- "GET" + (char) 0 + " /test HTTP/1.1" + CRLF +
- "Host: localhost:8080" + CRLF +
- "Connection: close" + CRLF +
- CRLF;
-
setRequest(request);
processRequest(); // blocks until response has been read
diff --git a/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
b/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
new file mode 100644
index 0000000..82315f5
--- /dev/null
+++ b/test/org/apache/coyote/http11/TestHttp11InputBufferCRLF.java
@@ -0,0 +1,156 @@
+/*
+ * 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.coyote.http11;
+
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+
+import org.apache.catalina.Context;
+import org.apache.catalina.startup.SimpleHttpClient;
+import org.apache.catalina.startup.TesterServlet;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+
+@RunWith(Parameterized.class)
+public class TestHttp11InputBufferCRLF extends TomcatBaseTest {
+
+ private static final String CR = "\r";
+ private static final String LF = "\n";
+ private static final String CRLF = CR + LF;
+
+ @Parameterized.Parameters
+ public static Collection<Object[]> parameters() {
+ List<Object[]> parameterSets = new ArrayList<>();
+
+ // Requests to exercise code that allows HT in place of SP
+ parameterSets.add(new Object[] { Boolean.FALSE, new String[] {
+ "GET\thttp://localhost:8080/test\tHTTP/1.1"; + CRLF +
+ "Host: localhost:8080" + CRLF +
+ "Connection: close" + CRLF +
+ CRLF } } );
+
+ // Requests to simulate package boundaries
+ // HTTP/0.9 request
+ addRequestWithSplits("GET /test" + CRLF, Boolean.TRUE, parameterSets);
+
+ // HTTP/0.9 request (no optional CR)
+ addRequestWithSplits("GET /test" + LF, Boolean.TRUE, parameterSets);
+
+ // Standard HTTP/1.1 request
+ addRequestWithSplits("GET http://localhost:8080/test HTTP/1.1" + CRLF +
+ "Host: localhost:8080" + CRLF +
+ "Connection: close" + CRLF +
+ CRLF,
+ Boolean.FALSE, parameterSets);
+
+ return parameterSets;
+ }
+
+
+ private static void addRequestWithSplits(String request, Boolean isHttp09,
List<Object[]> parameterSets) {
+ // Add as a single String
+ parameterSets.add(new Object[] { isHttp09, new String[] { request } }
);
+
+ // Add with all CRLF split between the CR and LF
+ List<String> parts = new ArrayList<>();
+ int lastPos = 0;
+ int pos = request.indexOf("\n");
+ while (pos > -1) {
+ parts.add(request.substring(lastPos, pos));
+ lastPos = pos;
+ pos = request.indexOf("\n", lastPos + 1);
+ }
+ parts.add(request.substring(lastPos));
+ parameterSets.add(new Object[] { isHttp09, parts.toArray(new
String[0]) });
+
+ // Add with a split between each character
+ List<String> chars = new ArrayList<>();
+ for (char c : request.toCharArray()) {
+ chars.add(Character.toString(c));
+ }
+ parameterSets.add(new Object[] { isHttp09, chars.toArray(new
String[0]) });
+ }
+
+ @Parameter(0)
+ public boolean isHttp09;
+
+ @Parameter(1)
+ public String[] request;
+
+ @Test
+ public void testBug54947() {
+
+ Client client = new Client(request, isHttp09);
+
+ client.doRequest();
+ Assert.assertTrue(client.isResponseBodyOK());
+ }
+
+
+ private class Client extends SimpleHttpClient {
+
+ public Client(String[] request, boolean isHttp09) {
+ setRequest(request);
+ setUseHttp09(isHttp09);
+ }
+
+ private Exception doRequest() {
+
+ Tomcat tomcat = getTomcatInstance();
+
+ Context root = tomcat.addContext("", TEMP_DIR);
+ Tomcat.addServlet(root, "TesterServlet", new TesterServlet());
+ root.addServletMappingDecoded("/test", "TesterServlet");
+
+ try {
+ tomcat.start();
+ setPort(tomcat.getConnector().getLocalPort());
+ setRequestPause(20);
+
+ // Open connection
+ connect();
+
+ setRequest(request);
+ processRequest(); // blocks until response has been read
+
+ // Close the connection
+ disconnect();
+ } catch (Exception e) {
+ return e;
+ }
+ return null;
+ }
+
+ @Override
+ public boolean isResponseBodyOK() {
+ if (getResponseBody() == null) {
+ return false;
+ }
+ if (!getResponseBody().contains("OK")) {
+ return false;
+ }
+ return true;
+ }
+ }
+}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 858324e..395a5f4 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -45,12 +45,23 @@
issues do not "pop up" wrt. others).
-->
<section name="Tomcat 9.0.33 (markt)" rtext="in development">
+ <subsection name="Coyote">
+ <changelog>
+ <fix>
+ <bug>64210</bug>: Correct a regression in the improvements to HTTP
+ header validation that caused requests to be incorrectly treated as
+ invalid if a <code>CRLF</code> sequence was split between TCP packets.
+ Improve validation of request lines, including for HTTP/0.9 requests.
+ (markt)
+ </fix>
+ </changelog>
+ </subsection>
<subsection name="Other">
<changelog>
<fix>
- <bug>64206</bug>: Correct regression introduced in 10.0.0-M1 that meant
- the HTTP specified when using the Windows Installer was ignored and
8080
- was always used. (markt)
+ <bug>64206</bug>: Correct regression introduced in 9.0.31 that meant
+ the HTTP port specified when using the Windows Installer was ignored
and
+ 8080 was always used. (markt)
</fix>
</changelog>
</subsection>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
