Skip to content

Commit

Permalink
Add additional checks for valid characters to the HTTP request line
Browse files Browse the repository at this point in the history 
parsing so invalid request lines are rejected sooner.

git-svn-id: https://svn.apache.org/repos/asf/tomcat/tc8.5.x/trunk@1767645 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@markt-asf
markt-asf committed Nov 2, 2016
1 parent 48450a5 commit f96f575
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 83 deletions.
61 changes: 8 additions & 53 deletions java/org/apache/coyote/http11/Http11InputBuffer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.tomcat.util.buf.ByteChunk;
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;
Expand All @@ -48,56 +49,6 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
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);

Expand Down Expand Up @@ -461,7 +412,7 @@ boolean parseRequestLine(boolean keptAlive) throws IOException {
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"));
}
Expand Down Expand Up @@ -512,6 +463,8 @@ boolean parseRequestLine(boolean keptAlive) throws IOException {
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) {
Expand Down Expand Up @@ -549,7 +502,7 @@ boolean parseRequestLine(boolean keptAlive) throws IOException {
if (parsingRequestLinePhase == 6) {
//
// Reading the protocol
// Protocol is always US-ASCII
// Protocol is always "HTTP/" DIGIT "." DIGIT
//
while (!parsingRequestLineEol) {
// Read new bytes if needed
Expand All @@ -567,6 +520,8 @@ boolean parseRequestLine(boolean keptAlive) throws IOException {
end = pos;
}
parsingRequestLineEol = true;
} else if (!HttpParser.isHttpProtocol(chr)) {
throw new IllegalArgumentException(sm.getString("iib.invalidHttpProtocol"));
}
}

Expand Down Expand Up @@ -831,7 +786,7 @@ private HeaderParseStatus parseHeader() throws IOException {
headerData.realPos = pos;
headerData.lastSignificantChar = pos;
break;
} else if (chr < 0 || !HTTP_TOKEN_CHAR[chr]) {
} else if (!HttpParser.isToken(chr)) {
// If a non-token header is detected, skip the line and
// ignore the header
headerData.lastSignificantChar = pos;
Expand Down
4 changes: 3 additions & 1 deletion java/org/apache/coyote/http11/LocalStrings.properties
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,10 @@ iib.available.readFail=A non-blocking read failed while attempting to determine
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
Expand Down
44 changes: 43 additions & 1 deletion java/org/apache/tomcat/util/http/parser/HttpParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Expand All @@ -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;
}
}
}

Expand Down Expand Up @@ -99,6 +116,7 @@ public static String unquote(String input) {
return result.toString();
}


public static boolean isToken(int c) {
// Fast for correct values, slower for incorrect ones
try {
Expand All @@ -108,15 +126,39 @@ public static boolean isToken(int c) {
}
}


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) {
return false;
}
}


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 {

Expand Down
59 changes: 31 additions & 28 deletions test/org/apache/catalina/valves/rewrite/TestRewriteValve.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -232,7 +232,7 @@ public void testUtf8WithBothQsFlagsRNE() throws Exception {
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*)/(.*) /c/\u00A1$1?$2 [R,NE]",
"/b/%C2%A1/id=%C2%A1?di=%C2%AE", "/c/\u00C2\u00A1\u00C2\u00A1", "id=\u00C2\u00A1");
"/b/%C2%A1/id=%C2%A1?di=%C2%AE", null);
}


Expand All @@ -242,7 +242,7 @@ public void testUtf8WithBothQsFlagsRBNE() throws Exception {
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*)/(.*) /c/\u00A1$1?$2 [R,B,NE]",
"/b/%C2%A1/id=%C2%A1?di=%C2%AE", "/c/\u00C2\u00A1%C2%A1", "id=%C2%A1");
"/b/%C2%A1/id=%C2%A1?di=%C2%AE", null);
}


Expand Down Expand Up @@ -279,8 +279,7 @@ public void testUtf8WithBothQsFlagsRNEQSA() throws Exception {
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*)/(.*) /c/\u00A1$1?$2 [R,NE,QSA]",
"/b/%C2%A1/id=%C2%A1?di=%C2%AE", "/c/\u00C2\u00A1\u00C2\u00A1",
"id=\u00C2\u00A1&di=\u00C2\u00AE");
"/b/%C2%A1/id=%C2%A1?di=%C2%AE", null);
}


Expand All @@ -290,8 +289,7 @@ public void testUtf8WithBothQsFlagsRBNEQSA() throws Exception {
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*)/(.*) /c/\u00A1$1?$2 [R,B,NE,QSA]",
"/b/%C2%A1/id=%C2%A1?di=%C2%AE", "/c/\u00C2\u00A1%C2%A1",
"id=%C2%A1&di=\u00C2\u00AE");
"/b/%C2%A1/id=%C2%A1?di=%C2%AE", null);
}


