444 lines
18 KiB
Diff
444 lines
18 KiB
Diff
From 8bfb0ff7f25fe7555a5eb2f7984f73546c11aa26 Mon Sep 17 00:00:00 2001
|
|
From: Mark Thomas <markt@apache.org>
|
|
Date: Mon, 6 Jan 2020 20:53:25 +0000
|
|
Subject: [PATCH] Stricter header value parsing
|
|
|
|
---
|
|
.../coyote/http11/AbstractHttp11Protocol.java | 51 ++++++++++---
|
|
.../coyote/http11/Http11InputBuffer.java | 51 +++++++++----
|
|
.../apache/coyote/http11/Http11Processor.java | 2 +-
|
|
.../apache/tomcat/util/http/MimeHeaders.java | 2 +-
|
|
.../tomcat/util/http/parser/HttpParser.java | 11 +++
|
|
.../coyote/http11/TestHttp11InputBuffer.java | 74 +++++++++++++++----
|
|
webapps/docs/config/http.xml | 11 ++-
|
|
8 files changed, 164 insertions(+), 43 deletions(-)
|
|
|
|
diff --git a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
|
|
index e5ab885..9d10cbf 100644
|
|
--- a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
|
|
+++ b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
|
|
@@ -136,27 +136,56 @@ public abstract class AbstractHttp11Protocol<S> extends AbstractProtocol<S> {
|
|
}
|
|
|
|
|
|
- private boolean rejectIllegalHeaderName = true;
|
|
+ private boolean rejectIllegalHeader = true;
|
|
/**
|
|
- * If an HTTP request is received that contains an illegal header name (i.e.
|
|
- * the header name is not a token) will the request be rejected (with a 400
|
|
- * response) or will the illegal header be ignored.
|
|
+ * If an HTTP request is received that contains an illegal header name or
|
|
+ * value (e.g. the header name is not a token) will the request be rejected
|
|
+ * (with a 400 response) or will the illegal header be ignored?
|
|
*
|
|
* @return {@code true} if the request will be rejected or {@code false} if
|
|
* the header will be ignored
|
|
*/
|
|
- public boolean getRejectIllegalHeaderName() { return rejectIllegalHeaderName; }
|
|
+ public boolean getRejectIllegalHeader() { return rejectIllegalHeader; }
|
|
/**
|
|
- * If an HTTP request is received that contains an illegal header name (i.e.
|
|
- * the header name is not a token) should the request be rejected (with a
|
|
- * 400 response) or should the illegal header be ignored.
|
|
+ * If an HTTP request is received that contains an illegal header name or
|
|
+ * value (e.g. the header name is not a token) should the request be
|
|
+ * rejected (with a 400 response) or should the illegal header be ignored?
|
|
+ *
|
|
+ * @param rejectIllegalHeader {@code true} to reject requests with illegal
|
|
+ * header names or values, {@code false} to
|
|
+ * ignore the header
|
|
+ */
|
|
+ public void setRejectIllegalHeader(boolean rejectIllegalHeader) {
|
|
+ this.rejectIllegalHeader = rejectIllegalHeader;
|
|
+ }
|
|
+ /**
|
|
+ * If an HTTP request is received that contains an illegal header name or
|
|
+ * value (e.g. the header name is not a token) will the request be rejected
|
|
+ * (with a 400 response) or will the illegal header be ignored?
|
|
+ *
|
|
+ * @return {@code true} if the request will be rejected or {@code false} if
|
|
+ * the header will be ignored
|
|
+ *
|
|
+ * @deprecated Now an alias for {@link #getRejectIllegalHeader()}. Will be
|
|
+ * removed in Tomcat 10 onwards.
|
|
+ */
|
|
+ @Deprecated
|
|
+ public boolean getRejectIllegalHeaderName() { return rejectIllegalHeader; }
|
|
+ /**
|
|
+ * If an HTTP request is received that contains an illegal header name or
|
|
+ * value (e.g. the header name is not a token) should the request be
|
|
+ * rejected (with a 400 response) or should the illegal header be ignored?
|
|
*
|
|
* @param rejectIllegalHeaderName {@code true} to reject requests with
|
|
- * illegal header names, {@code false} to
|
|
- * ignore the header
|
|
+ * illegal header names or values,
|
|
+ * {@code false} to ignore the header
|
|
+ *
|
|
+ * @deprecated Now an alias for {@link #setRejectIllegalHeader(boolean)}.
|
|
+ * Will be removed in Tomcat 10 onwards.
|
|
*/
|
|
+ @Deprecated
|
|
public void setRejectIllegalHeaderName(boolean rejectIllegalHeaderName) {
|
|
- this.rejectIllegalHeaderName = rejectIllegalHeaderName;
|
|
+ this.rejectIllegalHeader = rejectIllegalHeaderName;
|
|
}
|
|
|
|
|
|
diff --git a/java/org/apache/coyote/http11/Http11InputBuffer.java b/java/org/apache/coyote/http11/Http11InputBuffer.java
|
|
index 2dc7c17..57c670e 100644
|
|
--- a/java/org/apache/coyote/http11/Http11InputBuffer.java
|
|
+++ b/java/org/apache/coyote/http11/Http11InputBuffer.java
|
|
@@ -64,7 +64,7 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
|
|
private final MimeHeaders headers;
|
|
|
|
|
|
- private final boolean rejectIllegalHeaderName;
|
|
+ private final boolean rejectIllegalHeader;
|
|
|
|
/**
|
|
* State.
|
|
@@ -150,13 +150,13 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
|
|
// ----------------------------------------------------------- Constructors
|
|
|
|
public Http11InputBuffer(Request request, int headerBufferSize,
|
|
- boolean rejectIllegalHeaderName, HttpParser httpParser) {
|
|
+ boolean rejectIllegalHeader, HttpParser httpParser) {
|
|
|
|
this.request = request;
|
|
headers = request.getMimeHeaders();
|
|
|
|
this.headerBufferSize = headerBufferSize;
|
|
- this.rejectIllegalHeaderName = rejectIllegalHeaderName;
|
|
+ this.rejectIllegalHeader = rejectIllegalHeader;
|
|
this.httpParser = httpParser;
|
|
|
|
filterLibrary = new InputFilter[0];
|
|
@@ -752,6 +752,8 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
|
|
//
|
|
|
|
byte chr = 0;
|
|
+ byte prevChr = 0;
|
|
+
|
|
while (headerParsePos == HeaderParsePosition.HEADER_START) {
|
|
|
|
// Read new bytes if needed
|
|
@@ -762,17 +764,23 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
|
|
}
|
|
}
|
|
|
|
+ prevChr = chr;
|
|
chr = byteBuffer.get();
|
|
|
|
- if (chr == Constants.CR) {
|
|
- // Skip
|
|
- } else if (chr == Constants.LF) {
|
|
+ if (chr == Constants.CR && prevChr != Constants.CR) {
|
|
+ // Possible start of CRLF - process the next byte.
|
|
+ } else if (prevChr == Constants.CR && chr == Constants.LF) {
|
|
return HeaderParseStatus.DONE;
|
|
} else {
|
|
- byteBuffer.position(byteBuffer.position() - 1);
|
|
+ if (prevChr == 0) {
|
|
+ // Must have only read one byte
|
|
+ byteBuffer.position(byteBuffer.position() - 1);
|
|
+ } else {
|
|
+ // Must have read two bytes (first was CR, second was not LF)
|
|
+ byteBuffer.position(byteBuffer.position() - 2);
|
|
+ }
|
|
break;
|
|
}
|
|
-
|
|
}
|
|
|
|
if (headerParsePos == HeaderParsePosition.HEADER_START) {
|
|
@@ -868,11 +876,22 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
|
|
}
|
|
}
|
|
|
|
+ prevChr = chr;
|
|
chr = byteBuffer.get();
|
|
if (chr == Constants.CR) {
|
|
- // Skip
|
|
- } else if (chr == Constants.LF) {
|
|
+ // Possible start of CRLF - process the next byte.
|
|
+ } else if (prevChr == Constants.CR && chr == Constants.LF) {
|
|
eol = true;
|
|
+ } else if (prevChr == Constants.CR) {
|
|
+ // Invalid value
|
|
+ // Delete the header (it will be the most recent one)
|
|
+ headers.removeHeader(headers.size() - 1);
|
|
+ return skipLine();
|
|
+ } else if (chr != Constants.HT && HttpParser.isControl(chr)) {
|
|
+ // Invalid value
|
|
+ // Delete the header (it will be the most recent one)
|
|
+ headers.removeHeader(headers.size() - 1);
|
|
+ return skipLine();
|
|
} else if (chr == Constants.SP || chr == Constants.HT) {
|
|
byteBuffer.put(headerData.realPos, chr);
|
|
headerData.realPos++;
|
|
@@ -924,6 +943,9 @@ 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) {
|
|
|
|
@@ -935,21 +957,22 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
|
|
}
|
|
|
|
int pos = byteBuffer.position();
|
|
- byte chr = byteBuffer.get();
|
|
+ prevChr = chr;
|
|
+ chr = byteBuffer.get();
|
|
if (chr == Constants.CR) {
|
|
// Skip
|
|
- } else if (chr == Constants.LF) {
|
|
+ } else if (prevChr == Constants.CR && chr == Constants.LF) {
|
|
eol = true;
|
|
} else {
|
|
headerData.lastSignificantChar = pos;
|
|
}
|
|
}
|
|
- if (rejectIllegalHeaderName || log.isDebugEnabled()) {
|
|
+ if (rejectIllegalHeader || log.isDebugEnabled()) {
|
|
String message = sm.getString("iib.invalidheader",
|
|
new String(byteBuffer.array(), headerData.start,
|
|
headerData.lastSignificantChar - headerData.start + 1,
|
|
StandardCharsets.ISO_8859_1));
|
|
- if (rejectIllegalHeaderName) {
|
|
+ if (rejectIllegalHeader) {
|
|
throw new IllegalArgumentException(message);
|
|
}
|
|
log.debug(message);
|
|
diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java
|
|
index df002e0..86556ec 100644
|
|
--- a/java/org/apache/coyote/http11/Http11Processor.java
|
|
+++ b/java/org/apache/coyote/http11/Http11Processor.java
|
|
@@ -153,7 +153,7 @@ public class Http11Processor extends AbstractProcessor {
|
|
protocol.getRelaxedQueryChars());
|
|
|
|
inputBuffer = new Http11InputBuffer(request, protocol.getMaxHttpHeaderSize(),
|
|
- protocol.getRejectIllegalHeaderName(), httpParser);
|
|
+ protocol.getRejectIllegalHeader(), httpParser);
|
|
request.setInputBuffer(inputBuffer);
|
|
|
|
outputBuffer = new Http11OutputBuffer(response, protocol.getMaxHttpHeaderSize());
|
|
diff --git a/java/org/apache/tomcat/util/http/MimeHeaders.java b/java/org/apache/tomcat/util/http/MimeHeaders.java
|
|
index 59504ee..b76b274 100644
|
|
--- a/java/org/apache/tomcat/util/http/MimeHeaders.java
|
|
+++ b/java/org/apache/tomcat/util/http/MimeHeaders.java
|
|
@@ -396,7 +396,7 @@ public class MimeHeaders {
|
|
* reset and swap with last header
|
|
* @param idx the index of the header to remove.
|
|
*/
|
|
- private void removeHeader(int idx) {
|
|
+ public void removeHeader(int idx) {
|
|
MimeHeaderField mh = headers[idx];
|
|
|
|
mh.recycle();
|
|
diff --git a/java/org/apache/tomcat/util/http/parser/HttpParser.java b/java/org/apache/tomcat/util/http/parser/HttpParser.java
|
|
index 827f2c5..644b1d1 100644
|
|
--- a/java/org/apache/tomcat/util/http/parser/HttpParser.java
|
|
+++ b/java/org/apache/tomcat/util/http/parser/HttpParser.java
|
|
@@ -317,6 +317,17 @@ public class HttpParser {
|
|
}
|
|
|
|
|
|
+ public static boolean isControl(int c) {
|
|
+ // Fast for valid control characters, slower for some incorrect
|
|
+ // ones
|
|
+ try {
|
|
+ return IS_CONTROL[c];
|
|
+ } catch (ArrayIndexOutOfBoundsException ex) {
|
|
+ return false;
|
|
+ }
|
|
+ }
|
|
+
|
|
+
|
|
// Skip any LWS and position to read the next character. The next character
|
|
// is returned as being able to 'peek()' it allows a small optimisation in
|
|
// some cases.
|
|
diff --git a/test/org/apache/coyote/http11/TestHttp11InputBuffer.java b/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
|
|
index 131fa21..e60a474 100644
|
|
--- a/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
|
|
+++ b/test/org/apache/coyote/http11/TestHttp11InputBuffer.java
|
|
@@ -163,13 +163,13 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
|
|
|
|
|
|
@Test
|
|
- public void testBug51557Separators() throws Exception {
|
|
+ public void testBug51557SeparatorsInName() throws Exception {
|
|
char httpSeparators[] = new char[] {
|
|
'\t', ' ', '\"', '(', ')', ',', '/', ':', ';', '<',
|
|
'=', '>', '?', '@', '[', '\\', ']', '{', '}' };
|
|
|
|
for (char s : httpSeparators) {
|
|
- doTestBug51557Char(s);
|
|
+ doTestBug51557CharInName(s);
|
|
tearDown();
|
|
setUp();
|
|
}
|
|
@@ -177,13 +177,38 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
|
|
|
|
|
|
@Test
|
|
- public void testBug51557Ctl() throws Exception {
|
|
+ public void testBug51557CtlInName() throws Exception {
|
|
for (int i = 0; i < 31; i++) {
|
|
- doTestBug51557Char((char) i);
|
|
+ doTestBug51557CharInName((char) i);
|
|
+ tearDown();
|
|
+ setUp();
|
|
+ }
|
|
+ doTestBug51557CharInName((char) 127);
|
|
+ }
|
|
+
|
|
+
|
|
+ @Test
|
|
+ public void testBug51557CtlInValue() throws Exception {
|
|
+ for (int i = 0; i < 31; i++) {
|
|
+ if (i == '\t') {
|
|
+ // TAB is allowed
|
|
+ continue;
|
|
+ }
|
|
+ doTestBug51557InvalidCharInValue((char) i);
|
|
+ tearDown();
|
|
+ setUp();
|
|
+ }
|
|
+ doTestBug51557InvalidCharInValue((char) 127);
|
|
+ }
|
|
+
|
|
+
|
|
+ @Test
|
|
+ public void testBug51557ObsTextInValue() throws Exception {
|
|
+ for (int i = 128; i < 255; i++) {
|
|
+ doTestBug51557ValidCharInValue((char) i);
|
|
tearDown();
|
|
setUp();
|
|
}
|
|
- doTestBug51557Char((char) 127);
|
|
}
|
|
|
|
|
|
@@ -226,7 +251,7 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
|
|
}
|
|
|
|
|
|
- private void doTestBug51557Char(char s) {
|
|
+ private void doTestBug51557CharInName(char s) {
|
|
Bug51557Client client =
|
|
new Bug51557Client("X-Bug" + s + "51557", "invalid");
|
|
|
|
@@ -236,6 +261,29 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
|
|
Assert.assertTrue(client.isResponseBodyOK());
|
|
}
|
|
|
|
+
|
|
+ private void doTestBug51557InvalidCharInValue(char s) {
|
|
+ Bug51557Client client =
|
|
+ new Bug51557Client("X-Bug51557-Invalid", "invalid" + s + "invalid");
|
|
+
|
|
+ client.doRequest();
|
|
+ Assert.assertTrue("Testing [" + (int) s + "]", client.isResponse200());
|
|
+ Assert.assertEquals("Testing [" + (int) s + "]", "abcd", client.getResponseBody());
|
|
+ Assert.assertTrue(client.isResponseBodyOK());
|
|
+ }
|
|
+
|
|
+
|
|
+ private void doTestBug51557ValidCharInValue(char s) {
|
|
+ Bug51557Client client =
|
|
+ new Bug51557Client("X-Bug51557-Valid", "valid" + s + "valid");
|
|
+
|
|
+ client.doRequest();
|
|
+ Assert.assertTrue("Testing [" + (int) s + "]", client.isResponse200());
|
|
+ Assert.assertEquals("Testing [" + (int) s + "]", "valid" + s + "validabcd", client.getResponseBody());
|
|
+ Assert.assertTrue(client.isResponseBodyOK());
|
|
+ }
|
|
+
|
|
+
|
|
/**
|
|
* Bug 51557 test client.
|
|
*/
|
|
@@ -243,12 +291,12 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
|
|
|
|
private final String headerName;
|
|
private final String headerLine;
|
|
- private final boolean rejectIllegalHeaderName;
|
|
+ private final boolean rejectIllegalHeader;
|
|
|
|
public Bug51557Client(String headerName) {
|
|
this.headerName = headerName;
|
|
this.headerLine = headerName;
|
|
- this.rejectIllegalHeaderName = false;
|
|
+ this.rejectIllegalHeader = false;
|
|
}
|
|
|
|
public Bug51557Client(String headerName, String headerValue) {
|
|
@@ -256,10 +304,10 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
|
|
}
|
|
|
|
public Bug51557Client(String headerName, String headerValue,
|
|
- boolean rejectIllegalHeaderName) {
|
|
+ boolean rejectIllegalHeader) {
|
|
this.headerName = headerName;
|
|
this.headerLine = headerName + ": " + headerValue;
|
|
- this.rejectIllegalHeaderName = rejectIllegalHeaderName;
|
|
+ this.rejectIllegalHeader = rejectIllegalHeader;
|
|
}
|
|
|
|
private Exception doRequest() {
|
|
@@ -273,8 +321,8 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
|
|
|
|
try {
|
|
Connector connector = tomcat.getConnector();
|
|
- connector.setProperty("rejectIllegalHeaderName",
|
|
- Boolean.toString(rejectIllegalHeaderName));
|
|
+ connector.setProperty("rejectIllegalHeader",
|
|
+ Boolean.toString(rejectIllegalHeader));
|
|
tomcat.start();
|
|
setPort(connector.getLocalPort());
|
|
|
|
@@ -548,7 +596,7 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
|
|
|
|
try {
|
|
Connector connector = tomcat.getConnector();
|
|
- connector.setProperty("rejectIllegalHeaderName", "false");
|
|
+ connector.setProperty("rejectIllegalHeader", "false");
|
|
tomcat.start();
|
|
setPort(connector.getLocalPort());
|
|
|
|
diff --git a/webapps/docs/config/http.xml b/webapps/docs/config/http.xml
|
|
index ebb277d..3902c9a 100644
|
|
--- a/webapps/docs/config/http.xml
|
|
+++ b/webapps/docs/config/http.xml
|
|
@@ -551,14 +551,19 @@
|
|
expected concurrent requests (synchronous and asynchronous).</p>
|
|
</attribute>
|
|
|
|
- <attribute name="rejectIllegalHeaderName" required="false">
|
|
- <p>If an HTTP request is received that contains an illegal header name
|
|
- (i.e. the header name is not a token) this setting determines if the
|
|
+ <attribute name="rejectIllegalHeader" required="false">
|
|
+ <p>If an HTTP request is received that contains an illegal header name or
|
|
+ value (e.g. the header name is not a token) this setting determines if the
|
|
request will be rejected with a 400 response (<code>true</code>) or if the
|
|
illegal header be ignored (<code>false</code>). The default value is
|
|
<code>true</code> which will cause the request to be rejected.</p>
|
|
</attribute>
|
|
|
|
+ <attribute name="rejectIllegalHeaderName" required="false">
|
|
+ <p>This attribute is deprecated. It will be removed in Tomcat 10 onwards.
|
|
+ It is now an alias for <strong>rejectIllegalHeader</strong>.</p>
|
|
+ </attribute>
|
|
+
|
|
<attribute name="relaxedPathChars" required="false">
|
|
<p>The <a href="https://tools.ietf.org/rfc/rfc7230.txt">HTTP/1.1
|
|
specification</a> requires that certain characters are %nn encoded when
|
|
--
|
|
2.23.0
|
|
|