Bug 50453 - Multiple X-Forwarded-For headers not handled by RemoteIP valve
Multiple X-Forwarded-For headers not handled by RemoteIP valve
Status: RESOLVED FIXED
Product: Tomcat 6
Classification: Unclassified
Component: Catalina
6.0.29
PC Linux
: P2 normal (vote)
: default
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2010-12-10 09:29 UTC by brett.dellegrazie
Modified: 2011-01-07 13:28 UTC (History)
2 users (show)



Attachments
v1 (4.12 KB, patch)
2010-12-20 14:50 UTC, Jim Riggs
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description brett.dellegrazie 2010-12-10 09:29:39 UTC
When a request comes in with multiple X-Forwarded-For headers the RemoteIP valve should be examining all of them in reverse order.

As defined by the standard:
"Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded."

thus:
(a)
X-Forwarded-For: 192.168.0.3
X-Forwarded-For: 222.234.0.4

Is semantically equivalent to:
(b)
X-Forwarded-For: 192.168.0.3, 222.234.0.4

However (a) is not handled by the RemoteIP valve as it only ever looks at the first header.

For reference, this was raised on the HAproxy mailing list:
http://www.formilux.org/archives/haproxy/1012/4122.html
and tomcat user's mailing list:
http://mail-archives.apache.org/mod_mbox/tomcat-users/201012.mbox/%3C4D022C57.1070005@apache.org%3E

Tomcat users suggested raising a bug. Hence this.
Comment 1 Jim Riggs 2010-12-10 09:39:04 UTC
This is due to RemoteIpValve's use of request.getHeader() rather than the enumeration from request.getHeaders().  I'll try to whip up a patch...
Comment 2 Rainer Jung 2010-12-10 10:19:00 UTC
I think there is no given standard concerning whether the first or the last header is the "right" one (coming from the closest proxy). The same for a comma-separated multi-valued header.

mod_remoteip for the Apache Web Server claims:

When multiple, comma delimited remote IP addresses are listed in the header value, they are processed in Right-to-Left order. Processing halts when a given remote IP address is not trusted to present the preceeding IP address. The header field is updated to this remaining list of unconfirmed IP addresses, or if all IP addresses were trusted, this header is removed from the request altogether.

In replacing the remote_ip, the module stores the list of intermediate hosts in a remoteip-proxy-ip-list note, which mod_log_config can record using the %{remoteip-proxy-ip-list}n format token. If the administrator needs to store this as an additional header, this same value can also be recording as a header using the directive RemoteIPProxiesHeader.


So it might be a good idea to handle the IPs from right to left resp. later headers before earlier ones, as long as the previous IP is trusted.
Comment 3 Mark Thomas 2010-12-10 10:46:07 UTC
RFC2616 is clear on how headers should be combined so order is retained. In short
...
X-Forwarded-For : A, B
...
X-Forwarded-For : C
...
X-Forwarded-For : D, E

must be treated exactly the same way as:
X-Forwarded-For: A, B, C, D, E

Tomcat will return values for a header in the correct order although it may return the above as "A, B", "C", "D, E" - I haven't checked.
Comment 4 Christopher Schultz 2010-12-10 14:40:29 UTC
So, should the RemoteIPValve/Filter should be looking at the first X-Forwarded-For (A) or the last X-Forwarded-For (E) address?

It sounds like the reporter was expecting "E" but the spec seems to imply that "A" should be returned.
Comment 5 William A. Rowe Jr. 2010-12-10 15:24:59 UTC
Rainer, the spec is absolutely clear about precidence, the last (rightmost) value is added by the nearest adjacent proxy to the server agent.  Ergo, the first (leftmost) value was added by the proxy closest to the user agent.

mod_remoteip uses this in combination of proxy trust metrics to determine how far back to unwind that from the rightmost value.  Perhaps only the proxies under the control of the server administrator will be trusted, in which case only the last or near-last value will be used, or perhaps all servers are trusted and any arbitrary value presented by any proxy will be accepted, in which case the first value is used.

It entirely depends on what the administrator wants, either trusted values of X-Forwarded-For presented by known proxy agents, or any arbitrary/potentially spammed values.
Comment 6 Jim Riggs 2010-12-10 16:48:05 UTC
(In reply to comment #4)
> So, should the RemoteIPValve/Filter should be looking at the first
> X-Forwarded-For (A) or the last X-Forwarded-For (E) address?

Chris -

It should be looking at them as if they were combined in a single comma-separated list and evaluating them from right-to-left.  As Mark said, regardless of how the header(s) are received, it should be treated as "X-Forwarded-For: A, B, C, D, E" and RemoteIP should "trust" them in the order E - D - C - B - A.  I will make sure that the switch to getHeaders() in my patch does this.
Comment 7 Christopher Schultz 2010-12-13 11:08:42 UTC
> "X-Forwarded-For: A, B, C, D, E" and RemoteIP should "trust" them in the order
> E - D - C - B - A.

I'm not convinced that order matters, here, since all addresses will be checked and if one of them matches, the request will be allowed. Am I missing something?
Comment 8 Mark Thomas 2010-12-13 11:17:55 UTC
Yes. Order is critical in this use case since a) proxy order matters b) not all proxies are trusted. Jim is heading in the right direction.
Comment 9 Jim Riggs 2010-12-20 14:50:27 UTC
Created attachment 26428 [details]
v1

Here is a first pass at this.  I have done rudimentary testing of both the valve and filter.  Both appear to be working correctly, but it would be good if someone with a real-life test/dev setup could run it through the gauntlet.

I tried to make as few changes as possible.  Effectively, all this patch does is concatenate multiple headers into a single comma-delimited string before passing it off to commaDelimitedListToStringArray().  Then processing continues as it did before.

Note that this only addresses the remoteIpHeader (e.g. X-Forwarded-For).  It does not do anything with proxiesHeader or protocolHeader.
Comment 10 Christopher Schultz 2010-12-20 16:13:45 UTC
Don't forget RemoteIPFilter in trunk.
Comment 11 Jim Riggs 2010-12-20 16:23:25 UTC
The attachment patches both the valve and the filter, and my comment says that I tested both.
Comment 12 Christopher Schultz 2010-12-20 16:56:21 UTC
D'oh. This head cold is killing my ability to concentrate on even reading 5 or 6 sentences. Apologies for the noise.
Comment 13 Mark Thomas 2011-01-05 10:08:01 UTC
Fixed in trunk for 7.0.x and will be included in 7.0.6 onwards. I also updated the test cases.

The Valve fix has been proposed for 6.0.x.
Comment 14 Mark Thomas 2011-01-07 13:28:50 UTC
Fixed in Tomcat 6.0.x and will be included in 6.0.30 onwards.