Skip to content

Commit

Permalink
http: fix mishandling of very long headers or URIs, and mishandling o…
Browse files Browse the repository at this point in the history
…f unicode

Also, nng_err is now a distinct type which might be nicer in debuggers.
  • Loading branch information
gdamore committed Jan 12, 2025
1 parent c34abc2 commit d88484c
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 23 deletions.
4 changes: 2 additions & 2 deletions include/nng/nng.h
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down
18 changes: 14 additions & 4 deletions src/supplemental/http/http_conn.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 {
Expand Down
20 changes: 12 additions & 8 deletions src/supplemental/http/http_msg.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 1 addition & 9 deletions src/supplemental/http/http_server.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down
91 changes: 91 additions & 0 deletions src/supplemental/http/http_server_test.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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 },
Expand Down

0 comments on commit d88484c

Please sign in to comment.