Bug 54651 - mod_remoteip ends up trusting IPs that it doesn't check
Summary: mod_remoteip ends up trusting IPs that it doesn't check
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_remoteip (show other bugs)
Version: 2.5-HEAD
Hardware: All Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords: FixedInTrunk, PatchAvailable
Depends on:
Blocks:
 
Reported: 2013-03-07 01:25 UTC by EugeneL
Modified: 2014-02-17 16:55 UTC (History)
1 user (show)



Attachments
Patch to use correct IP address for trusted proxy comparison. (897 bytes, patch)
2014-01-06 16:30 UTC, Mike Rumph
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description EugeneL 2013-03-07 01:25:50 UTC
I have confirmed a bug in mod_remoteip.c's remoteip_modify_request function.

This bug was reported by yoshinori.ehara@gmail.com in 2012 in this thread:

http://mail-archives.apache.org/mod_mbox/httpd-users/201210.mbox/%3CCAHa2qaJSW7Hvk68grWMbbiFSA=zAxQ1nr_-A-K-pDWbAB0Gd1Q@mail.gmail.com%3E

The bug appears to still be in httpd/trunk.

The bug here is that, even though temp_sa gets assigned to a new IP with every iteration of the while-loop, the apr_ipsubnet_test continues to check the list of proxy match_ip against the same connection IP (using c->client_addr) over and over again.  Thus, if c->client_addr matches, the code always walks to the very beginning of the X-Forwarded-For header.


--- modules/metadata/mod_remoteip.c	(revision 1407459)
+++ modules/metadata/mod_remoteip.c	(working copy)
@@ -246,16 +246,16 @@
     temp_sa = c->client_addr;

     while (remote) {

-        /* verify c->client_addr is trusted if there is a trusted proxy list
+        /* verify temp_sa is trusted if there is a trusted proxy list
          */
         if (config->proxymatch_ip) {
             int i;
             remoteip_proxymatch_t *match;
             match = (remoteip_proxymatch_t *)config->proxymatch_ip->elts;
             for (i = 0; i < config->proxymatch_ip->nelts; ++i) {
-                if (apr_ipsubnet_test(match[i].ip, c->client_addr)) {
+                if (apr_ipsubnet_test(match[i].ip, temp_sa)) {
                     internal = match[i].internal;
                     break;
                 }
             }

The fix is to replace apr_ipsubnet_test(match[i].ip, c->client_addr) with apr_ipsubnet_test(match[i].ip, temp_sa) , and to correct the mention of c->client_addr comment.  Once fixed, the module works great.


To reproduce this bug, you have to setup mod_remoteip with these directives:

RemoteIPHeader X-Forwarded-For
RemoteIPInternalProxy 127.0.0.1

Then, hit make two requests:

1) curl --header 'X-Forwarded-For: 1.2.3.4' http://localhost:80/
2) curl --header 'X-Forwarded-For: 1.2.3.4, 5.6.7.8' http://localhost:80/

For (1) the r->useragent_ip logged is expected to be 1.2.3.4 .  The code behaves correctly for this case.

For (2) the r->useragent_ip logged should be 5.6.7.8 .  The current code logs 1.2.3.4 still.  This is not the behavior as documented because 5.6.7.8 is not configured to be "trusted".

EugeneL
Comment 1 Mike Rumph 2013-12-16 19:12:12 UTC
Hello Eugene,

Thanks for pointing out this bug report on the Apache httpd dev mailing list.
It answers a mystery I had with regard to bug 55635.
As you can see in comment 1 (of 55635), I submitted results that were somewhat different from those of the bug reporter.
In comment 3, I gave an explanation of the bug reporter's results.
But that did not explain my own results.

Once I applied your patch from this bug, my results matched those of the bug reporter in 55635.
It appears that the bug reporter in 55635 had the patch for 54651 applied.
So I confirm that your patch is indeed valid and useful.

Since I am a relatively new developer (1 year) for the Apache httpd project, I do not have committer access.
Perhaps there is a committer who is interested in mod_remoteip that will consider committing the 54651 patch to trunk.


Thanks again,

Mike Rumph
Comment 2 Mike Rumph 2014-01-06 16:30:50 UTC
Created attachment 31174 [details]
Patch to use correct IP address for trusted proxy comparison.

Worked the patch in the bug description as an attachment.
This patch is an essential fix for mod_remoteip to function properly.
This fix should be receiving some attention.

Without this fix the remoteip_modify_request() function in mod_remoteip.c will not be using the correct IP address for comparison against the trusted proxy list when the RemoteIPHeader header value is a list.
The first pass of the while will work correctly,
but the subsequent passes will not.
Comment 3 Mike Rumph 2014-02-03 21:33:42 UTC
Committed to trunk in https://svn.apache.org/viewvc?view=revision&revision=1564052 and proposed for httpd 2.4.x.
Comment 4 Mike Rumph 2014-02-17 16:55:00 UTC
Backported to httpd 2.4.8 by r1569006.