Incorrect trusted proxy match test in mod_remoteip

[William Shallum]

Hi,

I checked and the latest version in trusty-updates is missing the patch for PR 54651 (link below):

https://svn.apache.org/viewvc?view=revision&revision=1569006

This fixes the case where mod_remoteip trusts multiple IP addresses in X-Forwarded-For if the client IP is trusted. This allows anyone to spoof the remote address by sending an X-Forwarded-For header to a trusted proxy (which will append its own IP to it).

Is it possible to ship this patch in trusty-updates?

To answer the common questions:

$ lsb_release -rd
Description: Ubuntu 14.04.3 LTS
Release: 14.04

$ apt-cache policy apache2-bin
apache2-bin:
  Installed: 2.4.7-1ubuntu4.8
  Candidate: 2.4.7-1ubuntu4.8
  Version table:
 *** 2.4.7-1ubuntu4.8 0
        500 http://kartolo.sby.datautama.net.id/ubuntu/ trusty-updates/main amd64 Packages
        100 /var/lib/dpkg/status
     2.4.7-1ubuntu4.5 0
        500 http://security.ubuntu.com/ubuntu/ trusty-security/main amd64 Packages
     2.4.7-1ubuntu4 0
        500 http://kartolo.sby.datautama.net.id/ubuntu/ trusty/main amd64 Packages

Expected to happen: mod_remoteip should use the rightmost X-Forwarded-For entry if the client IP is in the trusted proxy list. It should then use the second rightmost entry if the rightmost entry is in the trusted proxy list, and so on.

What happened instead: mod_remoteip always checks the client IP against the trusted proxy list as it goes down the X-Forwarded-For entries. It will always set the remote IP to the leftmost entry in X-Forwarded-For if the client IP is trusted.

Regards,
William

[Eugenel (eugenel)]

I have confirmed a bug in mod_remoteip.c's remoteip_modify_request function.

This bug was reported by <email address hidden> in 2012 in this thread:

http://mail-archives.apache.org/mod_mbox/httpd-users/201210.mbox/%<email address hidden>%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

[Mike-rumph (mike-rumph)]

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

[Mike-rumph (mike-rumph)]

Created attachment 31174
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.

[Mike-rumph (mike-rumph)]

Committed to trunk in https://svn.apache.org/viewvc?view=revision&revision=1564052 and proposed for httpd 2.4.x.

[Mike-rumph (mike-rumph)]

Backported to httpd 2.4.8 by r1569006.

[Robie Basak (racb)]

Thank you for taking the time to report this bug and helping to make Ubuntu better.

Looks like this was fixed upstream in 2.4.8 so it is fixed in Vivid, Wily and Xenial, but affects Trusty. It looks like this should be trivial to cherry-pick to Trusty. I will put this on the server team's backlog.

In the meantime, please could you provide exact steps to reproduce this issue so that we are able to verify that the fix works?

And if you'd like to help further, the full procedure is documented at https://wiki.ubuntu.com/StableReleaseUpdates#Procedure. Thanks!

[Robie Basak (racb)]

The patch applied to the upstream 2.4 branch is https://svn.apache.org/viewvc/httpd/httpd/branches/2.4.x/modules/metadata/mod_remoteip.c?r1=1569003&r2=1569006&pathrev=1569006

[William Shallum (william)]

Steps to reproduce (not exact):

apache config:

LoadModule rewrite_module modules/mod_rewrite.so
LoadModule remoteip_module modules/mod_remoteip.so

Listen 18000
<VirtualHost *:18000>
        RemoteIPHeader X-Forwarded-For
        RemoteIPTrustedProxy 127.0.0.1
        RewriteEngine on
        RewriteRule ^/?(.*) http://test.invalid/%{REMOTE_ADDR} [R=301,L]
</VirtualHost>

Let's assume we are a proxy on 127.0.0.1.

If a connection comes from 1.2.3.4 without an existing header we will set X-Forwarded-For: 1.2.3.4 and Apache should trust us.

curl -vH 'X-Forwarded-For: 1.2.3.4' 'http://127.0.0.1:18000/'
...
< Location: http://test.invalid/1.2.3.4
...

This is OK as the connection comes from 127.0.0.1 and it is trusted to present the IP 1.2.3.4

If a connection comes from 1.2.3.4 with an existing "X-Forwarded-For: 5.6.7.8", we should add the IP 1.2.3.4 at the end, like so:

curl -vH 'X-Forwarded-For: 5.6.7.8, 1.2.3.4' 'http://127.0.0.1:18000/'
...
< Location: http://test.invalid/5.6.7.8
...

This shows that Apache thinks the REMOTE_ADDR should be 5.6.7.8. This is not OK as the IP 5.6.7.8 comes from 1.2.3.4 and 1.2.3.4 is not trusted.

