From bb6f842b37b57c3d8e191db948e9165c59af7daf Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 11 Jan 2016 16:18:39 +0900 Subject: [PATCH] Check request/response submission error based side of session Disallow request from server, and response from client respectively. When the violation is detected, return NGHTTP2_ERR_PROTO from nghttp2_submit_request, nghttp2_submit_response, nghttp2_submit_headers. We also did some refactoring, and now self-dependency detection is placed where it is only required. --- lib/includes/nghttp2/nghttp2.h | 6 +++ lib/nghttp2_submit.c | 89 +++++++++++++++++++++++++++------- tests/nghttp2_session_test.c | 79 ++++++++++++++++++++++++++---- 3 files changed, 146 insertions(+), 28 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 3c985332..27c4710b 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -3164,6 +3164,8 @@ nghttp2_priority_spec_check_default(const nghttp2_priority_spec *pri_spec); * :enum:`NGHTTP2_ERR_INVALID_ARGUMENT` * Trying to depend on itself (new stream ID equals * ``pri_spec->stream_id``). + * :enum:`NGHTTP2_ERR_PROTO` + * The |session| is server session. * * .. warning:: * @@ -3236,6 +3238,8 @@ nghttp2_submit_request(nghttp2_session *session, * processed yet. Normally, this does not happen, but when * application wrongly calls `nghttp2_submit_response()` twice, * this may happen. + * :enum:`NGHTTP2_ERR_PROTO` + * The |session| is client session. * * .. warning:: * @@ -3380,6 +3384,8 @@ NGHTTP2_EXTERN int nghttp2_submit_trailer(nghttp2_session *session, * DATA or HEADERS has been already submitted and not fully * processed yet. This happens if stream denoted by |stream_id| * is in reserved state. + * :enum:`NGHTTP2_ERR_PROTO` + * The |stream_id| is -1, and |session| is server session. * * .. warning:: * diff --git a/lib/nghttp2_submit.c b/lib/nghttp2_submit.c index 67ecf4f6..70271b7b 100644 --- a/lib/nghttp2_submit.c +++ b/lib/nghttp2_submit.c @@ -32,6 +32,35 @@ #include "nghttp2_helper.h" #include "nghttp2_priority_spec.h" +/* + * Detects the dependency error, that is stream attempted to depend on + * itself. If |stream_id| is -1, we use session->next_stream_id as + * stream ID. + * + * This function returns 0 if it succeeds, or one of the following + * error codes: + * + * NGHTTP2_ERR_INVALID_ARGUMENT + * Stream attempted to depend on itself. + */ +static int detect_self_dependency(nghttp2_session *session, int32_t stream_id, + const nghttp2_priority_spec *pri_spec) { + assert(pri_spec); + + if (stream_id == -1) { + if ((int32_t)session->next_stream_id == pri_spec->stream_id) { + return NGHTTP2_ERR_INVALID_ARGUMENT; + } + return 0; + } + + if (stream_id == pri_spec->stream_id) { + return NGHTTP2_ERR_INVALID_ARGUMENT; + } + + return 0; +} + /* This function takes ownership of |nva_copy|. Regardless of the return value, the caller must not free |nva_copy| after this function returns. */ @@ -50,21 +79,6 @@ static int32_t submit_headers_shared(nghttp2_session *session, uint8_t flags, mem = &session->mem; - if (stream_id == 0) { - rv = NGHTTP2_ERR_INVALID_ARGUMENT; - goto fail; - } - - if (stream_id == -1) { - if ((int32_t)session->next_stream_id == pri_spec->stream_id) { - rv = NGHTTP2_ERR_INVALID_ARGUMENT; - goto fail; - } - } else if (stream_id == pri_spec->stream_id) { - rv = NGHTTP2_ERR_INVALID_ARGUMENT; - goto fail; - } - item = nghttp2_mem_malloc(mem, sizeof(nghttp2_outbound_item)); if (item == NULL) { rv = NGHTTP2_ERR_NOMEM; @@ -156,6 +170,10 @@ static int32_t submit_headers_shared_nva(nghttp2_session *session, int nghttp2_submit_trailer(nghttp2_session *session, int32_t stream_id, const nghttp2_nv *nva, size_t nvlen) { + if (stream_id <= 0) { + return NGHTTP2_ERR_INVALID_ARGUMENT; + } + return (int)submit_headers_shared_nva(session, NGHTTP2_FLAG_END_STREAM, stream_id, NULL, nva, nvlen, NULL, NULL); @@ -166,9 +184,24 @@ int32_t nghttp2_submit_headers(nghttp2_session *session, uint8_t flags, const nghttp2_priority_spec *pri_spec, const nghttp2_nv *nva, size_t nvlen, void *stream_user_data) { + int rv; + + if (stream_id == -1) { + if (session->server) { + return NGHTTP2_ERR_PROTO; + } + } else if (stream_id <= 0) { + return NGHTTP2_ERR_INVALID_ARGUMENT; + } + flags &= NGHTTP2_FLAG_END_STREAM; if (pri_spec && !nghttp2_priority_spec_check_default(pri_spec)) { + rv = detect_self_dependency(session, stream_id, pri_spec); + if (rv != 0) { + return rv; + } + flags |= NGHTTP2_FLAG_PRIORITY; } else { pri_spec = NULL; @@ -281,7 +314,7 @@ int32_t nghttp2_submit_push_promise(nghttp2_session *session, uint8_t flags _U_, mem = &session->mem; - if (stream_id == 0 || nghttp2_session_is_my_stream_id(session, stream_id)) { + if (stream_id <= 0 || nghttp2_session_is_my_stream_id(session, stream_id)) { return NGHTTP2_ERR_INVALID_ARGUMENT; } @@ -396,8 +429,18 @@ int32_t nghttp2_submit_request(nghttp2_session *session, const nghttp2_data_provider *data_prd, void *stream_user_data) { uint8_t flags; + int rv; - if (pri_spec && nghttp2_priority_spec_check_default(pri_spec)) { + if (session->server) { + return NGHTTP2_ERR_PROTO; + } + + if (pri_spec && !nghttp2_priority_spec_check_default(pri_spec)) { + rv = detect_self_dependency(session, -1, pri_spec); + if (rv != 0) { + return rv; + } + } else { pri_spec = NULL; } @@ -418,7 +461,17 @@ static uint8_t set_response_flags(const nghttp2_data_provider *data_prd) { int nghttp2_submit_response(nghttp2_session *session, int32_t stream_id, const nghttp2_nv *nva, size_t nvlen, const nghttp2_data_provider *data_prd) { - uint8_t flags = set_response_flags(data_prd); + uint8_t flags; + + if (stream_id <= 0) { + return NGHTTP2_ERR_INVALID_ARGUMENT; + } + + if (!session->server) { + return NGHTTP2_ERR_PROTO; + } + + flags = set_response_flags(data_prd); return submit_headers_shared_nva(session, flags, stream_id, NULL, nva, nvlen, data_prd, NULL); } diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index dcc8220f..aeb5c8db 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -3897,6 +3897,15 @@ void test_nghttp2_submit_request_with_data(void) { CU_ASSERT(0 == ud.data_source_length); nghttp2_session_del(session); + + /* nghttp2_submit_request() with server session is error */ + nghttp2_session_server_new(&session, &callbacks, NULL); + + CU_ASSERT(NGHTTP2_ERR_PROTO == nghttp2_submit_request(session, NULL, reqnv, + ARRLEN(reqnv), NULL, + NULL)); + + nghttp2_session_del(session); } void test_nghttp2_submit_request_without_data(void) { @@ -3984,6 +3993,19 @@ void test_nghttp2_submit_response_with_data(void) { CU_ASSERT(0 == ud.data_source_length); nghttp2_session_del(session); + + /* Various error cases */ + nghttp2_session_client_new(&session, &callbacks, NULL); + + /* Calling nghttp2_submit_response() with client session is error */ + CU_ASSERT(NGHTTP2_ERR_PROTO == + nghttp2_submit_response(session, 1, resnv, ARRLEN(resnv), NULL)); + + /* Stream ID <= 0 is error */ + CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + nghttp2_submit_response(session, 0, resnv, ARRLEN(resnv), NULL)); + + nghttp2_session_del(session); } void test_nghttp2_submit_response_without_data(void) { @@ -4115,6 +4137,18 @@ void test_nghttp2_submit_trailer(void) { nghttp2_frame_headers_free(&frame.headers, mem); nghttp2_hd_inflate_free(&inflater); nghttp2_session_del(session); + + /* Specifying stream ID <= 0 is error */ + nghttp2_session_server_new(&session, &callbacks, NULL); + open_recv_stream(session, 1); + + CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + nghttp2_submit_trailer(session, 0, trailernv, ARRLEN(trailernv))); + + CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + nghttp2_submit_trailer(session, -1, trailernv, ARRLEN(trailernv))); + + nghttp2_session_del(session); } void test_nghttp2_submit_headers_start_stream(void) { @@ -4305,6 +4339,22 @@ void test_nghttp2_submit_headers(void) { reqnv, ARRLEN(reqnv), NULL)); nghttp2_session_del(session); + + /* Error cases with invalid stream ID */ + nghttp2_session_server_new(&session, &callbacks, NULL); + + /* Sending nghttp2_submit_headers() with stream_id == 1 and server + session is error */ + CU_ASSERT(NGHTTP2_ERR_PROTO == + nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, -1, NULL, reqnv, + ARRLEN(reqnv), NULL)); + + /* Sending stream ID <= 0 is error */ + CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, 0, NULL, resnv, + ARRLEN(resnv), NULL)); + + nghttp2_session_del(session); } void test_nghttp2_submit_headers_continuation(void) { @@ -4642,7 +4692,12 @@ void test_nghttp2_submit_push_promise(void) { /* submit PUSH_PROMISE while associated stream is not opened */ CU_ASSERT(NGHTTP2_ERR_STREAM_CLOSED == nghttp2_submit_push_promise(session, NGHTTP2_FLAG_NONE, 3, reqnv, - ARRLEN(reqnv), &ud)); + ARRLEN(reqnv), NULL)); + + /* Stream ID <= 0 is error */ + CU_ASSERT(NGHTTP2_ERR_INVALID_ARGUMENT == + nghttp2_submit_push_promise(session, NGHTTP2_FLAG_NONE, 0, reqnv, + ARRLEN(reqnv), NULL)); nghttp2_session_del(session); } @@ -4850,19 +4905,10 @@ void test_nghttp2_submit_invalid_nv(void) { CU_ASSERT(0 == nghttp2_session_server_new(&session, &callbacks, NULL)); - /* nghttp2_submit_request */ - CU_ASSERT(0 < nghttp2_submit_request(session, NULL, empty_name_nv, - ARRLEN(empty_name_nv), NULL, NULL)); - /* nghttp2_submit_response */ CU_ASSERT(0 == nghttp2_submit_response(session, 2, empty_name_nv, ARRLEN(empty_name_nv), NULL)); - /* nghttp2_submit_headers */ - CU_ASSERT(0 < nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, -1, NULL, - empty_name_nv, ARRLEN(empty_name_nv), - NULL)); - /* nghttp2_submit_push_promise */ open_recv_stream(session, 1); @@ -4871,6 +4917,19 @@ void test_nghttp2_submit_invalid_nv(void) { ARRLEN(empty_name_nv), NULL)); nghttp2_session_del(session); + + CU_ASSERT(0 == nghttp2_session_client_new(&session, &callbacks, NULL)); + + /* nghttp2_submit_request */ + CU_ASSERT(0 < nghttp2_submit_request(session, NULL, empty_name_nv, + ARRLEN(empty_name_nv), NULL, NULL)); + + /* nghttp2_submit_headers */ + CU_ASSERT(0 < nghttp2_submit_headers(session, NGHTTP2_FLAG_NONE, -1, NULL, + empty_name_nv, ARRLEN(empty_name_nv), + NULL)); + + nghttp2_session_del(session); } void test_nghttp2_session_open_stream(void) {