From d88484cafbf973d55dc95b7edcae5064efa8bad0 Mon Sep 17 00:00:00 2001 From: Garrett D'Amore Date: Sun, 12 Jan 2025 13:50:32 -0800 Subject: [PATCH] http: fix mishandling of very long headers or URIs, and mishandling of unicode Also, nng_err is now a distinct type which might be nicer in debuggers. --- include/nng/nng.h | 4 +- src/supplemental/http/http_conn.c | 18 +++-- src/supplemental/http/http_msg.c | 20 +++--- src/supplemental/http/http_server.c | 10 +-- src/supplemental/http/http_server_test.c | 91 ++++++++++++++++++++++++ 5 files changed, 120 insertions(+), 23 deletions(-) diff --git a/include/nng/nng.h b/include/nng/nng.h index 6f01363ff..516cdeb93 100644 --- a/include/nng/nng.h +++ b/include/nng/nng.h @@ -1020,7 +1020,7 @@ NNG_DECL void nng_device_aio(nng_aio *, nng_socket, nng_socket); // errors, from different transports. It should only be used when none // of the other options are available. -enum nng_errno_enum { +typedef enum nng_errno_enum { NNG_EINTR = 1, NNG_ENOMEM = 2, NNG_EINVAL = 3, @@ -1054,7 +1054,7 @@ enum nng_errno_enum { NNG_EINTERNAL = 1000, NNG_ESYSERR = 0x10000000, NNG_ETRANERR = 0x20000000 -}; +} nng_err; // nng_url_parse parses a URL string into a structured form. // Note that the u_port member will be filled out with a numeric diff --git a/src/supplemental/http/http_conn.c b/src/supplemental/http/http_conn.c index e143d9b3f..d4cea914c 100644 --- a/src/supplemental/http/http_conn.c +++ b/src/supplemental/http/http_conn.c @@ -260,12 +260,22 @@ http_rd_buf(nni_http_conn *conn, nni_aio *aio) if (rv == NNG_EAGAIN) { nni_iov iov1; http_buf_pull_up(conn); + if (conn->rd_put == conn->bufsz) { + nng_http_set_status(conn, + conn->req.data.parsed + ? NNG_HTTP_STATUS_HEADERS_TOO_LARGE + : NNG_HTTP_STATUS_URI_TOO_LONG, + NULL); + // leave a "header" so we don't confuse parsing + // We want to ensure this is an overlong + // request. + strcpy((char *) conn->buf, "NNG-DISCARD: X"); + conn->rd_get = 0; + conn->rd_put = strlen((char *) conn->buf); + } iov1.iov_buf = conn->buf + conn->rd_put; iov1.iov_len = conn->bufsz - conn->rd_put; conn->buffered = true; - if (iov1.iov_len == 0) { - return (NNG_EMSGSIZE); - } nni_aio_set_iov(&conn->rd_aio, 1, &iov1); nng_stream_recv(conn->sock, &conn->rd_aio); } @@ -702,7 +712,7 @@ http_snprintf(nng_http *conn, char *buf, size_t sz) n = http_sprintf_headers(buf, sz, hdrs); len += n; - if (len < sz) { + if (n < sz) { sz -= n; buf += n; } else { diff --git a/src/supplemental/http/http_msg.c b/src/supplemental/http/http_msg.c index b147c811f..0cef876a4 100644 --- a/src/supplemental/http/http_msg.c +++ b/src/supplemental/http/http_msg.c @@ -177,16 +177,16 @@ nni_http_res_init(nni_http_res *res) res->data.own = false; } -static int +static nng_err http_scan_line(void *vbuf, size_t n, size_t *lenp) { - size_t len; - char lc; - char *buf = vbuf; + size_t len; + char lc; + uint8_t *buf = vbuf; lc = 0; for (len = 0; len < n; len++) { - char c = buf[len]; + uint8_t c = buf[len]; if (c == '\n') { // Technically we should be receiving CRLF, but // debugging is easier with just LF, so we behave @@ -210,13 +210,17 @@ http_scan_line(void *vbuf, size_t n, size_t *lenp) return (NNG_EAGAIN); } -static int +static nng_err http_req_parse_line(nng_http *conn, void *line) { char *method; char *uri; char *version; + if (nni_http_get_status(conn) >= NNG_HTTP_STATUS_BAD_REQUEST) { + // we've already failed it, nothing else for us to do + return (0); + } method = line; if ((uri = strchr(method, ' ')) == NULL) { nni_http_set_status(conn, NNG_HTTP_STATUS_BAD_REQUEST, NULL); @@ -248,7 +252,7 @@ http_req_parse_line(nng_http *conn, void *line) return (nni_http_set_uri(conn, uri, NULL)); } -static int +static nng_err http_res_parse_line(nng_http *conn, uint8_t *line) { char *reason; @@ -317,7 +321,7 @@ nni_http_req_parse(nng_http *conn, void *buf, size_t n, size_t *lenp) } } - if (rv == 0) { + if (rv != NNG_EAGAIN) { req->data.parsed = false; } *lenp = len; diff --git a/src/supplemental/http/http_server.c b/src/supplemental/http/http_server.c index 9a9a619fa..0092299d4 100644 --- a/src/supplemental/http/http_server.c +++ b/src/supplemental/http/http_server.c @@ -433,15 +433,7 @@ http_sconn_rxdone(void *arg) const char *cls; if ((rv = nni_aio_result(aio)) != 0) { - if (rv == NNG_EMSGSIZE) { - sc->close = true; - http_sconn_error(sc, - nni_http_parsed(sc->conn) - ? NNG_HTTP_STATUS_HEADERS_TOO_LARGE - : NNG_HTTP_STATUS_URI_TOO_LONG); - } else { - http_sconn_close(sc); - } + http_sconn_close(sc); return; } diff --git a/src/supplemental/http/http_server_test.c b/src/supplemental/http/http_server_test.c index 940248fde..e091cd89f 100644 --- a/src/supplemental/http/http_server_test.c +++ b/src/supplemental/http/http_server_test.c @@ -488,6 +488,94 @@ test_server_wrong_method(void) server_free(&st); } +static void +test_server_uri_too_long(void) +{ + struct server_test st; + nng_http_handler *h; + char buf[32768]; + + NUTS_PASS(nng_http_handler_alloc_static( + &h, "/home.html", doc1, strlen(doc1), "text/html")); + + server_setup(&st, h); + + memset(buf, 'a', sizeof(buf) - 1); + buf[0] = '/'; + buf[sizeof(buf) - 1] = 0; + + NUTS_PASS(nng_http_set_uri(st.conn, buf, NULL)); + nng_http_write_request(st.conn, st.aio); + + nng_aio_wait(st.aio); + NUTS_PASS(nng_aio_result(st.aio)); + + nng_http_read_response(st.conn, st.aio); + nng_aio_wait(st.aio); + NUTS_PASS(nng_aio_result(st.aio)); + + NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_URI_TOO_LONG); + + server_free(&st); +} + +static void +test_server_header_too_long(void) +{ + struct server_test st; + nng_http_handler *h; + char buf[32768]; + + NUTS_PASS(nng_http_handler_alloc_static( + &h, "/home.html", doc1, strlen(doc1), "text/html")); + + server_setup(&st, h); + + memset(buf, 'a', sizeof(buf) - 1); + buf[0] = '/'; + buf[sizeof(buf) - 1] = 0; + + NUTS_PASS(nng_http_set_uri(st.conn, "/home.html", NULL)); + NUTS_PASS(nng_http_set_header(st.conn, "Referrer", buf)); + nng_http_write_request(st.conn, st.aio); + + nng_aio_wait(st.aio); + NUTS_PASS(nng_aio_result(st.aio)); + + nng_http_read_response(st.conn, st.aio); + nng_aio_wait(st.aio); + NUTS_PASS(nng_aio_result(st.aio)); + + NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_HEADERS_TOO_LARGE); + + server_free(&st); +} + +static void +test_server_invalid_utf8(void) +{ + struct server_test st; + nng_http_handler *h; + + NUTS_PASS(nng_http_handler_alloc_static( + &h, "/home.html", doc1, strlen(doc1), "text/html")); + + server_setup(&st, h); + + NUTS_PASS(nng_http_set_uri(st.conn, "/home\xFF.html", NULL)); + nng_http_write_request(st.conn, st.aio); + + nng_aio_wait(st.aio); + + nng_http_read_response(st.conn, st.aio); + nng_aio_wait(st.aio); + NUTS_PASS(nng_aio_result(st.aio)); + + NUTS_HTTP_STATUS(st.conn, NNG_HTTP_STATUS_BAD_REQUEST); + + server_free(&st); +} + static void test_server_post_handler(void) { @@ -1026,6 +1114,9 @@ NUTS_TESTS = { { "server missing host", test_server_missing_host }, { "server wrong method", test_server_wrong_method }, { "server method too long", test_server_method_too_long }, + { "server uri too long", test_server_uri_too_long }, + { "server header too long", test_server_header_too_long }, + { "server invalid utf", test_server_invalid_utf8 }, { "server post handler", test_server_post_handler }, { "server get redirect", test_server_get_redirect }, { "server tree redirect", test_server_tree_redirect },