Skip to content

Commit

Permalink
fix potential null dereference in ext_authz (envoyproxy#36268)
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@antoniovleonti
antoniovleonti authored Oct 15, 2024
1 parent 5b0d56d commit 9c75441
Show file tree
Hide file tree
Showing 3 changed files with 100 additions and 37 deletions.
30 changes: 22 additions & 8 deletions source/extensions/filters/http/ext_authz/ext_authz.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExtAuthzLoggingInfo>(decoder_callbacks_->filterConfigName())) {
filter_state->setData(decoder_callbacks_->filterConfigName(),
std::make_shared<ExtAuthzLoggingInfo>(config_->filterMetadata()),
Envoy::StreamInfo::FilterState::StateType::Mutable,
Envoy::StreamInfo::FilterState::LifeSpan::Request);
if ((config_->emitFilterStateStats() || config_->filterMetadata().has_value())) {
if (!filter_state->hasData<ExtAuthzLoggingInfo>(decoder_callbacks_->filterConfigName())) {
filter_state->setData(decoder_callbacks_->filterConfigName(),
std::make_shared<ExtAuthzLoggingInfo>(config_->filterMetadata()),
Envoy::StreamInfo::FilterState::StateType::Mutable,
Envoy::StreamInfo::FilterState::LifeSpan::Request);

logging_info_ =
filter_state->getDataMutable<ExtAuthzLoggingInfo>(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<ExtAuthzLoggingInfo>(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<FilterConfigPerRoute> maybe_merged_per_route_config;
Expand Down Expand Up @@ -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<std::chrono::microseconds>(
Expand Down
5 changes: 3 additions & 2 deletions source/extensions/filters/http/ext_authz/ext_authz.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -402,7 +403,7 @@ class Filter : public Logger::Loggable<Logger::Id::ext_authz>,
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_{};
Expand Down
102 changes: 75 additions & 27 deletions test/extensions/filters/http/ext_authz/ext_authz_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,17 +242,13 @@ class EmitFilterStateTest
auto upstream_cluster_info = std::make_shared<NiceMock<Upstream::MockClusterInfo>>();
stream_info_->upstream_cluster_info_ = upstream_cluster_info;

absl::optional<Envoy::ProtobufWkt::Struct> 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
Expand All @@ -268,6 +264,13 @@ class EmitFilterStateTest

prepareCheck();

auto& filter_state = decoder_filter_callbacks_.streamInfo().filterState();
absl::optional<ExtAuthzLoggingInfo> preexisting_data_copy =
filter_state->hasData<ExtAuthzLoggingInfo>(FilterConfigName)
? absl::make_optional(
*filter_state->getDataReadOnly<ExtAuthzLoggingInfo>(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,
Expand All @@ -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<Filters::Common::ExtAuthz::Response>(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<ExtAuthzLoggingInfo>(FilterConfigName),
std::get<1>(GetParam()) || std::get<2>(GetParam()));
ASSERT_EQ(filter_state->hasData<ExtAuthzLoggingInfo>(FilterConfigName), expect_logging_info);

if (std::get<1>(GetParam())) {
auto actual = filter_state->getDataReadOnly<ExtAuthzLoggingInfo>(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<ExtAuthzLoggingInfo>(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<ExtAuthzLoggingInfo>(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());
}
}

Expand Down Expand Up @@ -4016,6 +4029,41 @@ TEST_P(EmitFilterStateTest, NullUpstreamHost) {
test(response);
}

// hasData<ExtAuthzLoggingInfo>() will return false, setData() will succeed because this is
// mutable, thus getMutableData<ExtAuthzLoggingInfo> 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<ExtAuthzLoggingInfo>(...), it will return nullptr.
std::make_shared<TestObject>(), 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<ExtAuthzLoggingInfo>() 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<ExtAuthzLoggingInfo>(...), it will return nullptr.
std::make_shared<ExtAuthzLoggingInfo>(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
Expand Down

0 comments on commit 9c75441

Please sign in to comment.