You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2012/10/31 11:30:22 UTC

[Bug 54080] New: Comma related bug in org.apache.catalina.valves.RemoteIpValve

https://issues.apache.org/bugzilla/show_bug.cgi?id=54080

          Priority: P2
            Bug ID: 54080
          Assignee: dev@tomcat.apache.org
           Summary: Comma related bug in
                    org.apache.catalina.valves.RemoteIpValve
          Severity: normal
    Classification: Unclassified
                OS: Linux
          Reporter: simon.dean@moneysupermarket.com
          Hardware: PC
            Status: NEW
           Version: 6.0.35
         Component: Catalina
           Product: Tomcat 6

Hi 

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"
        internalProxies="10\.\d{1,3}\.\d{1,3}\.\d{1,3}"
        protocolHeader="Example-Header"
        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

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


[Bug 54080] Comma related bug in org.apache.catalina.valves.RemoteIpValve

Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54080

Simon Dean <si...@moneysupermarket.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simon.dean@moneysupermarket
                   |                            |.com

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


[Bug 54080] Comma related bug in org.apache.catalina.valves.RemoteIpValve

Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54080

--- Comment #1 from Christopher Schultz <ch...@christopherschultz.net> ---
Created attachment 29545
  --> https://issues.apache.org/bugzilla/attachment.cgi?id=29545&action=edit
Documentation patch

Patch which hopefully clarifies the dangers of using commas in regular
expressions.

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


[Bug 54080] Comma related bug in org.apache.catalina.valves.RemoteIpValve

Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54080

Konstantin Kolinko <kn...@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
          Component|Catalina                    |Documentation
         Resolution|---                         |FIXED

--- Comment #3 from Konstantin Kolinko <kn...@gmail.com> ---
Tomcat 6 documentation corrected by r1444396 , will be in 6.0.37.

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


[Bug 54080] Comma related bug in org.apache.catalina.valves.RemoteIpValve

Posted by bu...@apache.org.
https://issues.apache.org/bugzilla/show_bug.cgi?id=54080

--- Comment #2 from Konstantin Kolinko <kn...@gmail.com> ---
It should be possible to write '\d{1,3}' simply as '\d\d?\d?'.

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org