Author: markt
Date: Wed Apr 25 12:03:32 2018
New Revision: 1830068
URL: http://svn.apache.org/viewvc?rev=1830068&view=rev
Log:
First step in addressing https://bz.apache.org/bugzilla/show_bug.cgi?id=62273
This commit actually tightens up the parsing by validating each part of the
request target individually. Subsequent commits will introduce options to
separately relax the parsing of the path segments and the query string.
Modified:
tomcat/trunk/java/org/apache/coyote/http11/Http11InputBuffer.java
tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java
tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java
tomcat/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java
tomcat/trunk/test/org/apache/catalina/core/TestApplicationContext.java
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=1830068&r1=1830067&r2=1830068&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11InputBuffer.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11InputBuffer.java Wed Apr
25 12:03:32 2018
@@ -456,7 +456,13 @@ public class Http11InputBuffer implement
end = pos;
} else if (chr == Constants.QUESTION && parsingRequestLineQPos
== -1) {
parsingRequestLineQPos = pos;
+ } else if (parsingRequestLineQPos != -1 &&
!HttpParser.isQuery(chr)) {
+ // %nn decoding will be checked at the point of decoding
+ throw new
IllegalArgumentException(sm.getString("iib.invalidRequestTarget"));
} else if (HttpParser.isNotRequestTarget(chr)) {
+ // This is a general check that aims to catch problems
early
+ // Detailed checking of each part of the request target
will
+ // happen in Http11Processor#prepareRequest()
throw new
IllegalArgumentException(sm.getString("iib.invalidRequestTarget"));
}
}
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=1830068&r1=1830067&r2=1830068&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11Processor.java Wed Apr 25
12:03:32 2018
@@ -51,6 +51,7 @@ import org.apache.tomcat.util.buf.ByteCh
import org.apache.tomcat.util.buf.MessageBytes;
import org.apache.tomcat.util.http.FastHttpDateFormat;
import org.apache.tomcat.util.http.MimeHeaders;
+import org.apache.tomcat.util.http.parser.HttpParser;
import org.apache.tomcat.util.log.UserDataHelper;
import org.apache.tomcat.util.net.AbstractEndpoint.Handler.SocketState;
import org.apache.tomcat.util.net.SSLSupport;
@@ -652,32 +653,63 @@ public class Http11Processor extends Abs
}
}
- // Check for a full URI (including protocol://host:port/)
+ // Check for an absolute-URI less the query string which has already
+ // been removed during the parsing of the request line
ByteChunk uriBC = request.requestURI().getByteChunk();
+ byte[] uriB = uriBC.getBytes();
if (uriBC.startsWithIgnoreCase("http", 0)) {
-
- int pos = uriBC.indexOf("://", 0, 3, 4);
- int uriBCStart = uriBC.getStart();
- int slashPos = -1;
- if (pos != -1) {
+ int pos = 4;
+ // Check for https
+ if (uriBC.startsWithIgnoreCase("s", pos)) {
+ pos++;
+ }
+ // Next 3 characters must be "://"
+ if (uriBC.startsWith("://", pos)) {
pos += 3;
- byte[] uriB = uriBC.getBytes();
- slashPos = uriBC.indexOf('/', pos);
+ int uriBCStart = uriBC.getStart();
+
+ // '/' does not appear in the authority so use the first
+ // instance to split the authority and the path segments
+ int slashPos = uriBC.indexOf('/', pos);
+ // '@' in the authority delimits the userinfo
int atPos = uriBC.indexOf('@', pos);
+ if (slashPos > -1 && atPos > slashPos) {
+ // First '@' is in the path segments so no userinfo
+ atPos = -1;
+ }
+
if (slashPos == -1) {
slashPos = uriBC.getLength();
- // Set URI as "/"
- request.requestURI().setBytes
- (uriB, uriBCStart + pos - 2, 1);
+ // Set URI as "/". Use 6 as it will always be a '/'.
+ // 01234567
+ // http://
+ // https://
+ request.requestURI().setBytes(uriB, uriBCStart + 6, 1);
} else {
- request.requestURI().setBytes
- (uriB, uriBCStart + slashPos,
- uriBC.getLength() - slashPos);
+ request.requestURI().setBytes(uriB, uriBCStart + slashPos,
uriBC.getLength() - slashPos);
}
+
// Skip any user info
if (atPos != -1) {
+ // Validate the userinfo
+ for (; pos < atPos; pos++) {
+ byte c = uriB[uriBCStart + pos];
+ if (!HttpParser.isUserInfo(c)) {
+ // Strictly there needs to be a check for valid %nn
+ // encoding here but skip it since it will never be
+ // decoded because the userinfo is ignored
+ response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ if (log.isDebugEnabled()) {
+
log.debug(sm.getString("http11processor.request.invalidUserInfo"));
+ }
+ break;
+ }
+ }
+ // Skip the '@'
pos = atPos + 1;
}
+
if (http11) {
// Missing host header is illegal but handled above
if (hostValueMB != null) {
@@ -710,6 +742,25 @@ public class Http11Processor extends Abs
hostValueMB = headers.setValue("host");
hostValueMB.setBytes(uriB, uriBCStart + pos, slashPos -
pos);
}
+ } else {
+ response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ if (log.isDebugEnabled()) {
+
log.debug(sm.getString("http11processor.request.invalidScheme"));
+ }
+ }
+ }
+
+ // Validate the characters in the URI. %nn decoding will be checked at
+ // the point of decoding.
+ for (int i = uriBC.getStart(); i < uriBC.getEnd(); i++) {
+ if (!HttpParser.isAbsolutePath(uriB[i])) {
+ response.setStatus(400);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ if (log.isDebugEnabled()) {
+
log.debug(sm.getString("http11processor.request.invalidUri"));
+ }
+ break;
}
}
@@ -748,20 +799,19 @@ public class Http11Processor extends Abs
headers.removeHeader("content-length");
request.setContentLength(-1);
} else {
- inputBuffer.addActiveFilter
- (inputFilters[Constants.IDENTITY_FILTER]);
+
inputBuffer.addActiveFilter(inputFilters[Constants.IDENTITY_FILTER]);
contentDelimitation = true;
}
}
+ // Validate host name and extract port if present
parseHost(hostValueMB);
if (!contentDelimitation) {
// If there's no content length
// (broken HTTP/1.0 or HTTP/1.1), assume
// the client is not broken and didn't send a body
- inputBuffer.addActiveFilter
- (inputFilters[Constants.VOID_FILTER]);
+ inputBuffer.addActiveFilter(inputFilters[Constants.VOID_FILTER]);
contentDelimitation = true;
}
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=1830068&r1=1830067&r2=1830068&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
(original)
+++ tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties Wed Apr
25 12:03:32 2018
@@ -21,6 +21,9 @@ http11processor.fallToDebug=\n Note: fur
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.invalidScheme=The HTTP request contained an absolute
URI with an invalid scheme
+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.multipleHosts=The request contained multiple host
headers
http11processor.request.noHostHeader=The HTTP/1.1 request did not provide a
host header
http11processor.request.prepare=Error preparing request
Modified: tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java?rev=1830068&r1=1830067&r2=1830068&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/buf/ByteChunk.java Wed Apr 25
12:03:32 2018
@@ -670,7 +670,33 @@ public final class ByteChunk extends Abs
/**
- * Returns true if the buffer starts with the specified string.
+ * Returns true if the buffer starts with the specified string when tested
+ * in a case sensitive manner.
+ *
+ * @param s the string
+ * @param pos The position
+ *
+ * @return <code>true</code> if the start matches
+ */
+ public boolean startsWith(String s, int pos) {
+ byte[] b = buff;
+ int len = s.length();
+ if (b == null || len + pos > end - start) {
+ return false;
+ }
+ int off = start + pos;
+ for (int i = 0; i < len; i++) {
+ if (b[off++] != s.charAt(i)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+
+ /**
+ * Returns true if the buffer starts with the specified string when tested
+ * in a case insensitive manner.
*
* @param s the string
* @param pos The position
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=1830068&r1=1830067&r2=1830068&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
Apr 25 12:03:32 2018
@@ -46,6 +46,11 @@ public class HttpParser {
private static final boolean[] IS_HTTP_PROTOCOL = new boolean[ARRAY_SIZE];
private static final boolean[] IS_ALPHA = new boolean[ARRAY_SIZE];
private static final boolean[] IS_NUMERIC = new boolean[ARRAY_SIZE];
+ private static final boolean[] IS_UNRESERVED = new boolean[ARRAY_SIZE];
+ private static final boolean[] IS_SUBDELIM = new boolean[ARRAY_SIZE];
+ private static final boolean[] IS_USERINFO = new boolean[ARRAY_SIZE];
+ private static final boolean[] IS_ABSOLUTEPATH = new boolean[ARRAY_SIZE];
+ private static final boolean[] IS_QUERY = new boolean[ARRAY_SIZE];
static {
for (int i = 0; i < ARRAY_SIZE; i++) {
@@ -94,6 +99,40 @@ public class HttpParser {
if (i >= 'a' && i <= 'z' || i >= 'A' && i <= 'Z') {
IS_ALPHA[i] = true;
}
+
+ if (IS_ALPHA[i] || IS_NUMERIC[i] || i == '-' || i == '.' || i ==
'_' || i == '~') {
+ IS_UNRESERVED[i] = true;
+ }
+
+ if (i == '!' || i == '$' || i == '&' || i == '\'' || i == '(' || i
== ')' || i == '*' || i == '+' ||
+ i == ',' || i == ';' || i == '=') {
+ IS_SUBDELIM[i] = true;
+ }
+
+ // userinfo = *( unreserved / pct-encoded / sub-delims / ":" )
+ if (IS_UNRESERVED[i] || i == '%' || IS_SUBDELIM[i] || i == ':') {
+ IS_USERINFO[i] = true;
+ }
+
+ /*
+ * absolute-path = 1*( "/" segment )
+ * segment = *pchar
+ * pchar = unreserved / pct-encoded / sub-delims / ":" /
"@"
+ *
+ * Note pchar allows everything userinfo allows plus "@"
+ */
+ if (IS_USERINFO[i] || i == '@' || i == '/') {
+ IS_ABSOLUTEPATH[i] = true;
+ }
+
+ /*
+ * query = *( pchar / "/" / "?" )
+ *
+ * Note query allows everything absolute-path allows plus "?"
+ */
+ if (IS_ABSOLUTEPATH[i] || i == '?') {
+ IS_QUERY[i] = true;
+ }
}
}
@@ -193,6 +232,39 @@ public class HttpParser {
} catch (ArrayIndexOutOfBoundsException ex) {
return false;
}
+ }
+
+
+ public static boolean isUserInfo(int c) {
+ // Fast for valid user info characters, slower for some incorrect
+ // ones
+ try {
+ return IS_USERINFO[c];
+ } catch (ArrayIndexOutOfBoundsException ex) {
+ return false;
+ }
+ }
+
+
+ public static boolean isAbsolutePath(int c) {
+ // Fast for valid user info characters, slower for some incorrect
+ // ones
+ try {
+ return IS_ABSOLUTEPATH[c];
+ } catch (ArrayIndexOutOfBoundsException ex) {
+ return false;
+ }
+ }
+
+
+ public static boolean isQuery(int c) {
+ // Fast for valid user info characters, slower for some incorrect
+ // ones
+ try {
+ return IS_QUERY[c];
+ } catch (ArrayIndexOutOfBoundsException ex) {
+ return false;
+ }
}
Modified: tomcat/trunk/test/org/apache/catalina/core/TestApplicationContext.java
URL:
http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/catalina/core/TestApplicationContext.java?rev=1830068&r1=1830067&r2=1830068&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/catalina/core/TestApplicationContext.java
(original)
+++ tomcat/trunk/test/org/apache/catalina/core/TestApplicationContext.java Wed
Apr 25 12:03:32 2018
@@ -68,7 +68,7 @@ public class TestApplicationContext exte
ByteChunk res = new ByteChunk();
int rc = getUrl("http://localhost:"; + getPort() +
- "/test/bug5nnnn/bug53467].jsp", res, null);
+ "/test/bug5nnnn/bug53467%5D.jsp", res, null);
Assert.assertEquals(HttpServletResponse.SC_OK, rc);
Assert.assertTrue(res.toString().contains("<p>OK</p>"));
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
