Bug 57832 - Reduction of response splitting attacks consequences in mod_proxy
Summary: Reduction of response splitting attacks consequences in mod_proxy
Status: RESOLVED FIXED
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_proxy (show other bugs)
Version: 2.4.12
Hardware: All All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-04-19 19:21 UTC by regilero
Modified: 2016-11-14 12:26 UTC (History)
2 users (show)



Attachments
proposed patch on httpd 2.4.12 (3.68 KB, patch)
2015-04-25 07:31 UTC, regilero
Details | Diff
Proposed patch for trunk (3.84 KB, patch)
2015-04-29 21:18 UTC, Yann Ylavic
Details | Diff
Patch for 2.4.x (r1516930+r1516965+r1530603+r1530792+r1656259+PR57832) (40.86 KB, patch)
2015-05-13 09:29 UTC, Yann Ylavic
Details | Diff
Proposed patch for 2.4.x on top of r1656259 (1.82 KB, patch)
2015-05-13 09:46 UTC, Yann Ylavic
Details | Diff
Proposed patch for 2.4.x on top of r1656259 (3.03 KB, patch)
2015-05-17 22:51 UTC, Yann Ylavic
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description regilero 2015-04-19 19:21:52 UTC
The situation is a backend which is not apache httpd and where a successful
response splitting attack can be done (It can be because of an application flaw
or because of a bad implementation of something in the http protocol), and
mod_proxy is used as a reverse proxy for this backend.

I'd like mod_proxy to implement a basic detection of a bad behavior from the
backend by closing the keep-alive connection to this backend.

The current situation with Apache httpd 2.4.12 and before is that mod_proxy
trust the backend and does not even detect obvious problems, like having new 
http responses coming back from a backend before sending new requests.

Note that this was discussed previously in the security mailing list. The
problem is not on httpd side, so it's not a security problem, it is simply an
hardening feature for mod_proxy to reduce impacts of bad things when they
happen.

For example, here is what happen if the response splitting attacks generates two
responses when mod_proxy sends one request to the backend.


     HTTP Client    | mod_proxy       |  Compromised backend
                    |                 |
      query1 -------|--> query1       |
                    |      \----------|--> sending back 2 responses
                    |                 |   response1+BAD response
                    |  read only  <---|---------/
                    |  response1 [*]  |
                    |      /          |
      receive  <----|------           |
     response1      |                 |
                    |    [ BAD response is still there, stacked
                    |     in the mod_proxy<->backend connection
                    |     if the keep-alive status is maintained ]
                    |                 |
      query2 -------|--> query2  [**] |
                    |      \----------|--> sending back
                    |                 |      response2
                    |      read <-----|------/
                    |    BAD response | <======= here is the problem
      receive  <----|----/            |
       BAD response |

This can be tested also by sending a pipeline of two queries, where the first
query contains a splitting attack for the backend. mod_proxy will send back the
splitted response for the second pipelined query (for tests you can also use a
php script sending two http responses instead of one, as described below).

If the splitting attack can generate 100 http responses, theses 100 responses
will be present in the socket connection to the backend. The next 100 queries
from mod_proxy directed to the same backend and reusing this tainted connection
from the pool will read theses false 100 responses, one by one, and send the
results to the wrong users (or to the wrong caches entries).

The main problem here is not in httpd, it's the compromised backend, but shit
happens.

What mod_proxy could detect, is that this backend was sending http responses
without any http request; This could be detected in [*] or [**], i.e. when the
1st response is extracted from the brigade or when the connection was requested
from the connection pool, before sending a new query in this connection.
**The backend socket should be empty** when we have read the response or when we
want to send a request.

The only way to have something in the socket would be using pipelines on the
backend communications, and AFAIK mod_proxy is not using pipelines, even when
pipelined queries are received mod_proxy is splitting each query in a classical
1-request/1-response mode. And I don't think the expected-100-continue mode
could impact that. I'm pretty sure nothing should allow contents in this
connection before we send the request, but I'm maybe wrong.

The new way of handling the reverse proxy job could be expressed this way:

You have a socket opened in keepalive mode with a backend:
 1 - You send only one query, never a pipeline of queries (done)
 2 - you read 1 response even if several responses came back (done)
 3 - you send the unique response to the initial requester (done)
 4 - when receiving a new request (or for the next one the pipeline)
     you ask the pool for a new connection to the backend and the pool ensure
     that this connection is empty (no response waiting in the socket).
 5 - you send the next query (back to step 1)

Closing the backend connection if the socket is not empty is the most secure
thing, but you could also just flush the socket content.

