From 3beda5f31bf8ed63489afa24060006faea69bf8c Mon Sep 17 00:00:00 2001 From: Caio Lucas Date: Sat, 16 Mar 2019 14:54:16 -0300 Subject: [PATCH] count headers, URL and start lines separately Counting headers, URL and start lines separately because it was counting everything as headers before the headers_done state, throwing HPE_HEADER_OVERFLOW errors even when what really caused the overflow was the request URL being too long Creating an HPE_URL_OVERFLOW err to make sure embedders still have a request line limit, but now differentiating what went over such limit. Refs: https://github.com/nodejs/node/pull/26553 --- http_parser.c | 55 ++++++++++++++++++++++---- http_parser.h | 104 +++++++++++++++++++++++++++++--------------------- test.c | 28 ++++++++++++++ 3 files changed, 135 insertions(+), 52 deletions(-) diff --git a/http_parser.c b/http_parser.c index e2fc5d2e..339c44fb 100644 --- a/http_parser.c +++ b/http_parser.c @@ -26,6 +26,7 @@ #include static uint32_t max_header_size = HTTP_MAX_HEADER_SIZE; +static uint32_t max_url_size = HTTP_MAX_URL_SIZE; #ifndef ULLONG_MAX # define ULLONG_MAX ((uint64_t) -1) /* 2^64-1 */ @@ -160,6 +161,24 @@ do { \ } \ } while (0) +/* Don't allow the total size of the request uri to exceed + * the max_url_size. If we don't check the request uri size and + * it exceeds the max_header_size, the embedder will get an + * HPE_HEADER_OVERFLOW, which is ambiguous in situations where + * what caused the overflow matters, e.g. servers deciding to + * reply either 414 or 431 status. + */ +#include +#define COUNT_URL_SIZE(V, O) \ +do { \ + nread += (uint32_t)(V); \ + url_offset = (uint8_t)(O); \ + if (UNLIKELY(nread > url_offset && \ + (nread - url_offset) > max_url_size)) { \ + SET_ERRNO(HPE_URL_OVERFLOW); \ + goto error; \ + } \ +} while(0) \ #define PROXY_CONNECTION "proxy-connection" #define CONNECTION "connection" @@ -173,12 +192,17 @@ do { \ static const char *method_strings[] = { -#define XX(num, name, string) #string, +#define XX(num, name, string, length) #string, HTTP_METHOD_MAP(XX) #undef XX }; - +static const uint8_t method_lengths[] = + { +#define XX(num, name, string, length) length, + HTTP_METHOD_MAP(XX) +#undef XX + }; /* Tokens as defined by rfc 2616. Also lowercases them. * token = 1* * separators = "(" | ")" | "<" | ">" | "@" @@ -358,9 +382,9 @@ enum state , s_message_done }; - -#define PARSING_HEADER(state) (state <= s_headers_done) - +#define PARSING_HEADER(state) (state > s_req_http_end && state <= s_headers_done) +#define PARSING_URL(state) (state > s_req_spaces_before_url && state < s_req_http_start) +#define PARSING_START_LINE(state) (state <= s_req_spaces_before_url || (state >= s_req_http_start && state <= s_req_http_end)) enum header_states { h_general = 0 @@ -642,6 +666,8 @@ size_t http_parser_execute (http_parser *parser, { char c, ch; int8_t unhex_val; + uint8_t url_offset; + uint32_t spaces_before_url = 0; const char *p = data; const char *header_field_mark = 0; const char *header_value_mark = 0; @@ -707,9 +733,14 @@ size_t http_parser_execute (http_parser *parser, for (p=data; p != data + len; p++) { ch = *p; - if (PARSING_HEADER(CURRENT_STATE())) + if (PARSING_HEADER(CURRENT_STATE())) { COUNT_HEADER_SIZE(1); - + } else if (PARSING_URL(CURRENT_STATE())) { + COUNT_URL_SIZE(1, method_lengths[parser->method] + spaces_before_url); + } else if (PARSING_START_LINE(CURRENT_STATE())) { + nread++; + } + reexecute: switch (CURRENT_STATE()) { @@ -1015,7 +1046,10 @@ size_t http_parser_execute (http_parser *parser, case s_req_spaces_before_url: { - if (ch == ' ') break; + if (ch == ' ') { + ++spaces_before_url; + break; + } MARK(url); if (parser->method == HTTP_CONNECT) { @@ -2499,3 +2533,8 @@ void http_parser_set_max_header_size(uint32_t size) { max_header_size = size; } + +void +http_parser_set_max_uri_size(uint32_t size) { + max_url_size = size; +} diff --git a/http_parser.h b/http_parser.h index 880ed278..cbfa0d9b 100644 --- a/http_parser.h +++ b/http_parser.h @@ -63,6 +63,17 @@ typedef unsigned __int64 uint64_t; # define HTTP_MAX_HEADER_SIZE (80*1024) #endif +/* Maximium uri size allowed. If the macro is not defined + * before including this header then the default is used. To + * change the maximum uri size, define the macro in the build + * environment (e.g. -DHTTP_MAX_URL_SIZE=). To remove + * the effective limit on the size of the uri, define the macro + * to a very large number (e.g. -DHTTP_MAX_URL_SIZE=0x7fffffff) + */ +#ifndef HTTP_MAX_URL_SIZE +# define HTTP_MAX_URL_SIZE (80*1024) +#endif + typedef struct http_parser http_parser; typedef struct http_parser_settings http_parser_settings; @@ -160,53 +171,53 @@ enum http_status /* Request Methods */ -#define HTTP_METHOD_MAP(XX) \ - XX(0, DELETE, DELETE) \ - XX(1, GET, GET) \ - XX(2, HEAD, HEAD) \ - XX(3, POST, POST) \ - XX(4, PUT, PUT) \ - /* pathological */ \ - XX(5, CONNECT, CONNECT) \ - XX(6, OPTIONS, OPTIONS) \ - XX(7, TRACE, TRACE) \ - /* WebDAV */ \ - XX(8, COPY, COPY) \ - XX(9, LOCK, LOCK) \ - XX(10, MKCOL, MKCOL) \ - XX(11, MOVE, MOVE) \ - XX(12, PROPFIND, PROPFIND) \ - XX(13, PROPPATCH, PROPPATCH) \ - XX(14, SEARCH, SEARCH) \ - XX(15, UNLOCK, UNLOCK) \ - XX(16, BIND, BIND) \ - XX(17, REBIND, REBIND) \ - XX(18, UNBIND, UNBIND) \ - XX(19, ACL, ACL) \ - /* subversion */ \ - XX(20, REPORT, REPORT) \ - XX(21, MKACTIVITY, MKACTIVITY) \ - XX(22, CHECKOUT, CHECKOUT) \ - XX(23, MERGE, MERGE) \ - /* upnp */ \ - XX(24, MSEARCH, M-SEARCH) \ - XX(25, NOTIFY, NOTIFY) \ - XX(26, SUBSCRIBE, SUBSCRIBE) \ - XX(27, UNSUBSCRIBE, UNSUBSCRIBE) \ - /* RFC-5789 */ \ - XX(28, PATCH, PATCH) \ - XX(29, PURGE, PURGE) \ - /* CalDAV */ \ - XX(30, MKCALENDAR, MKCALENDAR) \ - /* RFC-2068, section 19.6.1.2 */ \ - XX(31, LINK, LINK) \ - XX(32, UNLINK, UNLINK) \ - /* icecast */ \ - XX(33, SOURCE, SOURCE) \ +#define HTTP_METHOD_MAP(XX) \ + XX(0, DELETE, DELETE, 6) \ + XX(1, GET, GET, 3) \ + XX(2, HEAD, HEAD, 4) \ + XX(3, POST, POST, 4) \ + XX(4, PUT, PUT, 3) \ + /* pathological */ \ + XX(5, CONNECT, CONNECT, 7) \ + XX(6, OPTIONS, OPTIONS, 7) \ + XX(7, TRACE, TRACE, 5) \ + /* WebDAV */ \ + XX(8, COPY, COPY, 4) \ + XX(9, LOCK, LOCK, 4) \ + XX(10, MKCOL, MKCOL, 5) \ + XX(11, MOVE, MOVE, 4) \ + XX(12, PROPFIND, PROPFIND, 8) \ + XX(13, PROPPATCH, PROPPATCH, 9) \ + XX(14, SEARCH, SEARCH, 6) \ + XX(15, UNLOCK, UNLOCK, 6) \ + XX(16, BIND, BIND, 4) \ + XX(17, REBIND, REBIND, 6) \ + XX(18, UNBIND, UNBIND, 6) \ + XX(19, ACL, ACL, 3) \ + /* subversion */ \ + XX(20, REPORT, REPORT, 6) \ + XX(21, MKACTIVITY, MKACTIVITY, 10) \ + XX(22, CHECKOUT, CHECKOUT, 8) \ + XX(23, MERGE, MERGE, 5) \ + /* upnp */ \ + XX(24, MSEARCH, M-SEARCH, 7) \ + XX(25, NOTIFY, NOTIFY, 6) \ + XX(26, SUBSCRIBE, SUBSCRIBE, 9) \ + XX(27, UNSUBSCRIBE, UNSUBSCRIBE, 11) \ + /* RFC-5789 */ \ + XX(28, PATCH, PATCH, 5) \ + XX(29, PURGE, PURGE, 5) \ + /* CalDAV */ \ + XX(30, MKCALENDAR, MKCALENDAR, 10) \ + /* RFC-2068, section 19.6.1.2 */ \ + XX(31, LINK, LINK, 4) \ + XX(32, UNLINK, UNLINK, 6) \ + /* icecast */ \ + XX(33, SOURCE, SOURCE, 6) \ enum http_method { -#define XX(num, name, string) HTTP_##name = num, +#define XX(num, name, string, length) HTTP_##name = num, HTTP_METHOD_MAP(XX) #undef XX }; @@ -252,6 +263,8 @@ enum flags XX(INVALID_EOF_STATE, "stream ended at an unexpected time") \ XX(HEADER_OVERFLOW, \ "too many header bytes seen; overflow detected") \ + XX(URL_OVERFLOW, \ + "too many url bytes seen; overflow detected") \ XX(CLOSED_CONNECTION, \ "data received after completed connection: close message") \ XX(INVALID_VERSION, "invalid HTTP version") \ @@ -433,6 +446,9 @@ int http_body_is_final(const http_parser *parser); /* Change the maximum header size provided at compile time. */ void http_parser_set_max_header_size(uint32_t size); +/* Change the maximum uri size provided at compile time. */ +void http_parser_set_max_uri_size(uint32_t size); + #ifdef __cplusplus } #endif diff --git a/test.c b/test.c index c3fddd50..05a2fec6 100644 --- a/test.c +++ b/test.c @@ -3756,6 +3756,32 @@ test_header_overflow_error (int req) abort(); } +void +test_URL_OVERFLOW_error () +{ + http_parser parser; + http_parser_init(&parser, HTTP_REQUEST); + size_t parsed; + const char *buf; + buf = "GET /"; + parsed = http_parser_execute(&parser, &settings_null, buf, strlen(buf)); + assert(parsed == strlen(buf)); + + buf = "a"; + size_t buflen = strlen(buf); + + int i; + for (i = 0; i < HTTP_MAX_URL_SIZE - 1; i++) { //-1 because it started with a slash + parsed = http_parser_execute(&parser, &settings_null, buf, buflen); + if (parsed != buflen) { + assert(HTTP_PARSER_ERRNO(&parser) == HPE_URL_OVERFLOW); + return; + } + } + + fprintf(stderr, "\n*** Error expected but none in uri overflow test ***\n"); + abort(); +} void test_header_nread_value () @@ -4159,6 +4185,8 @@ main (void) //// OVERFLOW CONDITIONS test_no_overflow_parse_url(); + test_URL_OVERFLOW_error(); + test_header_overflow_error(HTTP_REQUEST); test_no_overflow_long_body(HTTP_REQUEST, 1000); test_no_overflow_long_body(HTTP_REQUEST, 100000);