the realip module and multiple X-Forwarded-For lines

Thu Jul 17 18:58:53 MSD 2008

Evan,

thank you very much for reviewing what i came up with.  yes, my coding style
is very much like a guy who hasn't done anything with C since 1994, and who
wasn't trying or expecting to make something like an nginx-committer would
make given
typical time pressures of my job and being a newbie to the internals of
nginx.

thanks again!

Martin Stitt

On Wed, Jul 16, 2008 at 11:46 PM, Evan Miller <emmiller at gmail.com> wrote:

> 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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://nginx.org/pipermail/nginx/attachments/20080717/71061d29/attachment.html>