the realip module and multiple X-Forwarded-For lines

[Evan Miller]

Martin Stitt wrote:
> hi,
> 
> we are currently using nginx v0.5.35 in our production environment.
> fast, lean and stable. thanks!
> 
> and, we have run into one problem recently.
> 
> i've devised what i believe is a fix.  the attached a patch file shows
> what i've done.  my hope is that it could be helpful to share this
> patch, and that i might get some feedback, such as: "yes, your fix
> seems workable", or "you've overlooked this whole other situation...".
> 
> i developed this patch against latest stable release, v0.6.32 and have
> been running it on a staging machine.  hoping to get it proofed enough
> to move into production soon.  it was designed to just minimally
> attend to the specific problem i am having.  i realize it's not a
> general-coverage type of solution (more on this below).
> 
> our web server has recently had an appliance known as "netscaler" put
> in front of it.  an overview:
> 
>   http://www.infoworld.com/article/07/11/12/46TC-citrix-netscaler_1.html
> 
> as a result, we now need to count on nginx's realip module to derive
> the remote_addr value based on the X-Forwarded-For headers that come
> through.  we want a remote_addr value that reflects the client's ip
> number (or their most outward proxy) as much as possible to make
> logging meaningful, and so that we can derive location info through
> the geo module.
> 
> some incoming HTTP headers have a single X-Forwarded-For line, with a
> single value:
> 
>   X-Forwarded-For: 66.249.70.42 <http://66.249.70.42>
> 
> or a single line with a set of comma separated values:
> 
>   X-Forwarded-For: 66.249.70.40 <http://66.249.70.40>, 66.249.70.41 
> <http://66.249.70.41>
> 
> both of these cases are handled ok. 
> 
> when multiple values exist the realip module always takes the
> rightmost ip number.  this seems to work ok based on the emperical
> evidence i see from a tcpdump.
> 
> the problem comes in when a header contains multiple X-Forwarded-For
> lines:
> 
>   GET /stylesheets/search.css?1210024315 HTTP/1.0
>   Accept: */*
>   Referer: http://www.somewhere.com/com/usa/en/searches
>   Accept-Language: en-us
>   UA-CPU: x86
>   If-Modified-Since: Mon, 05 May 2008 21:51:55 GMT; length=4732
>   User-Agent: Mozilla/4.0 (compatible; MSIE 7.0; Windows NT 5.1; .NET 
> CLR 1.1.4322; .NET CLR 2.0.50727; InfoPath.1; .NET CLR 3.0.04506.30; 
> .NET CLR 3.0.04506.648)
>   Host: www.somewhere.com <http://www.somewhere.com>
>   X-Forwarded-For: 10.1.1.69 <http://10.1.1.69>
>   Cache-Control: max-age=259200
>   Connection: keep-alive
>   X-Forwarded-For: 64.126.102.126 <http://64.126.102.126>
> 
> the behavior i see in both v0.5.35 and v0.6.32 is that the value from
> the first-encountered X-Forwarded-For line is used and any remaining
> ones seem to be ignored.  so, for the above case, our remote_addr ends
> up being the non-public value of 10.1.1.69 <http://10.1.1.69>.
> 
> after some searching about how multiple lines of the same type are
> supposed to be handled, i found this:
> 
>   http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2
> 
> which contains:
> 
>   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.
> 
> so, for that header above, if i could have the values from the
> multiple lines coalesced, it would be as if i received a single line
> like this:
> 
>   X-Forwarded-For: 10.1.1.69 <http://10.1.1.69>, 64.126.102.126 
> <http://64.126.102.126>
> 
> i have developed a coding change that makes nginx accumulate the
> values for any and all X-Forwarded-For lines within an incoming
> header.  i found that this is almost enough let the realip module
> operate properly.
> 
> once this coding change was put in place, i could proceed with
> testing.  i then discovered a bug within the realip module that makes
> it fail to replace the ip address in some cases.  a string was not
> being properly zero-terminated so, just depending on what came after
> it in memory, the replacement would work sometimes, but not others.  i
> made another coding change to correct this.
> 
> the attached patch file modifies two source files:
> 
>   ./nginx-0.6.32/src/http/ngx_http_request.c
>   ./nginx-0.6.32/src/http/modules/ngx_http_realip_module.c
> 
> the changes to ngx_http_request.c have to do with accumulating
> the values from any and all X-Forwarded-For lines.
> 
> the changes to ngx_http_realip_module.c have to do with making sure
> the ip replacement decision is made with a zero-terminated string.
> 
> when i wrote the value-accumualation code within ngx_http_request.c, i
> ended up needing a local buffer.  i'm allocating that buffer's memory
> from the request pool.  my belief is that i don't need to explicitly
> free this memory since the entire request-based pool will be dropped
> once the request is fully processed.  i do not see any free calls
> being made for any of the other allocations made against this
> request-based pool.  is my presumption correct?
> 
> from my reading of that rfc2616 text, i believe that nginx needs to
> apply this type of value coalescing to any and all cases of multiple
> header lines of the same type.  my modification only applies this
> treatment for the X-Forwarded-For case.  i did this to reduce the
> amount of testing required.
> 
> i hope this info is helpful and that, if i'm overlooking something,
> someone might raise a warning to me.
> 
> thanks in advance...
> 
> martin stitt
> 
> 


Why don't you just make r->headers_in.x_forwarded_for point to the last 
X-F-F header instead of the first? Won't that achieve what you want 
without munging headers?



Some comments on the coding style FWIW:

1. The "max size" seems a little brittle. You should probably see how 
much space is needed for all the IP addresses, allocate that amount, 
then create the new string. Maybe in the header loop save references to 
the X-Forwarded-For headers in an array. Then outside the loop iterate 
over your x_forwarded_for array once to get the length, again to copy 
the values. Check out src/core/ngx_array.c for array usage.

2. I think the logged messages here should be at level "debug", not "info".

3. As a matter of patch politesse, you should probably change the mjs_ 
prefixes and get rid of the commented-out code.

4. Good catch with ngx_inet_addr.


Evan