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 <[email protected]>
envoyproxy · Oct 15, 2024 · 9c75441 · 9c75441
1 parent 5b0d56d
commit 9c75441
Show file tree

Hide file tree

Showing 3 changed files with 100 additions and 37 deletions.
diff --git 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<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;
@@ -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>(

diff --git 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<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_{};

diff --git 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<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
@@ -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,
@@ -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());
     }
   }
 
@@ -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