admin: Fix crashes on invalid FieldMasks

source/server/admin/config_dump_handler.cc

@@ -12,6 +12,21 @@ namespace Envoy {
 namespace Server {
 
 namespace {
+
+// Validates that `field_mask` is valid for `message` and applies `TrimMessage`.
+// Necessary because TrimMessage crashes if `field_mask` is invalid.
+// Returns `true` on success.
+bool checkFieldMaskAndTrimMessage(const Protobuf::FieldMask& field_mask,
+                                  Protobuf::Message& message) {
+  for (const auto& path : field_mask.paths()) {
+    if (!ProtobufUtil::FieldMaskUtil::GetFieldDescriptors(message.GetDescriptor(), path, nullptr)) {
+      return false;
+    }
+  }
+  ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, &message);
+  return true;
+}
+
 // Apply a field mask to a resource message. A simple field mask might look
 // like "cluster.name,cluster.alt_stat_name,last_updated" for a StaticCluster
 // resource. Unfortunately, since the "cluster" field is Any and the in-built
@@ -31,7 +46,10 @@ namespace {
 // this to allow arbitrary indexing through Any fields. This is pretty
 // complicated, we would need to build a FieldMask tree similar to how the C++
 // Protobuf library does this internally.
-void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Message& message) {
+/**
+ * @return true on success, false if `field_mask` is invalid.
+ */
+bool trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Message& message) {
   const Protobuf::Descriptor* descriptor = message.GetDescriptor();
   const Protobuf::Reflection* reflection = message.GetReflection();
   // Figure out which paths cover Any fields. For each field, gather the paths to
@@ -87,13 +105,15 @@ void trimResourceMessage(const Protobuf::FieldMask& field_mask, Protobuf::Messag
       inner_message.reset(dmf.GetPrototype(inner_descriptor)->New());
       MessageUtil::unpackTo(any_message, *inner_message);
       // Trim message.
-      ProtobufUtil::FieldMaskUtil::TrimMessage(inner_field_mask, inner_message.get());
+      if (!checkFieldMaskAndTrimMessage(inner_field_mask, *inner_message)) {
+        return false;
+      }
       // Pack it back into the Any resource.
       any_message.PackFrom(*inner_message);
       reflection->MutableMessage(&message, any_field)->CopyFrom(any_message);
     }
   }
-  ProtobufUtil::FieldMaskUtil::TrimMessage(outer_field_mask, &message);
+  return checkFieldMaskAndTrimMessage(outer_field_mask, message);
 }
 
 // Helper method to get the resource parameter.
@@ -126,14 +146,18 @@ Http::Code ConfigDumpHandler::handlerConfigDump(absl::string_view url,
 
   envoy::admin::v3::ConfigDump dump;
 
+  absl::optional<std::pair<Http::Code, std::string>> err;
   if (resource.has_value()) {
-    auto err = addResourceToDump(dump, mask, resource.value(), include_eds);
-    if (err.has_value()) {
-      response.add(err.value().second);
-      return err.value().first;
-    }
+    err = addResourceToDump(dump, mask, resource.value(), include_eds);
   } else {
-    addAllConfigToDump(dump, mask, include_eds);
+    err = addAllConfigToDump(dump, mask, include_eds);
+  }
+  if (err.has_value()) {
+    response_headers.addReference(Http::Headers::get().XContentTypeOptions,
+                                  Http::Headers::get().XContentTypeOptionValues.Nosniff);
+    response_headers.setReferenceContentType(Http::Headers::get().ContentTypeValues.Text);
+    response.add(err.value().second);
+    return err.value().first;
   }
   MessageUtil::redact(dump);
 
@@ -176,7 +200,11 @@ ConfigDumpHandler::addResourceToDump(envoy::admin::v3::ConfigDump& dump,
       if (mask.has_value()) {
         Protobuf::FieldMask field_mask;
         ProtobufUtil::FieldMaskUtil::FromString(mask.value(), &field_mask);
-        trimResourceMessage(field_mask, msg);
+        if (!trimResourceMessage(field_mask, msg)) {
+          return absl::optional<std::pair<Http::Code, std::string>>{std::make_pair(
+              Http::Code::BadRequest, absl::StrCat("FieldMask ", field_mask.DebugString(),
+                                                   " could not be successfully used."))};
+        }
       }
       auto* config = dump.add_configs();
       config->PackFrom(msg);
@@ -191,9 +219,10 @@ ConfigDumpHandler::addResourceToDump(envoy::admin::v3::ConfigDump& dump,
       std::make_pair(Http::Code::NotFound, fmt::format("{} not found in config dump", resource))};
 }
 
-void ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
-                                           const absl::optional<std::string>& mask,
-                                           bool include_eds) const {
+absl::optional<std::pair<Http::Code, std::string>>
+ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
+                                      const absl::optional<std::string>& mask,
+                                      bool include_eds) const {
   Envoy::Server::ConfigTracker::CbsMap callbacks_map = config_tracker_.getCallbacksMap();
   if (include_eds) {
     // TODO(mattklein123): Add ability to see warming clusters in admin output.
@@ -213,12 +242,17 @@ void ConfigDumpHandler::addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
       ProtobufUtil::FieldMaskUtil::FromString(mask.value(), &field_mask);
       // We don't use trimMessage() above here since masks don't support
       // indexing through repeated fields.
-      ProtobufUtil::FieldMaskUtil::TrimMessage(field_mask, message.get());
+      if (!checkFieldMaskAndTrimMessage(field_mask, *message)) {
+        return absl::optional<std::pair<Http::Code, std::string>>{std::make_pair(
+            Http::Code::BadRequest, absl::StrCat("FieldMask ", field_mask.DebugString(),
+                                                 " could not be successfully used."))};
+      }
     }
 
     auto* config = dump.add_configs();
     config->PackFrom(*message);
   }
+  return absl::nullopt;
 }
 
 ProtobufTypes::MessagePtr ConfigDumpHandler::dumpEndpointConfigs() const {

source/server/admin/config_dump_handler.h

@@ -27,8 +27,9 @@ class ConfigDumpHandler : public HandlerContextBase {
                                Buffer::Instance& response, AdminStream&) const;
 
 private:
-  void addAllConfigToDump(envoy::admin::v3::ConfigDump& dump,
-                          const absl::optional<std::string>& mask, bool include_eds) const;
+  absl::optional<std::pair<Http::Code, std::string>>
+  addAllConfigToDump(envoy::admin::v3::ConfigDump& dump, const absl::optional<std::string>& mask,
+                     bool include_eds) const;
   /**
    * Add the config matching the passed resource to the passed config dump.
    * @return absl::nullopt on success, else the Http::Code and an error message that should be added

test/server/admin/config_dump_handler_test.cc

@@ -564,24 +564,16 @@ TEST_P(AdminInstanceTest, ConfigDumpFiltersByResourceAndMask) {
   EXPECT_EQ(expected_json, output);
 }
 
-// Test that no fields are present in the JSON output if there is no intersection between the fields
+// Test that BadRequest is returned if there is no intersection between the fields
 // of the config dump and the fields present in the mask query parameter.
 TEST_P(AdminInstanceTest, ConfigDumpNonExistentMask) {
   Buffer::OwnedImpl response;
   Http::TestResponseHeaderMapImpl header_map;
   auto clusters = admin_.getConfigTracker().add("clusters", testDumpClustersConfig);
-  const std::string expected_json = R"EOF({
- "configs": [
-  {
-   "@type": "type.googleapis.com/envoy.admin.v3.ClustersConfigDump.StaticCluster"
-  }
- ]
-}
-)EOF";
-  EXPECT_EQ(Http::Code::OK,
+  EXPECT_EQ(Http::Code::BadRequest,
             getCallback("/config_dump?resource=static_clusters&mask=bad", header_map, response));
   std::string output = response.toString();
-  EXPECT_EQ(expected_json, output);
+  EXPECT_THAT(output, testing::HasSubstr("could not be successfully used"));
 }
 
 // Test that a 404 Not found is returned if a non-existent resource is passed in as the
@@ -611,5 +603,55 @@ TEST_P(AdminInstanceTest, ConfigDumpResourceNotRepeated) {
             getCallback("/config_dump?resource=version_info", header_map, response));
 }
 
+TEST_P(AdminInstanceTest, InvalidFieldMaskWithResourceDoesNotCrash) {
+  Buffer::OwnedImpl response;
+  Http::TestResponseHeaderMapImpl header_map;
+  auto clusters = admin_.getConfigTracker().add("clusters", [] {
+    auto msg = std::make_unique<envoy::admin::v3::ClustersConfigDump>();
+    auto* static_cluster = msg->add_static_clusters();
+    envoy::config::cluster::v3::Cluster inner_cluster;
+    inner_cluster.add_transport_socket_matches()->set_name("match1");
+    inner_cluster.add_transport_socket_matches()->set_name("match2");
+    static_cluster->mutable_cluster()->PackFrom(inner_cluster);
+    return msg;
+  });
+
+  // `transport_socket_matches` is a repeated field, and cannot be indexed through in a FieldMask.
+  EXPECT_EQ(Http::Code::BadRequest,
+            getCallback(
+                "/config_dump?resource=static_clusters&mask=cluster.transport_socket_matches.name",
+                header_map, response));
+  EXPECT_EQ("FieldMask paths: \"cluster.transport_socket_matches.name\"\n could not be "
+            "successfully used.",
+            response.toString());
+  EXPECT_EQ(header_map.ContentType()->value().getStringView(),
+            Http::Headers::get().ContentTypeValues.Text);
+  EXPECT_EQ(header_map.get(Http::Headers::get().XContentTypeOptions)[0]->value(),
+            Http::Headers::get().XContentTypeOptionValues.Nosniff);
+}
+
+TEST_P(AdminInstanceTest, InvalidFieldMaskWithoutResourceDoesNotCrash) {
+  Buffer::OwnedImpl response;
+  Http::TestResponseHeaderMapImpl header_map;
+  auto bootstrap = admin_.getConfigTracker().add("bootstrap", [] {
+    auto msg = std::make_unique<envoy::admin::v3::BootstrapConfigDump>();
+    auto* bootstrap = msg->mutable_bootstrap();
+    bootstrap->mutable_node()->add_extensions()->set_name("ext1");
+    bootstrap->mutable_node()->add_extensions()->set_name("ext2");
+    return msg;
+  });
+
+  // `extensions` is a repeated field, and cannot be indexed through in a FieldMask.
+  EXPECT_EQ(Http::Code::BadRequest,
+            getCallback("/config_dump?mask=bootstrap.node.extensions.name", header_map, response));
+  EXPECT_EQ("FieldMask paths: \"bootstrap.node.extensions.name\"\n could not be "
+            "successfully used.",
+            response.toString());
+  EXPECT_EQ(header_map.ContentType()->value().getStringView(),
+            Http::Headers::get().ContentTypeValues.Text);
+  EXPECT_EQ(header_map.get(Http::Headers::get().XContentTypeOptions)[0]->value(),
+            Http::Headers::get().XContentTypeOptionValues.Nosniff);
+}
+
 } // namespace Server
 } // namespace Envoy