Bug 54080 - Comma related bug in org.apache.catalina.valves.RemoteIpValve
Summary: Comma related bug in org.apache.catalina.valves.RemoteIpValve
Alias: None
Product: Tomcat 6
Classification: Unclassified
Component: Documentation (show other bugs)
Version: 6.0.35
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: default
Assignee: Tomcat Developers Mailing List
Depends on:
Reported: 2012-10-31 10:30 UTC by Simon Dean
Modified: 2013-02-09 17:16 UTC (History)
1 user (show)

Documentation patch (2.32 KB, patch)
2012-11-02 21:41 UTC, Christopher Schultz
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Dean 2012-10-31 10:30:22 UTC

I'm using Tomcat 6.0.35 on RHEL 6.x and Windows 7.   I've stumbled upon a bug in org.apache.catalina.valves.RemoteIpValve.   

The issue is related to using commas in any regular expressions used with the "internalProxies" or "trustedProxies" attributes.  

*** Steps to reproduce ***

Add the following XML to $CATALINA_HOME/conf/context.xml  under the Context element: 

    <Valve className="org.apache.catalina.valves.RemoteIpValve"
        protocolHeaderHttpsValue="example-value" />

The bug is triggered by the comma in {1,3}

The XML above is based on the regex given on the pages http://tomcat.apache.org/tomcat-6.0-doc/config/valve.html and http://tomcat.apache.org/tomcat-6.0-doc/api/org/apache/catalina/valves/RemoteIpValve.html: 

    10\.\d{1,3}\.\d{1,3}\.\d{1,3}, 192\.168\.\d{1,3}\.\d{1,3}, 169\.254\.\d{1,3}\.\d{1,3}, 127\.\d{1,3}\.\d{1,3}\.\d{1,3}

*** Symptoms of the bug ***

RemoteIpValve incorrectly splits the ="10\.\d{1,3}\.\d{1,3}\.\d{1,3}" string into the two regular expressions "10\.\d{1" and "3}\.\d{1,3}\.\d{1,3}" (RemoteIpValve.java line 396).  In then attempts to parse the first regular expression which throws a PatternSyntaxException (line 382).  The class catches this and throws a IllegalArgumentException (line 384).  

Because an exemption is thrown, the "internalProxies" will keep its default value (set on line 445) instead of being cleared.  This is confusing for someone trying to diagnose why the value they set "internalProxies" to is not working.  It would be better if the class cleared the value of "internalProxies" in this case.  

*** Suggested fix ***

1) Instead of throwing the IllegalArgumentException, the class could log a debug message against the class's logger with information on the string that could not be parse as a regular expression.  This would a) help people with identifying and diagnosing the issue and b) stop RemoteIpValve from falling back to its default list of "internalProxies".  

2) When internalProxies is initialized (line 445), use commaDelimitedListToPatternArray to parse the default regular expressions from a single string instead of parsing the four regular expressions separately.  This change would instantly flag up any issues (e.g. a comma) in the default regular expressions.  

3) An update to the RemoteIpValve documentation http://tomcat.apache.org/tomcat-6.0-doc/config/valve.html and http://tomcat.apache.org/tomcat-6.0-doc/api/org/apache/catalina/valves/RemoteIpValve.html to remove the commas from the documentation's example regular expressions.  For example, this regular expression 

    10\.\d{1,3}\.\d{1,3}\.\d{1,3}, 192\.168\.\d{1,3}\.\d{1,3}, 169\.254\.\d{1,3}\.\d{1,3}, 127\.\d{1,3}\.\d{1,3}\.\d{1,3}

could be replaced with 

    10\.\d+\.\d+\.\d+, 192\.168\.\d+\.\d+, 169\.254\.\d+\.\d+, 127\.\d+\.\d+\.\d+

4) An update to the class's documentation to make it clear that commas should not be used in the regular expressions, other than to separate multiple regular expressions.  

*** Other things that might be affected by the same bug ***

RemoteIpValue.java seems unchanged in Tomcat 6.0.36 so the bug should affect that version too.  

Looking at RemoteIpValve and RemoteIpFilter in Tomcat 7, I can see that this bug has been avoided by treating "internalProxies" and "trustedProxies" as a single regular expressons instead of as comma separated lists of regular expressions.  That looks like an elgant solution.  

Kind regards
Simon Dean
Comment 1 Christopher Schultz 2012-11-02 21:41:16 UTC
Created attachment 29545 [details]
Documentation patch

Patch which hopefully clarifies the dangers of using commas in regular expressions.
Comment 2 Konstantin Kolinko 2012-11-06 18:53:55 UTC
It should be possible to write '\d{1,3}' simply as '\d\d?\d?'.
Comment 3 Konstantin Kolinko 2013-02-09 17:16:16 UTC
Tomcat 6 documentation corrected by r1444396 , will be in 6.0.37.