From 05b8901d69790e7cc5db2fb1a4be634b36efa186 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 26 Feb 2015 22:59:07 +0900 Subject: [PATCH] Call on_invalid_frame_recv_callback on bad HTTP messaging --- lib/includes/nghttp2/nghttp2.h | 5 +++ lib/nghttp2_http.c | 61 +++++++++++++++++----------------- lib/nghttp2_http.h | 14 +++++--- lib/nghttp2_int.h | 7 +++- lib/nghttp2_session.c | 32 ++++++++++++------ tests/nghttp2_session_test.c | 12 +++++-- 6 files changed, 84 insertions(+), 47 deletions(-) diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 57345ad5..91380ca5 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -348,6 +348,11 @@ typedef enum { * `nghttp2_session_terminate_session()` is called. */ NGHTTP2_ERR_SESSION_CLOSING = -530, + /** + * Invalid HTTP header field was received and stream is going to be + * closed. + */ + NGHTTP2_ERR_HTTP_HEADER = -531, /** * The errors < :enum:`NGHTTP2_ERR_FATAL` mean that the library is * under unexpected condition and processing was terminated (e.g., diff --git a/lib/nghttp2_http.c b/lib/nghttp2_http.c index 1f4cfa8b..70ccbd3d 100644 --- a/lib/nghttp2_http.c +++ b/lib/nghttp2_http.c @@ -232,7 +232,7 @@ static int http_request_on_header(nghttp2_stream *stream, nghttp2_nv *nv, if (nv->name[0] == ':') { if (trailer || (stream->http_flags & NGHTTP2_HTTP_FLAG_PSEUDO_HEADER_DISALLOWED)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } } @@ -241,55 +241,55 @@ static int http_request_on_header(nghttp2_stream *stream, nghttp2_nv *nv, switch (token) { case NGHTTP2_TOKEN__AUTHORITY: if (!check_pseudo_header(stream, nv, NGHTTP2_HTTP_FLAG__AUTHORITY)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; case NGHTTP2_TOKEN__METHOD: if (!check_pseudo_header(stream, nv, NGHTTP2_HTTP_FLAG__METHOD)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } if (streq("HEAD", nv->value, nv->valuelen)) { stream->http_flags |= NGHTTP2_HTTP_FLAG_METH_HEAD; } else if (streq("CONNECT", nv->value, nv->valuelen)) { if (stream->stream_id % 2 == 0) { /* we won't allow CONNECT for push */ - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } stream->http_flags |= NGHTTP2_HTTP_FLAG_METH_CONNECT; if (stream->http_flags & (NGHTTP2_HTTP_FLAG__PATH | NGHTTP2_HTTP_FLAG__SCHEME)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } } break; case NGHTTP2_TOKEN__PATH: if (stream->http_flags & NGHTTP2_HTTP_FLAG_METH_CONNECT) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } if (!check_pseudo_header(stream, nv, NGHTTP2_HTTP_FLAG__PATH)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; case NGHTTP2_TOKEN__SCHEME: if (stream->http_flags & NGHTTP2_HTTP_FLAG_METH_CONNECT) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } if (!check_pseudo_header(stream, nv, NGHTTP2_HTTP_FLAG__SCHEME)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; case NGHTTP2_TOKEN_HOST: if (!check_pseudo_header(stream, nv, NGHTTP2_HTTP_FLAG_HOST)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; case NGHTTP2_TOKEN_CONTENT_LENGTH: { if (stream->content_length != -1) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } stream->content_length = parse_uint(nv->value, nv->valuelen); if (stream->content_length == -1) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; } @@ -299,15 +299,15 @@ static int http_request_on_header(nghttp2_stream *stream, nghttp2_nv *nv, case NGHTTP2_TOKEN_PROXY_CONNECTION: case NGHTTP2_TOKEN_TRANSFER_ENCODING: case NGHTTP2_TOKEN_UPGRADE: - return -1; + return NGHTTP2_ERR_HTTP_HEADER; case NGHTTP2_TOKEN_TE: if (!strieq("trailers", nv->value, nv->valuelen)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; default: if (nv->name[0] == ':') { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } } @@ -325,7 +325,7 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_nv *nv, if (nv->name[0] == ':') { if (trailer || (stream->http_flags & NGHTTP2_HTTP_FLAG_PSEUDO_HEADER_DISALLOWED)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } } @@ -334,24 +334,24 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_nv *nv, switch (token) { case NGHTTP2_TOKEN__STATUS: { if (!check_pseudo_header(stream, nv, NGHTTP2_HTTP_FLAG__STATUS)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } if (nv->valuelen != 3) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } stream->status_code = parse_uint(nv->value, nv->valuelen); if (stream->status_code == -1) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; } case NGHTTP2_TOKEN_CONTENT_LENGTH: { if (stream->content_length != -1) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } stream->content_length = parse_uint(nv->value, nv->valuelen); if (stream->content_length == -1) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; } @@ -361,15 +361,15 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_nv *nv, case NGHTTP2_TOKEN_PROXY_CONNECTION: case NGHTTP2_TOKEN_TRANSFER_ENCODING: case NGHTTP2_TOKEN_UPGRADE: - return -1; + return NGHTTP2_ERR_HTTP_HEADER; case NGHTTP2_TOKEN_TE: if (!strieq("trailers", nv->value, nv->valuelen)) { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } break; default: if (nv->name[0] == ':') { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } } @@ -392,31 +392,32 @@ int nghttp2_http_on_header(nghttp2_session *session, nghttp2_stream *stream, if (!nghttp2_check_header_name(nv->name, nv->namelen)) { size_t i; if (nv->namelen > 0 && nv->name[0] == ':') { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } /* header field name must be lower-cased without exception */ for (i = 0; i < nv->namelen; ++i) { char c = nv->name[i]; if ('A' <= c && c <= 'Z') { - return -1; + return NGHTTP2_ERR_HTTP_HEADER; } } /* When ignoring regular headers, we set this flag so that we still enforce header field ordering rule for pseudo header fields. */ stream->http_flags |= NGHTTP2_HTTP_FLAG_PSEUDO_HEADER_DISALLOWED; - return 1; + return NGHTTP2_ERR_IGN_HTTP_HEADER; } if (!nghttp2_check_header_value(nv->value, nv->valuelen)) { - if (nv->namelen > 0 && nv->name[0] == ':') { - return -1; + assert(nv->namelen > 0); + if (nv->name[0] == ':') { + return NGHTTP2_ERR_HTTP_HEADER; } /* When ignoring regular headers, we set this flag so that we still enforce header field ordering rule for pseudo header fields. */ stream->http_flags |= NGHTTP2_HTTP_FLAG_PSEUDO_HEADER_DISALLOWED; - return 1; + return NGHTTP2_ERR_IGN_HTTP_HEADER; } if (session->server || frame->hd.type == NGHTTP2_PUSH_PROMISE) { diff --git a/lib/nghttp2_http.h b/lib/nghttp2_http.h index 6fa74eca..f7966c67 100644 --- a/lib/nghttp2_http.h +++ b/lib/nghttp2_http.h @@ -36,10 +36,16 @@ /* * This function is called when HTTP header field |nv| in |frame| is * received for |stream|. This function will validate |nv| against - * the current state of stream. This function returns 0 if it - * succeeds, or 1 if the header field should be ignored, or -1 if the - * header field contains totally unforgivable piece of junk and stream - * should be killed. + * the current state of stream. + * + * This function returns 0 if it succeeds, or one of the following + * negative error codes: + * + * NGHTTP2_ERR_HTTP_HEADER + * Invalid HTTP header field was received. + * NGHTTP2_ERR_IGN_HTTP_HEADER + * Invalid HTTP header field was received but it can be treated as + * if it was not received because of compatibility reasons. */ int nghttp2_http_on_header(nghttp2_session *session, nghttp2_stream *stream, nghttp2_frame *frame, nghttp2_nv *nv, int trailer); diff --git a/lib/nghttp2_int.h b/lib/nghttp2_int.h index effd667c..3d77eaab 100644 --- a/lib/nghttp2_int.h +++ b/lib/nghttp2_int.h @@ -46,7 +46,12 @@ typedef int (*nghttp2_compar)(const void *lhs, const void *rhs); typedef enum { NGHTTP2_ERR_CREDENTIAL_PENDING = -101, NGHTTP2_ERR_IGN_HEADER_BLOCK = -103, - NGHTTP2_ERR_IGN_PAYLOAD = -104 + NGHTTP2_ERR_IGN_PAYLOAD = -104, + /* + * Invalid HTTP header field was received but it can be treated as + * if it was not received because of compatibility reasons. + */ + NGHTTP2_ERR_IGN_HTTP_HEADER = -105, } nghttp2_internal_error; #endif /* NGHTTP2_INT_H */ diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index f0f2b155..906b8580 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3023,11 +3023,12 @@ static int session_handle_frame_size_error(nghttp2_session *session, return nghttp2_session_terminate_session(session, NGHTTP2_FRAME_SIZE_ERROR); } -static int session_handle_invalid_stream(nghttp2_session *session, - nghttp2_frame *frame, - uint32_t error_code) { +static int session_handle_invalid_stream2(nghttp2_session *session, + int32_t stream_id, + nghttp2_frame *frame, + uint32_t error_code) { int rv; - rv = nghttp2_session_add_rst_stream(session, frame->hd.stream_id, error_code); + rv = nghttp2_session_add_rst_stream(session, stream_id, error_code); if (rv != 0) { return rv; } @@ -3040,6 +3041,13 @@ static int session_handle_invalid_stream(nghttp2_session *session, return 0; } +static int session_handle_invalid_stream(nghttp2_session *session, + nghttp2_frame *frame, + uint32_t error_code) { + return session_handle_invalid_stream2(session, frame->hd.stream_id, frame, + error_code); +} + static int session_inflate_handle_invalid_stream(nghttp2_session *session, nghttp2_frame *frame, uint32_t error_code) { @@ -3170,18 +3178,22 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame, if (subject_stream && session_enforce_http_messaging(session)) { rv = nghttp2_http_on_header(session, subject_stream, frame, &nv, trailer); - if (rv == -1) { + if (rv == NGHTTP2_ERR_HTTP_HEADER) { DEBUGF(fprintf( stderr, "recv: HTTP error: type=%d, id=%d, header %.*s: %.*s\n", frame->hd.type, subject_stream->stream_id, (int)nv.namelen, nv.name, (int)nv.valuelen, nv.value)); - rv = nghttp2_session_add_rst_stream( - session, subject_stream->stream_id, NGHTTP2_PROTOCOL_ERROR); + + rv = + session_handle_invalid_stream2(session, subject_stream->stream_id, + frame, NGHTTP2_PROTOCOL_ERROR); if (nghttp2_is_fatal(rv)) { return rv; } return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; - } else if (rv == 1) { + } + + if (rv == NGHTTP2_ERR_IGN_HTTP_HEADER) { /* header is ignored */ DEBUGF(fprintf( stderr, "recv: HTTP ignored: type=%d, id=%d, header %.*s: %.*s\n", @@ -3189,7 +3201,7 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame, nv.name, (int)nv.valuelen, nv.value)); } } - if (rv == 0 && call_header_cb) { + if (rv == 0) { rv = session_call_on_header(session, frame, &nv); /* This handles NGHTTP2_ERR_PAUSE and NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE as well */ @@ -3348,7 +3360,7 @@ static int session_after_header_block_received(nghttp2_session *session) { call_cb = 0; - rv = nghttp2_session_add_rst_stream(session, stream_id, + rv = session_handle_invalid_stream2(session, stream_id, frame, NGHTTP2_PROTOCOL_ERROR); if (nghttp2_is_fatal(rv)) { return rv; diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 1ca79b86..77178a23 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -6820,10 +6820,13 @@ static void check_nghttp2_http_recv_headers_fail( ssize_t rv; nghttp2_outbound_item *item; nghttp2_bufs bufs; + my_user_data *ud; mem = nghttp2_mem_default(); frame_pack_bufs_init(&bufs); + ud = session->user_data; + if (stream_state != -1) { nghttp2_session_open_stream(session, stream_id, NGHTTP2_STREAM_FLAG_NONE, &pri_spec_default, stream_state, NULL); @@ -6833,6 +6836,8 @@ static void check_nghttp2_http_recv_headers_fail( nvlen, mem); CU_ASSERT(0 == rv); + ud->invalid_frame_recv_cb_called = 0; + rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos, nghttp2_buf_len(&bufs.head->buf)); @@ -6841,6 +6846,7 @@ static void check_nghttp2_http_recv_headers_fail( item = nghttp2_session_get_next_ob_item(session); CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); + CU_ASSERT(1 == ud->invalid_frame_recv_cb_called); CU_ASSERT(0 == nghttp2_session_send(session)); @@ -6852,6 +6858,7 @@ void test_nghttp2_http_mandatory_headers(void) { nghttp2_session_callbacks callbacks; nghttp2_hd_deflater deflater; nghttp2_mem *mem; + my_user_data ud; /* test case for response */ const nghttp2_nv nostatus_resnv[] = {MAKE_NV("server", "foo")}; const nghttp2_nv dupstatus_resnv[] = {MAKE_NV(":status", "200"), @@ -6907,8 +6914,9 @@ void test_nghttp2_http_mandatory_headers(void) { memset(&callbacks, 0, sizeof(nghttp2_session_callbacks)); callbacks.send_callback = null_send_callback; + callbacks.on_invalid_frame_recv_callback = on_invalid_frame_recv_callback; - nghttp2_session_client_new(&session, &callbacks, NULL); + nghttp2_session_client_new(&session, &callbacks, &ud); nghttp2_hd_deflate_init(&deflater, mem); @@ -6957,7 +6965,7 @@ void test_nghttp2_http_mandatory_headers(void) { nghttp2_session_del(session); /* check server side */ - nghttp2_session_server_new(&session, &callbacks, NULL); + nghttp2_session_server_new(&session, &callbacks, &ud); nghttp2_hd_deflate_init(&deflater, mem);