From eefb3de5799409f8689b849d8a069ad293a002c0 Mon Sep 17 00:00:00 2001 From: millert Date: Wed, 29 Nov 2023 15:35:07 +0000 Subject: [PATCH] relay_read_http: defer header parsing until after line continuation Wait until we have a complete line before parsing the Content-Length, Transfer-Encoding and Host headers. This prevents potential request smuggling attacks. Filtering already happens after header line continuation has been performed. Reported by Ben Kallus. OK claudio@ --- usr.sbin/relayd/relay_http.c | 472 ++++++++++++++++++----------------- 1 file changed, 249 insertions(+), 223 deletions(-) diff --git a/usr.sbin/relayd/relay_http.c b/usr.sbin/relayd/relay_http.c index 2b4e06c27e1..0216de02762 100644 --- a/usr.sbin/relayd/relay_http.c +++ b/usr.sbin/relayd/relay_http.c @@ -1,4 +1,4 @@ -/* $OpenBSD: relay_http.c,v 1.85 2023/11/28 18:36:55 millert Exp $ */ +/* $OpenBSD: relay_http.c,v 1.86 2023/11/29 15:35:07 millert Exp $ */ /* * Copyright (c) 2006 - 2016 Reyk Floeter @@ -210,10 +210,15 @@ relay_read_http(struct bufferevent *bev, void *arg) goto done; } - while (!cre->done) { + for (;;) { line = evbuffer_readln(src, &linelen, EVBUFFER_EOL_CRLF); - if (line == NULL) + if (line == NULL) { + /* + * We do not process the last header on premature + * EOF as it may not be complete. + */ break; + } /* * An empty line indicates the end of the request. @@ -222,9 +227,13 @@ relay_read_http(struct bufferevent *bev, void *arg) if (linelen == 0) { cre->done = 1; free(line); + line = NULL; + if (cre->line > 1) { + /* Process last (complete) header line. */ + goto last_header; + } break; } - key = line; /* Limit the total header length minus \r\n */ cre->headerlen += linelen; @@ -240,26 +249,144 @@ relay_read_http(struct bufferevent *bev, void *arg) goto abort; } + hs = con->se_priv; + DPRINTF("%s: session %d http_session %p", __func__, + con->se_id, hs); + /* * The first line is the GET/POST/PUT/... request, * subsequent lines are HTTP headers. */ if (++cre->line == 1) { + key = line; if ((value = strchr(key, ' ')) == NULL) { relay_abort_http(con, 400, "malformed", 0); goto abort; } - } else if (*key == ' ' || *key == '\t') { - /* Multiline headers wrap with a space or tab */ - value = NULL; - } else { - if ((value = strchr(key, ':')) == NULL) { - relay_abort_http(con, 400, "malformed", 0); - goto abort; + *value++ = '\0'; + + if (cre->dir == RELAY_DIR_RESPONSE) { + desc->http_method = HTTP_METHOD_RESPONSE; + hmn = SIMPLEQ_FIRST(&hs->hs_methods); + + /* + * There is nothing preventing the relay from + * sending an unbalanced response. Be prepared. + */ + if (hmn == NULL) { + request_method = HTTP_METHOD_NONE; + DPRINTF("%s: session %d unbalanced " + "response", __func__, con->se_id); + } else { + SIMPLEQ_REMOVE_HEAD(&hs->hs_methods, + hmn_entry); + request_method = hmn->hmn_method; + DPRINTF("%s: session %d dequeuing %s", + __func__, con->se_id, + relay_httpmethod_byid(request_method)); + free(hmn); + } + + /* + * Decode response path and query + */ + desc->http_version = strdup(key); + if (desc->http_version == NULL) { + free(line); + goto fail; + } + desc->http_rescode = strdup(value); + if (desc->http_rescode == NULL) { + free(line); + goto fail; + } + desc->http_resmesg = strchr(desc->http_rescode, + ' '); + if (desc->http_resmesg == NULL) { + free(line); + goto fail; + } + *desc->http_resmesg++ = '\0'; + desc->http_resmesg = strdup(desc->http_resmesg); + if (desc->http_resmesg == NULL) { + free(line); + goto fail; + } + desc->http_status = strtonum(desc->http_rescode, + 100, 599, &errstr); + if (errstr) { + DPRINTF( + "%s: http_status %s: errno %d, %s", + __func__, desc->http_rescode, errno, + errstr); + free(line); + goto fail; + } + DPRINTF("http_version %s http_rescode %s " + "http_resmesg %s", desc->http_version, + desc->http_rescode, desc->http_resmesg); + } else if (cre->dir == RELAY_DIR_REQUEST) { + desc->http_method = + relay_httpmethod_byname(key); + if (desc->http_method == HTTP_METHOD_NONE) { + free(line); + goto fail; + } + if ((hmn = calloc(1, sizeof *hmn)) == NULL) { + free(line); + goto fail; + } + hmn->hmn_method = desc->http_method; + DPRINTF("%s: session %d enqueuing %s", + __func__, con->se_id, + relay_httpmethod_byid(hmn->hmn_method)); + SIMPLEQ_INSERT_TAIL(&hs->hs_methods, hmn, + hmn_entry); + /* + * Decode request path and query + */ + desc->http_path = strdup(value); + if (desc->http_path == NULL) { + free(line); + goto fail; + } + desc->http_version = strchr(desc->http_path, + ' '); + if (desc->http_version == NULL) { + free(line); + goto fail; + } + *desc->http_version++ = '\0'; + desc->http_query = strchr(desc->http_path, '?'); + if (desc->http_query != NULL) + *desc->http_query++ = '\0'; + + /* + * Have to allocate the strings because they + * could be changed independently by the + * filters later. + */ + if ((desc->http_version = + strdup(desc->http_version)) == NULL) { + free(line); + goto fail; + } + if (desc->http_query != NULL && + (desc->http_query = + strdup(desc->http_query)) == NULL) { + free(line); + goto fail; + } } + + free(line); + continue; } - if (value == NULL) { - if (cre->line <= 2) { + + /* Multiline headers wrap with a space or tab. */ + if (*line == ' ' || *line == '\t') { + if (cre->line == 2) { + /* First header line cannot start with space. */ relay_abort_http(con, 400, "malformed", 0); goto abort; } @@ -274,235 +401,134 @@ relay_read_http(struct bufferevent *bev, void *arg) free(line); continue; } - if (*value == ':') { - *value++ = '\0'; - value += strspn(value, " \t\r\n"); - - if (!relay_http_header_name_valid(key)) { - relay_abort_http(con, 400, "malformed", 0); - goto abort; - } - } else { - *value++ = '\0'; - } - DPRINTF("%s: session %d: header '%s: %s'", __func__, - con->se_id, key, value); + /* Process the last complete header line. */ + last_header: + if (desc->http_lastheader != NULL) { + key = desc->http_lastheader->kv_key; + value = desc->http_lastheader->kv_value; - hs = con->se_priv; - DPRINTF("%s: session %d http_session %p", __func__, - con->se_id, hs); - - /* - * Identify and handle specific HTTP request methods - */ - if (cre->line == 1 && cre->dir == RELAY_DIR_RESPONSE) { - desc->http_method = HTTP_METHOD_RESPONSE; - hmn = SIMPLEQ_FIRST(&hs->hs_methods); - - /* - * There is nothing preventing the relay to send - * an unbalanced response. Be prepared. - */ - if (hmn == NULL) { - request_method = HTTP_METHOD_NONE; - DPRINTF("%s: session %d unbalanced response", - __func__, con->se_id); - } else { - SIMPLEQ_REMOVE_HEAD(&hs->hs_methods, hmn_entry); - request_method = hmn->hmn_method; - DPRINTF("%s: session %d dequeuing %s", __func__, - con->se_id, - relay_httpmethod_byid(request_method)); - free(hmn); - } - - /* - * Decode response path and query - */ - desc->http_version = strdup(line); - if (desc->http_version == NULL) { - free(line); - goto fail; - } - desc->http_rescode = strdup(value); - if (desc->http_rescode == NULL) { - free(line); - goto fail; - } - desc->http_resmesg = strchr(desc->http_rescode, ' '); - if (desc->http_resmesg == NULL) { - free(line); - goto fail; - } - *desc->http_resmesg++ = '\0'; - if ((desc->http_resmesg = strdup(desc->http_resmesg)) - == NULL) { - free(line); - goto fail; - } - desc->http_status = strtonum(desc->http_rescode, 100, - 599, &errstr); - if (errstr) { - DPRINTF("%s: http_status %s: errno %d, %s", - __func__, desc->http_rescode, errno, - errstr); - free(line); - goto fail; - } - 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) { - free(line); - goto fail; - } - if ((hmn = calloc(1, sizeof *hmn)) == NULL) { - free(line); - goto fail; - } - hmn->hmn_method = desc->http_method; - DPRINTF("%s: session %d enqueuing %s", __func__, - con->se_id, - relay_httpmethod_byid(hmn->hmn_method)); - SIMPLEQ_INSERT_TAIL(&hs->hs_methods, hmn, hmn_entry); - /* - * Decode request path and query - */ - desc->http_path = strdup(value); - if (desc->http_path == NULL) { - free(line); - goto fail; - } - desc->http_version = strchr(desc->http_path, ' '); - if (desc->http_version == NULL) { - free(line); - goto fail; - } - *desc->http_version++ = '\0'; - desc->http_query = strchr(desc->http_path, '?'); - if (desc->http_query != NULL) - *desc->http_query++ = '\0'; + DPRINTF("%s: session %d: header '%s: %s'", __func__, + con->se_id, key, value); - /* - * Have to allocate the strings because they could - * be changed independently by the filters later. - */ - if ((desc->http_version = - strdup(desc->http_version)) == NULL) { - free(line); - goto fail; - } - if (desc->http_query != NULL && - (desc->http_query = - strdup(desc->http_query)) == NULL) { - free(line); - goto fail; - } - } else if (desc->http_method != HTTP_METHOD_NONE && - strcasecmp("Content-Length", key) == 0) { - /* - * These methods should not have a body - * and thus no Content-Length header. - */ - if (desc->http_method == HTTP_METHOD_TRACE || - desc->http_method == HTTP_METHOD_CONNECT) { - relay_abort_http(con, 400, "malformed", 0); - goto abort; - } - /* - * HEAD responses may provide a Content-Length header, - * but if so it should just be ignored, since there is - * no actual payload in the response. - */ - if (desc->http_method != HTTP_METHOD_RESPONSE - || request_method != HTTP_METHOD_HEAD) { + if (desc->http_method != HTTP_METHOD_NONE && + strcasecmp("Content-Length", key) == 0) { /* - * Need to read data from the client after the - * HTTP header. - * XXX What about non-standard clients not - * using the carriage return? And some browsers - * seem to include the line length in the - * content-length. + * These methods should not have a body + * and thus no Content-Length header. */ - if (*value == '+' || *value == '-') { - errstr = "invalid"; - } else { - cre->toread = strtonum(value, 0, - LLONG_MAX, &errstr); + if (desc->http_method == HTTP_METHOD_TRACE || + desc->http_method == HTTP_METHOD_CONNECT) { + relay_abort_http(con, 400, "malformed", + 0); + goto abort; } - if (errstr) { - relay_abort_http(con, 500, errstr, 0); + /* + * HEAD responses may provide a Content-Length + * header, but if so it should just be ignored, + * since there is no actual payload in the + * response. + */ + if (desc->http_method != HTTP_METHOD_RESPONSE + || request_method != HTTP_METHOD_HEAD) { + /* + * Need to read data from the client + * after the HTTP header. + * XXX What about non-standard clients + * not using the carriage return? And + * some browsers seem to include the + * line length in the content-length. + */ + if (*value == '+' || *value == '-') { + errstr = "invalid"; + } else { + cre->toread = strtonum(value, 0, + LLONG_MAX, &errstr); + } + if (errstr) { + relay_abort_http(con, 500, + errstr, 0); + goto abort; + } + } + /* + * Response with a status code of 1xx + * (Informational) or 204 (No Content) MUST + * not have a Content-Length (rfc 7230 3.3.3) + * Instead we check for value != 0 because there + * are servers that do not follow the rfc and + * send Content-Length: 0. + */ + if (desc->http_method == HTTP_METHOD_RESPONSE && + (((desc->http_status >= 100 && + desc->http_status < 200) || + desc->http_status == 204)) && + cre->toread != 0) { + relay_abort_http(con, 502, + "Bad Gateway", 0); goto abort; } } - /* - * response with a status code of 1xx - * (Informational) or 204 (No Content) MUST - * not have a Content-Length (rfc 7230 3.3.3) - * Instead we check for value != 0 because there are - * servers that do not follow the rfc and send - * Content-Length: 0. - */ - if (desc->http_method == HTTP_METHOD_RESPONSE && ( - ((desc->http_status >= 100 && - desc->http_status < 200) || - desc->http_status == 204)) && - cre->toread != 0) { - relay_abort_http(con, 502, - "Bad Gateway", 0); - goto abort; - } - } - lookup: - if (strcasecmp("Transfer-Encoding", key) == 0) { - /* We don't support other encodings. */ - if (strcasecmp("chunked", value) != 0) { - relay_abort_http(con, 400, - "malformed", 0); - goto abort; - } - desc->http_chunked = 1; - } - - /* The following header should only occur once */ - if (strcasecmp("Host", key) == 0) { - unique = 1; - - /* - * The path may contain a URL. The host in the - * URL has to match the Host: value. - */ - if (parse_url(desc->http_path, - &urlproto, &host, &path) == 0) { - ret = strcasecmp(host, value); - free(urlproto); - free(host); - free(path); - if (ret != 0) { + if (strcasecmp("Transfer-Encoding", key) == 0) { + /* We don't support other encodings. */ + if (strcasecmp("chunked", value) != 0) { relay_abort_http(con, 400, - "malformed host", 0); + "malformed", 0); goto abort; } + desc->http_chunked = 1; } - } else - unique = 0; - if (cre->line != 1) { - if ((hdr = kv_add(&desc->http_headers, key, - value, unique)) == NULL) { - relay_abort_http(con, 400, - "malformed header", 0); - goto abort; + if (strcasecmp("Host", key) == 0) { + /* + * The path may contain a URL. The host in the + * URL has to match the Host: value. + */ + if (parse_url(desc->http_path, + &urlproto, &host, &path) == 0) { + ret = strcasecmp(host, value); + free(urlproto); + free(host); + free(path); + if (ret != 0) { + relay_abort_http(con, 400, + "malformed host", 0); + goto abort; + } + } } - desc->http_lastheader = hdr; } + if (cre->done) + break; + + /* Validate header field name and check for missing value. */ + key = line; + if ((value = strchr(line, ':')) == NULL) { + relay_abort_http(con, 400, "malformed", 0); + goto abort; + } + *value++ = '\0'; + value += strspn(value, " \t\r\n"); + + if (!relay_http_header_name_valid(key)) { + relay_abort_http(con, 400, "malformed", 0); + goto abort; + } + + /* The "Host" header must only occur once. */ + unique = strcasecmp("Host", key) == 0; + + if ((hdr = kv_add(&desc->http_headers, key, + value, unique)) == NULL) { + relay_abort_http(con, 400, "malformed header", 0); + goto abort; + } + desc->http_lastheader = hdr; + free(line); } + if (cre->done) { if (desc->http_method == HTTP_METHOD_NONE) { relay_abort_http(con, 406, "no method", 0); -- 2.20.1