This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 8.5.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/8.5.x by this push: new 8fbe2e9 Stricter header value parsing 8fbe2e9 is described below commit 8fbe2e962f0ea138d92361921643fe5abe0c4f56 Author: Mark Thomas <ma...@apache.org> AuthorDate: Mon Jan 6 20:53:25 2020 +0000 Stricter header value parsing --- .../coyote/http11/AbstractHttp11Protocol.java | 51 +++++++++++---- .../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/changelog.xml | 5 ++ webapps/docs/config/http.xml | 11 +++- 8 files changed, 163 insertions(+), 42 deletions(-) diff --git a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java index 5332f9b..c94c1bd 100644 --- a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java +++ b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java @@ -145,27 +145,56 @@ public abstract class AbstractHttp11Protocol<S> extends AbstractProtocol<S> { } - private boolean rejectIllegalHeaderName = false; + private boolean rejectIllegalHeader = false; /** - * 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 ef0b498..daecc72 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]; @@ -760,6 +760,8 @@ public class Http11InputBuffer implements InputBuffer, ApplicationBufferHandler // byte chr = 0; + byte prevChr = 0; + while (headerParsePos == HeaderParsePosition.HEADER_START) { // Read new bytes if needed @@ -770,17 +772,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) { @@ -877,11 +885,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++; @@ -933,6 +952,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) { @@ -944,20 +966,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 7091f49..763ac25 100644 --- a/java/org/apache/coyote/http11/Http11Processor.java +++ b/java/org/apache/coyote/http11/Http11Processor.java @@ -188,7 +188,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 b089b03..7258955 100644 --- a/java/org/apache/tomcat/util/http/parser/HttpParser.java +++ b/java/org/apache/tomcat/util/http/parser/HttpParser.java @@ -337,6 +337,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 f725868..73c7b03 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/changelog.xml b/webapps/docs/changelog.xml index 5ee7d88..b470b42 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -174,6 +174,11 @@ the <code>Transfer-Encoding</code> header were ignored rather than treated as an error. (markt) </fix> + <fix> + Rename the HTTP Connector attribute <code>rejectIllegalHeaderName</code> + to <code>rejectIllegalHeader</code> and expand the underlying + implementation to include header values as well as names. (markt) + </fix> </changelog> </subsection> <subsection name="Jasper"> diff --git a/webapps/docs/config/http.xml b/webapps/docs/config/http.xml index d8e6932..1f9c23a 100644 --- a/webapps/docs/config/http.xml +++ b/webapps/docs/config/http.xml @@ -575,15 +575,20 @@ 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>false</code> which will cause the request to be processed but the illegal header will be ignored.</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 --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org