Author: markt Date: Wed Dec 16 19:24:16 2015 New Revision: 1720418 URL: http://svn.apache.org/viewvc?rev=1720418&view=rev Log: Improve HTTP header validation. This is a partial back-port of various refactorings and fixes that have been made in 7.0.x onwards.
Added: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/AbstractInputBuffer.java (with props) Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalAprInputBuffer.java tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalInputBuffer.java tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalNioInputBuffer.java tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Added: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/AbstractInputBuffer.java URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/AbstractInputBuffer.java?rev=1720418&view=auto ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/AbstractInputBuffer.java (added) +++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/AbstractInputBuffer.java Wed Dec 16 19:24:16 2015 @@ -0,0 +1,72 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.coyote.http11; + +import org.apache.coyote.InputBuffer; + +public abstract class AbstractInputBuffer implements InputBuffer { + + protected static final boolean[] HTTP_TOKEN_CHAR = new boolean[128]; + + static { + for (int i = 0; i < 128; i++) { + if (i < 32) { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == 127) { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '(') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == ')') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '<') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '>') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '@') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == ',') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == ';') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == ':') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '\\') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '\"') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '/') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '[') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == ']') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '?') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '=') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '{') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == '}') { + HTTP_TOKEN_CHAR[i] = false; + } else if (i == ' ') { + HTTP_TOKEN_CHAR[i] = false; + } else { + HTTP_TOKEN_CHAR[i] = true; + } + } + } +} Propchange: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/AbstractInputBuffer.java ------------------------------------------------------------------------------ svn:eol-style = native Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalAprInputBuffer.java URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalAprInputBuffer.java?rev=1720418&r1=1720417&r2=1720418&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalAprInputBuffer.java (original) +++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalAprInputBuffer.java Wed Dec 16 19:24:16 2015 @@ -14,8 +14,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - - package org.apache.coyote.http11; import java.io.IOException; @@ -31,6 +29,8 @@ import org.apache.tomcat.util.http.MimeH import org.apache.tomcat.util.res.StringManager; import org.apache.coyote.InputBuffer; import org.apache.coyote.Request; +import org.apache.juli.logging.Log; +import org.apache.juli.logging.LogFactory; /** * Implementation of InputBuffer which provides HTTP request header parsing as @@ -38,11 +38,9 @@ import org.apache.coyote.Request; * * @author <a href="mailto:r...@apache.org">Remy Maucherat</a> */ -public class InternalAprInputBuffer implements InputBuffer { - - - // -------------------------------------------------------------- Constants +public class InternalAprInputBuffer extends AbstractInputBuffer { + private static final Log log = LogFactory.getLog(InternalAprInputBuffer.class); // ----------------------------------------------------------- Constructors @@ -552,6 +550,7 @@ public class InternalAprInputBuffer impl * @return false after reading a blank line (which indicates that the * HTTP header parsing is done */ + @SuppressWarnings("null") // headerValue cannot be null public boolean parseHeader() throws IOException { @@ -570,11 +569,11 @@ public class InternalAprInputBuffer impl chr = buf[pos]; - if ((chr == Constants.CR) || (chr == Constants.LF)) { - if (chr == Constants.LF) { - pos++; - return false; - } + if (chr == Constants.CR) { + // Skip + } else if (chr == Constants.LF) { + pos++; + return false; } else { break; } @@ -605,6 +604,11 @@ public class InternalAprInputBuffer impl if (buf[pos] == Constants.COLON) { colon = true; headerValue = headers.addValue(buf, start, pos - start); + } else if (!HTTP_TOKEN_CHAR[buf[pos]]) { + // If a non-token header is detected, skip the line and + // ignore the header + skipLine(start); + return true; } chr = buf[pos]; if ((chr >= Constants.A) && (chr <= Constants.Z)) { @@ -659,6 +663,7 @@ public class InternalAprInputBuffer impl } if (buf[pos] == Constants.CR) { + // Skip } else if (buf[pos] == Constants.LF) { eol = true; } else if (buf[pos] == Constants.SP) { @@ -706,6 +711,38 @@ public class InternalAprInputBuffer impl } + private void skipLine(int start) throws IOException { + boolean eol = false; + int lastRealByte = start; + if (pos - 1 > start) { + lastRealByte = pos - 1; + } + + while (!eol) { + + // Read new bytes if needed + if (pos >= lastValid) { + if (!fill()) + throw new EOFException(sm.getString("iib.eof.error")); + } + + if (buf[pos] == Constants.CR) { + // Skip + } else if (buf[pos] == Constants.LF) { + eol = true; + } else { + lastRealByte = pos; + } + pos++; + } + + if (log.isDebugEnabled()) { + log.debug(sm.getString("iib.invalidheader", new String(buf, start, + lastRealByte - start + 1, "ISO-8859-1"))); + } + } + + /** * Available bytes (note that due to encoding, this may not correspond ) */ Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalInputBuffer.java URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalInputBuffer.java?rev=1720418&r1=1720417&r2=1720418&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalInputBuffer.java (original) +++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalInputBuffer.java Wed Dec 16 19:24:16 2015 @@ -14,8 +14,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - - package org.apache.coyote.http11; import java.io.IOException; @@ -29,6 +27,8 @@ import org.apache.tomcat.util.res.String import org.apache.coyote.InputBuffer; import org.apache.coyote.Request; +import org.apache.juli.logging.Log; +import org.apache.juli.logging.LogFactory; /** * Implementation of InputBuffer which provides HTTP request header parsing as @@ -36,9 +36,10 @@ import org.apache.coyote.Request; * * @author <a href="mailto:r...@apache.org">Remy Maucherat</a> */ -public class InternalInputBuffer implements InputBuffer { - +public class InternalInputBuffer extends AbstractInputBuffer { + private static final Log log = LogFactory.getLog(InternalInputBuffer.class); + // -------------------------------------------------------------- Constants @@ -539,6 +540,7 @@ public class InternalInputBuffer impleme * @return false after reading a blank line (which indicates that the * HTTP header parsing is done */ + @SuppressWarnings("null") // headerValue cannot be null public boolean parseHeader() throws IOException { @@ -557,11 +559,11 @@ public class InternalInputBuffer impleme chr = buf[pos]; - if ((chr == Constants.CR) || (chr == Constants.LF)) { - if (chr == Constants.LF) { - pos++; - return false; - } + if (chr == Constants.CR) { + // Skip + } else if (chr == Constants.LF) { + pos++; + return false; } else { break; } @@ -592,7 +594,13 @@ public class InternalInputBuffer impleme if (buf[pos] == Constants.COLON) { colon = true; headerValue = headers.addValue(buf, start, pos - start); + } else if (!HTTP_TOKEN_CHAR[buf[pos]]) { + // If a non-token header is detected, skip the line and + // ignore the header + skipLine(start); + return true; } + chr = buf[pos]; if ((chr >= Constants.A) && (chr <= Constants.Z)) { buf[pos] = (byte) (chr - Constants.LC_OFFSET); @@ -646,6 +654,7 @@ public class InternalInputBuffer impleme } if (buf[pos] == Constants.CR) { + // Skip } else if (buf[pos] == Constants.LF) { eol = true; } else if (buf[pos] == Constants.SP) { @@ -712,7 +721,38 @@ public class InternalInputBuffer impleme // ------------------------------------------------------ Protected Methods + private void skipLine(int start) throws IOException { + boolean eol = false; + int lastRealByte = start; + if (pos - 1 > start) { + lastRealByte = pos - 1; + } + + while (!eol) { + + // Read new bytes if needed + if (pos >= lastValid) { + if (!fill()) + throw new EOFException(sm.getString("iib.eof.error")); + } + + if (buf[pos] == Constants.CR) { + // Skip + } else if (buf[pos] == Constants.LF) { + eol = true; + } else { + lastRealByte = pos; + } + pos++; + } + + if (log.isDebugEnabled()) { + log.debug(sm.getString("iib.invalidheader", new String(buf, start, + lastRealByte - start + 1, "ISO-8859-1"))); + } + } + /** * Fill the internal buffer using data from the undelying input stream. * Modified: tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalNioInputBuffer.java URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalNioInputBuffer.java?rev=1720418&r1=1720417&r2=1720418&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalNioInputBuffer.java (original) +++ tomcat/tc6.0.x/trunk/java/org/apache/coyote/http11/InternalNioInputBuffer.java Wed Dec 16 19:24:16 2015 @@ -14,8 +14,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - - package org.apache.coyote.http11; import java.io.EOFException; @@ -39,18 +37,56 @@ import org.apache.tomcat.util.net.NioEnd * @author <a href="mailto:r...@apache.org">Remy Maucherat</a> * @author Filip Hanik */ -public class InternalNioInputBuffer implements InputBuffer { +public class InternalNioInputBuffer extends AbstractInputBuffer { - /** - * Logger. - */ private static final org.apache.juli.logging.Log log = org.apache.juli.logging.LogFactory.getLog(InternalNioInputBuffer.class); // -------------------------------------------------------------- Constants - enum HeaderParseStatus {DONE, HAVE_MORE_HEADERS, NEED_MORE_DATA} - enum HeaderParsePosition {HEADER_START, HEADER_NAME, HEADER_VALUE, HEADER_MULTI_LINE} + enum HeaderParseStatus { + DONE, HAVE_MORE_HEADERS, NEED_MORE_DATA + } + + enum HeaderParsePosition { + /** + * Start of a new header. A CRLF here means that there are no more + * headers. Any other character starts a header name. + */ + HEADER_START, + /** + * Reading a header name. All characters of header are HTTP_TOKEN_CHAR. + * Header name is followed by ':'. No whitespace is allowed.<br /> + * Any non-HTTP_TOKEN_CHAR (this includes any whitespace) encountered + * before ':' will result in the whole line being ignored. + */ + HEADER_NAME, + /** + * Skipping whitespace before text of header value starts, either on the + * first line of header value (just after ':') or on subsequent lines + * when it is known that subsequent line starts with SP or HT. + */ + HEADER_VALUE_START, + /** + * Reading the header value. We are inside the value. Either on the + * first line or on any subsequent line. We come into this state from + * HEADER_VALUE_START after the first non-SP/non-HT byte is encountered + * on the line. + */ + HEADER_VALUE, + /** + * Before reading a new line of a header. Once the next byte is peeked, + * the state changes without advancing our position. The state becomes + * either HEADER_VALUE_START (if that first byte is SP or HT), or + * HEADER_START (otherwise). + */ + HEADER_MULTI_LINE, + /** + * Reading all bytes until the next CRLF. The line is being ignored. + */ + HEADER_SKIPLINE + } + // ----------------------------------------------------------- Constructors @@ -190,7 +226,8 @@ public class InternalNioInputBuffer impl /** - * Maximum allowed size of the HTTP request line plus headers. + * Maximum allowed size of the HTTP request line plus headers plus any + * leading blank lines. */ private final int headerBufferSize; @@ -420,7 +457,7 @@ public class InternalNioInputBuffer impl * using it. * * @throws IOException If an exception occurs during the underlying socket - * read operations, or if the given buffer is not big enough to accomodate + * read operations, or if the given buffer is not big enough to accommodate * the whole line. * @return true if data is properly fed; false if no data is available * immediately and thread should be freed @@ -716,11 +753,11 @@ public class InternalNioInputBuffer impl chr = buf[pos]; - if ((chr == Constants.CR) || (chr == Constants.LF)) { - if (chr == Constants.LF) { - pos++; - return HeaderParseStatus.DONE; - } + if (chr == Constants.CR) { + // Skip + } else if (chr == Constants.LF) { + pos++; + return HeaderParseStatus.DONE; } else { break; } @@ -740,8 +777,6 @@ public class InternalNioInputBuffer impl // Header name is always US-ASCII // - - while (headerParsePos == HeaderParsePosition.HEADER_NAME) { // Read new bytes if needed @@ -751,58 +786,67 @@ public class InternalNioInputBuffer impl } } - if (buf[pos] == Constants.COLON) { - headerParsePos = HeaderParsePosition.HEADER_VALUE; + chr = buf[pos]; + if (chr == Constants.COLON) { + headerParsePos = HeaderParsePosition.HEADER_VALUE_START; headerData.headerValue = headers.addValue(buf, headerData.start, pos - headerData.start); + pos++; + // Mark the current buffer position + headerData.start = pos; + headerData.realPos = pos; + headerData.lastSignificantChar = pos; + break; + } else if (!HTTP_TOKEN_CHAR[chr]) { + // If a non-token header is detected, skip the line and + // ignore the header + headerData.lastSignificantChar = pos; + return skipLine(); } - chr = buf[pos]; + + // chr is next byte of header name. Convert to lowercase. if ((chr >= Constants.A) && (chr <= Constants.Z)) { buf[pos] = (byte) (chr - Constants.LC_OFFSET); } - pos++; - if ( headerParsePos == HeaderParsePosition.HEADER_VALUE ) { - // Mark the current buffer position - headerData.start = pos; - headerData.realPos = pos; - } } - + // Skip the line and ignore the header + if (headerParsePos == HeaderParsePosition.HEADER_SKIPLINE) { + return skipLine(); + } + // // Reading the header value (which can be spanned over multiple lines) // - boolean eol = false; - - while (headerParsePos == HeaderParsePosition.HEADER_VALUE || + while (headerParsePos == HeaderParsePosition.HEADER_VALUE_START || + headerParsePos == HeaderParsePosition.HEADER_VALUE || headerParsePos == HeaderParsePosition.HEADER_MULTI_LINE) { - if ( headerParsePos == HeaderParsePosition.HEADER_VALUE ) { - - boolean space = true; + if ( headerParsePos == HeaderParsePosition.HEADER_VALUE_START ) { // Skipping spaces - while (space) { - + while (true) { // Read new bytes if needed if (pos >= lastValid) { if (!fill(true,false)) {//parse header - //HEADER_VALUE, should already be set + //HEADER_VALUE_START return HeaderParseStatus.NEED_MORE_DATA; } } - if ((buf[pos] == Constants.SP) || (buf[pos] == Constants.HT)) { + chr = buf[pos]; + if (chr == Constants.SP || chr == Constants.HT) { pos++; } else { - space = false; + headerParsePos = HeaderParsePosition.HEADER_VALUE; + break; } - } - - headerData.lastSignificantChar = headerData.realPos; + } + if ( headerParsePos == HeaderParsePosition.HEADER_VALUE ) { // Reading bytes until the end of the line + boolean eol = false; while (!eol) { // Read new bytes if needed @@ -811,25 +855,26 @@ public class InternalNioInputBuffer impl //HEADER_VALUE return HeaderParseStatus.NEED_MORE_DATA; } - } - if (buf[pos] == Constants.CR) { - } else if (buf[pos] == Constants.LF) { + chr = buf[pos]; + if (chr == Constants.CR) { + // Skip + } else if (chr == Constants.LF) { eol = true; - } else if (buf[pos] == Constants.SP) { - buf[headerData.realPos] = buf[pos]; + } else if (chr == Constants.SP || chr == Constants.HT) { + buf[headerData.realPos] = chr; headerData.realPos++; } else { - buf[headerData.realPos] = buf[pos]; + buf[headerData.realPos] = chr; headerData.realPos++; headerData.lastSignificantChar = headerData.realPos; } pos++; - } + // Ignore whitespaces at the end of the line headerData.realPos = headerData.lastSignificantChar; // Checking the first character of the new line. If the character @@ -849,27 +894,86 @@ public class InternalNioInputBuffer impl if ( headerParsePos == HeaderParsePosition.HEADER_MULTI_LINE ) { if ( (chr != Constants.SP) && (chr != Constants.HT)) { headerParsePos = HeaderParsePosition.HEADER_START; + break; } else { - eol = false; // Copying one extra space in the buffer (since there must // be at least one space inserted between the lines) buf[headerData.realPos] = chr; headerData.realPos++; - headerParsePos = HeaderParsePosition.HEADER_VALUE; + headerParsePos = HeaderParsePosition.HEADER_VALUE_START; } } } // Set the header value - headerData.headerValue.setBytes(buf, headerData.start, headerData.realPos - headerData.start); + headerData.headerValue.setBytes(buf, headerData.start, + headerData.lastSignificantChar - headerData.start); headerData.recycle(); return HeaderParseStatus.HAVE_MORE_HEADERS; } - protected HeaderParseData headerData = new HeaderParseData(); + private HeaderParseStatus skipLine() throws IOException { + headerParsePos = HeaderParsePosition.HEADER_SKIPLINE; + boolean eol = false; + + // Reading bytes until the end of the line + while (!eol) { + + // Read new bytes if needed + if (pos >= lastValid) { + if (!fill(true,false)) { + return HeaderParseStatus.NEED_MORE_DATA; + } + } + + if (buf[pos] == Constants.CR) { + // Skip + } else if (buf[pos] == Constants.LF) { + eol = true; + } else { + headerData.lastSignificantChar = pos; + } + + pos++; + } + if (log.isDebugEnabled()) { + log.debug(sm.getString("iib.invalidheader", new String(buf, + headerData.start, + headerData.lastSignificantChar - headerData.start + 1, + "ISO-8859-1"))); + } + + headerParsePos = HeaderParsePosition.HEADER_START; + return HeaderParseStatus.HAVE_MORE_HEADERS; + } + + private HeaderParseData headerData = new HeaderParseData(); public static class HeaderParseData { + /** + * When parsing header name: first character of the header.<br /> + * When skipping broken header line: first character of the header.<br /> + * When parsing header value: first character after ':'. + */ int start = 0; + /** + * When parsing header name: not used (stays as 0).<br /> + * When skipping broken header line: not used (stays as 0).<br /> + * When parsing header value: starts as the first character after ':'. + * Then is increased as far as more bytes of the header are harvested. + * Bytes from buf[pos] are copied to buf[realPos]. Thus the string from + * [start] to [realPos-1] is the prepared value of the header, with + * whitespaces removed as needed.<br /> + */ int realPos = 0; + /** + * When parsing header name: not used (stays as 0).<br /> + * When skipping broken header line: last non-CR/non-LF character.<br /> + * When parsing header value: position after the last not-LWS character.<br /> + */ int lastSignificantChar = 0; + /** + * MB that will store the value of the header. It is null while parsing + * header name and is created after the name has been parsed. + */ MessageBytes headerValue = null; public void recycle() { start = 0; @@ -968,11 +1072,6 @@ public class InternalNioInputBuffer impl pos = lastValid; return (length); - } - - } - - } Modified: tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml URL: http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml?rev=1720418&r1=1720417&r2=1720418&view=diff ============================================================================== --- tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml (original) +++ tomcat/tc6.0.x/trunk/webapps/docs/changelog.xml Wed Dec 16 19:24:16 2015 @@ -116,6 +116,9 @@ <bug>57943</bug>: Prevent the same socket being added to the cache twice. Patch based on analysis by Ian Luo / Sun Qi. (markt/kkolinko) </fix> + <fix> + Improve HTTP header validation. (markt) + </fix> </changelog> </subsection> <subsection name="Web applications"> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org