Finally, adding such protection in mod_proxy would not prevent all http
splitting attacks from poisoning httpd's cache or mod_proxy request/responses
matching. Combined with the right timing a splitting attack could prevent the
extra-responses to be send too early and could successfully wait until new
requests are made by mod_proxy in this same connection (the socket would be
empty at step 4).
But adding temporisation in an http spitting attack is a complex task, and is
not always available. And even if this protection is not absolute it would at
least remove a very comfortable situation for the attacker where a big number
of http responses can be stacked to mod_proxy right after the end of the first
response.

How to reproduce
-----------------

This is an example, tested on apache 2.4.12:

- port 8888 apache webserver with mod_proxy
- port 80 apache webserver (or nginx, anything) with php support (php-fpm in m
  y case), PHP is used here to mimic a compromised application.

    <VirtualHost *:8888>
        ServerAdmin webmaster@dummy-host2.example.com
        DocumentRoot "/opt/apache2/htdocs"
        ServerName proxy-host.example.com
        ErrorLog "logs/proxy.example.com-error_log"
        CustomLog "logs/proxy.example.com-access_log" common
        ProxyPass / http://www.dummy-host.example.com:80/
        ProxyPassReverse / http://www.dummy-host.example.com:80/
        LogLevel trace8 proxy:trace8
    </VirtualHost>

On port 80 we add support for PHP

    <VirtualHost *:80>
        DocumentRoot "/opt/apache2/docs/dummy-host.example.com"
        ServerName dummy-host.example.com
        ServerAlias www.dummy-host.example.com
        DirectoryIndex index.php
        ProxyPassMatch ^/.*\.php(/.*)?$ fcgi://127.0.0.1:9000/opt/apache2/docs/dummy-host.example.com
        <Directory /opt/apache2/docs/dummy-host.example.com>
            Require all granted
        </Directory>
    </VirtualHost>

And finally the /opt/apache2/docs/dummy-host.example.com/index.php file:

    <?
    # I know that writing such script is killing your
    # own server. But PHP is not the only way of sending
    # 2 responses for one query. that's a test script.
    # 1st regular HTTP response ---------
    header("Content-Length: 17");
    
    echo "this may be ok.\r\n";
    # 2nd HTTP response ---------
    echo "HTTP/1.1 200 OK\r\n";
    echo "Server: Fake\r\n";
    echo "Date: Tue, 17 Mar 2015 21:33:18 GMT\r\n";
    echo "Content-Type: text/html\r\n";
    echo "Content-Length: 28\r\n";
    echo "Connection: keep-alive\r\n\r\n";
    echo "THIS SHOULD NEVER BE SEEN!\r\n";
    exit(0);

By requesting the index.php file via mod_proxy several times you should get the 
"THIS SHOULD NEVER BE SEEN!" result incoming.
Comment 1 regilero 2015-04-25 07:31:15 UTC
Created attachment 32685 [details]
proposed patch on httpd 2.4.12

Here is a proposal. That's my first try with buckets and brigades so it needs a deep review. But "it works for me".

Here I add a check on the stream BEFORE sending any new request, I check that the input stream is empty (but allow one remaining crlf for backward compatibility with old backends). If it's not empty the bakend keepalived connection is rejected and marked for closure.

This is made in a retry loop (which was there to allow 2 retry on some sort of backend alive test), but I did not increment the retry counter. So if all backend connections are bad we can spend a quite long time in this loop, cleaning all bad connections, but I think this is better than sending 503 responses.

I did not try to patch ap_proxy_connect_backend, where the backend connection is retrieved, because it may be used by some other protocols, with different expectations.
Comment 2 regilero 2015-04-27 14:07:57 UTC
At least one fix to do, in the comment: It's RFC 7230 and not 7320
Comment 3 Yann Ylavic 2015-04-29 21:18:44 UTC
Created attachment 32698 [details]
Proposed patch for trunk

An effort has been made in r1656259 (trunk only) to reduce the time between the backend connection is checked for availability/readability and if that's OK then reused for the next request.

Thus I think the attached patch (on top) should be enough to address this potential issue.

The check is done in ap_proxy_connect_backend() because all proxy modules (reusing connections) currently handle/assume a transactional protocol (one request leading to one response for HTTP, AJP, ...).
Since r1656259, ap_proxy_connect_backend() is called just before sending the (next) request, so it looks like the right place to fail if some data are readable.

