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@

1 files 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 <reyk@openbsd.org>
@@ -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);