From e082b7be72cddee2e964a81d4c8d756ba875a762 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 3 Nov 2016 17:00:05 +0900 Subject: [PATCH 1/4] nghttpx: Strict handling for Content-Length or Transfer-Encoding in h1 We now treat Content-Length or Transfer-Encoding as error if they come with 204 or 1xx status code, or 200 to a CONNECT request in HTTP/1 response. --- src/shrpx_http_downstream_connection.cc | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/shrpx_http_downstream_connection.cc b/src/shrpx_http_downstream_connection.cc index d42acd02..46d35514 100644 --- a/src/shrpx_http_downstream_connection.cc +++ b/src/shrpx_http_downstream_connection.cc @@ -701,6 +701,18 @@ int htp_hdrs_completecb(http_parser *htp) { downstream->set_downstream_addr_group(dconn->get_downstream_addr_group()); downstream->set_addr(dconn->get_addr()); + // Server MUST NOT send Transfer-Encoding with a status code 1xx or + // 204. Also server MUST NOT send Transfer-Encoding with a status + // code 200 to a CONNECT request. Same holds true with + // Content-Length. + if (resp.http_status == 204 || resp.http_status / 100 == 1 || + (resp.http_status == 200 && req.method == HTTP_CONNECT)) { + if (resp.fs.header(http2::HD_CONTENT_LENGTH) || + resp.fs.header(http2::HD_TRANSFER_ENCODING)) { + return -1; + } + } + if (resp.fs.parse_content_length() != 0) { downstream->set_response_state(Downstream::MSG_BAD_HEADER); return -1; From 6ad9ddcdeac06c927f90bb65ea2645f205577d6a Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 3 Nov 2016 17:26:32 +0900 Subject: [PATCH 2/4] Disallow content-length in 1xx, 204, or 200 to a CONNECT request --- lib/nghttp2_http.c | 5 +++++ tests/nghttp2_session_test.c | 23 +++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/lib/nghttp2_http.c b/lib/nghttp2_http.c index e3259514..23db1cbc 100644 --- a/lib/nghttp2_http.c +++ b/lib/nghttp2_http.c @@ -250,6 +250,11 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_hd_nv *nv, break; } case NGHTTP2_TOKEN_CONTENT_LENGTH: { + if (stream->status_code == 204 || stream->status_code / 100 == 1 || + (stream->status_code == 200 && + (stream->http_flags & NGHTTP2_HTTP_FLAG_METH_CONNECT))) { + return NGHTTP2_ERR_HTTP_HEADER; + } if (stream->content_length != -1) { return NGHTTP2_ERR_HTTP_HEADER; } diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index c11ff922..064adbf9 100644 --- a/tests/nghttp2_session_test.c +++ b/tests/nghttp2_session_test.c @@ -10200,6 +10200,10 @@ void test_nghttp2_http_mandatory_headers(void) { MAKE_NV("content-length", "0")}; const nghttp2_nv badhd_resnv[] = {MAKE_NV(":status", "200"), MAKE_NV("connection", "close")}; + const nghttp2_nv cl1xx_resnv[] = {MAKE_NV(":status", "100"), + MAKE_NV("content-length", "0")}; + const nghttp2_nv cl204_resnv[] = {MAKE_NV(":status", "204"), + MAKE_NV("content-length", "0")}; /* test case for request */ const nghttp2_nv nopath_reqnv[] = {MAKE_NV(":scheme", "https"), @@ -10298,6 +10302,16 @@ void test_nghttp2_http_mandatory_headers(void) { NGHTTP2_STREAM_OPENING, badhd_resnv, ARRLEN(badhd_resnv)); + /* response header has content-length with 100 status code */ + check_nghttp2_http_recv_headers_fail(session, &deflater, 17, + NGHTTP2_STREAM_OPENING, cl1xx_resnv, + ARRLEN(cl1xx_resnv)); + + /* response header has content-length with 204 status code */ + check_nghttp2_http_recv_headers_fail(session, &deflater, 19, + NGHTTP2_STREAM_OPENING, cl204_resnv, + ARRLEN(cl204_resnv)); + nghttp2_hd_deflate_free(&deflater); nghttp2_session_del(session); @@ -11001,6 +11015,7 @@ void test_nghttp2_http_record_request_method(void) { nghttp2_bufs bufs; nghttp2_hd_deflater deflater; nghttp2_mem *mem; + nghttp2_outbound_item *item; mem = nghttp2_mem_default(); frame_pack_bufs_init(&bufs); @@ -11033,6 +11048,14 @@ void test_nghttp2_http_record_request_method(void) { CU_ASSERT((NGHTTP2_HTTP_FLAG_METH_CONNECT & stream->http_flags) > 0); CU_ASSERT(-1 == stream->content_length); + /* content-length is now allowed in 200 response to a CONNECT + request */ + item = nghttp2_session_get_next_ob_item(session); + + CU_ASSERT(NULL != item); + CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type); + CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == item->frame.rst_stream.error_code); + nghttp2_hd_deflate_free(&deflater); nghttp2_session_del(session); nghttp2_bufs_free(&bufs); From 6eb2829ee8b5cdb1b43d20fd46cfa197c0ecc325 Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 3 Nov 2016 22:25:15 +0900 Subject: [PATCH 3/4] nghttpx: Strip content-length with 204 or 200 to CONNECT in mruby --- src/shrpx_mruby_module_response.cc | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/src/shrpx_mruby_module_response.cc b/src/shrpx_mruby_module_response.cc index da3dd45b..e0f96427 100644 --- a/src/shrpx_mruby_module_response.cc +++ b/src/shrpx_mruby_module_response.cc @@ -190,6 +190,7 @@ namespace { mrb_value response_return(mrb_state *mrb, mrb_value self) { auto data = static_cast(mrb->ud); auto downstream = data->downstream; + auto &req = downstream->request(); auto &resp = downstream->response(); int rv; @@ -215,16 +216,28 @@ mrb_value response_return(mrb_state *mrb, mrb_value self) { bodylen = vallen; } - auto content_length = util::make_string_ref_uint(balloc, bodylen); - auto cl = resp.fs.header(http2::HD_CONTENT_LENGTH); - if (cl) { - cl->value = content_length; + + if (resp.http_status == 204 || + (resp.http_status == 200 && req.method == HTTP_CONNECT)) { + if (cl) { + // Delete content-length here + cl->name = StringRef{}; + } + + resp.fs.content_length = -1; } else { - resp.fs.add_header_token(StringRef::from_lit("content-length"), - content_length, false, http2::HD_CONTENT_LENGTH); + auto content_length = util::make_string_ref_uint(balloc, vallen); + + if (cl) { + cl->value = content_length; + } else { + resp.fs.add_header_token(StringRef::from_lit("content-length"), + content_length, false, http2::HD_CONTENT_LENGTH); + } + + resp.fs.content_length = vallen; } - resp.fs.content_length = bodylen; auto date = resp.fs.header(http2::HD_DATE); if (!date) { From c171097deaff3fbbd41ce511ba9726c8df0e92eb Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Thu, 3 Nov 2016 23:09:30 +0900 Subject: [PATCH 4/4] Document that libnghttp2's behaviour about Content-Length --- doc/programmers-guide.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/doc/programmers-guide.rst b/doc/programmers-guide.rst index 2e763a40..e03b7a14 100644 --- a/doc/programmers-guide.rst +++ b/doc/programmers-guide.rst @@ -173,6 +173,11 @@ parsed as 64 bit signed integer. The sum of data length in the following DATA frames must match with the number in "Content-Length" header field if it is present (this does not include padding bytes). +RFC 7230 says that server must not send "Content-Length" in any +response with 1xx, and 204 status code. It also says that +"Content-Length" is not allowed in any response with 200 status code +to a CONNECT request. nghttp2 enforces them as well. + Any deviation results in stream error of type PROTOCOL_ERROR. If error is found in PUSH_PROMISE frame, stream error is raised against promised stream.