From 78d7160a9935cbff511704d7741dc3b68a534b40 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Fri, 28 Apr 2017 23:33:58 +0900 Subject: [PATCH] Treat incoming invalid regular header field as stream error Previously, the incoming invalid regular header field was ignored by default. With this commit, they are now treated as stream error, and the stream is reset by default. The error code used is now PROTOCOL_ERROR, instead of INTERNAL_ERROR. --- doc/programmers-guide.rst | 10 +++--- lib/includes/nghttp2/nghttp2.h | 16 +++++---- lib/nghttp2_session.c | 61 ++++++++++++++++++---------------- tests/nghttp2_session_test.c | 9 +++-- 4 files changed, 53 insertions(+), 43 deletions(-) diff --git a/doc/programmers-guide.rst b/doc/programmers-guide.rst index 93af2a0a..1e7d1e68 100644 --- a/doc/programmers-guide.rst +++ b/doc/programmers-guide.rst @@ -149,11 +149,11 @@ header fields must not appear: "Connection", "Keep-Alive", Each header field name and value must obey the field-name and field-value production rules described in `RFC 7230, section 3.2. `_. -Additionally, all field name must be lower cased. While the pseudo -header fields must satisfy these rules, we just ignore illegal regular -headers (this means that these header fields are not passed to -application callback). If application wants to treat these headers as -error, use `nghttp2_on_invalid_header_callback `_. +Additionally, all field name must be lower cased. The invalid header +fields are treated as stream error, and that stream is reset. If +application wants to treat these headers in their own way, use +`nghttp2_on_invalid_header_callback +`_. For "http" or "https" URIs, ":path" pseudo header fields must start with "/". The only exception is OPTIONS request, in that case, "*" is diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 15901004..ef1d610e 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/lib/includes/nghttp2/nghttp2.h @@ -1741,11 +1741,12 @@ typedef int (*nghttp2_on_header_callback2)(nghttp2_session *session, * The parameter and behaviour are similar to * :type:`nghttp2_on_header_callback`. The difference is that this * callback is only invoked when a invalid header name/value pair is - * received which is silently ignored if this callback is not set. - * Only invalid regular header field are passed to this callback. In - * other words, invalid pseudo header field is not passed to this - * callback. Also header fields which includes upper cased latter are - * also treated as error without passing them to this callback. + * received which is treated as stream error if this callback is not + * set. Only invalid regular header field are passed to this + * callback. In other words, invalid pseudo header field is not + * passed to this callback. Also header fields which includes upper + * cased latter are also treated as error without passing them to this + * callback. * * This callback is only considered if HTTP messaging validation is * turned on (which is on by default, see @@ -1754,10 +1755,13 @@ typedef int (*nghttp2_on_header_callback2)(nghttp2_session *session, * With this callback, application inspects the incoming invalid * field, and it also can reset stream from this callback by returning * :enum:`NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE`. By default, the - * error code is :enum:`NGHTTP2_INTERNAL_ERROR`. To change the error + * error code is :enum:`NGHTTP2_PROTOCOL_ERROR`. To change the error * code, call `nghttp2_submit_rst_stream()` with the error code of * choice in addition to returning * :enum:`NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE`. + * + * If 0 is returned, the header field is ignored, and the stream is + * not reset. */ typedef int (*nghttp2_on_invalid_header_callback)( nghttp2_session *session, const nghttp2_frame *frame, const uint8_t *name, diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index d4408ab3..4fbbe72d 100644 --- a/lib/nghttp2_session.c +++ b/lib/nghttp2_session.c @@ -3321,7 +3321,7 @@ static int session_call_on_invalid_header(nghttp2_session *session, session, frame, nv->name->base, nv->name->len, nv->value->base, nv->value->len, nv->flags, session->user_data); } else { - return 0; + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } if (rv == NGHTTP2_ERR_PAUSE || rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { @@ -3589,6 +3589,37 @@ 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 == NGHTTP2_ERR_IGN_HTTP_HEADER) { + /* Don't overwrite rv here */ + int rv2; + + rv2 = session_call_on_invalid_header(session, frame, &nv); + if (rv2 == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) { + rv = NGHTTP2_ERR_HTTP_HEADER; + } else { + if (rv2 != 0) { + return rv2; + } + + /* header is ignored */ + DEBUGF("recv: HTTP ignored: type=%u, id=%d, header %.*s: %.*s\n", + frame->hd.type, frame->hd.stream_id, (int)nv.name->len, + nv.name->base, (int)nv.value->len, nv.value->base); + + rv2 = session_call_error_callback( + session, + "Ignoring received invalid HTTP header field: frame type: " + "%u, stream: %d, name: [%.*s], value: [%.*s]", + frame->hd.type, frame->hd.stream_id, (int)nv.name->len, + nv.name->base, (int)nv.value->len, nv.value->base); + + if (nghttp2_is_fatal(rv2)) { + return rv2; + } + } + } + if (rv == NGHTTP2_ERR_HTTP_HEADER) { DEBUGF("recv: HTTP error: type=%u, id=%d, header %.*s: %.*s\n", frame->hd.type, frame->hd.stream_id, (int)nv.name->len, @@ -3612,34 +3643,6 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame, } return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } - - if (rv == NGHTTP2_ERR_IGN_HTTP_HEADER) { - /* Don't overwrite rv here */ - int rv2; - - rv2 = session_call_on_invalid_header(session, frame, &nv); - /* This handles NGHTTP2_ERR_PAUSE and - NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE as well */ - if (rv2 != 0) { - return rv2; - } - - /* header is ignored */ - DEBUGF("recv: HTTP ignored: type=%u, id=%d, header %.*s: %.*s\n", - frame->hd.type, frame->hd.stream_id, (int)nv.name->len, - nv.name->base, (int)nv.value->len, nv.value->base); - - rv2 = session_call_error_callback( - session, - "Ignoring received invalid HTTP header field: frame type: " - "%u, stream: %d, name: [%.*s], value: [%.*s]", - frame->hd.type, frame->hd.stream_id, (int)nv.name->len, - nv.name->base, (int)nv.value->len, nv.value->base); - - if (nghttp2_is_fatal(rv2)) { - return rv2; - } - } } if (rv == 0) { rv = session_call_on_header(session, frame, &nv); diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 28ca0ac6..c1bc84ad 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -11104,9 +11104,12 @@ void test_nghttp2_http_ignore_regular_header(void) { rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos + proclen, nghttp2_buf_len(&bufs.head->buf) - proclen); CU_ASSERT_FATAL(rv > 0); - /* header field "foo" must be ignored because it has illegal value. - So we have "bar" header field for 5th header. */ - CU_ASSERT(nghttp2_nv_equal(&bad_ansnv[4], &ud.nv)); + /* Without on_invalid_frame_recv_callback, bad header causes stream + reset */ + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); + proclen += (size_t)rv; CU_ASSERT(nghttp2_buf_len(&bufs.head->buf) == proclen);