From fb80502513bf0140c5e595714967f75ea3e1e5d3 Mon Sep 17 00:00:00 2001 From: Valentin Bartenev Date: Tue, 17 Nov 2020 16:50:06 +0300 Subject: HTTP parser: allowed more characters in header field names. Previously, all requests that contained in header field names characters other than alphanumeric, or "-", or "_" were rejected with a 400 "Bad Request" error response. Now, the parser allows the same set of characters as specified in RFC 7230, including: "!", "#", "$", "%", "&", "'", "*", "+", ".", "^", "`", "|", and "~". Header field names that contain only these characters are considered valid. Also, there's a new option introduced: "discard_unsafe_fields". It accepts boolean value and it is set to "true" by default. When this option is "true", all header field names that contain characters in valid range, but other than alphanumeric or "-" are skipped during parsing. When the option is "false", these header fields aren't skipped. Requests with non-valid characters in header field names according to RFC 7230 are rejected regardless of "discard_unsafe_fields" setting. This closes #422 issue on GitHub. --- src/nxt_conf_validation.c | 3 ++ src/nxt_h1proto.c | 6 +++- src/nxt_http_parse.c | 67 +++++++++++++++++++++++++++--------------- src/nxt_http_parse.h | 14 +++++---- src/nxt_router.c | 7 +++++ src/nxt_router.h | 2 ++ src/test/nxt_http_parse_test.c | 38 +++++++++++++++++++----- 7 files changed, 100 insertions(+), 37 deletions(-) (limited to 'src') diff --git a/src/nxt_conf_validation.c b/src/nxt_conf_validation.c index dca56881..f4e7c358 100644 --- a/src/nxt_conf_validation.c +++ b/src/nxt_conf_validation.c @@ -273,6 +273,9 @@ static nxt_conf_vldt_object_t nxt_conf_vldt_http_members[] = { }, { .name = nxt_string("body_temp_path"), .type = NXT_CONF_VLDT_STRING, + }, { + .name = nxt_string("discard_unsafe_fields"), + .type = NXT_CONF_VLDT_BOOLEAN, }, { .name = nxt_string("websocket"), .type = NXT_CONF_VLDT_OBJECT, diff --git a/src/nxt_h1proto.c b/src/nxt_h1proto.c index dc23d7c4..dccbe56c 100644 --- a/src/nxt_h1proto.c +++ b/src/nxt_h1proto.c @@ -467,6 +467,7 @@ nxt_h1p_conn_request_init(nxt_task_t *task, void *obj, void *data) nxt_int_t ret; nxt_conn_t *c; nxt_h1proto_t *h1p; + nxt_socket_conf_t *skcf; nxt_http_request_t *r; nxt_socket_conf_joint_t *joint; @@ -503,11 +504,14 @@ nxt_h1p_conn_request_init(nxt_task_t *task, void *obj, void *data) joint->count++; r->conf = joint; + skcf = joint->socket_conf; if (c->local == NULL) { - c->local = joint->socket_conf->sockaddr; + c->local = skcf->sockaddr; } + h1p->parser.discard_unsafe_fields = skcf->discard_unsafe_fields; + nxt_h1p_conn_request_header_parse(task, c, h1p); return; } diff --git a/src/nxt_http_parse.c b/src/nxt_http_parse.c index 22004cc1..338b0a90 100644 --- a/src/nxt_http_parse.c +++ b/src/nxt_http_parse.c @@ -288,11 +288,13 @@ continue_target: case NXT_HTTP_TARGET_SPACE: rp->target_end = p; goto space_after_target; - +#if 0 case NXT_HTTP_TARGET_QUOTE_MARK: rp->quoted_target = 1; goto rest_of_target; - +#else + case NXT_HTTP_TARGET_QUOTE_MARK: +#endif case NXT_HTTP_TARGET_HASH: rp->complex_target = 1; goto rest_of_target; @@ -378,7 +380,7 @@ space_after_target: } } - rp->space_in_target = 1; + //rp->space_in_target = 1; if (rest) { goto rest_of_target; @@ -397,7 +399,7 @@ space_after_target: goto space_after_target; } - rp->space_in_target = 1; + //rp->space_in_target = 1; if (rest) { goto rest_of_target; @@ -432,7 +434,12 @@ space_after_target: *pos = p + 10; } - if (rp->complex_target != 0 || rp->quoted_target != 0) { + if (rp->complex_target != 0 +#if 0 + || rp->quoted_target != 0 +#endif + ) + { rc = nxt_http_parse_complex_target(rp); if (nxt_slow_path(rc != NXT_OK)) { @@ -518,11 +525,13 @@ nxt_http_parse_field_name(nxt_http_request_parse_t *rp, u_char **pos, static const u_char normal[256] nxt_aligned(64) = "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" - "\0\0\0\0\0\0\0\0\0\0\0\0\0-\0\0" "0123456789\0\0\0\0\0\0" + /* \s ! " # $ % & ' ( ) * + , . / : ; < = > ? */ + "\0\1\0\1\1\1\1\1\0\0\1\1\0" "-" "\1\0" "0123456789" "\0\0\0\0\0\0" - /* These 64 bytes should reside in one cache line. */ - "\0abcdefghijklmnopqrstuvwxyz\0\0\0\0_" - "\0abcdefghijklmnopqrstuvwxyz\0\0\0\0\0" + /* @ [ \ ] ^ _ */ + "\0" "abcdefghijklmnopqrstuvwxyz" "\0\0\0\1\1" + /* ` { | } ~ */ + "\1" "abcdefghijklmnopqrstuvwxyz" "\0\1\0\1\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0" @@ -538,9 +547,14 @@ nxt_http_parse_field_name(nxt_http_request_parse_t *rp, u_char **pos, \ c = normal[ch]; \ \ - if (nxt_slow_path(c == '\0')) { \ - p = &(ch); \ - goto name_end; \ + if (nxt_slow_path(c <= '\1')) { \ + if (c == '\0') { \ + p = &(ch); \ + goto name_end; \ + } \ + \ + rp->skip_field = rp->discard_unsafe_fields; \ + c = ch; \ } \ \ hash = nxt_http_field_hash_char(hash, c); @@ -777,20 +791,25 @@ nxt_http_parse_field_end(nxt_http_request_parse_t *rp, u_char **pos, *pos = p + 1; if (rp->field_name.length != 0) { - field = nxt_list_add(rp->fields); + if (rp->skip_field) { + rp->skip_field = 0; - if (nxt_slow_path(field == NULL)) { - return NXT_ERROR; - } + } else { + field = nxt_list_add(rp->fields); - field->hash = nxt_http_field_hash_end(rp->field_hash); - field->skip = 0; - field->hopbyhop = 0; + if (nxt_slow_path(field == NULL)) { + return NXT_ERROR; + } - field->name_length = rp->field_name.length; - field->value_length = rp->field_value.length; - field->name = rp->field_name.start; - field->value = rp->field_value.start; + field->hash = nxt_http_field_hash_end(rp->field_hash); + field->skip = 0; + field->hopbyhop = 0; + + field->name_length = rp->field_name.length; + field->value_length = rp->field_value.length; + field->name = rp->field_name.start; + field->value = rp->field_value.start; + } rp->field_hash = NXT_HTTP_FIELD_HASH_INIT; @@ -1023,7 +1042,7 @@ nxt_http_parse_complex_target(nxt_http_request_parse_t *rp) break; case sw_quoted: - rp->quoted_target = 1; + //rp->quoted_target = 1; if (ch >= '0' && ch <= '9') { high = (u_char) (ch - '0'); diff --git a/src/nxt_http_parse.h b/src/nxt_http_parse.h index cbfc8433..3cd9bd15 100644 --- a/src/nxt_http_parse.h +++ b/src/nxt_http_parse.h @@ -55,15 +55,19 @@ struct nxt_http_request_parse_s { uint32_t field_hash; + uint8_t skip_field; /* 1 bit */ + uint8_t discard_unsafe_fields; /* 1 bit */ + /* target with "/." */ - uint8_t complex_target; /* 1 bit */ + uint8_t complex_target; /* 1 bit */ +#if 0 /* target with "%" */ - uint8_t quoted_target; /* 1 bit */ + uint8_t quoted_target; /* 1 bit */ /* target with " " */ - uint8_t space_in_target; /* 1 bit */ - + uint8_t space_in_target; /* 1 bit */ +#endif /* Preserve encoded '/' (%2F) and '%' (%25). */ - uint8_t encoded_slashes; /* 1 bit */ + uint8_t encoded_slashes; /* 1 bit */ }; diff --git a/src/nxt_router.c b/src/nxt_router.c index 95c8c160..9dd5c30e 100644 --- a/src/nxt_router.c +++ b/src/nxt_router.c @@ -1262,6 +1262,12 @@ static nxt_conf_map_t nxt_router_http_conf[] = { NXT_CONF_MAP_STR, offsetof(nxt_socket_conf_t, body_temp_path), }, + + { + nxt_string("discard_unsafe_fields"), + NXT_CONF_MAP_INT8, + offsetof(nxt_socket_conf_t, discard_unsafe_fields), + }, }; @@ -1649,6 +1655,7 @@ nxt_router_conf_create(nxt_task_t *task, nxt_router_temp_conf_t *tmcf, skcf->header_buffer_size = 2048; skcf->large_header_buffer_size = 8192; skcf->large_header_buffers = 4; + skcf->discard_unsafe_fields = 1; skcf->body_buffer_size = 16 * 1024; skcf->max_body_size = 8 * 1024 * 1024; skcf->proxy_header_buffer_size = 64 * 1024; diff --git a/src/nxt_router.h b/src/nxt_router.h index 512f1810..5804840f 100644 --- a/src/nxt_router.h +++ b/src/nxt_router.h @@ -191,6 +191,8 @@ typedef struct { nxt_str_t body_temp_path; + uint8_t discard_unsafe_fields; /* 1 bit */ + #if (NXT_TLS) nxt_tls_conf_t *tls; #endif diff --git a/src/test/nxt_http_parse_test.c b/src/test/nxt_http_parse_test.c index 9630b21c..540309c1 100644 --- a/src/test/nxt_http_parse_test.c +++ b/src/test/nxt_http_parse_test.c @@ -23,9 +23,15 @@ typedef struct { } nxt_http_parse_test_request_line_t; +typedef struct { + nxt_int_t result; + unsigned discard_unsafe_fields:1; +} nxt_http_parse_test_fields_t; + + typedef union { void *pointer; - nxt_int_t result; + nxt_http_parse_test_fields_t fields; nxt_http_parse_test_request_line_t request_line; } nxt_http_parse_test_data_t; @@ -324,10 +330,11 @@ static nxt_http_parse_test_case_t nxt_http_test_cases[] = { { nxt_string("GET / HTTP/1.1\r\n" "X-Unknown-Header: value\r\n" - "X-Good-Header: value\r\n\r\n"), + "X-Good-Header: value\r\n" + "!#$%&'*+.^_`|~: skipped\r\n\r\n"), NXT_DONE, &nxt_http_parse_test_fields, - { .result = NXT_OK } + { .fields = { NXT_OK, 1 } } }, { nxt_string("GET / HTTP/1.1\r\n" @@ -336,7 +343,14 @@ static nxt_http_parse_test_case_t nxt_http_test_cases[] = { "X-Bad-Header: value\r\n\r\n"), NXT_DONE, &nxt_http_parse_test_fields, - { .result = NXT_ERROR } + { .fields = { NXT_ERROR, 1 } } + }, + { + nxt_string("GET / HTTP/1.1\r\n" + "!#$%&'*+.^_`|~: allowed\r\n\r\n"), + NXT_DONE, + &nxt_http_parse_test_fields, + { .fields = { NXT_ERROR, 0 } } }, }; @@ -349,6 +363,10 @@ static nxt_http_field_proc_t nxt_http_test_fields[] = { { nxt_string("X-Good-Header"), &nxt_http_test_header_return, NXT_OK }, + + { nxt_string("!#$%&'*+.^_`|~"), + &nxt_http_test_header_return, + NXT_ERROR }, }; @@ -540,6 +558,10 @@ nxt_http_parse_test(nxt_thread_t *thr) return NXT_ERROR; } + if (test->handler == &nxt_http_parse_test_fields) { + rp.discard_unsafe_fields = test->data.fields.discard_unsafe_fields; + } + rc = nxt_http_parse_test_run(&rp, &test->request); if (rc != test->result) { @@ -740,7 +762,7 @@ nxt_http_parse_test_request_line(nxt_http_request_parse_t *rp, return NXT_ERROR; } - if (rp->complex_target != test->complex_target) { + if (rp->complex_target != (test->complex_target | test->quoted_target)) { nxt_log_alert(log, "http parse test case failed:\n" " - request:\n\"%V\"\n" " - complex_target: %d (expected: %d)", @@ -748,6 +770,7 @@ nxt_http_parse_test_request_line(nxt_http_request_parse_t *rp, return NXT_ERROR; } +#if 0 if (rp->quoted_target != test->quoted_target) { nxt_log_alert(log, "http parse test case failed:\n" " - request:\n\"%V\"\n" @@ -763,6 +786,7 @@ nxt_http_parse_test_request_line(nxt_http_request_parse_t *rp, request, rp->space_in_target, test->space_in_target); return NXT_ERROR; } +#endif return NXT_OK; } @@ -776,11 +800,11 @@ nxt_http_parse_test_fields(nxt_http_request_parse_t *rp, rc = nxt_http_fields_process(rp->fields, &nxt_http_test_fields_hash, NULL); - if (rc != data->result) { + if (rc != data->fields.result) { nxt_log_alert(log, "http parse test hash failed:\n" " - request:\n\"%V\"\n" " - result: %i (expected: %i)", - request, rc, data->result); + request, rc, data->fields.result); return NXT_ERROR; } -- cgit