Bug 61355

Created attachment 36295 [details]
Bug 61355 - make http_scheme use X-Forwarded-Proto header

As DirectorySlash honors the scheme given in ServerName the scheme in http_scheme() from http_core.c should respect X-Forwarded-Proto header instead of fixing mod_dir.c for this bug. This has been implemented in the attached patch. Should also be mentioned in the doc. Add respect of X-Forwarded-Proto to http_scheme() instead of fixing mod_dir

It should be noted that this introduces a monstrous security hole.

mod_remoteip uses explicit lists of trusted peers to pass valid X-F-F data for
interpretation. This hack is no different in trust requirements in order for the
project to consider this submission.

Ok - but then following config is the same threat:

SetEnvIf X-Forwarded-Proto https HTTPS=on
SetEnvIf X-Forwarded-Proto https REQUEST_SCHEME=https

And this is recommended everywhere to do!

Anyways i will try to create a patch for mod_remoteip
which uses the list of trusted peers.

Created attachment 36337 [details]
get the proxied protocol from header with mod_remoteip

This patch adds information about the proxied protocol into the server-request using a new header in mod_remoteip. The trust-requirements are the same as with the remote ip. The path includes also a patch for the documentation of mod_remoteip.

> [The] following config is the same threat:
> 
> SetEnvIf X-Forwarded-Proto https HTTPS=on
> SetEnvIf X-Forwarded-Proto https REQUEST_SCHEME=https
> 
> And this is recommended everywhere to do!

Yes. That is a threat, unless the internally-trusted front end ahead of all
external routes to that server unilaterally clears and then forces the true
value of the X-F-P header. When you do see that recommended, you would be
doing a great service to comment on the potential hazard of those directives.

Thank you for your patch submission. Entirely returned from holiday schedules,
so I'll examine your patch shortly.

In general I like the patch very much.

But there is a problem with the proposal;
+    <example><title>Proxy Example</title>
+    <highlight language="config">
+        RemoteIPProtoHeader X-Forwarded-Proto
+        </highlight>
+    </example>

Can you suggest any case where it would be legitimate to accept a different protocol
other than the true protocol used to deliver the request across the internet?

This seems like an entirely-intranet convention, should use only the trusted Internal
proxy list, and accept any protocol (not only HTTPS) presented by that internal
gateway agent. WDYT?

sorry - i do not understand your question. In apaches http_core.c the schemas apache uses are fixed to http and https - see this comment in http_scheme():

    /*
     * The http module shouldn't return anything other than
     * "http" (the default) or "https".
     */

so i reduced the patch also to these protocols.

I needed this behaviour not in an intranet environment but in a real-live website which is running behind haproxy which also does SSL-offloading. This page uses an iframe in which links to directories are generated - these were redirected to trailing-slash versions by mod_dir with the wrong protocol. So i needed this solution and did not like the ServerName solution, because this does not work when both http and https are available. In apache the config is only available once, because of the haproxy loadbalancer in front of apache and apache sees no difference in the requests other than the x-forwarded-proto header.

(In reply to Axel Reinhold from comment #7)
> sorry - i do not understand your question. In apaches http_core.c the
> schemas apache uses are fixed to http and https - see this comment in
> http_scheme():
> 
>     /*
>      * The http module shouldn't return anything other than
>      * "http" (the default) or "https".
>      */
> 
> so i reduced the patch also to these protocols.

That makes sense!

> I needed this behaviour not in an intranet environment but in a real-live
> website which is running behind haproxy which also does SSL-offloading. This
> page uses an iframe in which links to directories are generated - these were
> redirected to trailing-slash versions by mod_dir with the wrong protocol. So
> i needed this solution and did not like the ServerName solution, because
> this does not work when both http and https are available. In apache the
> config is only available once, because of the haproxy loadbalancer in front
> of apache and apache sees no difference in the requests other than the
> x-forwarded-proto header.

So I just want to clarify, both PROXY protocol and RemoteIPInternalProxy
lists represent the intranet, absolutely safe routes which can be trusted,
so they should toggle the decoding of the designated RemoteIPProtoHeader.

If the route comes instead only through RemoteIPTrustedProxy or through
unrecognized proxies, the protocol should not be overridden, IMO. Would 
you concur?

Looks like we are close to having a patch to commit to trunk for further
feedback and potential backporting.

In my configuration the proxy is in my local virtual network which has no routing outside the virtual machines running on the host:

# grep -i remoteip *
modules.conf:LoadModule remoteip_module modules/mod_remoteip.so
httpd.conf:<IfModule mod_remoteip.c>
httpd.conf:RemoteIPHeader X-Forwarded-For
httpd.conf:RemoteIPProtoHeader X-Forwarded-Proto
httpd.conf:RemoteIPInternalProxy 192.168.37.37

The patch is running in production since two weeks without any issue.

(In reply to William A. Rowe Jr. from comment #8)
> 
> Looks like we are close to having a patch to commit to trunk for further
> feedback and potential backporting.

Do you think, it will be merged?

I've used the patch in production on 2.4.43 and had random Segmentation faults. Fixed by replacing:

+            r->server->server_scheme = scheme;

with:

+            r->server->server_scheme = "https";

(In reply to Cyril B. from comment #11)
> I've used the patch in production on 2.4.43 and had random Segmentation
> faults. Fixed by replacing:
> 
> +            r->server->server_scheme = scheme;
> 
> with:
> 
> +            r->server->server_scheme = "https";

This is a good catch, but I think both approaches are incorrect. It is not valid for a per-request header to change the state of anything like r->-server->* . 

In your case it will probably not lead to incorrect results since it does not actually vary per-request, but then you should also just use a scheme in the ServerName directive if it does not vary.

I think the approach that fits w/ the Apache API is for something like mod_remoteip to implement the http_scheme callback to override the server scheme.

Or, mod_dir should stop calling ap_construct_url and just return non fully qualified URL's and let the browser sort it out. This was not spec in the original HTTP/1.1 RFC but was in use forever and is valid now.

I had a similar issue with a reverse proxy ensuring the TLS termination and forwarding requests to Apache as http with X-Forwarded-Proto set.

Since we eventually phased out HTTP entirely for public consumption, the canonical URL, the canonical URL always have https:// . I have simply added it to the ServerName directive since that is intended to represent the canonical URL:

 ServerName https://www.example.org
 DirectorySlash On

It is used by mod_dir when crafting the redirect and solved the issue for us :]



( which was https://phabricator.wikimedia.org/T213509 )

What about just making DirectorySlash output a relative Location, rather than an absolute one?

-----
diff --git a/modules/mappers/mod_dir.c b/modules/mappers/mod_dir.c
index d13babf818..b5b26bb68e 100644
--- a/modules/mappers/mod_dir.c
+++ b/modules/mappers/mod_dir.c
@@ -292,8 +292,8 @@ static int fixup_dir(request_rec *r)
                                 "/", NULL);
         }

-        apr_table_setn(r->headers_out, "Location",
-                       ap_construct_url(r->pool, ifile, r));
+        apr_table_setn(r->headers_out, "Location", ifile);
+
         return HTTP_MOVED_PERMANENTLY;
     }
-----

That way there’s no change to http_scheme() and thus no effect elsewhere.

Pull request with my last suggestion:

https://github.com/apache/httpd/pull/325

Also note that https://bz.apache.org/bugzilla/show_bug.cgi?id=59267 reports a similar issue.