The realip module and multiple X-Forwarded-For lines

Martin_Stitt · July 16, 2008, 9:47pm

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/1.1: HTTP Message

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

Martin_Stitt · July 17, 2008, 6:53am

Martin Stitt wrote:

seems workable", or “you’ve overlooked this whole other situation…”.
http://www.infoworld.com/article/07/11/12/46TC-citrix-netscaler_1.html

rightmost ip number. this seems to work ok based on the emperical
If-Modified-Since: Mon, 05 May 2008 21:51:55 GMT; length=4732
the first-encountered X-Forwarded-For line is used and any remaining
Multiple message-header fields with the same field-name MAY be

operate properly.
./nginx-0.6.32/src/http/ngx_http_request.c
from the request pool. my belief is that i don’t need to explicitly

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:

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.
I think the logged messages here should be at level “debug”, not
“info”.
As a matter of patch politesse, you should probably change the mjs_
prefixes and get rid of the commented-out code.
Good catch with ngx_inet_addr.

Evan

Martin_Stitt · July 17, 2008, 5:07pm

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