RE: Comma related bug in org.apache.catalina.valves.RemoteIpValve

2012-11-07 Thread Simon Dean
Chris, 

 -Original Message-
 From: Christopher Schultz [mailto:ch...@christopherschultz.net]
 Sent: 02 November 2012 21:09
 To: Tomcat Users List
 Subject: Re: Comma related bug in
 org.apache.catalina.valves.RemoteIpValve
 
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 Simon,
 
 On 11/2/12 12:27 PM, Simon Dean wrote:
  -Original Message- From: Caldarale, Charles R
  [mailto:chuck.caldar...@unisys.com] Sent: 31 October 2012 20:35
  To: Tomcat Users List Subject: RE: Comma related bug in
  org.apache.catalina.valves.RemoteIpValve
 
  From: André Warnier [mailto:a...@ice-sa.com] Subject: Re: Comma
  related bug in org.apache.catalina.valves.RemoteIpValve
 
  We'll probably end up with something like
  tagregex1,regex2,.../tag.
  Or a single regex, with | between the alternatives (which could be
  a workaround for you now, I guess).
 
  I have a vague memory of a discussion on either the dev or users'
  list about simply removing the comma separation, and using just regex
  standard formats.  As I recall, the final resolution was to remove
  the comma separation in Tomcat 7, but keep it in 6 for compatibility
  - even if it is broken and not completely resolvable.  If you look at
  the RemoteIpValve doc for 7, you'll see there's no mention of
  comma-separated regexes.
 
  Yep. Tomcat 7 is treating the values as whole regexes (taking
 advantage of regular expressions' logical OR operation - the pipe symbol).
 
  In Tomcat 6 though, there is a real bug with the current documentation
  and implementation. The documentation gives example values for
  internalProxies that have commas in the regex. See
  http://tomcat.apache.org/tomcat-6.0-
 doc/config/valve.html#Remote_IP_Va
  lve
  and
  http://tomcat.apache.org/tomcat-6.0-doc/api/org/apache/catalina/valves
  /RemoteIpValve.html
 
 
 Both specify the following as the default value for internalProxies:
 
  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}
 
  But that value won't work if you assigned it to internalProxies
  because it has commas in the {1,3} bit. Worse still, the valve
  silently fails, giving no feedback that there's anything wrong.
 
  Quick remedies would be to change the code and documentation to use
  this as the default:
 
  10\.\d{1,3}\.\d+\.\d{1,3}, 192\.168\.\d+\.\d+, 169\.254\.\d+\.\d+,
  127\.\d+\.\d+\.\d+
 
  Which replaces {1,3} with +.
 
 You missed a few, and you could be safer. Instead of using \d{1,3} in general
 for an octet, I would recommend something like this:
 
 0|1[0-9][0-9]?|2([0-4][0-9]|5[0-5]|[6-9])
 
 This disallows things like 123.456.789.999, though it is a bit more 
 complicated.
 It does not contain any commas, though. For an example on the site, though,
 a simple \d+ should suffice.

I thought it better to go with a simpler regular expression than to guard 
against IPv4 addresses that contain more than 3 digits in a segment (e.g. 
10.0.0.).  The risk of making a mistake in a more complicated regex 
probably outweighs guarding against the edge case (and could Tomcat ever 
receive such an IP address?).  

 
  Also adding a warning about commas to the code and documentation
 would
  also go a long way.
 
 Patches -- especially to the documentation -- are always welcome.

Thanks for the patched Java Documentation I see you've added to the bug report! 
 Is there a way for me to raise a patch for 
http://tomcat.apache.org/tomcat-6.0-doc/config/valve.html#Remote_IP_Valve 

 
 - -chris
 -BEGIN PGP SIGNATURE-
 Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
 Comment: GPGTools - http://gpgtools.org
 Comment: Using GnuPG with Mozilla - http://www.enigmail.net/
 
 iEYEARECAAYFAlCUNncACgkQ9CaO5/Lv0PBamwCePBZqFmdCcGOA8xyoN0R
 4RbRV
 HuIAn38zzplbPoxHuvr9r9JuJKnzavDv
 =8e47
 -END PGP SIGNATURE-
 
 -
 To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
 For additional commands, e-mail: users-h...@tomcat.apache.org
 