Expand Down Expand Up @@ -333,7 +331,7 @@ public void testUtf8WithOriginalQsFlagsRNE() throws Exception {
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*) /c/\u00A1$1 [R,NE]",
"/b/%C2%A1?id=%C2%A1", "/c/\u00C2\u00A1\u00C2\u00A1", "id=\u00C2\u00A1");
"/b/%C2%A1?id=%C2%A1", null);
}


Expand All @@ -343,7 +341,7 @@ public void testUtf8WithOriginalQsFlagsRBNE() throws Exception {
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*) /c/\u00A1$1 [R,B,NE]",
"/b/%C2%A1?id=%C2%A1", "/c/\u00C2\u00A1%C2%A1", "id=\u00C2\u00A1");
"/b/%C2%A1?id=%C2%A1", null);
}


Expand Down Expand Up @@ -394,7 +392,7 @@ public void testUtf8WithRewriteQsFlagsRNE() throws Exception {
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*)/(.*) /c/\u00A1$1?$2 [R,NE]",
"/b/%C2%A1/id=%C2%A1", "/c/\u00C2\u00A1\u00C2\u00A1", "id=\u00C2\u00A1");
"/b/%C2%A1/id=%C2%A1", null);
}


Expand All @@ -404,7 +402,7 @@ public void testUtf8WithRewriteQsFlagsRBNE() throws Exception {
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*)/(.*) /c/\u00A1$1?$2 [R,B,NE]",
"/b/%C2%A1/id=%C2%A1", "/c/\u00C2\u00A1%C2%A1", "id=%C2%A1");
"/b/%C2%A1/id=%C2%A1", null);
}


Expand Down Expand Up @@ -450,8 +448,7 @@ public void testUtf8FlagsRNE() throws Exception {
// Note %C2%A1 == \u00A1
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*) /c/\u00A1$1 [R,NE]",
"/b/%C2%A1", "/c/\u00C2\u00A1\u00C2\u00A1");
doTestRewrite("RewriteRule ^/b/(.*) /c/\u00A1$1 [R,NE]", "/b/%C2%A1", null);
}


Expand All @@ -460,8 +457,7 @@ public void testUtf8FlagsRBNE() throws Exception {
// Note %C2%A1 == \u00A1
// Failing to escape the redirect means UTF-8 bytes in the Location
// header which will be treated as if they are ISO-8859-1
doTestRewrite("RewriteRule ^/b/(.*) /c/\u00A1$1 [R,B,NE]",
"/b/%C2%A1", "/c/\u00C2\u00A1%C2%A1");
doTestRewrite("RewriteRule ^/b/(.*) /c/\u00A1$1 [R,B,NE]", "/b/%C2%A1", null);
}


Expand Down Expand Up @@ -515,22 +511,29 @@ private void doTestRewrite(String config, String request, String expectedURI,

tomcat.start();

ByteChunk res = getUrl("http://localhost:" + getPort() + request);
ByteChunk res = new ByteChunk();
int rc = getUrl("http://localhost:" + getPort() + request, res, null);
res.setCharset(StandardCharsets.UTF_8);

String body = res.toString();
RequestDescriptor requestDesc = SnoopResult.parse(body);
String requestURI = requestDesc.getRequestInfo("REQUEST-URI");
Assert.assertEquals(expectedURI, requestURI);

if (expectedQueryString != null) {
String queryString = requestDesc.getRequestInfo("REQUEST-QUERY-STRING");
Assert.assertEquals(expectedQueryString, queryString);
}

if (expectedAttributeValue != null) {
String attributeValue = requestDesc.getAttribute("X-Test");
Assert.assertEquals(expectedAttributeValue, attributeValue);
if (expectedURI == null) {
// Rewrite is expected to fail. Probably because invalid characters
// were written into the request target
Assert.assertEquals(400, rc);
} else {
String body = res.toString();
RequestDescriptor requestDesc = SnoopResult.parse(body);
String requestURI = requestDesc.getRequestInfo("REQUEST-URI");
Assert.assertEquals(expectedURI, requestURI);

if (expectedQueryString != null) {
String queryString = requestDesc.getRequestInfo("REQUEST-QUERY-STRING");
Assert.assertEquals(expectedQueryString, queryString);
}

if (expectedAttributeValue != null) {
String attributeValue = requestDesc.getAttribute("X-Test");
Assert.assertEquals(expectedAttributeValue, attributeValue);
}
}
}
}
4 changes: 4 additions & 0 deletions webapps/docs/changelog.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,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">
Expand Down

0 comments on commit f96f575

Please sign in to comment.