Re: relayd - HTTP Desync Attacks
Patch inlined
Best regards
Index: usr.sbin/relayd/http.h
===
RCS file: /cvs/src/usr.sbin/relayd/http.h,v
retrieving revision 1.11
diff -u -p -u -r1.11 http.h
--- usr.sbin/relayd/http.h 8 May 2019 23:22:19 - 1.11
+++ usr.sbin/relayd/http.h 19 Aug 2019 19:35:54 -
@@ -239,7 +239,6 @@ struct http_descriptor {
char *http_host;
enum httpmethod http_method;
- int http_chunked;
char *http_version;
unsigned int http_status;
Index: usr.sbin/relayd/relay_http.c
===
RCS file: /cvs/src/usr.sbin/relayd/relay_http.c,v
retrieving revision 1.78
diff -u -p -u -r1.78 relay_http.c
--- usr.sbin/relayd/relay_http.c 13 Jul 2019 06:53:00 - 1.78
+++ usr.sbin/relayd/relay_http.c 19 Aug 2019 19:35:54 -
@@ -159,7 +159,7 @@ relay_read_http(struct bufferevent *bev,
int action, unique, ret;
const char *errstr;
size_t size, linelen;
- struct kv *hdr = NULL;
+ struct kv *hdr = NULL, lookup;
struct kv *upgrade = NULL, *upgrade_ws = NULL;
getmonotime(&con->se_tv_last);
@@ -219,8 +219,7 @@ relay_read_http(struct bufferevent *bev,
}
/* Append line to the last header, if present */
- if (kv_extend(&desc->http_headers,
-desc->http_lastheader, line) == NULL) {
+ if (kv_extend(desc->http_lastheader, line) == NULL) {
free(line);
goto fail;
}
@@ -279,7 +278,6 @@ relay_read_http(struct bufferevent *bev,
DPRINTF("http_version %s http_rescode %s "
"http_resmesg %s", desc->http_version,
desc->http_rescode, desc->http_resmesg);
- goto lookup;
} else if (cre->line == 1 && cre->dir == RELAY_DIR_REQUEST) {
if ((desc->http_method = relay_httpmethod_byname(key))
== HTTP_METHOD_NONE) {
@@ -360,11 +358,7 @@ relay_read_http(struct bufferevent *bev,
goto abort;
}
}
- lookup:
- if (strcasecmp("Transfer-Encoding", key) == 0 &&
-strcasecmp("chunked", value) == 0)
- desc->http_chunked = 1;
-
+
/* The following header should only occur once */
if (strcasecmp("Host", key) == 0) {
unique = 1;
@@ -516,10 +510,16 @@ relay_read_http(struct bufferevent *bev,
bev->readcb = relay_read_http;
break;
}
- if (desc->http_chunked) {
- /* Chunked transfer encoding */
- cre->toread = TOREAD_HTTP_CHUNK_LENGTH;
- bev->readcb = relay_read_httpchunks;
+
+ lookup.kv_key = "Transfer-Encoding";
+ hdr = kv_find(&desc->http_headers, &lookup);
+ if (hdr != NULL && hdr->kv_value != NULL) {
+ value = hdr->kv_value + strspn(hdr->kv_value, " \t\r\n");
+ if (strcasecmp("chunked", value) == 0) {
+ /* Chunked transfer encoding */
+ cre->toread = TOREAD_HTTP_CHUNK_LENGTH;
+ bev->readcb = relay_read_httpchunks;
+ }
}
/*
@@ -772,7 +772,6 @@ relay_reset_http(struct ctl_relay_event
relay_httpdesc_free(desc);
desc->http_method = 0;
- desc->http_chunked = 0;
cre->headerlen = 0;
cre->line = 0;
cre->done = 0;
Index: usr.sbin/relayd/relayd.c
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.c,v
retrieving revision 1.180
diff -u -p -u -r1.180 relayd.c
--- usr.sbin/relayd/relayd.c 26 Jun 2019 12:13:47 - 1.180
+++ usr.sbin/relayd/relayd.c 19 Aug 2019 19:35:54 -
@@ -713,7 +713,7 @@ kv_delete(struct kvtree *keys, struct kv
}
struct kv *
-kv_extend(struct kvtree *keys, struct kv *kv, char *value)
+kv_extend(struct kv *kv, char *value)
{
char *newvalue;
Index: usr.sbin/relayd/relayd.h
===
RCS file: /cvs/src/usr.sbin/relayd/relayd.h,v
retrieving revision 1.259
diff -u -p -u -r1.259 relayd.h
--- usr.sbin/relayd/relayd.h 26 Jun 2019 12:13:47 - 1.259
+++ usr.sbin/relayd/relayd.h 19 Aug 2019 19:35:55 -
@@ -1349,7 +1349,7 @@ struct kv *kv_add(struct kvtree *, char
int kv_set(struct kv *, char *, ...);
int kv_setkey(struct kv *, char *, ...);
void kv_delete(struct kvtree *, struct kv *);
-struct kv *kv_extend(struct kvtree *, struct kv *, char *);
+struct kv *kv_extend(struct kv *, char *);
void kv_purge(struct kvtree *);
void kv_free(struct kv *);
struct kv *kv_inherit(struct kv *, struct kv *);
On Wed, Aug 14, 2019 at 12:40 PM Pablo Caballero wrote:
> "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 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 tel
Re: relayd - HTTP Desync Attacks
"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 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
>
relayd - HTTP Desync Attacks
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