Kind regards 
Simon 

-
The information contained in this message may be CONFIDENTIAL and is intended 
for the addressee only. Any unauthorised use, dissemination of the information, 
or copying of this message is prohibited. If you are not the addressee, please 
notify the sender immediately by return e-mail and delete this message. 
Although this e-mail and any attachments are believed to be free of any virus, 
or other defect which might affect any computer or system into which they are 
received and opened, it is the responsibility of the recipient to ensure that 
they are virus free and no responsibility is accepted by Moneysupermarket.com 
Financial Group Limited for any loss or damage from receipt or use thereof. 
The views expressed are of the individual, and do not necessarily reflect the 
views

RE: Comma related bug in org.apache.catalina.valves.RemoteIpValve

2012-11-02 Thread Simon Dean
 -Original Message-
 From: Caldarale, Charles R [mailto:chuck.caldar...@unisys.com]
 Sent: 31 October 2012 20:35
 To: Tomcat Users List
 Subject: RE: Comma related bug in
 org.apache.catalina.valves.RemoteIpValve
 
  From: André Warnier [mailto:a...@ice-sa.com]
  Subject: Re: Comma related bug in
  org.apache.catalina.valves.RemoteIpValve
 
  We'll probably end up with something like
 tagregex1,regex2,.../tag.
  Or a single regex, with | between the alternatives (which could be a
  workaround for you now, I guess).
 
 I have a vague memory of a discussion on either the dev or users' list about
 simply removing the comma separation, and using just regex standard
 formats.  As I recall, the final resolution was to remove the comma
 separation in Tomcat 7, but keep it in 6 for compatibility - even if it is 
 broken
 and not completely resolvable.  If you look at the RemoteIpValve doc for 7,
 you'll see there's no mention of comma-separated regexes.

Yep.  Tomcat 7 is treating the values as whole regexes (taking advantage of 
regular expressions' logical OR operation - the pipe symbol).  

In Tomcat 6 though, there is a real bug with the current documentation and 
implementation. The documentation gives example values for internalProxies that 
have commas in the regex.  See 
http://tomcat.apache.org/tomcat-6.0-doc/config/valve.html#Remote_IP_Valve and 
http://tomcat.apache.org/tomcat-6.0-doc/api/org/apache/catalina/valves/RemoteIpValve.html
Both specify the following as the default value for internalProxies: 

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}

But that value won't work if you assigned it to internalProxies because it has 
commas in the {1,3} bit.  Worse still, the valve silently fails, giving no 
feedback that there's anything wrong.  

Quick remedies would be to change the code and documentation to use this as the 
default: 

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

Which replaces {1,3} with +.  

Also adding a warning about commas to the code and documentation would also go 
a long way.   

 
 The moral of the story: upgrade.

 
  - Chuck

-
The information contained in this message may be CONFIDENTIAL and is intended 
for the addressee only. Any unauthorised use, dissemination of the information, 
or copying of this message is prohibited. If you are not the addressee, please 
notify the sender immediately by return e-mail and delete this message. 
Although this e-mail and any attachments are believed to be free of any virus, 
or other defect which might affect any computer or system into which they are 
received and opened, it is the responsibility of the recipient to ensure that 
they are virus free and no responsibility is accepted by Moneysupermarket.com 
Financial Group Limited for any loss or damage from receipt or use thereof. 
The views expressed are of the individual, and do not necessarily reflect the 
views of Moneysupermarket.com Financial Group Limited.
Moneysupermarket.com Limited is an appointed representative of 
Moneysupermarket.com Financial Group Limited, which is authorised and regulated 
by the Financial Services Authority (FSA FRN 303190). 
Moneysupermarket.com Financial Group Limited, registered in England No. 
3157344. 
Registered Office: Moneysupermarket House, St. David’s Park, Ewloe, CH5 3UZ. 
Telephone 01244 665700.


-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org



RE: Comma related bug in org.apache.catalina.valves.RemoteIpValve

