> LGTM. Thanks for seeing this through. Awesome, no problem :)
On Wed, Feb 8, 2017 at 2:46 PM, Mark Thomas <ma...@apache.org> wrote: > On 08/02/17 19:42, Coty Sutherland wrote: >> >> Thanks for the suggestions; I implemented them in >> http://svn.apache.org/r1782240 > > > LGTM. Thanks for seeing this through. > > Mark > > > >> >> On Tue, Feb 7, 2017 at 7:13 PM, Mark Thomas <ma...@apache.org> wrote: >>> >>> On 07/02/17 18:13, csuth...@apache.org wrote: >>>> >>>> >>>> Author: csutherl >>>> Date: Tue Feb 7 18:13:40 2017 >>>> New Revision: 1782037 >>>> >>>> URL: http://svn.apache.org/viewvc?rev=1782037&view=rev >>>> Log: >>>> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=60594 >>>> Adding implementation of whitelist patch >>>> >>>> Modified: >>>> tomcat/tc8.5.x/trunk/conf/catalina.properties >>>> >>>> >>>> tomcat/tc8.5.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java >>>> tomcat/tc8.5.x/trunk/webapps/docs/changelog.xml >>>> tomcat/tc8.5.x/trunk/webapps/docs/config/systemprops.xml >>>> >>>> Modified: tomcat/tc8.5.x/trunk/conf/catalina.properties >>>> URL: >>>> >>>> http://svn.apache.org/viewvc/tomcat/tc8.5.x/trunk/conf/catalina.properties?rev=1782037&r1=1782036&r2=1782037&view=diff >>>> >>>> >>>> ============================================================================== >>>> --- tomcat/tc8.5.x/trunk/conf/catalina.properties (original) >>>> +++ tomcat/tc8.5.x/trunk/conf/catalina.properties Tue Feb 7 18:13:40 >>>> 2017 >>>> @@ -146,3 +146,6 @@ tomcat.util.buf.StringCache.byte.enabled >>>> #tomcat.util.buf.StringCache.char.enabled=true >>>> #tomcat.util.buf.StringCache.trainThreshold=500000 >>>> #tomcat.util.buf.StringCache.cacheSize=5000 >>>> + >>>> +# Allow for changes to HTTP request validation >>> >>> >>> >>> I'd add here: >>> >>> # WARNING: Using this option will expose the server to CVE-2016-6816 >>> >>> >>>> +#tomcat.util.http.parser.HttpParser.requestTargetAllow=| >>>> >>>> Modified: >>>> >>>> tomcat/tc8.5.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java >>>> URL: >>>> >>>> http://svn.apache.org/viewvc/tomcat/tc8.5.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java?rev=1782037&r1=1782036&r2=1782037&view=diff >>>> >>>> >>>> ============================================================================== >>>> --- >>>> >>>> tomcat/tc8.5.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java >>>> (original) >>>> +++ >>>> >>>> tomcat/tc8.5.x/trunk/java/org/apache/tomcat/util/http/parser/HttpParser.java >>>> Tue Feb 7 18:13:40 2017 >>>> @@ -19,6 +19,9 @@ package org.apache.tomcat.util.http.pars >>>> import java.io.IOException; >>>> import java.io.StringReader; >>>> >>>> +import org.apache.juli.logging.Log; >>>> +import org.apache.juli.logging.LogFactory; >>>> + >>>> /** >>>> * HTTP header value parser implementation. Parsing HTTP headers as per >>>> RFC2616 >>>> * is not always as simple as it first appears. For headers that only >>>> use >>>> tokens >>>> @@ -34,6 +37,8 @@ import java.io.StringReader; >>>> */ >>>> public class HttpParser { >>>> >>>> + private static final Log log = LogFactory.getLog(HttpParser.class); >>>> + >>>> private static final int ARRAY_SIZE = 128; >>>> >>>> private static final boolean[] IS_CONTROL = new >>>> boolean[ARRAY_SIZE]; >>>> @@ -42,8 +47,22 @@ public class HttpParser { >>>> private static final boolean[] IS_HEX = new boolean[ARRAY_SIZE]; >>>> private static final boolean[] IS_NOT_REQUEST_TARGET = new >>>> boolean[ARRAY_SIZE]; >>>> private static final boolean[] IS_HTTP_PROTOCOL = new >>>> boolean[ARRAY_SIZE]; >>>> + private static final boolean[] REQUEST_TARGET_ALLOW = new >>>> boolean[ARRAY_SIZE]; >>>> >>>> static { >>>> + String prop = >>>> >>>> System.getProperty("tomcat.util.http.parser.HttpParser.requestTargetAllow"); >>>> + if (prop != null) { >>>> + for (int i = 0; i < prop.length(); i++) { >>>> + char c = prop.charAt(i); >>>> + if (c == '{' || c == '}' || c == '|') { >>>> + REQUEST_TARGET_ALLOW[c] = true; >>>> + } else { >>>> + log.warn("HttpParser: Character '" + c + "' is not >>>> allowed and will continue " >>>> + + "being rejected."); >>> >>> >>> >>> This should use the StringManager for i18n support. >>> >>> Also "... will continue to be rejected." sounds better. >>> >>> >>>> + } >>>> + } >>>> + } >>>> + >>>> for (int i = 0; i < ARRAY_SIZE; i++) { >>>> // Control> 0-31, 127 >>>> if (i < 32 || i == 127) { >>>> @@ -74,7 +93,9 @@ public class HttpParser { >>>> if (IS_CONTROL[i] || i > 127 || >>>> i == ' ' || i == '\"' || i == '#' || i == '<' || i >>>> == >>>> '>' || i == '\\' || >>>> i == '^' || i == '`' || i == '{' || i == '|' || i >>>> == >>>> '}') { >>>> - IS_NOT_REQUEST_TARGET[i] = true; >>>> + if (!REQUEST_TARGET_ALLOW[i]) { >>>> + IS_NOT_REQUEST_TARGET[i] = true; >>>> + } >>>> } >>>> >>>> // Not valid for HTTP protocol >>>> >>>> Modified: tomcat/tc8.5.x/trunk/webapps/docs/changelog.xml >>>> URL: >>>> >>>> http://svn.apache.org/viewvc/tomcat/tc8.5.x/trunk/webapps/docs/changelog.xml?rev=1782037&r1=1782036&r2=1782037&view=diff >>>> >>>> >>>> ============================================================================== >>>> --- tomcat/tc8.5.x/trunk/webapps/docs/changelog.xml (original) >>>> +++ tomcat/tc8.5.x/trunk/webapps/docs/changelog.xml Tue Feb 7 18:13:40 >>>> 2017 >>>> @@ -103,6 +103,12 @@ >>>> Ensure that executor thread pools used with connectors, >>>> pre-start >>>> the >>>> configured minimum number of idle threads. (markt) >>>> </fix> >>>> + <add> >>>> + <bug>60594</bug>: Allow some invalid characters that were >>>> recently >>>> + restricted to be processed in requests by using the system >>>> property >>>> + >>>> <code>tomcat.util.http.parser.HttpParser.requestTargetAllow</code>. >>>> + (csutherl) >>>> + </add> >>>> </changelog> >>>> </subsection> >>>> <subsection name="Jasper"> >>>> >>>> Modified: tomcat/tc8.5.x/trunk/webapps/docs/config/systemprops.xml >>>> URL: >>>> >>>> http://svn.apache.org/viewvc/tomcat/tc8.5.x/trunk/webapps/docs/config/systemprops.xml?rev=1782037&r1=1782036&r2=1782037&view=diff >>>> >>>> >>>> ============================================================================== >>>> --- tomcat/tc8.5.x/trunk/webapps/docs/config/systemprops.xml (original) >>>> +++ tomcat/tc8.5.x/trunk/webapps/docs/config/systemprops.xml Tue Feb 7 >>>> 18:13:40 2017 >>>> @@ -639,6 +639,14 @@ >>>> <p>If not specified, the default value of <code>3</code> will be >>>> used.</p> >>>> </property> >>>> >>>> + <property >>>> name="tomcat.util.http.parser.HttpParser.requestTargetAllow"> >>>> + <p>A string comprised of characters the server should allow even >>>> when they are not encoded. >>>> + These characters would normally result in a 400 status.</p> >>>> + <p>The acceptable characters for this property are: >>>> <code>|</code>, >>>> <code>{</code> >>>> + , and <code>}</code></p> >>>> + <p>If not specified, the default value of <code>null</code> will >>>> be >>>> used.</p> >>> >>> >>> >>> Again, I'd add something along the lines of: >>> >>> <strong>WARNING</strong>: Use of this option will expose the server to >>> CVE-2016-6816. >>> >>> >>>> + </property> >>>> + >>>> </properties> >>>> >>>> </section> >>>> >>>> >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org >>>> For additional commands, e-mail: dev-h...@tomcat.apache.org >>>> >>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org >>> For additional commands, e-mail: dev-h...@tomcat.apache.org >>> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org >> For additional commands, e-mail: dev-h...@tomcat.apache.org >> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > For additional commands, e-mail: dev-h...@tomcat.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
