relayd - HTTP Desync Attacks

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

relayd - HTTP Desync Attacks

pdcvgmh
Hi guys! I've been reading about HTTP Desync Attacks lately so I took a
look to relayd's source code to check if it's likely to be exploited.
I wasn't able to do a POC (I don't have a OBSD installation at the moment)
but I think it is.
I'm going to install a OpenBSD as soon as I can in order to test the
current code and send a patch if needed but I want to tell you what I've
seen so far.
There I go:

If a malicious user split a "Transfer-Encoding: chunked" http header in
multiple lines along with a Content-Length header it would trick relayd to
use the content length header while forwarding the
Transfer-Encoding header downstream.
The problem is in relay_read_http:

if (strcasecmp("Transfer-Encoding", key) == 0 &&
strcasecmp("chunked", value) == 0)
desc->http_chunked = 1;

The check for key == "Transfer-Encoding" && value == chunked isn't deferred
until the full header is read and it should.
Maybe we could defer the check until we are out of the loop.

Replacing
if (desc->http_chunked) {
/* Chunked transfer encoding */
cre->toread = TOREAD_HTTP_CHUNK_LENGTH;
bev->readcb = relay_read_httpchunks;
}

by doing a lookup into desc->http_headers

In order to achieve this, the function kv_extend should be modified in
order to alter the structure pointed by the first parameter (today it does
nothing with the first parameter)

Also, It seems like the "lookup" label along with the goto lookup sentence
could be removed.

Payload
POST ...
...
Transfer-Encoding:
 chunked
...
Content-Length: whatever greater than the chunk

10
1234567890123456
POISON!

If I'm right this payload would result in relayd honouring the
Content-Length header (breaking RFC 2616) and probably poisoning the socket
(assuming that the downstream server would honour the Transfer-Encoding
header)

PD: I haven't digged enough to realize if relayd reuse downstream
connections among different clients so I can't say the real impact of this
issue.

Best regards

Pablo
Reply | Threaded
Open this post in threaded view
|

Re: relayd - HTTP Desync Attacks

pdcvgmh
"In order to achieve this, the function kv_extend should be modified in
order to alter..." Wrong! The header (already added to desc->http_headers)
gets modified correctly through the desc->http_lastheader pointer.

I'm going to stop making assumptions... I have to get my hands on a working
OBSD setup and make some real test.

Best regards

On Wed, Aug 14, 2019 at 2:27 AM Pablo Caballero <[hidden email]> wrote:

> Hi guys! I've been reading about HTTP Desync Attacks lately so I took a
> look to relayd's source code to check if it's likely to be exploited.
> I wasn't able to do a POC (I don't have a OBSD installation at the moment)
> but I think it is.
> I'm going to install a OpenBSD as soon as I can in order to test the
> current code and send a patch if needed but I want to tell you what I've
> seen so far.
> There I go:
>
> If a malicious user split a "Transfer-Encoding: chunked" http header in
> multiple lines along with a Content-Length header it would trick relayd to
> use the content length header while forwarding the
> Transfer-Encoding header downstream.
> The problem is in relay_read_http:
>
> if (strcasecmp("Transfer-Encoding", key) == 0 &&
> strcasecmp("chunked", value) == 0)
> desc->http_chunked = 1;
>
> The check for key == "Transfer-Encoding" && value == chunked isn't
> deferred until the full header is read and it should.
> Maybe we could defer the check until we are out of the loop.
>
> Replacing
> if (desc->http_chunked) {
> /* Chunked transfer encoding */
> cre->toread = TOREAD_HTTP_CHUNK_LENGTH;
> bev->readcb = relay_read_httpchunks;
> }
>
> by doing a lookup into desc->http_headers
>
> In order to achieve this, the function kv_extend should be modified in
> order to alter the structure pointed by the first parameter (today it does
> nothing with the first parameter)
>
> Also, It seems like the "lookup" label along with the goto lookup sentence
> could be removed.
>
> Payload
> POST ...
> ...
> Transfer-Encoding:
>  chunked
> ...
> Content-Length: whatever greater than the chunk
>
> 10
> 1234567890123456
> POISON!
>
> If I'm right this payload would result in relayd honouring the
> Content-Length header (breaking RFC 2616) and probably poisoning the socket
> (assuming that the downstream server would honour the Transfer-Encoding
> header)
>
> PD: I haven't digged enough to realize if relayd reuse downstream
> connections among different clients so I can't say the real impact of this
> issue.
>
> Best regards
>
> Pablo
>