From a440bdf15eb544e03399ac3db9631f1133b4ed8b Mon Sep 17 00:00:00 2001 From: Tatsuhiro Tsujikawa Date: Mon, 19 Jan 2015 23:44:23 +0900 Subject: [PATCH] nghttpx: Response 502 if HTTP/2 backend receives invalid Content-Length --- src/shrpx_downstream.cc | 4 ++ src/shrpx_downstream.h | 6 ++- src/shrpx_http2_downstream_connection.cc | 1 + src/shrpx_http2_session.cc | 63 +++++++++++++++--------- src/shrpx_http2_upstream.cc | 7 +++ src/shrpx_https_upstream.cc | 9 ++++ src/shrpx_spdy_upstream.cc | 7 +++ 7 files changed, 73 insertions(+), 24 deletions(-) diff --git a/src/shrpx_downstream.cc b/src/shrpx_downstream.cc index e3adb3c9..9103e6b6 100644 --- a/src/shrpx_downstream.cc +++ b/src/shrpx_downstream.cc @@ -664,6 +664,10 @@ int64_t Downstream::get_response_sent_bodylen() const { return response_sent_bodylen_; } +int64_t Downstream::get_response_content_length() const { + return response_content_length_; +} + void Downstream::set_response_content_length(int64_t len) { response_content_length_ = len; } diff --git a/src/shrpx_downstream.h b/src/shrpx_downstream.h index ecc8c462..f8bfcb5c 100644 --- a/src/shrpx_downstream.h +++ b/src/shrpx_downstream.h @@ -172,7 +172,10 @@ public: STREAM_CLOSED, CONNECT_FAIL, IDLE, - MSG_RESET + MSG_RESET, + // header contains invalid header field. We can safely send error + // response (502) to a client. + MSG_BAD_HEADER, }; void set_request_state(int state); int get_request_state() const; @@ -223,6 +226,7 @@ public: int64_t get_response_bodylen() const; void add_response_sent_bodylen(size_t amount); int64_t get_response_sent_bodylen() const; + int64_t get_response_content_length() const; void set_response_content_length(int64_t len); // Validates that received response body length and content-length // matches. diff --git a/src/shrpx_http2_downstream_connection.cc b/src/shrpx_http2_downstream_connection.cc index 09080293..72f8645c 100644 --- a/src/shrpx_http2_downstream_connection.cc +++ b/src/shrpx_http2_downstream_connection.cc @@ -139,6 +139,7 @@ int Http2DownstreamConnection::submit_rst_stream(Downstream *downstream, downstream->get_downstream_stream_id() != -1) { switch (downstream->get_response_state()) { case Downstream::MSG_RESET: + case Downstream::MSG_BAD_HEADER: case Downstream::MSG_COMPLETE: break; default: diff --git a/src/shrpx_http2_session.cc b/src/shrpx_http2_session.cc index 4adf09f5..a373c219 100644 --- a/src/shrpx_http2_session.cc +++ b/src/shrpx_http2_session.cc @@ -654,10 +654,15 @@ int on_stream_close_callback(nghttp2_session *session, int32_t stream_id, downstream->get_upstream()->on_downstream_body_complete(downstream); downstream->set_response_state(Downstream::MSG_COMPLETE); } else if (error_code == NGHTTP2_NO_ERROR) { - if (downstream->get_response_state() != Downstream::MSG_COMPLETE) { + switch (downstream->get_response_state()) { + case Downstream::MSG_COMPLETE: + case Downstream::MSG_BAD_HEADER: + break; + default: downstream->set_response_state(Downstream::MSG_RESET); } - } else { + } else if (downstream->get_response_state() != + Downstream::MSG_BAD_HEADER) { downstream->set_response_state(Downstream::MSG_RESET); } call_downstream_readcb(http2session, downstream); @@ -727,6 +732,24 @@ int on_header_callback(nghttp2_session *session, const nghttp2_frame *frame, return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; } + if (token == http2::HD_CONTENT_LENGTH) { + auto len = util::parse_uint(value, valuelen); + if (len == -1) { + http2session->submit_rst_stream(frame->hd.stream_id, + NGHTTP2_PROTOCOL_ERROR); + downstream->set_response_state(Downstream::MSG_BAD_HEADER); + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; + } + auto cl = downstream->get_response_content_length(); + if (cl != -1 && cl != len) { + http2session->submit_rst_stream(frame->hd.stream_id, + NGHTTP2_PROTOCOL_ERROR); + downstream->set_response_state(Downstream::MSG_BAD_HEADER); + return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE; + } + downstream->set_response_content_length(len); + } + downstream->add_response_header(name, namelen, value, valuelen, flags & NGHTTP2_NV_FLAG_NO_INDEX, token); return 0; @@ -817,27 +840,21 @@ int on_response_headers(Http2Session *http2session, Downstream *downstream, return 0; } - auto content_length_hd = - downstream->get_response_header(http2::HD_CONTENT_LENGTH); - - if (content_length_hd) { - auto content_length = util::parse_uint(content_length_hd->value); - if (content_length != -1) { - downstream->set_response_content_length(content_length); - } else if (downstream->expect_response_body()) { - // Here we have response body but Content-Length is not known in - // advance. - if (downstream->get_request_major() <= 0 || - downstream->get_request_minor() <= 0) { - // We simply close connection for pre-HTTP/1.1 in this case. - downstream->set_response_connection_close(true); - } else if (downstream->get_request_method() != "CONNECT") { - // Otherwise, use chunked encoding to keep upstream connection - // open. In HTTP2, we are supporsed not to receive - // transfer-encoding. - downstream->add_response_header("transfer-encoding", "chunked"); - downstream->set_chunked_response(true); - } + if (downstream->get_response_content_length() == -1 && + downstream->expect_response_body()) { + // Here we have response body but Content-Length is not known in + // advance. + if (downstream->get_request_major() <= 0 || + (downstream->get_request_major() <= 1 && + downstream->get_request_minor() <= 0)) { + // We simply close connection for pre-HTTP/1.1 in this case. + downstream->set_response_connection_close(true); + } else if (downstream->get_request_method() != "CONNECT") { + // Otherwise, use chunked encoding to keep upstream connection + // open. In HTTP2, we are supporsed not to receive + // transfer-encoding. + downstream->add_response_header("transfer-encoding", "chunked"); + downstream->set_chunked_response(true); } } diff --git a/src/shrpx_http2_upstream.cc b/src/shrpx_http2_upstream.cc index c694224c..29d962f4 100644 --- a/src/shrpx_http2_upstream.cc +++ b/src/shrpx_http2_upstream.cc @@ -823,6 +823,13 @@ int Http2Upstream::downstream_read(DownstreamConnection *dconn) { downstream->pop_downstream_connection(); // dconn was deleted dconn = nullptr; + } else if (downstream->get_response_state() == Downstream::MSG_BAD_HEADER) { + if (error_reply(downstream, 502) != 0) { + return -1; + } + downstream->pop_downstream_connection(); + // dconn was deleted + dconn = nullptr; } else { auto rv = downstream->on_read(); if (rv == DownstreamConnection::ERR_EOF) { diff --git a/src/shrpx_https_upstream.cc b/src/shrpx_https_upstream.cc index f1489379..7f6cdbd6 100644 --- a/src/shrpx_https_upstream.cc +++ b/src/shrpx_https_upstream.cc @@ -460,6 +460,14 @@ int HttpsUpstream::downstream_read(DownstreamConnection *dconn) { return -1; } + if (downstream->get_response_state() == Downstream::MSG_BAD_HEADER) { + if (error_reply(502) != 0) { + return -1; + } + downstream->pop_downstream_connection(); + goto end; + } + if (rv == DownstreamConnection::ERR_EOF) { return downstream_eof(dconn); } @@ -472,6 +480,7 @@ int HttpsUpstream::downstream_read(DownstreamConnection *dconn) { return -1; } +end: handler_->signal_write(); return 0; diff --git a/src/shrpx_spdy_upstream.cc b/src/shrpx_spdy_upstream.cc index 1a3aa328..5ccc1336 100644 --- a/src/shrpx_spdy_upstream.cc +++ b/src/shrpx_spdy_upstream.cc @@ -531,6 +531,13 @@ int SpdyUpstream::downstream_read(DownstreamConnection *dconn) { downstream->get_response_rst_stream_error_code())); downstream->pop_downstream_connection(); dconn = nullptr; + } else if (downstream->get_response_state() == Downstream::MSG_BAD_HEADER) { + if (error_reply(downstream, 502) != 0) { + return -1; + } + downstream->pop_downstream_connection(); + // dconn was deleted + dconn = nullptr; } else { auto rv = downstream->on_read(); if (rv == DownstreamConnection::ERR_EOF) {