2012-10-31 Thread Simon Dean
 -Original Message-
 From: Christopher Schultz [mailto:ch...@christopherschultz.net]
 Sent: 31 October 2012 17:18
 To: Tomcat Users List
 Subject: Re: Comma related bug in
 org.apache.catalina.valves.RemoteIpValve
 
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 Simon,
 
 On 10/30/12 1:39 PM, Simon Dean wrote:
  I'm using Tomcat 6.0.35 on Windows 7 and RHEL 6.x.   I think I've
  stumbled upon a bug in org.apache.catalina.valves.RemoteIpValve.
 
 I think you have, too.
 
 Please log this in Tomcat's bugzilla:
 https://issues.apache.org/bugzilla/enter_bug.cgi?product=Tomcat%206

Thanks Chris.  I've now raised: 
https://issues.apache.org/bugzilla/show_bug.cgi?id=54080

 
 Thanks,
 - -chris
 -BEGIN PGP SIGNATURE-
 Version: GnuPG/MacGPG2 v2.0.17 (Darwin)
 Comment: GPGTools - http://gpgtools.org
 Comment: Using GnuPG with Mozilla - http://www.enigmail.net/
 
 iEYEARECAAYFAlCRXS4ACgkQ9CaO5/Lv0PDUoACfeydqUq443assy94UqMd16f
 Xv
 348AoIwaVHXf1/AhlQJeoR1EFjZvXdAO
 =bnWL
 -END PGP SIGNATURE-
 
 -
 To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
 For additional commands, e-mail: users-h...@tomcat.apache.org
 

-
The information contained in this message may be CONFIDENTIAL and is intended 
for the addressee only. Any unauthorised use, dissemination of the information, 
or copying of this message is prohibited. If you are not the addressee, please 
notify the sender immediately by return e-mail and delete this message. 
Although this e-mail and any attachments are believed to be free of any virus, 
or other defect which might affect any computer or system into which they are 
received and opened, it is the responsibility of the recipient to ensure that 
they are virus free and no responsibility is accepted by Moneysupermarket.com 
Financial Group Limited for any loss or damage from receipt or use thereof. 
The views expressed are of the individual, and do not necessarily reflect the 
views of Moneysupermarket.com Financial Group Limited.
Moneysupermarket.com Limited is an appointed representative of 
Moneysupermarket.com Financial Group Limited, which is authorised and regulated 
by the Financial Services Authority (FSA FRN 303190). 
Moneysupermarket.com Financial Group Limited, registered in England No. 
3157344. 
Registered Office: Moneysupermarket House, St. David’s Park, Ewloe, CH5 3UZ. 
Telephone 01244 665700.


Comma related bug in org.apache.catalina.valves.RemoteIpValve

2012-10-30 Thread Simon Dean
Hi 

I'm using Tomcat 6.0.35 on Windows 7 and RHEL 6.x.   I think 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 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) Update 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) Update 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 ***

It's worth cheching whether RemoteIpValve  and RemoveIpFilter in Tomcat 7.x are 
affected by the same bug.  


Kind regards 
Simon Dean
  

-
The information contained in this message may be CONFIDENTIAL and is intended 
for the addressee only. Any unauthorised use, dissemination of the information, 
or copying of this message is prohibited. If you are not the addressee, please 
notify the sender immediately by return e-mail and delete this message. 
Although this e-mail and any attachments are believed to be free of any virus, 
or other defect which might affect any computer or system into which they are 
received and opened, it is the responsibility of the recipient to ensure that 
they are virus free and no responsibility is accepted by Moneysupermarket.com 
Financial Group Limited for any loss or damage from receipt or use thereof. 
The views expressed are of the individual, and do not necessarily reflect the 
views of Moneysupermarket.com Financial Group Limited.
Moneysupermarket.com Limited is an appointed representative of 
Moneysupermarket.com Financial Group Limited, which is authorised and regulated 
by the Financial Services Authority (FSA FRN 303190). 
Moneysupermarket.com Financial Group Limited, registered in England No. 
3157344. 
Registered Office: Moneysupermarket House, St. David’s Park, Ewloe, CH5 3UZ. 
Telephone 01244 665700.


-
To unsubscribe, e-mail: users-unsubscr...@tomcat.apache.org
For additional commands, e-mail: users-h...@tomcat.apache.org