Bug 60251 - mod_remoteip discards additional address when mod_rewrite rule is triggered
Summary: mod_remoteip discards additional address when mod_rewrite rule is triggered
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_remoteip (show other bugs)
Version: 2.4.23
Hardware: PC Linux
: P2 normal (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-13 19:20 UTC by Ari Pringle
Modified: 2018-05-05 10:26 UTC (History)
2 users (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ari Pringle 2016-10-13 19:20:02 UTC
I've been testing this on a Debian Jessie build (apache 2.4.10), as well as a Debian Stretch build (apache 2.4.23), with the same results.

In my configuration, mod_remoteip has a single internal trusted proxy, and X-Forwarded-For is evaluated for additional IPs. Under normal circumstances, it correctly "stops" at the first untrusted IP, which becomes REMOTE_ADDR.

However, when a mod_rewrite rule is triggered, it seems to discard that IP address and continue moving up the X-Forwarded-For header, making the second untrusted IP become the REMOTE_ADDR.

I'm including what I believe is the relevant configuration, but let me know if further details are needed:

RemoteIPHeader X-Forwarded-For
RemoteIPInternalProxy ::1
DocumentRoot /app/httpdocs

<Directory /app/httpdocs>
    Require all granted
    AllowOverride None

    RewriteEngine On
    RewriteCond %{REQUEST_FILENAME} !-s
    RewriteRule ^.*$ index.php
</Directory>

In the following tests, I'll connecting from localhost, which is ::1 (the defined RemoteIPInternalProxy). My index.php file is just echoing out the relevant $_SERVER variables.

In the first case, I hit /index.php directly, which does NOT trigger a RewriteRule. The REMOTE_ADDR becomes the right-most untrusted IP address, 3.3.3.3. This is, I believe, the correct behavior:

# curl -H "X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3" http://localhost/index.php
X-Forwarded-For: 1.1.1.1, 2.2.2.2
REMOTE_ADDR: 3.3.3.3

Now, if I hit an invalid URL, the RewriteRule is triggered and rewritten to index.php. It appears that 3.3.3.3 is then discarded:

# curl -H "X-Forwarded-For: 1.1.1.1, 2.2.2.2, 3.3.3.3" http://localhost/invalidurl
X-Forwarded-For: 1.1.1.1
REMOTE_ADDR: 2.2.2.2

----

To show additional behavior, here's a more complicated example that shows that additional InternalProxies AND TrustedProxies are evaluated after the untrusted IP is discarded:

RemoteIPHeader X-Forwarded-For
RemoteIPInternalProxy ::1
RemoteIPTrustedProxy 100.100.100.0/24
RemoteIPProxiesHeader X-Trusted-Proxies

In the first case, the trusted proxy is added to the X-Trusted-Proxies header, and REMOTE_ADDR becomes the first untrusted IP (3.3.3.3). This is the correct behavior:

# curl -H "X-Forwarded-For: 1.1.1.1, 2.2.2.2, 100.100.100.2, ::1, 3.3.3.3, 100.100.100.1" http://localhost/index.php
X-Forwarded-For: 1.1.1.1, 2.2.2.2, 100.100.100.2, ::1
X-Trusted-Proxies: 100.100.100.1
REMOTE_ADDR: 3.3.3.3

But again, by triggering a RewriteRule, the 3.3.3.3 address is discarded, Internal & Trusted proxies seem to be evaluated again (X-Trusted-Proxies is now 100.100.100.2 instead of 100.100.100.1), and the REMOTE_ADDR becomes the second untrusted IP, 2.2.2.2:

# curl -H "X-Forwarded-For: 1.1.1.1, 2.2.2.2, 100.100.100.2, ::1, 3.3.3.3, 100.100.100.1" http://localhost/invalidurl
X-Forwarded-For: 1.1.1.1
X-Trusted-Proxies: 100.100.100.2
REMOTE_ADDR: 2.2.2.2
Comment 1 Ari Pringle 2016-10-13 21:30:00 UTC
I enabled LogLevel rewrite:trace8 remoteip:trace8 on the system, and included the error log output below. It seems that mod_remoteip is getting executed again after mod_rewrite performs the INTERNAL_REDIRECT, and presumably working on already-modified X-Forwarded-For header. Would it be feasible to detect if such a redirect has happened and not re-process the request in that case?

This case seems similar to this patch: https://bz.apache.org/bugzilla/show_bug.cgi?id=49839

For what it's worth, a colleague of mine noted that the code from the above patch doesn't seem to exist in the current mod_remoteip.c source.

---

[Thu Oct 13 21:19:29.913894 2016] [remoteip:trace1] [pid 37] mod_remoteip.c(423): [client 3.3.3.3:53539] Using 3.3.3.3 as client's IP by proxies 100.100.100.1
[Thu Oct 13 21:19:29.914754 2016] [rewrite:trace3] [pid 37] mod_rewrite.c(477): [client 3.3.3.3:53539] 3.3.3.3 - - [localhost/sid#7f85f6945470][rid#7f85f68bd0a0/initial] [perdir /app/httpdocs/] strip per-dir prefix: /app/httpdocs/invalidurl -> invalidurl
[Thu Oct 13 21:19:29.914771 2016] [rewrite:trace3] [pid 37] mod_rewrite.c(477): [client 3.3.3.3:53539] 3.3.3.3 - - [localhost/sid#7f85f6945470][rid#7f85f68bd0a0/initial] [perdir /app/httpdocs/] applying pattern '^.*$' to uri 'invalidurl'
[Thu Oct 13 21:19:29.914853 2016] [rewrite:trace4] [pid 37] mod_rewrite.c(477): [client 3.3.3.3:53539] 3.3.3.3 - - [localhost/sid#7f85f6945470][rid#7f85f68bd0a0/initial] [perdir /app/httpdocs/] RewriteCond: input='/app/httpdocs/invalidurl' pattern='!-s' => matched
[Thu Oct 13 21:19:29.914863 2016] [rewrite:trace2] [pid 37] mod_rewrite.c(477): [client 3.3.3.3:53539] 3.3.3.3 - - [localhost/sid#7f85f6945470][rid#7f85f68bd0a0/initial] [perdir /app/httpdocs/] rewrite 'invalidurl' -> 'index.php'
[Thu Oct 13 21:19:29.914872 2016] [rewrite:trace3] [pid 37] mod_rewrite.c(477): [client 3.3.3.3:53539] 3.3.3.3 - - [localhost/sid#7f85f6945470][rid#7f85f68bd0a0/initial] [perdir /app/httpdocs/] add per-dir prefix: index.php -> /app/httpdocs/index.php
[Thu Oct 13 21:19:29.914884 2016] [rewrite:trace2] [pid 37] mod_rewrite.c(477): [client 3.3.3.3:53539] 3.3.3.3 - - [localhost/sid#7f85f6945470][rid#7f85f68bd0a0/initial] [perdir /app/httpdocs/] strip document_root prefix: /app/httpdocs/index.php -> /index.php
[Thu Oct 13 21:19:29.914892 2016] [rewrite:trace1] [pid 37] mod_rewrite.c(477): [client 3.3.3.3:53539] 3.3.3.3 - - [localhost/sid#7f85f6945470][rid#7f85f68bd0a0/initial] [perdir /app/httpdocs/] internal redirect with /index.php [INTERNAL REDIRECT]
[Thu Oct 13 21:19:29.915247 2016] [remoteip:trace1] [pid 37] mod_remoteip.c(423): [client 2.2.2.2:53539] Using 2.2.2.2 as client's IP by proxies 100.100.100.2
[Thu Oct 13 21:19:29.915304 2016] [rewrite:trace3] [pid 37] mod_rewrite.c(477): [client 2.2.2.2:53539] 2.2.2.2 - - [localhost/sid#7f85f6945470][rid#7f85f68b9a60/initial/redir#1] [perdir /app/httpdocs/] strip per-dir prefix: /app/httpdocs/index.php -> index.php
[Thu Oct 13 21:19:29.915358 2016] [rewrite:trace3] [pid 37] mod_rewrite.c(477): [client 2.2.2.2:53539] 2.2.2.2 - - [localhost/sid#7f85f6945470][rid#7f85f68b9a60/initial/redir#1] [perdir /app/httpdocs/] applying pattern '^.*$' to uri 'index.php'
[Thu Oct 13 21:19:29.915372 2016] [rewrite:trace4] [pid 37] mod_rewrite.c(477): [client 2.2.2.2:53539] 2.2.2.2 - - [localhost/sid#7f85f6945470][rid#7f85f68b9a60/initial/redir#1] [perdir /app/httpdocs/] RewriteCond: input='/app/httpdocs/index.php' pattern='!-s' => not-matched
[Thu Oct 13 21:19:29.915380 2016] [rewrite:trace1] [pid 37] mod_rewrite.c(477): [client 2.2.2.2:53539] 2.2.2.2 - - [localhost/sid#7f85f6945470][rid#7f85f68b9a60/initial/redir#1] [perdir /app/httpdocs/] pass through /app/httpdocs/index.php
Comment 2 Ari Pringle 2016-10-14 16:01:45 UTC
I compiled the latest version of mod_remoteip.c from the github mirror, and this problem seems to be resolved. I believe it was fixed in commit r1688399 from June 2015, although that appears not to have made it into the any httpd releases yet (as of 2.4.23): 
https://mail-archives.apache.org/mod_mbox/httpd-cvs/201506.mbox/%3C20150630084017.C9616AC0043@hades.apache.org%3E

Any idea when this might make its way into a release?
Comment 3 Eric Covener 2016-10-14 16:14:33 UTC
(In reply to Ari Pringle from comment #2)
> I compiled the latest version of mod_remoteip.c from the github mirror, and
> this problem seems to be resolved. I believe it was fixed in commit r1688399
> from June 2015, although that appears not to have made it into the any httpd
> releases yet (as of 2.4.23): 
> https://mail-archives.apache.org/mod_mbox/httpd-cvs/201506.mbox/
> %3C20150630084017.C9616AC0043@hades.apache.org%3E
> 
> Any idea when this might make its way into a release?

Looks like it was not backported.  Will propose it shortly

-    temp_sa = c->client_addr;
+    temp_sa = r->useragent_addr ? r->useragent_addr : c->client_addr;

I think this simplifies to just r->useragent_addr since r->useragent_addr starts out life as a copy of c->client_addr (ap_read_request)

AP_DECLARE(const char *) ap_get_useragent_host(request_rec *r,
                                               int type, int *str_is_ip)
{
    conn_rec *conn = r->connection;
    int hostname_lookups;
    int ignored_str_is_ip;

    /* Guard here when examining the host before the read_request hook
     * has populated an r->useragent_addr
     */
    if (!r->useragent_addr || (r->useragent_addr == conn->client_addr)) { 

This also seems to be a little misleading in kind of the same way.
Comment 4 Christophe JAILLET 2018-03-27 18:24:00 UTC
This has been fixed in 2.4.x in r1767483 and is part of 2.4.24.

I still leave it open because of Eric's last comment.
Should anyone want to check and apply the proposed clean-up.