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 -0000 1.11
+++ usr.sbin/relayd/http.h 19 Aug 2019 19:35:54 -0000
@@ -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 -0000 1.78
+++ usr.sbin/relayd/relay_http.c 19 Aug 2019 19:35:54 -0000
@@ -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 -0000 1.180
+++ usr.sbin/relayd/relayd.c 19 Aug 2019 19:35:54 -0000
@@ -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 -0000 1.259
+++ usr.sbin/relayd/relayd.h 19 Aug 2019 19:35:55 -0000
@@ -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 <[email protected]> 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 <[email protected]> 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
>>
>
