From 9c754412b91dc966a79fd915ffcf41f20a52a244 Mon Sep 17 00:00:00 2001 From: "Antonio V. Leonti" <53806445+antoniovleonti@users.noreply.github.com> Date: Tue, 15 Oct 2024 10:35:05 -0400 Subject: [PATCH] fix potential null dereference in ext_authz (#36268) Commit Message: fix potential null dereference in ext_authz Additional Description: Previously, if ext_authz had emit filter state stats set to true and another filter added filter state under the ext authz filter's name, it would result in a null dereference. The member logging_info_ would not be set in initiateCall after seeing there was already data there. Later, we would dereference logging_info_ to update the stats as if it were initialized already. I've added a check for a null logging_info_ and added logging & a stat for when there's a filter state naming collision. I also made some readability improvements to the ext_authz test. Risk Level: low Testing: unit tested Docs Changes: none Release Notes: none Platform Specific Features: none --------- Signed-off-by: antoniovleonti --- .../filters/http/ext_authz/ext_authz.cc | 30 ++++-- .../filters/http/ext_authz/ext_authz.h | 5 +- .../filters/http/ext_authz/ext_authz_test.cc | 102 +++++++++++++----- 3 files changed, 100 insertions(+), 37 deletions(-) diff --git a/source/extensions/filters/http/ext_authz/ext_authz.cc b/source/extensions/filters/http/ext_authz/ext_authz.cc index 962aede2c2ca..7874c8a94b74 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.cc +++ b/source/extensions/filters/http/ext_authz/ext_authz.cc @@ -180,15 +180,25 @@ void Filter::initiateCall(const Http::RequestHeaderMap& headers) { // Now that we'll definitely be making the request, add filter state stats if configured to do so. const Envoy::StreamInfo::FilterStateSharedPtr& filter_state = decoder_callbacks_->streamInfo().filterState(); - if ((config_->emitFilterStateStats() || config_->filterMetadata().has_value()) && - !filter_state->hasData(decoder_callbacks_->filterConfigName())) { - filter_state->setData(decoder_callbacks_->filterConfigName(), - std::make_shared(config_->filterMetadata()), - Envoy::StreamInfo::FilterState::StateType::Mutable, - Envoy::StreamInfo::FilterState::LifeSpan::Request); + if ((config_->emitFilterStateStats() || config_->filterMetadata().has_value())) { + if (!filter_state->hasData(decoder_callbacks_->filterConfigName())) { + filter_state->setData(decoder_callbacks_->filterConfigName(), + std::make_shared(config_->filterMetadata()), + Envoy::StreamInfo::FilterState::StateType::Mutable, + Envoy::StreamInfo::FilterState::LifeSpan::Request); - logging_info_ = - filter_state->getDataMutable(decoder_callbacks_->filterConfigName()); + // This may return nullptr (if there's a value at this name whose type doesn't match or isn't + // mutable, for example), so we must check logging_info_ is not nullptr later. + logging_info_ = + filter_state->getDataMutable(decoder_callbacks_->filterConfigName()); + } + if (logging_info_ == nullptr) { + stats_.filter_state_name_collision_.inc(); + ENVOY_STREAM_LOG(debug, + "Could not find logging info at {}! (Did another filter already put data " + "at this name?)", + *decoder_callbacks_, decoder_callbacks_->filterConfigName()); + } } absl::optional maybe_merged_per_route_config; @@ -405,6 +415,10 @@ void Filter::updateLoggingInfo() { return; } + if (logging_info_ == nullptr) { + return; + } + // Latency is the only stat available if we aren't using envoy grpc. if (start_time_.has_value()) { logging_info_->setLatency(std::chrono::duration_cast( diff --git a/source/extensions/filters/http/ext_authz/ext_authz.h b/source/extensions/filters/http/ext_authz/ext_authz.h index 61ec151ccc7e..2505957eb93f 100644 --- a/source/extensions/filters/http/ext_authz/ext_authz.h +++ b/source/extensions/filters/http/ext_authz/ext_authz.h @@ -42,7 +42,8 @@ namespace ExtAuthz { COUNTER(disabled) \ COUNTER(failure_mode_allowed) \ COUNTER(invalid) \ - COUNTER(ignored_dynamic_metadata) + COUNTER(ignored_dynamic_metadata) \ + COUNTER(filter_state_name_collision) /** * Wrapper struct for ext_authz filter stats. @see stats_macros.h @@ -402,7 +403,7 @@ class Filter : public Logger::Loggable, Upstream::ClusterInfoConstSharedPtr cluster_; // The stats for the filter. ExtAuthzFilterStats stats_; - ExtAuthzLoggingInfo* logging_info_; + ExtAuthzLoggingInfo* logging_info_{nullptr}; // This is used to hold the final configs after we merge them with per-route configs. bool allow_partial_message_{}; diff --git a/test/extensions/filters/http/ext_authz/ext_authz_test.cc b/test/extensions/filters/http/ext_authz/ext_authz_test.cc index 7dd8d309710e..afa574dddd68 100644 --- a/test/extensions/filters/http/ext_authz/ext_authz_test.cc +++ b/test/extensions/filters/http/ext_authz/ext_authz_test.cc @@ -242,17 +242,13 @@ class EmitFilterStateTest auto upstream_cluster_info = std::make_shared>(); stream_info_->upstream_cluster_info_ = upstream_cluster_info; - absl::optional filter_metadata = absl::nullopt; - if (std::get<2>(GetParam())) { - filter_metadata = Envoy::ProtobufWkt::Struct(); - *(*filter_metadata->mutable_fields())["foo"].mutable_string_value() = "bar"; + if (std::get<1>(GetParam())) { + expected_output_.setLatency(std::chrono::milliseconds(1)); + expected_output_.setUpstreamHost(upstream_host); + expected_output_.setClusterInfo(upstream_cluster_info); + expected_output_.setBytesSent(123); + expected_output_.setBytesReceived(456); } - - expected_output_.setLatency(std::chrono::milliseconds(1)); - expected_output_.setUpstreamHost(upstream_host); - expected_output_.setClusterInfo(upstream_cluster_info); - expected_output_.setBytesSent(123); - expected_output_.setBytesReceived(456); } static std::string @@ -268,6 +264,13 @@ class EmitFilterStateTest prepareCheck(); + auto& filter_state = decoder_filter_callbacks_.streamInfo().filterState(); + absl::optional preexisting_data_copy = + filter_state->hasData(FilterConfigName) + ? absl::make_optional( + *filter_state->getDataReadOnly(FilterConfigName)) + : absl::nullopt; + // ext_authz makes a single call to the external auth service once it sees the end of stream. EXPECT_CALL(*client_, check(_, _, _, _)) .WillOnce(Invoke([&](Filters::Common::ExtAuthz::RequestCallbacks& callbacks, @@ -279,33 +282,43 @@ class EmitFilterStateTest })); EXPECT_CALL(*client_, streamInfo()).WillRepeatedly(Return(stream_info_.get())); - EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark, filter_->decodeHeaders(request_headers_, true)); request_callbacks_->onComplete(std::make_unique(response)); - auto filter_state = decoder_filter_callbacks_.streamInfo().filterState(); + // Will exist if stats or filter metadata is emitted or if there was preexisting logging info. + bool expect_logging_info = + (std::get<1>(GetParam()) || std::get<2>(GetParam()) || preexisting_data_copy); - // Will exist if either of stats or filter metadata is emitted or both. - ASSERT_EQ(filter_state->hasData(FilterConfigName), - std::get<1>(GetParam()) || std::get<2>(GetParam())); + ASSERT_EQ(filter_state->hasData(FilterConfigName), expect_logging_info); - if (std::get<1>(GetParam())) { - auto actual = filter_state->getDataReadOnly(FilterConfigName); + if (!expect_logging_info) { + return; + } - EXPECT_EQ(actual->latency(), expected_output_.latency()); - EXPECT_EQ(actual->upstreamHost(), expected_output_.upstreamHost()); - EXPECT_EQ(actual->clusterInfo(), expected_output_.clusterInfo()); - EXPECT_EQ(actual->bytesSent(), expected_output_.bytesSent()); - EXPECT_EQ(actual->bytesReceived(), expected_output_.bytesReceived()); + auto actual = filter_state->getDataReadOnly(FilterConfigName); + ASSERT_NE(actual, nullptr); + if (preexisting_data_copy) { + expectEq(*actual, *preexisting_data_copy); + EXPECT_EQ((std::get<1>(GetParam()) || std::get<2>(GetParam())) ? 1U : 0U, + config_->stats().filter_state_name_collision_.value()); + return; } - if (std::get<2>(GetParam())) { - auto actual = filter_state->getDataReadOnly(FilterConfigName); - ASSERT_TRUE(actual->filterMetadata().has_value()); - EXPECT_EQ(actual->filterMetadata()->DebugString(), - expected_output_.filterMetadata()->DebugString()); + expectEq(*actual, expected_output_); + } + + static void expectEq(const ExtAuthzLoggingInfo& actual, const ExtAuthzLoggingInfo& expected) { + EXPECT_EQ(actual.latency(), expected.latency()); + EXPECT_EQ(actual.upstreamHost(), expected.upstreamHost()); + EXPECT_EQ(actual.clusterInfo(), expected.clusterInfo()); + EXPECT_EQ(actual.bytesSent(), expected.bytesSent()); + EXPECT_EQ(actual.bytesReceived(), expected.bytesReceived()); + + ASSERT_EQ(actual.filterMetadata().has_value(), expected.filterMetadata().has_value()); + if (expected.filterMetadata().has_value()) { + EXPECT_EQ(actual.filterMetadata()->DebugString(), expected.filterMetadata()->DebugString()); } } @@ -4016,6 +4029,41 @@ TEST_P(EmitFilterStateTest, NullUpstreamHost) { test(response); } +// hasData() will return false, setData() will succeed because this is +// mutable, thus getMutableData will not be nullptr and the naming collision +// is silently ignored. +TEST_P(EmitFilterStateTest, PreexistingFilterStateDifferentTypeMutable) { + class TestObject : public Envoy::StreamInfo::FilterState::Object {}; + decoder_filter_callbacks_.stream_info_.filter_state_->setData( + FilterConfigName, + // This will not cast to ExtAuthzLoggingInfo, so when the filter tries to + // getMutableData(...), it will return nullptr. + std::make_shared(), Envoy::StreamInfo::FilterState::StateType::Mutable, + Envoy::StreamInfo::FilterState::LifeSpan::Request); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::OK; + + test(response); +} + +// hasData() will return true so the filter will not try to override the data. +TEST_P(EmitFilterStateTest, PreexistingFilterStateSameTypeMutable) { + class TestObject : public Envoy::StreamInfo::FilterState::Object {}; + decoder_filter_callbacks_.stream_info_.filter_state_->setData( + FilterConfigName, + // This will not cast to ExtAuthzLoggingInfo, so when the filter tries to + // getMutableData(...), it will return nullptr. + std::make_shared(absl::nullopt), + Envoy::StreamInfo::FilterState::StateType::Mutable, + Envoy::StreamInfo::FilterState::LifeSpan::Request); + + Filters::Common::ExtAuthz::Response response{}; + response.status = Filters::Common::ExtAuthz::CheckStatus::OK; + + test(response); +} + } // namespace } // namespace ExtAuthz } // namespace HttpFilters