Perform content-length validation in OgHttp2Session. This CL validates the content-length header value in HeaderValidator and allows the value to be checked in OgHttp2Session when comparing against actual received body lengths for a stream. If there is a mismatch between the content-length header value and the total body length received, OgHttp2Session invokes the visitor OnInvalidFrame() callback and enqueues a RST_STREAM. This change better aligns oghttp2 with nghttp2 on content-length validation, but at least one remaining difference is that nghttp2 invokes OnInvalidFrame() (leading to connection-level error-signalling in Envoy) only when the content-length mismatch happens with HEADERS ending the stream [1]. The OnInvalidFrame() --> connection error is important at least for a content-length integration test. Therefore, oghttp2 also calls OnInvalidFrame() for content-length mismatch, but regardless of whether HEADERS or DATA ends the stream for unity. (Ideally, content-length mismatch on one stream should not cause a connection-level error; perhaps this can be fixed once migration to oghttp2 is complete.) With this CL, MultiplexedIntegrationTest.InconsistentContentLength passes with oghttp2 (http://sponge2/663d811a-e662-44c0-b003-a4d7fcb13d42). [1] Compare HEADERS at http://google3/third_party/nghttp2/src/lib/nghttp2_session.c;l=3778,3790-3791;rcl=421446478 to DATA at http://google3/third_party/nghttp2/src/lib/nghttp2_session.c;l=4961-4963;rcl=421446478. PiperOrigin-RevId: 422597021
diff --git a/http2/adapter/header_validator.cc b/http2/adapter/header_validator.cc index 0c1d873..57afa54 100644 --- a/http2/adapter/header_validator.cc +++ b/http2/adapter/header_validator.cc
@@ -1,6 +1,7 @@ #include "http2/adapter/header_validator.h" #include "absl/strings/escaping.h" +#include "absl/strings/numbers.h" #include "common/platform/api/quiche_logging.h" namespace http2 { @@ -54,6 +55,7 @@ pseudo_headers_.clear(); status_.clear(); method_.clear(); + content_length_.reset(); } HeaderValidator::HeaderStatus HeaderValidator::ValidateSingleHeader( @@ -98,9 +100,11 @@ method_ = std::string(value); } pseudo_headers_.push_back(std::string(key)); - } else if (key == "content-length" && status_ == "204" && value != "0") { - // There should be no body in a "204 No Content" response. - return HEADER_FIELD_INVALID; + } else if (key == "content-length") { + const bool success = HandleContentLength(value); + if (!success) { + return HEADER_FIELD_INVALID; + } } return HEADER_OK; } @@ -123,5 +127,25 @@ return false; } +bool HeaderValidator::HandleContentLength(absl::string_view value) { + if (value.empty()) { + return false; + } + + if (status_ == "204" && value != "0") { + // There should be no body in a "204 No Content" response. + return false; + } + + size_t content_length = 0; + const bool valid = absl::SimpleAtoi(value, &content_length); + if (!valid) { + return false; + } + + content_length_ = content_length; + return true; +} + } // namespace adapter } // namespace http2
diff --git a/http2/adapter/header_validator.h b/http2/adapter/header_validator.h index b21b299..ddcabdf 100644 --- a/http2/adapter/header_validator.h +++ b/http2/adapter/header_validator.h
@@ -46,11 +46,16 @@ // For responses, returns the value of the ":status" header, if present. absl::string_view status_header() const { return status_; } + absl::optional<size_t> content_length() const { return content_length_; } + private: + bool HandleContentLength(absl::string_view value); + std::vector<std::string> pseudo_headers_; std::string status_; std::string method_; absl::optional<size_t> max_field_size_; + absl::optional<size_t> content_length_; bool allow_connect_ = false; };
diff --git a/http2/adapter/header_validator_test.cc b/http2/adapter/header_validator_test.cc index 5fe05a8..0d4d7fc 100644 --- a/http2/adapter/header_validator_test.cc +++ b/http2/adapter/header_validator_test.cc
@@ -1,12 +1,15 @@ #include "http2/adapter/header_validator.h" #include "absl/strings/str_cat.h" +#include "absl/types/optional.h" #include "common/platform/api/quiche_test.h" namespace http2 { namespace adapter { namespace test { +using ::testing::Optional; + TEST(HeaderValidatorTest, HeaderNameEmpty) { HeaderValidator v; HeaderValidator::HeaderStatus status = v.ValidateSingleHeader("", "value"); @@ -286,6 +289,42 @@ EXPECT_FALSE(v.FinishHeaderBlock(HeaderType::RESPONSE_TRAILER)); } +TEST(HeaderValidatorTest, ValidContentLength) { + HeaderValidator v; + + v.StartHeaderBlock(); + EXPECT_EQ(v.content_length(), absl::nullopt); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader("content-length", "41")); + EXPECT_THAT(v.content_length(), Optional(41)); + + v.StartHeaderBlock(); + EXPECT_EQ(v.content_length(), absl::nullopt); + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader("content-length", "42")); + EXPECT_THAT(v.content_length(), Optional(42)); +} + +TEST(HeaderValidatorTest, InvalidContentLength) { + HeaderValidator v; + + v.StartHeaderBlock(); + EXPECT_EQ(v.content_length(), absl::nullopt); + EXPECT_EQ(HeaderValidator::HEADER_FIELD_INVALID, + v.ValidateSingleHeader("content-length", "")); + EXPECT_EQ(v.content_length(), absl::nullopt); + EXPECT_EQ(HeaderValidator::HEADER_FIELD_INVALID, + v.ValidateSingleHeader("content-length", "nan")); + EXPECT_EQ(v.content_length(), absl::nullopt); + EXPECT_EQ(HeaderValidator::HEADER_FIELD_INVALID, + v.ValidateSingleHeader("content-length", "-42")); + EXPECT_EQ(v.content_length(), absl::nullopt); + // End on a positive note. + EXPECT_EQ(HeaderValidator::HEADER_OK, + v.ValidateSingleHeader("content-length", "42")); + EXPECT_THAT(v.content_length(), Optional(42)); +} + } // namespace test } // namespace adapter } // namespace http2
diff --git a/http2/adapter/http2_util.cc b/http2/adapter/http2_util.cc index 553c0bd..f93b161 100644 --- a/http2/adapter/http2_util.cc +++ b/http2/adapter/http2_util.cc
@@ -97,6 +97,8 @@ return "InvalidPushPromise"; case ConnectionError::kExceededMaxConcurrentStreams: return "ExceededMaxConcurrentStreams"; + case ConnectionError::kHttpMessaging: + return "HttpMessaging"; } return "UnknownConnectionError"; }
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h index 0e8619f..61c76b7 100644 --- a/http2/adapter/http2_visitor_interface.h +++ b/http2/adapter/http2_visitor_interface.h
@@ -78,6 +78,9 @@ kInvalidPushPromise, // The peer exceeded the max concurrent streams limit. kExceededMaxConcurrentStreams, + // The peer violated HTTP messaging semantics. + // TODO(b/214614028): Remove once these errors can be just RST_STREAM'd. + kHttpMessaging, }; virtual void OnConnectionError(ConnectionError error) = 0;
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index 51679f4..9b2809b 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -4446,12 +4446,12 @@ EXPECT_CALL(visitor, OnEndStream(1)); // Stream 3: content-length is not a number - // TODO(diannahu): Let oghttp2 validate content-length. EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 5)); EXPECT_CALL(visitor, OnBeginHeadersForStream(3)); - EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(5); - EXPECT_CALL(visitor, OnEndHeadersForStream(3)); - EXPECT_CALL(visitor, OnEndStream(3)); + EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(4); + EXPECT_CALL( + visitor, + OnInvalidFrame(3, Http2VisitorInterface::InvalidFrameError::kHttpHeader)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); @@ -4460,12 +4460,18 @@ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 3, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::HTTP2_NO_ERROR)); EXPECT_TRUE(adapter->want_write()); int result = adapter->Send(); EXPECT_EQ(0, result); EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, - spdy::SpdyFrameType::SETTINGS})); + spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::RST_STREAM})); } TEST(OgHttp2AdapterServerTest, ServerHandlesContentLengthMismatch) { @@ -4505,7 +4511,8 @@ EXPECT_CALL(visitor, OnSettingsEnd()); // Stream 1: content-length is larger than actual data - // TODO(diannahu): Let oghttp2 validate content-length. + // All data and then OnInvalidFrame() are delivered to the visitor. Note that + // nghttp2 does not deliver OnInvalidFrame(). EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4)); EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(5); @@ -4513,26 +4520,33 @@ EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 1)); EXPECT_CALL(visitor, OnBeginDataForStream(1, 1)); EXPECT_CALL(visitor, OnDataForStream(1, "h")); - EXPECT_CALL(visitor, OnEndStream(1)); + EXPECT_CALL(visitor, + OnInvalidFrame( + 1, Http2VisitorInterface::InvalidFrameError::kHttpMessaging)); // Stream 3: content-length is smaller than actual data - // TODO(diannahu): Let oghttp2 validate content-length. + // The beginning of data is delivered to the visitor, but not the actual data. + // OnInvalidFrame() is delivered (unlike with nghttp2). EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 4)); EXPECT_CALL(visitor, OnBeginHeadersForStream(3)); EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(5); EXPECT_CALL(visitor, OnEndHeadersForStream(3)); EXPECT_CALL(visitor, OnFrameHeader(3, _, DATA, 1)); EXPECT_CALL(visitor, OnBeginDataForStream(3, 5)); - EXPECT_CALL(visitor, OnDataForStream(3, "howdy")); - EXPECT_CALL(visitor, OnEndStream(3)); + EXPECT_CALL(visitor, + OnInvalidFrame( + 3, Http2VisitorInterface::InvalidFrameError::kHttpMessaging)); // Stream 5: content-length is invalid and HEADERS ends the stream - // TODO(diannahu): Let oghttp2 validate content-length. + // Only oghttp2 invokes OnEndHeadersForStream(), but both oghttp2 and nghttp2 + // invoke OnInvalidFrame(). EXPECT_CALL(visitor, OnFrameHeader(5, _, HEADERS, 5)); EXPECT_CALL(visitor, OnBeginHeadersForStream(5)); EXPECT_CALL(visitor, OnHeaderForStream(5, _, _)).Times(5); EXPECT_CALL(visitor, OnEndHeadersForStream(5)); - EXPECT_CALL(visitor, OnEndStream(5)); + EXPECT_CALL(visitor, + OnInvalidFrame( + 5, Http2VisitorInterface::InvalidFrameError::kHttpMessaging)); const int64_t stream_result = adapter->ProcessBytes(stream_frames); EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result)); @@ -4541,12 +4555,30 @@ EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, _, 0x1)); EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, _, 0x1, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 1, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 3, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(3, Http2ErrorCode::HTTP2_NO_ERROR)); + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 5, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 5, _, 0x0, + static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(5, Http2ErrorCode::HTTP2_NO_ERROR)); EXPECT_TRUE(adapter->want_write()); int result = adapter->Send(); EXPECT_EQ(0, result); EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, - spdy::SpdyFrameType::SETTINGS})); + spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::RST_STREAM, + spdy::SpdyFrameType::RST_STREAM, + spdy::SpdyFrameType::RST_STREAM})); } } // namespace
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index cc37780..ad1fd9d 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -1002,7 +1002,8 @@ void OgHttp2Session::OnDataFrameHeader(spdy::SpdyStreamId stream_id, size_t length, bool /*fin*/) { - if (!stream_map_.contains(stream_id) || streams_reset_.contains(stream_id)) { + auto iter = stream_map_.find(stream_id); + if (iter == stream_map_.end() || streams_reset_.contains(stream_id)) { // The stream does not exist; it could be an error or a benign close, e.g., // getting data for a stream this connection recently closed. if (static_cast<Http2StreamId>(stream_id) > highest_processed_stream_id_) { @@ -1018,6 +1019,16 @@ fatal_visitor_callback_failure_ = true; decoder_.StopProcessing(); } + + // Validate against the content-length if it exists. + if (iter->second.remaining_content_length.has_value()) { + if (length > *iter->second.remaining_content_length) { + HandleContentLengthError(stream_id); + iter->second.remaining_content_length.reset(); + } else { + *iter->second.remaining_content_length -= length; + } + } } void OgHttp2Session::OnStreamFrameData(spdy::SpdyStreamId stream_id, @@ -1044,10 +1055,20 @@ auto iter = stream_map_.find(stream_id); if (iter != stream_map_.end()) { iter->second.half_closed_remote = true; - if (!streams_reset_.contains(stream_id)) { - visitor_.OnEndStream(stream_id); + if (streams_reset_.contains(stream_id)) { + return; } + + // Validate against the content-length if it exists. + if (iter->second.remaining_content_length.has_value() && + *iter->second.remaining_content_length != 0) { + HandleContentLengthError(stream_id); + return; + } + + visitor_.OnEndStream(stream_id); } + auto queued_frames_iter = queued_frames_.find(stream_id); const bool no_queued_frames = queued_frames_iter == queued_frames_.end() || queued_frames_iter->second == 0; @@ -1096,6 +1117,7 @@ } else { it->second.received_header_type = headers_handler_.header_type(); } + it->second.remaining_content_length = headers_handler_.content_length(); headers_handler_.set_stream_id(0); } } @@ -1668,5 +1690,19 @@ } } +void OgHttp2Session::HandleContentLengthError(Http2StreamId stream_id) { + EnqueueFrame(absl::make_unique<spdy::SpdyRstStreamIR>( + stream_id, spdy::ERROR_CODE_PROTOCOL_ERROR)); + // TODO(b/214614028): The OnInvalidFrame() is for nghttp2 parity, but parity + // causes this error to be connection-level in Envoy. Revisit after migration. + const bool ok = visitor_.OnInvalidFrame( + stream_id, Http2VisitorInterface::InvalidFrameError::kHttpMessaging); + if (!ok) { + fatal_visitor_callback_failure_ = true; + LatchErrorAndNotify(Http2ErrorCode::REFUSED_STREAM, + ConnectionError::kHttpMessaging); + } +} + } // namespace adapter } // namespace http2
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h index 3cdcd12..aac2688 100644 --- a/http2/adapter/oghttp2_session.h +++ b/http2/adapter/oghttp2_session.h
@@ -219,6 +219,7 @@ void* user_data = nullptr; int32_t send_window = kInitialFlowControlWindowSize; absl::optional<HeaderType> received_header_type; + absl::optional<size_t> remaining_content_length; bool half_closed_local = false; bool half_closed_remote = false; // Indicates that `outbound_body` temporarily cannot produce data. @@ -258,6 +259,9 @@ type_ == HeaderType::RESPONSE_100); return validator_.status_header(); } + absl::optional<size_t> content_length() const { + return validator_.content_length(); + } void AllowConnect() { validator_.AllowConnect(); } void SetMaxFieldSize(uint32_t field_size) { validator_.SetMaxFieldSize(field_size); @@ -385,6 +389,8 @@ void DecrementQueuedFrameCount(uint32_t stream_id, uint8_t frame_type); + void HandleContentLengthError(Http2StreamId stream_id); + // Receives events when inbound frames are parsed. Http2VisitorInterface& visitor_;