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_;
