From dfcdea894be54444dd826034df469d21c3ef3521 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Wed, 25 Dec 2013 23:38:55 +0900 Subject: [PATCH] Move header name/value pair validation to on_*_received functions --- lib/nghttp2_frame.c | 19 ++++++++++++++- lib/nghttp2_frame.h | 13 +++++++--- lib/nghttp2_hd.c | 6 ----- lib/nghttp2_helper.c | 13 ++++++++++ lib/nghttp2_helper.h | 13 +++++++--- lib/nghttp2_session.c | 21 ++++++++++++++-- lib/nghttp2_submit.c | 4 +-- tests/main.c | 8 +++--- tests/nghttp2_frame_test.c | 27 +++++++++++++++++---- tests/nghttp2_frame_test.h | 2 +- tests/nghttp2_helper_test.c | 15 ++++++++++++ tests/nghttp2_helper_test.h | 1 + tests/nghttp2_session_test.c | 47 ++++++++++++++++++++++++++++++++++++ 13 files changed, 163 insertions(+), 26 deletions(-) diff --git a/lib/nghttp2_frame.c b/lib/nghttp2_frame.c index 843256e5..27b75998 100644 --- a/lib/nghttp2_frame.c +++ b/lib/nghttp2_frame.c @@ -585,13 +585,30 @@ nghttp2_settings_entry* nghttp2_frame_iv_copy(const nghttp2_settings_entry *iv, return iv_copy; } -int nghttp2_nv_array_check_null(const nghttp2_nv *nva, size_t nvlen) +int nghttp2_nv_array_check_nocase(const nghttp2_nv *nva, size_t nvlen) { size_t i; for(i = 0; i < nvlen; ++i) { if(!nghttp2_check_header_name_nocase(nva[i].name, nva[i].namelen)) { return 0; } + if(!nghttp2_check_header_value(nva[i].value, nva[i].valuelen)) { + return 0; + } + } + return 1; +} + +int nghttp2_nv_array_check(const nghttp2_nv *nva, size_t nvlen) +{ + size_t i; + for(i = 0; i < nvlen; ++i) { + if(!nghttp2_check_header_name(nva[i].name, nva[i].namelen)) { + return 0; + } + if(!nghttp2_check_header_value(nva[i].value, nva[i].valuelen)) { + return 0; + } } return 1; } diff --git a/lib/nghttp2_frame.h b/lib/nghttp2_frame.h index 585876a1..e7c8f27b 100644 --- a/lib/nghttp2_frame.h +++ b/lib/nghttp2_frame.h @@ -552,13 +552,20 @@ int nghttp2_nv_equal(const nghttp2_nv *a, const nghttp2_nv *b); void nghttp2_nv_array_del(nghttp2_nv *nva); /* - * Checks names are not empty string and do not contain control - * characters. This function allows captital alphabet letters in name. + * Checks header name/value pair is well-formed. This function allows + * captital alphabet letters in name. * * This function returns nonzero if it succeeds, or 0. */ -int nghttp2_nv_array_check_null(const nghttp2_nv *nva, size_t nvlen); +int nghttp2_nv_array_check_nocase(const nghttp2_nv *nva, size_t nvlen); +/* + * Checks header name/value pair is well-formed. This function does + * not allow captital alphabet letters in name. + * + * This function returns nonzero if it succeeds, or 0. + */ +int nghttp2_nv_array_check(const nghttp2_nv *nva, size_t nvlen); /* * Checks that the |iv|, which includes |niv| entries, does not have diff --git a/lib/nghttp2_hd.c b/lib/nghttp2_hd.c index 65553264..01a40f2f 100644 --- a/lib/nghttp2_hd.c +++ b/lib/nghttp2_hd.c @@ -1354,12 +1354,6 @@ ssize_t nghttp2_hd_inflate_hd(nghttp2_hd_context *inflater, } in += namelen; - if(!nghttp2_check_header_name(nv.name, nv.namelen)) { - free(decoded_huffman_name); - rv = NGHTTP2_ERR_HEADER_COMP; - goto fail; - } - if(in == last) { DEBUGF(fprintf(stderr, "No value found\n")); free(decoded_huffman_name); diff --git a/lib/nghttp2_helper.c b/lib/nghttp2_helper.c index 1a9f6a03..c2359b9d 100644 --- a/lib/nghttp2_helper.c +++ b/lib/nghttp2_helper.c @@ -219,6 +219,19 @@ int nghttp2_check_header_name_nocase(const uint8_t *name, size_t len) return check_header_name(name, len, 1); } +int nghttp2_check_header_value(const uint8_t* value, size_t len) +{ + size_t i; + for(i = 0; i < len; ++i) { + /* Only allow NUL or ASCII range [0x20, 0x7e], inclusive, to match + HTTP/1 sematics */ + if(value[i] != '\0' && (0x20u > value[i] || value[i] > 0x7eu)) { + return 0; + } + } + return 1; +} + const char* nghttp2_strerror(int error_code) { switch(error_code) { diff --git a/lib/nghttp2_helper.h b/lib/nghttp2_helper.h index 61ebbc3b..56d09523 100644 --- a/lib/nghttp2_helper.h +++ b/lib/nghttp2_helper.h @@ -119,20 +119,27 @@ int nghttp2_should_send_window_update(int32_t local_window_size, int32_t recv_window_size); /* - * Checks the header name in |name| with |len| bytes is valid. + * Checks the header name in |name| with |len| bytes is well-formed. * * This function returns nonzero if it succeeds, or 0. */ int nghttp2_check_header_name(const uint8_t *name, size_t len); /* - * Checks the header name in |name| with |len| bytes is valid. This - * function accepts also characters in [A-Z]. + * Checks the header name in |name| with |len| bytes is + * well-formed. This function accepts also characters in [A-Z]. * * This function returns nonzero if it succeeds, or 0. */ int nghttp2_check_header_name_nocase(const uint8_t *name, size_t len); +/* + * Checks the header value in |value| with |len| bytes is well-formed. + * + * This function returns nonzero if it succeeds, or 0. + */ +int nghttp2_check_header_value(const uint8_t* value, size_t len); + /* * Deallocates memory space pointed by |ptr|. This function exists for * the application to free the memory space allocated by the library diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index 30ac55a8..2814afe3 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -1909,6 +1909,10 @@ int nghttp2_session_on_request_headers_received(nghttp2_session *session, NGHTTP2_PROTOCOL_ERROR); } session->last_recv_stream_id = frame->hd.stream_id; + if(!nghttp2_nv_array_check(frame->headers.nva, frame->headers.nvlen)) { + return nghttp2_session_handle_invalid_stream(session, frame, + NGHTTP2_PROTOCOL_ERROR); + } error_code = nghttp2_session_validate_request_headers(session, &frame->headers); if(error_code != NGHTTP2_NO_ERROR) { @@ -1965,6 +1969,10 @@ int nghttp2_session_on_response_headers_received(nghttp2_session *session, return nghttp2_session_handle_invalid_stream(session, frame, NGHTTP2_STREAM_CLOSED); } + if(!nghttp2_nv_array_check(frame->headers.nva, frame->headers.nvlen)) { + return nghttp2_session_handle_invalid_stream(session, frame, + NGHTTP2_PROTOCOL_ERROR); + } stream->state = NGHTTP2_STREAM_OPENED; rv = nghttp2_session_call_on_frame_received(session, frame); if(rv != 0) { @@ -2001,6 +2009,10 @@ int nghttp2_session_on_push_response_headers_received(nghttp2_session *session, /* We don't accept new stream after GOAWAY is sent or received. */ return 0; } + if(!nghttp2_nv_array_check(frame->headers.nva, frame->headers.nvlen)) { + return nghttp2_session_handle_invalid_stream(session, frame, + NGHTTP2_PROTOCOL_ERROR); + } rv = nghttp2_session_validate_request_headers(session, &frame->headers); if(rv != 0) { return nghttp2_session_handle_invalid_stream(session, frame, rv); @@ -2055,6 +2067,10 @@ int nghttp2_session_on_headers_received(nghttp2_session *session, return nghttp2_session_handle_invalid_stream(session, frame, NGHTTP2_STREAM_CLOSED); } + if(!nghttp2_nv_array_check(frame->headers.nva, frame->headers.nvlen)) { + return nghttp2_session_handle_invalid_stream(session, frame, + NGHTTP2_PROTOCOL_ERROR); + } if(nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) { if(stream->state == NGHTTP2_STREAM_OPENED) { r = nghttp2_session_call_on_frame_received(session, frame); @@ -2561,12 +2577,13 @@ int nghttp2_session_on_push_promise_received(nghttp2_session *session, frame->push_promise.promised_stream_id, NGHTTP2_REFUSED_STREAM); } - if(!nghttp2_session_is_my_stream_id(session, frame->hd.stream_id)) { return nghttp2_session_handle_invalid_connection(session, frame, NGHTTP2_PROTOCOL_ERROR); } - if(stream->shut_flags & NGHTTP2_SHUT_RD) { + if((stream->shut_flags & NGHTTP2_SHUT_RD) || + !nghttp2_nv_array_check(frame->push_promise.nva, + frame->push_promise.nvlen)) { if(session->callbacks.on_invalid_frame_recv_callback) { if(session->callbacks.on_invalid_frame_recv_callback (session, frame, NGHTTP2_PROTOCOL_ERROR, session->user_data) != 0) { diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index 898ac07f..cc42c990 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -110,7 +110,7 @@ static int nghttp2_submit_headers_shared_nva { ssize_t rv; nghttp2_nv *nva_copy; - if(!nghttp2_nv_array_check_null(nva, nvlen)) { + if(!nghttp2_nv_array_check_nocase(nva, nvlen)) { return NGHTTP2_ERR_INVALID_ARGUMENT; } rv = nghttp2_nv_array_copy(&nva_copy, nva, nvlen); @@ -199,7 +199,7 @@ int nghttp2_submit_push_promise(nghttp2_session *session, uint8_t flags, uint8_t flags_copy; int rv; - if(!nghttp2_nv_array_check_null(nva, nvlen)) { + if(!nghttp2_nv_array_check_nocase(nva, nvlen)) { return NGHTTP2_ERR_INVALID_ARGUMENT; } if(nghttp2_session_get_stream(session, stream_id) == NULL) { diff --git a/tests/main.c b/tests/main.c index 5f1242aa..26e7364c 100644 --- a/tests/main.c +++ b/tests/main.c @@ -215,8 +215,8 @@ int main(int argc, char* argv[]) test_nghttp2_frame_pack_goaway) || !CU_add_test(pSuite, "frame_pack_window_update", test_nghttp2_frame_pack_window_update) || - !CU_add_test(pSuite, "nv_array_check_null", - test_nghttp2_nv_array_check_null) || + !CU_add_test(pSuite, "nv_array_check", + test_nghttp2_nv_array_check) || !CU_add_test(pSuite, "nv_array_copy", test_nghttp2_nv_array_copy) || !CU_add_test(pSuite, "iv_check", test_nghttp2_iv_check) || !CU_add_test(pSuite, "hd_deflate", test_nghttp2_hd_deflate) || @@ -250,7 +250,9 @@ int main(int argc, char* argv[]) !CU_add_test(pSuite, "adjust_local_window_size", test_nghttp2_adjust_local_window_size) || !CU_add_test(pSuite, "check_header_name", - test_nghttp2_check_header_name) + test_nghttp2_check_header_name) || + !CU_add_test(pSuite, "check_header_value", + test_nghttp2_check_header_value) ) { CU_cleanup_registry(); return CU_get_error(); diff --git a/tests/nghttp2_frame_test.c b/tests/nghttp2_frame_test.c index ad41deab..f3269130 100644 --- a/tests/nghttp2_frame_test.c +++ b/tests/nghttp2_frame_test.c @@ -361,7 +361,7 @@ void test_nghttp2_frame_pack_window_update(void) nghttp2_frame_window_update_free(&frame); } -void test_nghttp2_nv_array_check_null(void) +void test_nghttp2_nv_array_check(void) { nghttp2_nv nva1[] = {MAKE_NV("path", "/"), MAKE_NV("host", "a")}; @@ -370,11 +370,28 @@ void test_nghttp2_nv_array_check_null(void) nghttp2_nv nva3[] = {MAKE_NV("path", "/"), MAKE_NV("host\x01", "a")}; nghttp2_nv nva4[] = {MAKE_NV("PATH", "/")}; + nghttp2_nv nva5[] = {MAKE_NV("path", "foo"), + MAKE_NV("host", "foo\x00")}; + nghttp2_nv nva6[] = {MAKE_NV("path", "foo"), + MAKE_NV("host", "foo\x01")}; - CU_ASSERT(nghttp2_nv_array_check_null(nva1, ARRLEN(nva1))); - CU_ASSERT(0 == nghttp2_nv_array_check_null(nva2, ARRLEN(nva2))); - CU_ASSERT(0 == nghttp2_nv_array_check_null(nva3, ARRLEN(nva3))); - CU_ASSERT(nghttp2_nv_array_check_null(nva4, ARRLEN(nva4))); + CU_ASSERT(nghttp2_nv_array_check(nva1, ARRLEN(nva1))); + CU_ASSERT(nghttp2_nv_array_check_nocase(nva1, ARRLEN(nva1))); + + CU_ASSERT(0 == nghttp2_nv_array_check(nva2, ARRLEN(nva2))); + CU_ASSERT(0 == nghttp2_nv_array_check_nocase(nva2, ARRLEN(nva2))); + + CU_ASSERT(0 == nghttp2_nv_array_check(nva3, ARRLEN(nva3))); + CU_ASSERT(0 == nghttp2_nv_array_check_nocase(nva3, ARRLEN(nva3))); + + CU_ASSERT(0 == nghttp2_nv_array_check(nva4, ARRLEN(nva4))); + CU_ASSERT(nghttp2_nv_array_check_nocase(nva4, ARRLEN(nva4))); + + CU_ASSERT(nghttp2_nv_array_check(nva5, ARRLEN(nva5))); + CU_ASSERT(nghttp2_nv_array_check_nocase(nva5, ARRLEN(nva5))); + + CU_ASSERT(0 == nghttp2_nv_array_check(nva6, ARRLEN(nva6))); + CU_ASSERT(0 == nghttp2_nv_array_check_nocase(nva6, ARRLEN(nva6))); } void test_nghttp2_nv_array_copy(void) diff --git a/tests/nghttp2_frame_test.h b/tests/nghttp2_frame_test.h index 3ed76ff8..56c8ac48 100644 --- a/tests/nghttp2_frame_test.h +++ b/tests/nghttp2_frame_test.h @@ -34,7 +34,7 @@ void test_nghttp2_frame_pack_push_promise(void); void test_nghttp2_frame_pack_ping(void); void test_nghttp2_frame_pack_goaway(void); void test_nghttp2_frame_pack_window_update(void); -void test_nghttp2_nv_array_check_null(void); +void test_nghttp2_nv_array_check(void); void test_nghttp2_nv_array_copy(void); void test_nghttp2_iv_check(void); diff --git a/tests/nghttp2_helper_test.c b/tests/nghttp2_helper_test.c index 1bed11d0..2315929a 100644 --- a/tests/nghttp2_helper_test.c +++ b/tests/nghttp2_helper_test.c @@ -181,3 +181,18 @@ void test_nghttp2_check_header_name(void) CU_ASSERT(!check_header_name_nocase("")); CU_ASSERT(!check_header_name_nocase(":")); } + +#define check_header_value(S) \ + nghttp2_check_header_value((const uint8_t*)S, sizeof(S) - 1) + +void test_nghttp2_check_header_value(void) +{ + uint8_t goodval[] = { 'a', '\0', 'b' }; + uint8_t badval1[] = { 'a', 0x1fu, 'b' }; + uint8_t badval2[] = { 'a', 0x7fu, 'b' }; + + CU_ASSERT(check_header_value(" !|}~")); + CU_ASSERT(check_header_value(goodval)); + CU_ASSERT(!check_header_value(badval1)); + CU_ASSERT(!check_header_value(badval2)); +} diff --git a/tests/nghttp2_helper_test.h b/tests/nghttp2_helper_test.h index a7699cf9..8cc6f511 100644 --- a/tests/nghttp2_helper_test.h +++ b/tests/nghttp2_helper_test.h @@ -27,5 +27,6 @@ void test_nghttp2_adjust_local_window_size(void); void test_nghttp2_check_header_name(void); +void test_nghttp2_check_header_value(void); #endif /* NGHTTP2_HELPER_TEST_H */ diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 1e6a8c13..f0e74e51 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -794,6 +794,9 @@ void test_nghttp2_session_on_request_headers_received(void) nghttp2_frame frame; nghttp2_stream *stream; int32_t stream_id = 1; + nghttp2_nv malformed_nva[] = { MAKE_NV(":path", "\x01") }; + nghttp2_nv *nva; + size_t nvlen; memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); callbacks.on_frame_recv_callback = on_frame_recv_callback; @@ -839,6 +842,22 @@ void test_nghttp2_session_on_request_headers_received(void) nghttp2_frame_headers_free(&frame.headers); nghttp2_session_del(session); + + /* Check malformed headers */ + nghttp2_session_server_new(&session, &callbacks, &user_data); + nvlen = nghttp2_nv_array_copy(&nva, malformed_nva, ARRLEN(malformed_nva)); + nghttp2_frame_headers_init(&frame.headers, + NGHTTP2_FLAG_END_HEADERS | NGHTTP2_FLAG_PRIORITY, + 1, 1 << 20, nva, nvlen); + user_data.frame_recv_cb_called = 0; + user_data.invalid_frame_recv_cb_called = 0; + CU_ASSERT(0 == nghttp2_session_on_request_headers_received(session, &frame)); + CU_ASSERT(0 == user_data.frame_recv_cb_called); + CU_ASSERT(1 == user_data.invalid_frame_recv_cb_called); + + nghttp2_frame_headers_free(&frame.headers); + + nghttp2_session_del(session); } void test_nghttp2_session_on_response_headers_received(void) @@ -1137,6 +1156,9 @@ void test_nghttp2_session_on_push_promise_received(void) nghttp2_frame frame; nghttp2_stream *stream, *promised_stream; nghttp2_outbound_item *item; + nghttp2_nv malformed_nva[] = { MAKE_NV(":path", "\x01") }; + nghttp2_nv *nva; + size_t nvlen; memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); callbacks.send_callback = null_send_callback; @@ -1255,6 +1277,7 @@ void test_nghttp2_session_on_push_promise_received(void) CU_ASSERT(0 == user_data.frame_recv_cb_called); CU_ASSERT(1 == user_data.invalid_frame_recv_cb_called); + nghttp2_frame_push_promise_free(&frame.push_promise); nghttp2_session_del(session); /* Disable PUSH */ @@ -1276,6 +1299,30 @@ void test_nghttp2_session_on_push_promise_received(void) CU_ASSERT(0 == user_data.frame_recv_cb_called); CU_ASSERT(1 == user_data.invalid_frame_recv_cb_called); + nghttp2_frame_push_promise_free(&frame.push_promise); + nghttp2_session_del(session); + + /* Check malformed headers */ + nghttp2_session_client_new(&session, &callbacks, &user_data); + stream = nghttp2_session_open_stream(session, 1, NGHTTP2_FLAG_NONE, + NGHTTP2_PRI_DEFAULT, + NGHTTP2_STREAM_OPENING, NULL); + nvlen = nghttp2_nv_array_copy(&nva, malformed_nva, ARRLEN(malformed_nva)); + nghttp2_frame_push_promise_init(&frame.push_promise, + NGHTTP2_FLAG_END_PUSH_PROMISE, 1, 2, + nva, nvlen); + user_data.frame_recv_cb_called = 0; + user_data.invalid_frame_recv_cb_called = 0; + CU_ASSERT(0 == nghttp2_session_on_push_promise_received(session, &frame)); + + CU_ASSERT(0 == user_data.frame_recv_cb_called); + CU_ASSERT(1 == user_data.invalid_frame_recv_cb_called); + item = nghttp2_session_get_next_ob_item(session); + CU_ASSERT(NGHTTP2_RST_STREAM == OB_CTRL_TYPE(item)); + CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == OB_CTRL(item)->rst_stream.error_code); + CU_ASSERT(2 == OB_CTRL(item)->hd.stream_id); + + nghttp2_frame_push_promise_free(&frame.push_promise); nghttp2_session_del(session); }