diff -up ./java/org/apache/coyote/http11/AbstractHttp11Protocol.java.orig ./java/org/apache/coyote/http11/AbstractHttp11Protocol.java --- ./java/org/apache/coyote/http11/AbstractHttp11Protocol.java.orig 2020-04-24 14:57:33.400591691 -0400 +++ ./java/org/apache/coyote/http11/AbstractHttp11Protocol.java 2020-04-24 14:28:55.936278683 -0400 @@ -62,6 +62,59 @@ public abstract class AbstractHttp11Prot this.relaxedQueryChars = relaxedQueryChars; } + private boolean rejectIllegalHeader = false; + /** + * 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 getRejectIllegalHeader() { 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 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 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.rejectIllegalHeader = rejectIllegalHeaderName; + } + + private int socketBuffer = 9000; public int getSocketBuffer() { return socketBuffer; } public void setSocketBuffer(int socketBuffer) { diff -up ./java/org/apache/coyote/http11/AbstractInputBuffer.java.orig ./java/org/apache/coyote/http11/AbstractInputBuffer.java --- ./java/org/apache/coyote/http11/AbstractInputBuffer.java.orig 2020-04-24 14:57:33.402591687 -0400 +++ ./java/org/apache/coyote/http11/AbstractInputBuffer.java 2020-04-24 14:28:55.936278683 -0400 @@ -34,7 +34,6 @@ public abstract class AbstractInputBuffe */ protected static final StringManager sm = StringManager.getManager(Constants.Package); - /** * Associated Coyote request. */ @@ -112,6 +111,14 @@ public abstract class AbstractInputBuffe protected HttpParser httpParser; + /** + * 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; + + // ------------------------------------------------------------- Properties diff -up ./java/org/apache/coyote/http11/Http11AprProcessor.java.orig ./java/org/apache/coyote/http11/Http11AprProcessor.java --- ./java/org/apache/coyote/http11/Http11AprProcessor.java.orig 2020-04-24 14:57:33.387591718 -0400 +++ ./java/org/apache/coyote/http11/Http11AprProcessor.java 2020-04-24 14:46:07.375029423 -0400 @@ -61,8 +61,8 @@ public class Http11AprProcessor extends // ----------------------------------------------------------- Constructors - public Http11AprProcessor(int headerBufferSize, AprEndpoint endpoint, int maxTrailerSize, - Set allowedTrailerHeaders, + public Http11AprProcessor(int headerBufferSize, boolean rejectIllegalHeader, + AprEndpoint endpoint, int maxTrailerSize, Set allowedTrailerHeaders, int maxExtensionSize, int maxSwallowSize, String relaxedPathChars, String relaxedQueryChars) { @@ -70,7 +70,8 @@ public class Http11AprProcessor extends httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars); - inputBuffer = new InternalAprInputBuffer(request, headerBufferSize, httpParser); + inputBuffer = new InternalAprInputBuffer(request, headerBufferSize, rejectIllegalHeader, + httpParser); request.setInputBuffer(inputBuffer); outputBuffer = new InternalAprOutputBuffer(response, headerBufferSize); diff -up ./java/org/apache/coyote/http11/Http11AprProtocol.java.orig ./java/org/apache/coyote/http11/Http11AprProtocol.java --- ./java/org/apache/coyote/http11/Http11AprProtocol.java.orig 2020-04-24 14:57:33.393591706 -0400 +++ ./java/org/apache/coyote/http11/Http11AprProtocol.java 2020-04-24 14:45:37.327092381 -0400 @@ -299,9 +299,9 @@ public class Http11AprProtocol extends A @Override protected Http11AprProcessor createProcessor() { Http11AprProcessor processor = new Http11AprProcessor( - proto.getMaxHttpHeaderSize(), (AprEndpoint)proto.endpoint, - proto.getMaxTrailerSize(), proto.getAllowedTrailerHeadersAsSet(), - proto.getMaxExtensionSize(), + proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(), + (AprEndpoint)proto.endpoint, proto.getMaxTrailerSize(), + proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(), proto.getMaxSwallowSize(), proto.getRelaxedPathChars(), proto.getRelaxedQueryChars()); processor.setAdapter(proto.adapter); diff -up ./java/org/apache/coyote/http11/Http11NioProcessor.java.orig ./java/org/apache/coyote/http11/Http11NioProcessor.java --- ./java/org/apache/coyote/http11/Http11NioProcessor.java.orig 2020-04-24 14:57:33.399591693 -0400 +++ ./java/org/apache/coyote/http11/Http11NioProcessor.java 2020-04-24 14:45:24.452119361 -0400 @@ -66,8 +66,8 @@ public class Http11NioProcessor extends // ----------------------------------------------------------- Constructors - public Http11NioProcessor(int maxHttpHeaderSize, NioEndpoint endpoint, int maxTrailerSize, - Set allowedTrailerHeaders, + public Http11NioProcessor(int maxHttpHeaderSize, boolean rejectIllegalHeader, + NioEndpoint endpoint, int maxTrailerSize, Set allowedTrailerHeaders, int maxExtensionSize, int maxSwallowSize, String relaxedPathChars, String relaxedQueryChars) { @@ -75,7 +75,8 @@ public class Http11NioProcessor extends httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars); - inputBuffer = new InternalNioInputBuffer(request, maxHttpHeaderSize, httpParser); + inputBuffer = new InternalNioInputBuffer(request, maxHttpHeaderSize, + rejectIllegalHeader, httpParser); request.setInputBuffer(inputBuffer); outputBuffer = new InternalNioOutputBuffer(response, maxHttpHeaderSize); diff -up ./java/org/apache/coyote/http11/Http11NioProtocol.java.orig ./java/org/apache/coyote/http11/Http11NioProtocol.java --- ./java/org/apache/coyote/http11/Http11NioProtocol.java.orig 2020-04-24 14:57:33.392591708 -0400 +++ ./java/org/apache/coyote/http11/Http11NioProtocol.java 2020-04-24 14:28:55.937278681 -0400 @@ -264,9 +264,9 @@ public class Http11NioProtocol extends A @Override public Http11NioProcessor createProcessor() { Http11NioProcessor processor = new Http11NioProcessor( - proto.getMaxHttpHeaderSize(), (NioEndpoint)proto.endpoint, - proto.getMaxTrailerSize(), proto.getAllowedTrailerHeadersAsSet(), - proto.getMaxExtensionSize(), + proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(), + (NioEndpoint)proto.endpoint, proto.getMaxTrailerSize(), + proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(), proto.getMaxSwallowSize(), proto.getRelaxedPathChars(), proto.getRelaxedQueryChars()); processor.setAdapter(proto.adapter); diff -up ./java/org/apache/coyote/http11/Http11Processor.java.orig ./java/org/apache/coyote/http11/Http11Processor.java --- ./java/org/apache/coyote/http11/Http11Processor.java.orig 2020-04-24 14:57:33.390591712 -0400 +++ ./java/org/apache/coyote/http11/Http11Processor.java 2020-04-24 15:09:20.651109515 -0400 @@ -51,8 +51,8 @@ public class Http11Processor extends Abs // ------------------------------------------------------------ Constructor - public Http11Processor(int headerBufferSize, JIoEndpoint endpoint, int maxTrailerSize, - Set allowedTrailerHeaders, + public Http11Processor(int headerBufferSize, boolean rejectIllegalHeader, + JIoEndpoint endpoint, int maxTrailerSize, Set allowedTrailerHeaders, int maxExtensionSize, int maxSwallowSize, String relaxedPathChars, String relaxedQueryChars) { @@ -60,7 +60,7 @@ public class Http11Processor extends Abs httpParser = new HttpParser(relaxedPathChars, relaxedQueryChars); - inputBuffer = new InternalInputBuffer(request, headerBufferSize, httpParser); + inputBuffer = new InternalInputBuffer(request, headerBufferSize, rejectIllegalHeader, httpParser); request.setInputBuffer(inputBuffer); diff -up ./java/org/apache/coyote/http11/Http11Protocol.java.orig ./java/org/apache/coyote/http11/Http11Protocol.java --- ./java/org/apache/coyote/http11/Http11Protocol.java.orig 2020-04-24 14:57:33.395591701 -0400 +++ ./java/org/apache/coyote/http11/Http11Protocol.java 2020-04-24 14:45:08.641152489 -0400 @@ -163,9 +163,9 @@ public class Http11Protocol extends Abst @Override protected Http11Processor createProcessor() { Http11Processor processor = new Http11Processor( - proto.getMaxHttpHeaderSize(), (JIoEndpoint)proto.endpoint, - proto.getMaxTrailerSize(), proto.getAllowedTrailerHeadersAsSet(), - proto.getMaxExtensionSize(), + proto.getMaxHttpHeaderSize(), proto.getRejectIllegalHeader(), + (JIoEndpoint)proto.endpoint, proto.getMaxTrailerSize(), + proto.getAllowedTrailerHeadersAsSet(), proto.getMaxExtensionSize(), proto.getMaxSwallowSize(), proto.getRelaxedPathChars(), proto.getRelaxedQueryChars()); processor.setAdapter(proto.adapter); diff -up ./java/org/apache/coyote/http11/InternalAprInputBuffer.java.orig ./java/org/apache/coyote/http11/InternalAprInputBuffer.java --- ./java/org/apache/coyote/http11/InternalAprInputBuffer.java.orig 2020-04-24 14:57:33.398591695 -0400 +++ ./java/org/apache/coyote/http11/InternalAprInputBuffer.java 2020-04-24 15:08:35.338204464 -0400 @@ -52,7 +52,7 @@ public class InternalAprInputBuffer exte * Alternate constructor. */ public InternalAprInputBuffer(Request request, int headerBufferSize, - HttpParser httpParser) { + boolean rejectIllegalHeader, HttpParser httpParser) { this.request = request; headers = request.getMimeHeaders(); @@ -66,6 +66,8 @@ public class InternalAprInputBuffer exte this.httpParser = httpParser; + this.rejectIllegalHeaderName = rejectIllegalHeader; + inputStreamInputBuffer = new SocketInputBuffer(); filterLibrary = new InputFilter[0]; @@ -350,6 +352,8 @@ public class InternalAprInputBuffer exte // byte chr = 0; + byte prevChr = 0; + while (true) { // Read new bytes if needed @@ -358,14 +362,19 @@ public class InternalAprInputBuffer exte 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; } @@ -396,8 +405,9 @@ public class InternalAprInputBuffer exte colon = true; headerValue = headers.addValue(buf, start, pos - start); } else if (!HttpParser.isToken(buf[pos])) { - // If a non-token header is detected, skip the line and - // ignore the header + // Non-token characters are illegal in header names + // Parsing continues so the error can be reported in context + // skipLine() will handle the error skipLine(start); return true; } @@ -453,10 +463,24 @@ public class InternalAprInputBuffer exte 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(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(start); + return true; } else if (buf[pos] == Constants.SP) { buf[realPos] = buf[pos]; realPos++; @@ -509,6 +533,9 @@ public class InternalAprInputBuffer exte lastRealByte = pos - 1; } + byte chr = 0; + byte prevChr = 0; + while (!eol) { // Read new bytes if needed @@ -517,9 +544,12 @@ public class InternalAprInputBuffer exte 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; @@ -527,9 +557,13 @@ public class InternalAprInputBuffer exte pos++; } - if (log.isDebugEnabled()) { - log.debug(sm.getString("iib.invalidheader", new String(buf, start, - lastRealByte - start + 1, Charset.forName("ISO-8859-1")))); + if (rejectIllegalHeaderName || log.isDebugEnabled()) { + String message = sm.getString("iib.invalidheader", new String(buf, start, + lastRealByte - start + 1, Charset.forName("ISO-8859-1"))); + if (rejectIllegalHeaderName) { + throw new IllegalArgumentException(message); + } + log.debug(message); } } diff -up ./java/org/apache/coyote/http11/InternalInputBuffer.java.orig ./java/org/apache/coyote/http11/InternalInputBuffer.java --- ./java/org/apache/coyote/http11/InternalInputBuffer.java.orig 2020-04-24 14:57:33.389591714 -0400 +++ ./java/org/apache/coyote/http11/InternalInputBuffer.java 2020-04-24 15:08:54.534164236 -0400 @@ -53,13 +53,14 @@ public class InternalInputBuffer extends * Default constructor. */ public InternalInputBuffer(Request request, int headerBufferSize, - HttpParser httpParser) { + boolean rejectIllegalHeader, HttpParser httpParser) { this.request = request; headers = request.getMimeHeaders(); buf = new byte[headerBufferSize]; + this.rejectIllegalHeaderName = rejectIllegalHeader; this.httpParser = httpParser; inputStreamInputBuffer = new InputStreamInputBuffer(); @@ -302,6 +303,8 @@ public class InternalInputBuffer extends // byte chr = 0; + byte prevChr = 0; + while (true) { // Read new bytes if needed @@ -310,19 +313,23 @@ public class InternalInputBuffer extends 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 @@ -348,8 +355,9 @@ public class InternalInputBuffer extends colon = true; headerValue = headers.addValue(buf, start, pos - start); } else if (!HttpParser.isToken(buf[pos])) { - // If a non-token header is detected, skip the line and - // ignore the header + // Non-token characters are illegal in header names + // Parsing continues so the error can be reported in context + // skipLine() will handle the error skipLine(start); return true; } @@ -406,15 +414,29 @@ public class InternalInputBuffer extends 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(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(start); + return true; + } else if (chr == Constants.SP) { + buf[realPos] = chr; realPos++; } else { - buf[realPos] = buf[pos]; + buf[realPos] = chr; realPos++; lastSignificantChar = realPos; } @@ -480,6 +502,9 @@ public class InternalInputBuffer extends lastRealByte = pos - 1; } + byte chr = 0; + byte prevChr = 0; + while (!eol) { // Read new bytes if needed @@ -488,9 +513,12 @@ public class InternalInputBuffer extends 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; @@ -498,9 +526,13 @@ public class InternalInputBuffer extends pos++; } - if (log.isDebugEnabled()) { - log.debug(sm.getString("iib.invalidheader", new String(buf, start, - lastRealByte - start + 1, Charset.forName("ISO-8859-1")))); + if (rejectIllegalHeaderName || log.isDebugEnabled()) { + String message = sm.getString("iib.invalidheader", new String(buf, start, + lastRealByte - start + 1, Charset.forName("ISO-8859-1"))); + if (rejectIllegalHeaderName) { + throw new IllegalArgumentException(message); + } + log.debug(message); } } diff -up ./java/org/apache/coyote/http11/InternalNioInputBuffer.java.orig ./java/org/apache/coyote/http11/InternalNioInputBuffer.java --- ./java/org/apache/coyote/http11/InternalNioInputBuffer.java.orig 2020-04-24 14:57:33.396591699 -0400 +++ ./java/org/apache/coyote/http11/InternalNioInputBuffer.java 2020-04-24 14:28:58.728272282 -0400 @@ -99,12 +99,13 @@ public class InternalNioInputBuffer exte * Alternate constructor. */ public InternalNioInputBuffer(Request request, int headerBufferSize, - HttpParser httpParser) { + boolean rejectIllegalHeader, HttpParser httpParser) { this.request = request; headers = request.getMimeHeaders(); this.headerBufferSize = headerBufferSize; + this.rejectIllegalHeaderName = rejectIllegalHeader; this.httpParser = httpParser; inputStreamInputBuffer = new SocketInputBuffer(); @@ -514,6 +515,8 @@ public class InternalNioInputBuffer exte // byte chr = 0; + byte prevChr = 0; + while (headerParsePos == HeaderParsePosition.HEADER_START) { // Read new bytes if needed @@ -524,19 +527,23 @@ public class InternalNioInputBuffer exte } } + 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 ) { @@ -570,9 +577,10 @@ public class InternalNioInputBuffer exte headerData.lastSignificantChar = pos; break; } else if (!HttpParser.isToken(chr)) { - // If a non-token header is detected, skip the line and - // ignore the header + // Non-token characters are illegal in header names + // Parsing continues so the error can be reported in context headerData.lastSignificantChar = pos; + // skipLine() will handle the error return skipLine(); } @@ -630,11 +638,22 @@ public class InternalNioInputBuffer exte } } + 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++; @@ -692,6 +711,9 @@ public class InternalNioInputBuffer exte headerParsePos = HeaderParsePosition.HEADER_SKIPLINE; boolean eol = false; + byte chr = 0; + byte prevChr = 0; + // Reading bytes until the end of the line while (!eol) { @@ -702,9 +724,12 @@ public class InternalNioInputBuffer exte } } - 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; @@ -712,11 +737,13 @@ public class InternalNioInputBuffer exte pos++; } - if (log.isDebugEnabled()) { - log.debug(sm.getString("iib.invalidheader", new String(buf, - headerData.start, - headerData.lastSignificantChar - headerData.start + 1, - DEFAULT_CHARSET))); + if (rejectIllegalHeaderName || log.isDebugEnabled()) { + String message = sm.getString("iib.invalidheader", new String(buf, headerData.start, + headerData.lastSignificantChar - headerData.start + 1, DEFAULT_CHARSET)); + if (rejectIllegalHeaderName) { + throw new IllegalArgumentException(message); + } + log.debug(message); } headerParsePos = HeaderParsePosition.HEADER_START; diff -up ./java/org/apache/tomcat/util/http/MimeHeaders.java.orig ./java/org/apache/tomcat/util/http/MimeHeaders.java --- ./java/org/apache/tomcat/util/http/MimeHeaders.java.orig 2020-04-24 14:28:38.873317805 -0400 +++ ./java/org/apache/tomcat/util/http/MimeHeaders.java 2020-04-24 14:28:58.728272282 -0400 @@ -375,7 +375,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 -up ./java/org/apache/tomcat/util/http/parser/HttpParser.java.orig ./java/org/apache/tomcat/util/http/parser/HttpParser.java --- ./java/org/apache/tomcat/util/http/parser/HttpParser.java.orig 2020-04-24 14:28:58.729272279 -0400 +++ ./java/org/apache/tomcat/util/http/parser/HttpParser.java 2020-04-24 14:37:59.945050923 -0400 @@ -482,6 +482,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 return the next char private static int skipLws(StringReader input, boolean withReset) throws IOException { diff -up ./test/org/apache/coyote/http11/TestInternalInputBuffer.java.orig ./test/org/apache/coyote/http11/TestInternalInputBuffer.java --- ./test/org/apache/coyote/http11/TestInternalInputBuffer.java.orig 2020-04-24 14:57:33.385591722 -0400 +++ ./test/org/apache/coyote/http11/TestInternalInputBuffer.java 2020-04-24 14:28:58.730272277 -0400 @@ -33,6 +33,7 @@ import static org.junit.Assert.assertTru import org.junit.Test; import org.apache.catalina.Context; +import org.apache.catalina.connector.Connector; import org.apache.catalina.startup.SimpleHttpClient; import org.apache.catalina.startup.TesterServlet; import org.apache.catalina.startup.Tomcat; @@ -130,6 +131,28 @@ public class TestInternalInputBuffer ext @Test + public void testBug51557Valid() { + + Bug51557Client client = new Bug51557Client("X-Bug51557Valid", "1234"); + + client.doRequest(); + assertTrue(client.isResponse200()); + assertEquals("1234abcd", client.getResponseBody()); + assertTrue(client.isResponseBodyOK()); + } + + + @Test + public void testBug51557Invalid() { + + Bug51557Client client = new Bug51557Client("X-Bug51557=Invalid", "1234", true); + + client.doRequest(); + assertTrue(client.isResponse400()); + } + + + @Test public void testBug51557NoColon() { Bug51557Client client = new Bug51557Client("X-Bug51557NoColon"); @@ -142,27 +165,52 @@ public class TestInternalInputBuffer ext @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(); + } + } + + + @Test + public void testBug51557CtlInName() throws Exception { + for (int i = 0; i < 31; i++) { + doTestBug51557CharInName((char) i); tearDown(); setUp(); } + doTestBug51557CharInName((char) 127); } @Test - public void testBug51557Ctl() throws Exception { + public void testBug51557CtlInValue() throws Exception { for (int i = 0; i < 31; i++) { - doTestBug51557Char((char) 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); } @@ -205,7 +253,33 @@ public class TestInternalInputBuffer ext } - 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"); @@ -215,22 +289,53 @@ public class TestInternalInputBuffer ext 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. */ private class Bug51557Client extends SimpleHttpClient { - private String headerName; - private String headerLine; + private final String headerName; + private final String headerLine; + private final boolean rejectIllegalHeader; public Bug51557Client(String headerName) { this.headerName = headerName; this.headerLine = headerName; + this.rejectIllegalHeader = false; } public Bug51557Client(String headerName, String headerValue) { + this(headerName, headerValue, false); + } + + public Bug51557Client(String headerName, String headerValue, + boolean rejectIllegalHeader) { this.headerName = headerName; this.headerLine = headerName + ": " + headerValue; + this.rejectIllegalHeader = rejectIllegalHeader; } private Exception doRequest() { @@ -243,8 +348,11 @@ public class TestInternalInputBuffer ext root.addServletMapping("/test", "Bug51557"); try { + Connector connector = tomcat.getConnector(); + Assert.assertTrue(connector.setProperty( + "rejectIllegalHeader", Boolean.toString(rejectIllegalHeader))); tomcat.start(); - setPort(tomcat.getConnector().getLocalPort()); + setPort(connector.getLocalPort()); // Open connection connect(); @@ -480,6 +588,75 @@ public class TestInternalInputBuffer ext } + /** + * 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 -up webapps/docs/changelog.xml.orig webapps/docs/changelog.xml --- webapps/docs/changelog.xml.orig 2020-04-24 14:57:33.381591731 -0400 +++ webapps/docs/changelog.xml 2020-04-24 15:44:27.588766788 -0400 @@ -77,6 +77,19 @@
+ + + + Add an option to reject requests that contain HTTP headers with invalid + (non-token) header names with a 400 response. (markt) + + + Rename the HTTP Connector attribute rejectIllegalHeaderName + to rejectIllegalHeader and expand the underlying + implementation to include header values as well as names. (markt) + + + diff -up ./webapps/docs/config/http.xml.orig ./webapps/docs/config/http.xml --- ./webapps/docs/config/http.xml.orig 2020-04-24 14:57:33.383591727 -0400 +++ ./webapps/docs/config/http.xml 2020-04-24 14:37:07.540160722 -0400 @@ -542,6 +542,20 @@ present in the value will 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) this setting determines if the + request will be rejected with a 400 response (true) or if the + illegal header be ignored (false). The default value is + false which will cause the request to be processed but the + illegal header will be ignored.

+ + + +

This attribute is deprecated. It will be removed in Tomcat 10 onwards. + It is now an alias for rejectIllegalHeader.

+ +

The value is a regular expression (using java.util.regex) matching the user-agent header of HTTP clients for which