the realip module and multiple X-Forwarded-For lines

[Martin Stitt]

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

or a single line with a set of comma separated values:

  X-Forwarded-For: 66.249.70.40, 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
  X-Forwarded-For: 10.1.1.69
  Cache-Control: max-age=259200
  Connection: keep-alive
  X-Forwarded-For: 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.

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, 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://nginx.org/pipermail/nginx/attachments/20080716/c2de3feb/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mjs_nginx_mods_07_2008_v1.patch
Type: text/x-patch
Size: 4891 bytes
Desc: not available
URL: <http://nginx.org/pipermail/nginx/attachments/20080716/c2de3feb/attachment.bin>