This is an automated email from the ASF dual-hosted git repository. markt pushed a commit to branch 7.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/7.0.x by this push: new 702bf15 Stricter header value parsing 702bf15 is described below commit 702bf15bea292915684d931526d95d4990b2e73d 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/AbstractInputBuffer.java | 5 + .../apache/coyote/http11/Http11AprProcessor.java | 4 +- .../apache/coyote/http11/Http11AprProtocol.java | 2 +- .../apache/coyote/http11/Http11NioProcessor.java | 4 +- .../apache/coyote/http11/Http11NioProtocol.java | 2 +- java/org/apache/coyote/http11/Http11Processor.java | 4 +- java/org/apache/coyote/http11/Http11Protocol.java | 2 +- .../coyote/http11/InternalAprInputBuffer.java | 50 ++++-- .../apache/coyote/http11/InternalInputBuffer.java | 54 +++++-- .../coyote/http11/InternalNioInputBuffer.java | 43 ++++-- java/org/apache/tomcat/util/http/MimeHeaders.java | 2 +- .../apache/tomcat/util/http/parser/HttpParser.java | 11 ++ .../coyote/http11/TestInternalInputBuffer.java | 167 +++++++++++++++++++-- webapps/docs/changelog.xml | 5 + webapps/docs/config/http.xml | 11 +- 16 files changed, 345 insertions(+), 72 deletions(-) diff --git a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java index 8009380..632760c 100644 --- a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java +++ b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java @@ -83,27 +83,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/AbstractInputBuffer.java b/java/org/apache/coyote/http11/AbstractInputBuffer.java index 5ac9f11..379fab5 100644 --- a/java/org/apache/coyote/http11/AbstractInputBuffer.java +++ b/java/org/apache/coyote/http11/AbstractInputBuffer.java @@ -108,6 +108,11 @@ public abstract class AbstractInputBuffer<S> implements InputBuffer{ protected int lastActiveFilter; + /** + * Do HTTP headers with illegal names and/or values cause the request to be + * rejected? Note that the field name is not quite right but cannot be + * easily changed without breaking binary compatibility. + */ protected boolean rejectIllegalHeaderName; diff --git a/java/org/apache/coyote/http11/Http11AprProcessor.java b/java/org/apache/coyote/http11/Http11AprProcessor.java index cda37e1..2281bc1 100644 --- a/java/org/apache/coyote/http11/Http11AprProcessor.java +++ b/java/org/apache/coyote/http11/Http11AprProcessor.java @@ -61,7 +61,7 @@ public class Http11AprProcessor extends AbstractHttp11Processor<Long> { // ----------------------------------------------------------- Constructors - public Http11AprProcessor(int headerBufferSize, boolean rejectIllegalHeaderName, + public Http11AprProcessor(int headerBufferSize, boolean rejectIllegalHeader, AprEndpoint endpoint, int maxTrailerSize, Set<String> allowedTrailerHeaders, int maxExtensionSize, int maxSwallowSize, String relaxedPathChars, String relaxedQueryChars) { @@ -70,7 +70,7 @@ public class Http11AprProcessor extends AbstractHttp11Processor<Long> { httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars); - inputBuffer = new InternalAprInputBuffer(request, headerBufferSize, rejectIllegalHeaderName, + inputBuffer = new InternalAprInputBuffer(request, headerBufferSize, rejectIllegalHeader, httpParser); request.setInputBuffer(inputBuffer); diff --git a/java/org/apache/coyote/http11/Http11AprProtocol.java b/java/org/apache/coyote/http11/Http11AprProtocol.java index a5495d8..4afeac5 100644 --- a/java/org/apache/coyote/http11/Http11AprProtocol.java +++ b/java/org/apache/coyote/http11/Http11AprProtocol.java @@ -299,7 +299,7 @@ public class Http11AprProtocol extends AbstractHttp11Protocol<Long> { @Override protected Http11AprProcessor createProcessor() { Http11AprProcessor processor = new Http11AprProcessor( - proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeaderName(), + proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(), (AprEndpoint)proto.endpoint, proto.getMaxTrailerSize(), proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(), proto.getMaxSwallowSize(), proto.getRelaxedPathChars(), diff --git a/java/org/apache/coyote/http11/Http11NioProcessor.java b/java/org/apache/coyote/http11/Http11NioProcessor.java index 7419531..8cc5648 100644 --- a/java/org/apache/coyote/http11/Http11NioProcessor.java +++ b/java/org/apache/coyote/http11/Http11NioProcessor.java @@ -66,7 +66,7 @@ public class Http11NioProcessor extends AbstractHttp11Processor<NioChannel> { // ----------------------------------------------------------- Constructors - public Http11NioProcessor(int maxHttpHeaderSize, boolean rejectIllegalHeaderName, + public Http11NioProcessor(int maxHttpHeaderSize, boolean rejectIllegalHeader, NioEndpoint endpoint, int maxTrailerSize, Set<String> allowedTrailerHeaders, int maxExtensionSize, int maxSwallowSize, String relaxedPathChars, String relaxedQueryChars) { @@ -76,7 +76,7 @@ public class Http11NioProcessor extends AbstractHttp11Processor<NioChannel> { httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars); inputBuffer = new InternalNioInputBuffer(request, maxHttpHeaderSize, - rejectIllegalHeaderName, httpParser); + rejectIllegalHeader, httpParser); request.setInputBuffer(inputBuffer); outputBuffer = new InternalNioOutputBuffer(response, maxHttpHeaderSize); diff --git a/java/org/apache/coyote/http11/Http11NioProtocol.java b/java/org/apache/coyote/http11/Http11NioProtocol.java index fa51a3b..c61f3a0 100644 --- a/java/org/apache/coyote/http11/Http11NioProtocol.java +++ b/java/org/apache/coyote/http11/Http11NioProtocol.java @@ -264,7 +264,7 @@ public class Http11NioProtocol extends AbstractHttp11JsseProtocol<NioChannel> { @Override public Http11NioProcessor createProcessor() { Http11NioProcessor processor = new Http11NioProcessor( - proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeaderName(), + proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(), (NioEndpoint)proto.endpoint, proto.getMaxTrailerSize(), proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(), proto.getMaxSwallowSize(), proto.getRelaxedPathChars(), diff --git a/java/org/apache/coyote/http11/Http11Processor.java b/java/org/apache/coyote/http11/Http11Processor.java index 0ed0e1e..de6b8f3 100644 --- a/java/org/apache/coyote/http11/Http11Processor.java +++ b/java/org/apache/coyote/http11/Http11Processor.java @@ -51,7 +51,7 @@ public class Http11Processor extends AbstractHttp11Processor<Socket> { // ------------------------------------------------------------ Constructor - public Http11Processor(int headerBufferSize, boolean rejectIllegalHeaderName, + public Http11Processor(int headerBufferSize, boolean rejectIllegalHeader, JIoEndpoint endpoint, int maxTrailerSize, Set<String> allowedTrailerHeaders, int maxExtensionSize, int maxSwallowSize, String relaxedPathChars, String relaxedQueryChars) { @@ -60,7 +60,7 @@ public class Http11Processor extends AbstractHttp11Processor<Socket> { httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars); - inputBuffer = new InternalInputBuffer(request, headerBufferSize, rejectIllegalHeaderName, + inputBuffer = new InternalInputBuffer(request, headerBufferSize, rejectIllegalHeader, httpParser); request.setInputBuffer(inputBuffer); diff --git a/java/org/apache/coyote/http11/Http11Protocol.java b/java/org/apache/coyote/http11/Http11Protocol.java index 54a2ee3..2e59197 100644 --- a/java/org/apache/coyote/http11/Http11Protocol.java +++ b/java/org/apache/coyote/http11/Http11Protocol.java @@ -163,7 +163,7 @@ public class Http11Protocol extends AbstractHttp11JsseProtocol<Socket> { @Override protected Http11Processor createProcessor() { Http11Processor processor = new Http11Processor( - proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeaderName(), + proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(), (JIoEndpoint)proto.endpoint, proto.getMaxTrailerSize(), proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(), proto.getMaxSwallowSize(), proto.getRelaxedPathChars(), diff --git a/java/org/apache/coyote/http11/InternalAprInputBuffer.java b/java/org/apache/coyote/http11/InternalAprInputBuffer.java index da2846b..72850ba 100644 --- a/java/org/apache/coyote/http11/InternalAprInputBuffer.java +++ b/java/org/apache/coyote/http11/InternalAprInputBuffer.java @@ -52,7 +52,7 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { * Alternate constructor. */ public InternalAprInputBuffer(Request request, int headerBufferSize, - boolean rejectIllegalHeaderName, HttpParser httpParser) { + boolean rejectIllegalHeader, HttpParser httpParser) { this.request = request; headers = request.getMimeHeaders(); @@ -64,7 +64,7 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { bbuf = ByteBuffer.allocateDirect((headerBufferSize / 1500 + 1) * 1500); } - this.rejectIllegalHeaderName = rejectIllegalHeaderName; + this.rejectIllegalHeaderName = rejectIllegalHeader; this.httpParser = httpParser; inputStreamInputBuffer = new SocketInputBuffer(); @@ -351,6 +351,8 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { // byte chr = 0; + byte prevChr = 0; + while (true) { // Read new bytes if needed @@ -359,19 +361,23 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { throw new EOFException(sm.getString("iib.eof.error")); } + prevChr = chr; chr = buf[pos]; - if (chr == Constants.CR) { - // Skip - } else if (chr == Constants.LF) { - pos++; + if (chr == Constants.CR && prevChr != Constants.CR) { + // Possible start of CRLF - process the next byte. + } else if (prevChr == Constants.CR && chr == Constants.LF) { + pos++; return false; } else { + if (prevChr == Constants.CR) { + // Must have read two bytes (first was CR, second was not LF) + pos--; + } break; } pos++; - } // Mark the current buffer position @@ -456,10 +462,24 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { throw new EOFException(sm.getString("iib.eof.error")); } - if (buf[pos] == Constants.CR) { - // Skip - } else if (buf[pos] == Constants.LF) { + prevChr = chr; + chr = buf[pos]; + if (chr == Constants.CR) { + // 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); + skipLine(lineStart, start); + return true; + } 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); + skipLine(lineStart, start); + return true; } else if (buf[pos] == Constants.SP) { buf[realPos] = buf[pos]; realPos++; @@ -512,6 +532,9 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { lastRealByte = pos - 1; } + byte chr = 0; + byte prevChr = 0; + while (!eol) { // Read new bytes if needed @@ -520,9 +543,12 @@ public class InternalAprInputBuffer extends AbstractInputBuffer<Long> { throw new EOFException(sm.getString("iib.eof.error")); } - if (buf[pos] == Constants.CR) { + prevChr = chr; + chr = buf[pos]; + + if (chr == Constants.CR) { // Skip - } else if (buf[pos] == Constants.LF) { + } else if (prevChr == Constants.CR && chr == Constants.LF) { eol = true; } else { lastRealByte = pos; diff --git a/java/org/apache/coyote/http11/InternalInputBuffer.java b/java/org/apache/coyote/http11/InternalInputBuffer.java index 3d194e2..d1f9e48 100644 --- a/java/org/apache/coyote/http11/InternalInputBuffer.java +++ b/java/org/apache/coyote/http11/InternalInputBuffer.java @@ -53,14 +53,14 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { * Default constructor. */ public InternalInputBuffer(Request request, int headerBufferSize, - boolean rejectIllegalHeaderName, HttpParser httpParser) { + boolean rejectIllegalHeader, HttpParser httpParser) { this.request = request; headers = request.getMimeHeaders(); buf = new byte[headerBufferSize]; - this.rejectIllegalHeaderName = rejectIllegalHeaderName; + this.rejectIllegalHeaderName = rejectIllegalHeader; this.httpParser = httpParser; inputStreamInputBuffer = new InputStreamInputBuffer(); @@ -303,6 +303,8 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { // byte chr = 0; + byte prevChr = 0; + while (true) { // Read new bytes if needed @@ -311,19 +313,23 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { throw new EOFException(sm.getString("iib.eof.error")); } + prevChr = chr; chr = buf[pos]; - 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) { pos++; return false; } else { + if (prevChr == Constants.CR) { + // Must have read two bytes (first was CR, second was not LF) + pos--; + } break; } pos++; - } // Mark the current buffer position @@ -409,15 +415,29 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { throw new EOFException(sm.getString("iib.eof.error")); } - if (buf[pos] == Constants.CR) { - // Skip - } else if (buf[pos] == Constants.LF) { + prevChr = chr; + chr = buf[pos]; + if (chr == Constants.CR) { + // Possible start of CRLF - process the next byte. + } else if (prevChr == Constants.CR && chr == Constants.LF) { eol = true; - } else if (buf[pos] == Constants.SP) { - buf[realPos] = buf[pos]; + } else if (prevChr == Constants.CR) { + // Invalid value + // Delete the header (it will be the most recent one) + headers.removeHeader(headers.size() - 1); + skipLine(lineStart, start); + return true; + } 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); + skipLine(lineStart, start); + return true; + } else if (chr == Constants.SP) { + buf[realPos] = chr; realPos++; } else { - buf[realPos] = buf[pos]; + buf[realPos] = chr; realPos++; lastSignificantChar = realPos; } @@ -483,6 +503,9 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { lastRealByte = pos - 1; } + byte chr = 0; + byte prevChr = 0; + while (!eol) { // Read new bytes if needed @@ -491,9 +514,12 @@ public class InternalInputBuffer extends AbstractInputBuffer<Socket> { throw new EOFException(sm.getString("iib.eof.error")); } - if (buf[pos] == Constants.CR) { + prevChr = chr; + chr = buf[pos]; + + if (chr == Constants.CR) { // Skip - } else if (buf[pos] == Constants.LF) { + } else if (prevChr == Constants.CR && chr == Constants.LF) { eol = true; } else { lastRealByte = pos; diff --git a/java/org/apache/coyote/http11/InternalNioInputBuffer.java b/java/org/apache/coyote/http11/InternalNioInputBuffer.java index ee241b1..fba00a1 100644 --- a/java/org/apache/coyote/http11/InternalNioInputBuffer.java +++ b/java/org/apache/coyote/http11/InternalNioInputBuffer.java @@ -100,13 +100,13 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { * Alternate constructor. */ public InternalNioInputBuffer(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.rejectIllegalHeaderName = rejectIllegalHeader; this.httpParser = httpParser; inputStreamInputBuffer = new SocketInputBuffer(); @@ -515,6 +515,8 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { // byte chr = 0; + byte prevChr = 0; + while (headerParsePos == HeaderParsePosition.HEADER_START) { // Read new bytes if needed @@ -525,19 +527,23 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { } } + prevChr = chr; chr = buf[pos]; - 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) { pos++; return HeaderParseStatus.DONE; } else { + if (prevChr == Constants.CR) { + // Must have read two bytes (first was CR, second was not LF) + pos--; + } break; } pos++; - } if ( headerParsePos == HeaderParsePosition.HEADER_START ) { @@ -633,11 +639,22 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { } } + prevChr = chr; chr = buf[pos]; 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) { buf[headerData.realPos] = chr; headerData.realPos++; @@ -695,6 +712,9 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { headerParsePos = HeaderParsePosition.HEADER_SKIPLINE; boolean eol = false; + byte chr = 0; + byte prevChr = 0; + // Reading bytes until the end of the line while (!eol) { @@ -705,9 +725,12 @@ public class InternalNioInputBuffer extends AbstractInputBuffer<NioChannel> { } } - if (buf[pos] == Constants.CR) { + prevChr = chr; + chr = buf[pos]; + + if (chr == Constants.CR) { // Skip - } else if (buf[pos] == Constants.LF) { + } else if (prevChr == Constants.CR && chr == Constants.LF) { eol = true; } else { headerData.lastSignificantChar = pos; diff --git a/java/org/apache/tomcat/util/http/MimeHeaders.java b/java/org/apache/tomcat/util/http/MimeHeaders.java index f282a4d..ca6e295 100644 --- a/java/org/apache/tomcat/util/http/MimeHeaders.java +++ b/java/org/apache/tomcat/util/http/MimeHeaders.java @@ -393,7 +393,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 7c322f3..962b5f7 100644 --- a/java/org/apache/tomcat/util/http/parser/HttpParser.java +++ b/java/org/apache/tomcat/util/http/parser/HttpParser.java @@ -505,6 +505,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/TestInternalInputBuffer.java b/test/org/apache/coyote/http11/TestInternalInputBuffer.java index 401091b..24005fe 100644 --- a/test/org/apache/coyote/http11/TestInternalInputBuffer.java +++ b/test/org/apache/coyote/http11/TestInternalInputBuffer.java @@ -163,13 +163,13 @@ public class TestInternalInputBuffer 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 TestInternalInputBuffer 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,33 @@ public class TestInternalInputBuffer extends TomcatBaseTest { } - private void doTestBug51557Char(char s) { + @Test + public void testBug51557CRStartName() { + + Bug51557Client client = new Bug51557Client("\rName=", + "invalid"); + + client.doRequest(); + Assert.assertTrue(client.isResponse200()); + Assert.assertEquals("abcd", client.getResponseBody()); + Assert.assertTrue(client.isResponseBodyOK()); + } + + + @Test + public void testBug51557CR2StartName() { + + Bug51557Client client = new Bug51557Client("\r\rName=", + "invalid"); + + client.doRequest(); + Assert.assertTrue(client.isResponse200()); + Assert.assertEquals("abcd", client.getResponseBody()); + Assert.assertTrue(client.isResponseBodyOK()); + } + + + private void doTestBug51557CharInName(char s) { Bug51557Client client = new Bug51557Client("X-Bug" + s + "51557", "invalid"); @@ -236,6 +287,29 @@ public class TestInternalInputBuffer 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 +317,12 @@ public class TestInternalInputBuffer 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 +330,10 @@ public class TestInternalInputBuffer 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 +347,8 @@ public class TestInternalInputBuffer extends TomcatBaseTest { try { Connector connector = tomcat.getConnector(); - connector.setProperty("rejectIllegalHeaderName", - Boolean.toString(rejectIllegalHeaderName)); + Assert.assertTrue(connector.setProperty( + "rejectIllegalHeader", Boolean.toString(rejectIllegalHeader))); tomcat.start(); setPort(connector.getLocalPort()); @@ -515,6 +589,75 @@ public class TestInternalInputBuffer extends TomcatBaseTest { } + /** + * Test case for https://bz.apache.org/bugzilla/show_bug.cgi?id=59089 + */ + @Test + public void testBug59089() { + + Bug59089Client client = new Bug59089Client(); + + client.doRequest(); + Assert.assertTrue(client.isResponse200()); + Assert.assertTrue(client.isResponseBodyOK()); + } + + + /** + * Bug 59089 test client. + */ + private class Bug59089Client extends SimpleHttpClient { + + private Exception doRequest() { + + // Ensure body is read correctly + setUseContentLength(true); + + Tomcat tomcat = getTomcatInstance(); + + Context root = tomcat.addContext("", TEMP_DIR); + Tomcat.addServlet(root, "Bug59089", new TesterServlet()); + root.addServletMapping("/test", "Bug59089"); + + try { + Connector connector = tomcat.getConnector(); + Assert.assertTrue(connector.setProperty("rejectIllegalHeader", "false")); + tomcat.start(); + setPort(connector.getLocalPort()); + + // Open connection + connect(); + + String[] request = new String[1]; + request[0] = "GET http://localhost:8080/test HTTP/1.1" + CRLF + + "Host: localhost:8080" + CRLF + + "X-Header: Ignore" + CRLF + + "X-Header" + (char) 130 + ": Broken" + CRLF + CRLF; + + setRequest(request); + processRequest(); // blocks until response has been read + + // Close the connection + disconnect(); + } catch (Exception e) { + return e; + } + return null; + } + + @Override + public boolean isResponseBodyOK() { + if (getResponseBody() == null) { + return false; + } + if (!getResponseBody().contains("OK")) { + return false; + } + return true; + } + } + + @Test public void testInvalidMethod() { diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index 99d7e83..aaf2f39 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -134,6 +134,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 f6cb6bc..9a95902 100644 --- a/webapps/docs/config/http.xml +++ b/webapps/docs/config/http.xml @@ -540,15 +540,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