Can you give that a try (applying both r1656259 and the attached patch to 2.4.x seems to work too)?
Comment 4 regilero 2015-05-13 08:00:10 UTC
I've tried to apply things from r1656259 and then your proposed patch. But neither patch or myself are able to find where to apply theses changes in the 2.4 branch. I think you'll need to make a regular 2.4 patch version if you want me to try it :-)

By the way I was thinking about the cleanup made on the connection level. There are maybe some modules that would like to escape the 1 request/1 response mode later and switch to an http tunnel mode. There is for example a websocket mod_proxy module 2.5, or we could imagine that the mod_proxy_fcgi would evolve to allow streamed inputs and/or outputs, unbuffered communications (on my last tests using chunked inputs with this module to feed php-fpm with a POST was not working). I do not know the internals as much as you do, but I wonder if preventing any mod_proxy module to handle non-transactionnal communications is a good move.
Comment 5 Yann Ylavic 2015-05-13 09:29:50 UTC
Created attachment 32734 [details]
Patch for 2.4.x (r1516930+r1516965+r1530603+r1530792+r1656259+PR57832)

(In reply to regilero from comment #4)
> I've tried to apply things from r1656259 and then your proposed patch. But
> neither patch or myself are able to find where to apply theses changes in
> the 2.4 branch. I think you'll need to make a regular 2.4 patch version if
> you want me to try it :-)

Yes, sorry, there are missing bits before r1656259.
The attached patch applies both to 2.4.x and 2.4.12, thanks for testing!

> I wonder if preventing any mod_proxy module
> to handle non-transactionnal communications is a good move.

As you can see, this patch only affects mod_proxy_http and mod_proxy_ajp (both transactionnal), the others do not reuse backend connections when done (polling loop until EOF), hence don't use ap_proxy_is_socket_connected().
The only change here is that the caller can now also know if the socket is readable (in addition to simply still connected), transactionnal proxy modules may use that (as this patch does).
Comment 6 Yann Ylavic 2015-05-13 09:46:40 UTC
Created attachment 32735 [details]
Proposed patch for 2.4.x on top of r1656259

Hmm no, you really don't need all the previous stuff.
Here is a minimal patch that applies on 2.4.x/2.4.12 (still on top of r1656259 to address HRS).
Comment 7 regilero 2015-05-13 18:58:55 UTC
I made some tests with the last patch, on top of r1656259 's patch.

If the extra content injected after the 1st response is less than 8000 bytes (more or less) I get 1 for the return of is_socket_connected instead of 2 (USE_ALTERNATE_IS_CONNECTED && defined(APR_MSG_PEEK version). Not always, the attack succeed at 90%. With a big injected response the socket read is not empty and is_socket_connected is detecting this fact, but I do not get any response (no 502/503/400, just an RST).

So it means the real socket is empty (tested it with real reads and timeouts), but something as already stored this extra content and this storage is associated with the socket. So quite certainly something like some buckets which are not cleaned up after the 1st request. Note that this is hiding a potential problem that I had to fix on the 1st patch with backends sending one extra \r\n after the first response.

But I think the final solution will have to mix this is_socket_connected and a real cleanup of all data read from the socket while processing the first response.
Comment 8 Yann Ylavic 2015-05-17 22:51:35 UTC
Created attachment 32740 [details]
Proposed patch for 2.4.x on top of r1656259

(In reply to regilero from comment #7)
> I made some tests with the last patch, on top of r1656259 's patch.

Thanks for testing!

> With a big injected response the socket read is
> not empty and is_socket_connected is detecting this fact, but I do not get
> any response (no 502/503/400, just an RST).

Yes, I failed to reset "connected" from 2 to 0 in the previous patch.

> 
> But I think the final solution will have to mix this is_socket_connected and
> a real cleanup of all data read from the socket while processing the first
> response.

Right, we have to also check if data are already in the input filters' stack.
This is what the new attached patch does.

I tested it with both early read and stacked data, but you probably have a more exhaustive tests suite, can you please give it a (re)try?
Comment 9 regilero 2015-05-18 09:03:11 UTC
This version is OK.

Stored injections are always detected, small and big.
The only way to inject a new response is to have a fine control of the backend stream and use timers between responses, which ,by definition, cannot be detected by mod_proxy.

So it's almost good. I think there's maybe just one problem with responses from backends containing one extra CRLF. This is already managed by mod_proxy and allowed by the RFC. But here, if I'm not wrong on my tests, it makes a connection status 2.
Comment 10 Yann Ylavic 2016-11-14 12:26:59 UTC
Backported to upcoming 2.4.24 in r1766372.