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 2017/09/18 19:33:18 UTC
svn commit: r1808766 - in /tomcat/trunk:
java/org/apache/coyote/http11/Http11Processor.java
java/org/apache/coyote/http11/LocalStrings.properties
test/org/apache/coyote/http11/TestHttp11Processor.java
webapps/docs/changelog.xml
Author: markt
Date: Mon Sep 18 19:33:18 2017
New Revision: 1808766
URL: http://svn.apache.org/viewvc?rev=1808766&view=rev
Log:
Implement various Host header checks required by RFC 7230
- Host header must be present for HTTP/1.1 requests
- multiple host headers are invalid
- if the request line include the host, it must match the host header
Modified:
tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java
tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
tomcat/trunk/test/org/apache/coyote/http11/TestHttp11Processor.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java?rev=1808766&r1=1808765&r2=1808766&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java Mon Sep 18 19:33:18 2017
@@ -725,6 +725,30 @@ public class Http11Processor extends Abs
}
}
+
+ // Check host header
+ MessageBytes hostValueMB = null;
+ try {
+ hostValueMB = headers.getUniqueValue("host");
+ } catch (IllegalArgumentException iae) {
+ // Multiple Host headers are not permitted
+ // 400 - Bad request
+ response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("http11processor.request.multipleHosts"));
+ }
+ }
+ if (http11 && hostValueMB == null) {
+ // 400 - Bad request
+ response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("http11processor.request.prepare")+
+ " host header missing");
+ }
+ }
+
// Check for a full URI (including protocol://host:port/)
ByteChunk uriBC = request.requestURI().getByteChunk();
if (uriBC.startsWithIgnoreCase("http", 0)) {
@@ -733,21 +757,44 @@ public class Http11Processor extends Abs
int uriBCStart = uriBC.getStart();
int slashPos = -1;
if (pos != -1) {
+ pos += 3;
byte[] uriB = uriBC.getBytes();
- slashPos = uriBC.indexOf('/', pos + 3);
+ slashPos = uriBC.indexOf('/', pos);
+ int atPos = uriBC.indexOf('@', pos);
if (slashPos == -1) {
slashPos = uriBC.getLength();
// Set URI as "/"
request.requestURI().setBytes
- (uriB, uriBCStart + pos + 1, 1);
+ (uriB, uriBCStart + pos - 2, 1);
} else {
request.requestURI().setBytes
(uriB, uriBCStart + slashPos,
uriBC.getLength() - slashPos);
}
- MessageBytes hostMB = headers.setValue("host");
- hostMB.setBytes(uriB, uriBCStart + pos + 3,
- slashPos - pos - 3);
+ // Skip any user info
+ if (atPos != -1) {
+ pos = atPos + 1;
+ }
+ if (http11) {
+ // Missing host header is illegal but handled above
+ if (hostValueMB != null) {
+ // Any host in the request line must be consistent with
+ // the Host header
+ if (!hostValueMB.getByteChunk().equals(
+ uriB, uriBCStart + pos, slashPos - pos)) {
+ response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ if (log.isDebugEnabled()) {
+ log.debug(sm.getString("http11processor.request.inconsistentHosts"));
+ }
+ }
+ }
+ } 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);
+ }
}
}
@@ -792,20 +839,7 @@ public class Http11Processor extends Abs
}
}
- MessageBytes valueMB = headers.getValue("host");
-
- // Check host header
- if (http11 && (valueMB == null)) {
- // 400 - Bad request
- response.setStatus(400);
- setErrorState(ErrorState.CLOSE_CLEAN, null);
- if (log.isDebugEnabled()) {
- log.debug(sm.getString("http11processor.request.prepare")+
- " host header missing");
- }
- }
-
- parseHost(valueMB);
+ parseHost(hostValueMB);
if (!contentDelimitation) {
// If there's no content length
Modified: tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties?rev=1808766&r1=1808765&r2=1808766&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties Mon Sep 18 19:33:18 2017
@@ -20,6 +20,8 @@ abstractHttp11Protocol.httpUpgradeConfig
http11processor.fallToDebug=\n Note: further occurrences of HTTP header parsing errors will be logged at DEBUG level.
http11processor.header.parse=Error parsing HTTP request header
http11processor.neverused=This method should never be used
+http11processor.request.inconsistentHosts=The host specified in the request line is not consistent with the host header
+http11processor.request.multipleHosts=The request contained multiple host headers
http11processor.request.prepare=Error preparing request
http11processor.request.process=Error processing request
http11processor.request.finish=Error finishing request
Modified: tomcat/trunk/test/org/apache/coyote/http11/TestHttp11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http11/TestHttp11Processor.java?rev=1808766&r1=1808765&r2=1808766&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http11/TestHttp11Processor.java (original)
+++ tomcat/trunk/test/org/apache/coyote/http11/TestHttp11Processor.java Mon Sep 18 19:33:18 2017
@@ -1008,4 +1008,309 @@ public class TestHttp11Processor extends
resp.setStatus(205);
}
}
+
+ /*
+ * Multiple, different Host headers
+ */
+ @Test
+ public void testMultipleHostHeader01() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET /foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: a" + SimpleHttpClient.CRLF +
+ "Host: b" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 400 response.
+ assertTrue(client.isResponse400());
+ }
+
+ /*
+ * Multiple instances of the same Host header
+ */
+ @Test
+ public void testMultipleHostHeader02() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET /foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: a" + SimpleHttpClient.CRLF +
+ "Host: a" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 400 response.
+ assertTrue(client.isResponse400());
+ }
+
+ @Test
+ public void testMissingHostHeader() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET /foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 400 response.
+ assertTrue(client.isResponse400());
+ }
+
+ @Test
+ public void testInconsistentHostHeader01() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET http://a/foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: b" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 400 response.
+ assertTrue(client.isResponse400());
+ }
+
+ @Test
+ public void testInconsistentHostHeader02() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET http://a:8080/foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: b:8080" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 400 response.
+ assertTrue(client.isResponse400());
+ }
+
+ @Test
+ public void testInconsistentHostHeader03() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET http://user:pwd@a/foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: b" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 400 response.
+ assertTrue(client.isResponse400());
+ }
+
+ /*
+ * Request line host is an exact match for Host header (no port)
+ */
+ @Test
+ public void testConsistentHostHeader01() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET http://a/foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: a" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 200 response.
+ assertTrue(client.isResponse200());
+ }
+
+ /*
+ * Request line host is an exact match for Host header (with port)
+ */
+ @Test
+ public void testConsistentHostHeader02() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET http://a:8080/foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: a:8080" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 200 response.
+ assertTrue(client.isResponse200());
+ }
+
+ /*
+ * Request line host is an exact match for Host header
+ * (no port, with user info)
+ */
+ @Test
+ public void testConsistentHostHeader03() throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // This setting means the connection will be closed at the end of the
+ // request
+ tomcat.getConnector().setAttribute("maxKeepAliveRequests", "1");
+
+ // No file system docBase required
+ Context ctx = tomcat.addContext("", null);
+
+ // Add servlet
+ Tomcat.addServlet(ctx, "TesterServlet", new TesterServlet());
+ ctx.addServletMappingDecoded("/foo", "TesterServlet");
+
+ tomcat.start();
+
+ String request =
+ "GET http://user:pwd@a/foo HTTP/1.1" + SimpleHttpClient.CRLF +
+ "Host: a" + SimpleHttpClient.CRLF +
+ SimpleHttpClient.CRLF;
+
+ Client client = new Client(tomcat.getConnector().getLocalPort());
+ client.setRequest(new String[] {request});
+
+ client.connect();
+ client.processRequest();
+
+ // Expected response is a 200 response.
+ assertTrue(client.isResponse200());
+ }
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1808766&r1=1808765&r2=1808766&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Mon Sep 18 19:33:18 2017
@@ -57,6 +57,21 @@
(non-token) header names with a 400 response and reject such requests by
default. (markt)
</add>
+ <fix>
+ Implement the requirements of RFC 7230 (and RFC 2616) that HTTP/1.1
+ requests must include a <code>Host</code> header and any request that
+ does not must be rejected with a 400 response. (markt)
+ </fix>
+ <fix>
+ Implement the requirements of RFC 7230 that any HTTP/1.1 request that
+ specifies a host in the request line, must specify the same host in the
+ <code>Host</code> header. (markt)
+ </fix>
+ <fix>
+ Implement the requirements of RFC 7230 that any HTTP/1.1 request that
+ contains multiple <code>Host</code> headers is rejected with a 400
+ response. (markt)
+ </fix>
</changelog>
</subsection>
</section>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1808766 - in /tomcat/trunk:
java/org/apache/coyote/http11/Http11Processor.java
java/org/apache/coyote/http11/LocalStrings.properties
test/org/apache/coyote/http11/TestHttp11Processor.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 18/09/17 20:33, markt@apache.org wrote:
> Author: markt
> Date: Mon Sep 18 19:33:18 2017
> New Revision: 1808766
>
> URL: http://svn.apache.org/viewvc?rev=1808766&view=rev
> Log:
> Implement various Host header checks required by RFC 7230
Before I go any further with this work (I want to plug in the Host name
parser I wrote ~6 months ago) I wanted to get some feedback on these checks.
> - Host header must be present for HTTP/1.1 requests
> - multiple host headers are invalid
The new Tomcat behaviour (reject with 400) is consistent with httpd for
the above 2 tests.
> - if the request line include the host, it must match the host header
This goes further than httpd does (at the moment). Note RFC 2616 says in
this case the request line takes precedence (which is what httpd does)
and the old Tomcat code did. RFC 7230 says they must match.
I'm wondering which, if any, of the above tests we might want to make
optional in 9.0.x. I'm less concerned about the first 2 tests since the
behaviour is consistent with httpd. I am wondering about making the 3rd
test optional but enabled by default for 9.0.x. When the test is
disabled, the request line value would take precedence as it did before
this fix.
I'm also thinking about the same questions for 8.5.x, 8.0.x and 7.0.x.
My current thinking is as 9.0.x but change the default for the 3rd test
to disabled.
Thoughts? Comments?
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org