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 2020/02/04 14:51:22 UTC
[tomcat] branch master updated: Stricter header value parsing
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/master by this push:
new ae8c82e Stricter header value parsing
ae8c82e is described below
commit ae8c82eff96990878e79691819ae941538ee62fd
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Mon Jan 6 20:53:25 2020 +0000
Stricter header value parsing
---
.../coyote/http11/AbstractHttp11Protocol.java | 26 ++++----
.../apache/coyote/http11/Http11InputBuffer.java | 51 ++++++++++-----
java/org/apache/coyote/http11/Http11Processor.java | 2 +-
java/org/apache/tomcat/util/http/MimeHeaders.java | 2 +-
.../apache/tomcat/util/http/parser/HttpParser.java | 11 ++++
.../coyote/http11/TestHttp11InputBuffer.java | 72 ++++++++++++++++++----
webapps/docs/config/http.xml | 6 +-
7 files changed, 126 insertions(+), 44 deletions(-)
diff --git a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
index fd3ab74..3aecff6 100644
--- a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
+++ b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
@@ -145,27 +145,27 @@ 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 rejectIllegalHeaderName {@code true} to reject requests with
- * illegal header names, {@code false} to
- * ignore the header
+ * @param rejectIllegalHeader {@code true} to reject requests with illegal
+ * header names or values, {@code false} to
+ * ignore the header
*/
- public void setRejectIllegalHeaderName(boolean rejectIllegalHeaderName) {
- this.rejectIllegalHeaderName = rejectIllegalHeaderName;
+ public void setRejectIllegalHeader(boolean rejectIllegalHeader) {
+ this.rejectIllegalHeader = rejectIllegalHeader;
}
diff --git a/java/org/apache/coyote/http11/Http11InputBuffer.java b/java/org/apache/coyote/http11/Http11InputBuffer.java
index 7eb0669..04543ef 100644
--- a/java/org/apache/coyote/http11/Http11InputBuffer.java
+++ b/java/org/apache/coyote/http11/Http11InputBuffer.java
@@ -66,7 +66,7 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
private final MimeHeaders headers;
- private final boolean rejectIllegalHeaderName;
+ private final boolean rejectIllegalHeader;
/**
* State.
@@ -152,13 +152,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];
@@ -762,6 +762,8 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler
//
byte chr = 0;
+ byte prevChr = 0;
+
while (headerParsePos == HeaderParsePosition.HEADER_START) {
// Read new bytes if needed
@@ -772,17 +774,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) {
@@ -879,11 +887,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++;
@@ -935,6 +954,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) {
@@ -946,20 +968,21 @@ 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",
HeaderUtil.toPrintableString(byteBuffer.array(), headerData.lineStart,
headerData.lastSignificantChar - headerData.lineStart + 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 a365235..f6d0c6e 100644
--- a/java/org/apache/coyote/http11/Http11Processor.java
+++ b/java/org/apache/coyote/http11/Http11Processor.java
@@ -156,7 +156,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 a6aa684..113412c 100644
--- a/java/org/apache/tomcat/util/http/MimeHeaders.java
+++ b/java/org/apache/tomcat/util/http/MimeHeaders.java
@@ -395,7 +395,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 989be63..b06d468 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 61d6d60..2fbb846 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() {
@@ -274,7 +322,7 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
try {
Connector connector = tomcat.getConnector();
Assert.assertTrue(connector.setProperty(
- "rejectIllegalHeaderName", Boolean.toString(rejectIllegalHeaderName)));
+ "rejectIllegalHeader", Boolean.toString(rejectIllegalHeader)));
tomcat.start();
setPort(connector.getLocalPort());
@@ -548,7 +596,7 @@ public class TestHttp11InputBuffer extends TomcatBaseTest {
try {
Connector connector = tomcat.getConnector();
- Assert.assertTrue(connector.setProperty("rejectIllegalHeaderName", "false"));
+ Assert.assertTrue(connector.setProperty("rejectIllegalHeader", "false"));
tomcat.start();
setPort(connector.getLocalPort());
diff --git a/webapps/docs/config/http.xml b/webapps/docs/config/http.xml
index 54fbd42..ca46da7 100644
--- a/webapps/docs/config/http.xml
+++ b/webapps/docs/config/http.xml
@@ -556,9 +556,9 @@
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>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org