Expected:

After the patch is applied

curl -vH 'X-Forwarded-For: 5.6.7.8, 1.2.3.4' 'http://127.0.0.1:18000/'
...
< Location: http://test.invalid/1.2.3.4
...

[Wesley Wiedenmeier (wesley-wiedenmeier)]

Here's an updated debdiff to apply this patch, with better formatting for the changelog entry

[Wesley Wiedenmeier (wesley-wiedenmeier)]

It looks to me like the patched version resolves the bug. If anyone else wants to test before uploading there is a deb in my ppa here:
ppa:wesley-wiedenmeier/test2

[Wesley Wiedenmeier (wesley-wiedenmeier)]

Before this is uploaded it would be good if anyone who has been experienceing this bug could that the patched version resolves the issue for them, thanks.

The updated deb is available here:
https://launchpad.net/~wesley-wiedenmeier/+archive/ubuntu/test2/+build/10037671

[Wesley Wiedenmeier (wesley-wiedenmeier)]

There was recently a security update for apache2 that went in to trusty-updates, so I need to remake the debdiff based off of the updated version in trusty-proposed as the security update took the package version that the above debdiff had. When that is done I will update the version in ppa if anyone wants to test.

[Wesley Wiedenmeier (wesley-wiedenmeier)]

Here is the updated debdiff. A build is running with it, which I will test soon. It will be available in ppa:wesley-wiedenmeier/test2

It uses the version number '2.4.7-1ubuntu4.14'. There is another sru that I am working to get approved for apache2 that also uses 'ubuntu4.14'. If that one gets uploaded first I will update this patch again.

[Wesley Wiedenmeier (wesley-wiedenmeier)]

The build of apache 2.4.7-1ubuntu4.14 in the ppa appears to work for me.
It would be good if the original bug reporter could test the build in the ppa in order to verify it before it goes into trusty-proposed though, thanks.

Here is the test case I used:
root@ubuntu:~$ apt-cache show apache2 | grep Version
  Version: 2.4.7-1ubuntu4.13
root@ubuntu:/etc/apache2# a2enmod rewrite remoteip
root@ubuntu:~$ cat /etc/apache2/sites-available/remoteip.conf
  <VirtualHost *:80>
      RemoteIPHeader X-Forwarded-For
      RemoteIPTrustedProxy 127.0.0.1
      RewriteEngine on
      RewriteRule ^/?(.*) http://test.invalid/%{REMOTE_ADDR} [R=301,L]
  </VirtualHost>
root@ubuntu:/home/ubuntu# a2dissite 000-default
root@ubuntu:/home/ubuntu# a2ensite remoteip
root@ubuntu:/home/ubuntu# service apache2 restart
root@ubuntu:/home/ubuntu# curl -vH 'X-Forwarded-For: 1.2.3.4' http://127.0.0.1 2>&1 | grep Location
  < Location: http://test.invalid/1.2.3.4
root@ubuntu:/home/ubuntu# curl -vH 'X-Forwarded-For: 5.6.7.8, 1.2.3.4' http://127.0.0.1 2>&1 | grep Location
  < Location: http://test.invalid/5.6.7.8

root@ubuntu:/home/ubuntu# apt-add-repository ppa:wesley-wiedenmeier/test2
root@ubuntu:/home/ubuntu# apt-get update
root@ubuntu:/home/ubuntu# apt-get install apache2
root@ubuntu:/home/ubuntu# apt-cache show apache2 | grep Version
  Version: 2.4.7-1ubuntu4.14
root@ubuntu:/home/ubuntu# service apache2 restart
root@ubuntu:/home/ubuntu# curl -vH 'X-Forwarded-For: 1.2.3.4' http://127.0.0.1 2>&1 | grep Location
  < Location: http://test.invalid/1.2.3.4
root@ubuntu:/home/ubuntu# curl -vH 'X-Forwarded-For: 5.6.7.8, 1.2.3.4' http://127.0.0.1 2>&1 | grep Location
  < Location: http://test.invalid/1.2.3.4

[Wesley Wiedenmeier (wesley-wiedenmeier)]

Since a SRU for (LP: #1534538) was recently uploaded to trusty-proposed using the same version string as the current debdiff, I have redone the patch on top of the version of apache currently in trusty-proposed. The updated debdiff is attached.

A package with the updated debdiff is currently building in ppa:wesley-wiedenmeier/test2 if anyone would like to test it out. Since I have already run through a test case twice for this patch, and the patch itself is the same for each version, I will not redo the test case for this version, as it should come out the same. If any users would like to test the patch before it is uploaded, it is available in ppa.

Thanks