Author: markt Date: Wed Nov 2 11:57:14 2016 New Revision: 1767641 URL: http://svn.apache.org/viewvc?rev=1767641&view=rev Log: Add additional checks for valid characters to the HTTP request line parsing so invalid request lines are rejected sooner.
Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11InputBuffer.java tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java tomcat/trunk/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java tomcat/trunk/webapps/docs/changelog.xml Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11InputBuffer.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11InputBuffer.java?rev=1767641&r1=1767640&r2=1767641&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/http11/Http11InputBuffer.java (original) +++ tomcat/trunk/java/org/apache/coyote/http11/Http11InputBuffer.java Wed Nov 2 11:57:14 2016 @@ -27,6 +27,7 @@ import org.apache.juli.logging.Log; import org.apache.juli.logging.LogFactory; import org.apache.tomcat.util.buf.MessageBytes; import org.apache.tomcat.util.http.MimeHeaders; +import org.apache.tomcat.util.http.parser.HttpParser; import org.apache.tomcat.util.net.ApplicationBufferHandler; import org.apache.tomcat.util.net.SocketWrapperBase; import org.apache.tomcat.util.res.StringManager; @@ -47,56 +48,6 @@ public class Http11InputBuffer implement private static final StringManager sm = StringManager.getManager(Http11InputBuffer.class); - private static final boolean[] HTTP_TOKEN_CHAR = new boolean[128]; - static { - for (int i = 0; i < 128; i++) { - if (i < 32) { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == 127) { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '(') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == ')') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '<') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '>') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '@') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == ',') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == ';') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == ':') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '\\') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '\"') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '/') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '[') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == ']') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '?') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '=') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '{') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == '}') { - HTTP_TOKEN_CHAR[i] = false; - } else if (i == ' ') { - HTTP_TOKEN_CHAR[i] = false; - } else { - HTTP_TOKEN_CHAR[i] = true; - } - } - } - - private static final byte[] CLIENT_PREFACE_START = "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n".getBytes(StandardCharsets.ISO_8859_1); @@ -446,7 +397,7 @@ public class Http11InputBuffer implement space = true; request.method().setBytes(byteBuffer.array(), parsingRequestLineStart, pos - parsingRequestLineStart); - } else if (!HTTP_TOKEN_CHAR[chr]) { + } else if (!HttpParser.isToken(chr)) { byteBuffer.position(byteBuffer.position() - 1); throw new IllegalArgumentException(sm.getString("iib.invalidmethod")); } @@ -497,6 +448,8 @@ public class Http11InputBuffer implement end = pos; } else if (chr == Constants.QUESTION && parsingRequestLineQPos == -1) { parsingRequestLineQPos = pos; + } else if (HttpParser.isNotRequestTarget(chr)) { + throw new IllegalArgumentException(sm.getString("iib.invalidRequestTarget")); } } if (parsingRequestLineQPos >= 0) { @@ -534,7 +487,7 @@ public class Http11InputBuffer implement if (parsingRequestLinePhase == 6) { // // Reading the protocol - // Protocol is always US-ASCII + // Protocol is always "HTTP/" DIGIT "." DIGIT // while (!parsingRequestLineEol) { // Read new bytes if needed @@ -552,6 +505,8 @@ public class Http11InputBuffer implement end = pos; } parsingRequestLineEol = true; + } else if (!HttpParser.isHttpProtocol(chr)) { + throw new IllegalArgumentException(sm.getString("iib.invalidHttpProtocol")); } } @@ -816,7 +771,7 @@ public class Http11InputBuffer implement headerData.realPos = pos; headerData.lastSignificantChar = pos; break; - } else if (chr < 0 || !HTTP_TOKEN_CHAR[chr]) { + } else if (chr < 0 || !HttpParser.isToken(chr)) { // If a non-token header is detected, skip the line and // ignore the header headerData.lastSignificantChar = pos; 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=1767641&r1=1767640&r2=1767641&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties (original) +++ tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties Wed Nov 2 11:57:14 2016 @@ -31,8 +31,10 @@ iib.available.readFail=A non-blocking re iib.eof.error=Unexpected EOF read on the socket iib.failedread.apr=Read failed with APR/native error code [{0}] iib.filter.npe=You may not add a null filter -iib.invalidheader=The HTTP header line [{0}] does not conform to RFC 2616 and has been ignored. +iib.invalidheader=The HTTP header line [{0}] does not conform to RFC 7230 and has been ignored. iib.invalidmethod=Invalid character found in method name. HTTP method names must be tokens +iib.invalidRequestTarget=Invalid character found in the request target. The valid characters are defined in RFC 7230 and RFC 3986 +iib.invalidHttpProtocol=Invalid character found in the HTTP protocol iib.parseheaders.ise.error=Unexpected state: headers already parsed. Buffer not recycled? iib.readtimeout=Timeout attempting to read data from the socket iib.requestheadertoolarge.error=Request header is too large Modified: tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java?rev=1767641&r1=1767640&r2=1767641&view=diff ============================================================================== --- tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java (original) +++ tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java Wed Nov 2 11:57:14 2016 @@ -40,6 +40,8 @@ public class HttpParser { private static final boolean[] IS_SEPARATOR = new boolean[ARRAY_SIZE]; private static final boolean[] IS_TOKEN = new boolean[ARRAY_SIZE]; private static final boolean[] IS_HEX = new boolean[ARRAY_SIZE]; + private static final boolean[] IS_NOT_REQUEST_TARGET = new boolean[ARRAY_SIZE]; + private static final boolean[] IS_HTTP_PROTOCOL = new boolean[ARRAY_SIZE]; static { for (int i = 0; i < ARRAY_SIZE; i++) { @@ -65,6 +67,21 @@ public class HttpParser { if ((i >= '0' && i <='9') || (i >= 'a' && i <= 'f') || (i >= 'A' && i <= 'F')) { IS_HEX[i] = true; } + + // Not valid for request target. + // Combination of multiple rules from RFC7230 and RFC 3986. Must be + // ASCII, no controls plus a few additional characters excluded + if (IS_CONTROL[i] || i > 127 || + i == ' ' || i == '\"' || i == '#' || i == '<' || i == '>' || i == '\\' || + i == '^' || i == '`' || i == '{' || i == '|' || i == '}') { + IS_NOT_REQUEST_TARGET[i] = true; + } + + // Not valid for HTTP protocol + // "HTTP/" DIGIT "." DIGIT + if (i == 'H' || i == 'T' || i == 'P' || i == '/' || i == '.' || (i >= '0' && i <= '9')) { + IS_HTTP_PROTOCOL[i] = true; + } } } @@ -99,6 +116,7 @@ public class HttpParser { return result.toString(); } + public static boolean isToken(int c) { // Fast for correct values, slower for incorrect ones try { @@ -108,8 +126,9 @@ public class HttpParser { } } + public static boolean isHex(int c) { - // Fast for correct values, slower for incorrect ones + // Fast for correct values, slower for some incorrect ones try { return IS_HEX[c]; } catch (ArrayIndexOutOfBoundsException ex) { @@ -117,6 +136,29 @@ public class HttpParser { } } + + public static boolean isNotRequestTarget(int c) { + // Fast for valid request target characters, slower for some incorrect + // ones + try { + return IS_NOT_REQUEST_TARGET[c]; + } catch (ArrayIndexOutOfBoundsException ex) { + return true; + } + } + + + public static boolean isHttpProtocol(int c) { + // Fast for valid HTTP protocol characters, slower for some incorrect + // ones + try { + return IS_HTTP_PROTOCOL[c]; + } catch (ArrayIndexOutOfBoundsException ex) { + return false; + } + } + + // Skip any LWS and return the next char static int skipLws(StringReader input, boolean withReset) throws IOException { Modified: tomcat/trunk/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java?rev=1767641&r1=1767640&r2=1767641&view=diff ============================================================================== --- tomcat/trunk/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java (original) +++ tomcat/trunk/test/org/apache/catalina/valves/rewrite/TestRewriteValve.java Wed Nov 2 11:57:14 2016 @@ -19,6 +19,7 @@ package org.apache.catalina.valves.rewri import java.nio.charset.StandardCharsets; import org.junit.Assert; +import org.junit.Ignore; import org.junit.Test; import org.apache.catalina.Context; @@ -227,6 +228,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8WithBothQsFlagsRNE() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -237,6 +239,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8WithBothQsFlagsRBNE() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -274,6 +277,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8WithBothQsFlagsRNEQSA() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -285,6 +289,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8WithBothQsFlagsRBNEQSA() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -328,6 +333,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8WithOriginalQsFlagsRNE() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -338,6 +344,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8WithOriginalQsFlagsRBNE() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -389,6 +396,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8WithRewriteQsFlagsRNE() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -399,6 +407,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8WithRewriteQsFlagsRBNE() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -446,6 +455,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8FlagsRNE() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location @@ -456,6 +466,7 @@ public class TestRewriteValve extends To @Test + @Ignore // Use of NE results in invalid characters in request-target public void testUtf8FlagsRBNE() throws Exception { // Note %C2%A1 == \u00A1 // Failing to escape the redirect means UTF-8 bytes in the Location Modified: tomcat/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1767641&r1=1767640&r2=1767641&view=diff ============================================================================== --- tomcat/trunk/webapps/docs/changelog.xml (original) +++ tomcat/trunk/webapps/docs/changelog.xml Wed Nov 2 11:57:14 2016 @@ -174,6 +174,10 @@ Improve detection of I/O errors during async processing on non-container threads and trigger async error handling when they are detected. (markt) </fix> + <add> + Add additional checks for valid characters to the HTTP request line + parsing so invalid request lines are rejected sooner. (markt) + </add> </changelog> </subsection> <subsection name="Jasper"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org