config: fix crash when xDS endpoints receive unexpected typed config (envoyproxy#14709)

When a listener config contains an Any proto with an unknown proto type, the config
is rejected. If debug logging is turned on, though, Envoy first attempts to print the
JSON representation of the config. This caused a RELEASE_ASSERT to trigger,
crashing Envoy. This PR renames the unsafe proto-to-JSON conversion function
and replaces its usage with the safe version in the listener manager to prevent
the crash.

Signed-off-by: Alex Konradi <[email protected]>

Author: akonradi

docs/root/version_history/current.rst

@@ -36,6 +36,7 @@ Bug Fixes
 * grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false.
 * http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false.
 * http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false.
+* listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled.
 * upstream: fix handling of moving endpoints between priorities when active health checks are enabled. Previously moving to a higher numbered priority was a NOOP, and moving to a lower numbered priority caused an abort.
 
 Removed Config or Runtime

source/common/config/xds_context_params.cc

@@ -42,7 +42,7 @@ const NodeContextRenderers& nodeParamCbs() {
 void mergeMetadataJson(Protobuf::Map<std::string, std::string>& params,
                        const ProtobufWkt::Struct& metadata, const std::string& prefix) {
   for (const auto& it : metadata.fields()) {
-    params[prefix + it.first] = MessageUtil::getJsonStringFromMessage(it.second);
+    params[prefix + it.first] = MessageUtil::getJsonStringFromMessageOrDie(it.second);
   }
 }
 

source/common/formatter/substitution_formatter.cc

@@ -138,10 +138,11 @@ std::string JsonFormatterImpl::format(const Http::RequestHeaderMap& request_head
                                       const Http::ResponseTrailerMap& response_trailers,
                                       const StreamInfo::StreamInfo& stream_info,
                                       absl::string_view local_reply_body) const {
-  const auto output_struct = struct_formatter_.format(
+  const ProtobufWkt::Struct output_struct = struct_formatter_.format(
       request_headers, response_headers, response_trailers, stream_info, local_reply_body);
 
-  const std::string log_line = MessageUtil::getJsonStringFromMessage(output_struct, false, true);
+  const std::string log_line =
+      MessageUtil::getJsonStringFromMessageOrDie(output_struct, false, true);
   return absl::StrCat(log_line, "\n");
 }
 
@@ -1116,7 +1117,7 @@ MetadataFormatter::formatMetadata(const envoy::config::core::v3::Metadata& metad
     return absl::nullopt;
   }
 
-  std::string json = MessageUtil::getJsonStringFromMessage(value, false, true);
+  std::string json = MessageUtil::getJsonStringFromMessageOrDie(value, false, true);
   truncate(json, max_length_);
   return json;
 }

source/common/protobuf/protobuf.h

@@ -21,6 +21,7 @@
 #include "google/protobuf/service.h"
 #include "google/protobuf/struct.pb.h"
 #include "google/protobuf/stubs/status.h"
+#include "google/protobuf/stubs/statusor.h"
 #include "google/protobuf/text_format.h"
 #include "google/protobuf/util/field_mask_util.h"
 #include "google/protobuf/util/json_util.h"

source/common/protobuf/utility.cc

@@ -589,10 +589,14 @@ void MessageUtil::checkForUnexpectedFields(const Protobuf::Message& message,
 std::string MessageUtil::getYamlStringFromMessage(const Protobuf::Message& message,
                                                   const bool block_print,
                                                   const bool always_print_primitive_fields) {
-  std::string json = getJsonStringFromMessage(message, false, always_print_primitive_fields);
+
+  auto json_or_error = getJsonStringFromMessage(message, false, always_print_primitive_fields);
+  if (!json_or_error.ok()) {
+    throw EnvoyException(json_or_error.status().ToString());
+  }
   YAML::Node node;
   try {
-    node = YAML::Load(json);
+    node = YAML::Load(json_or_error.value());
   } catch (YAML::ParserException& e) {
     throw EnvoyException(e.what());
   } catch (YAML::BadConversion& e) {
@@ -612,9 +616,9 @@ std::string MessageUtil::getYamlStringFromMessage(const Protobuf::Message& messa
   return out.c_str();
 }
 
-std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& message,
-                                                  const bool pretty_print,
-                                                  const bool always_print_primitive_fields) {
+ProtobufUtil::StatusOr<std::string>
+MessageUtil::getJsonStringFromMessage(const Protobuf::Message& message, const bool pretty_print,
+                                      const bool always_print_primitive_fields) {
   Protobuf::util::JsonPrintOptions json_options;
   // By default, proto field names are converted to camelCase when the message is converted to JSON.
   // Setting this option makes debugging easier because it keeps field names consistent in JSON
@@ -629,12 +633,23 @@ std::string MessageUtil::getJsonStringFromMessage(const Protobuf::Message& messa
     json_options.always_print_primitive_fields = true;
   }
   std::string json;
-  const auto status = Protobuf::util::MessageToJsonString(message, &json, json_options);
-  // This should always succeed unless something crash-worthy such as out-of-memory.
-  RELEASE_ASSERT(status.ok(), "");
+  if (auto status = Protobuf::util::MessageToJsonString(message, &json, json_options);
+      !status.ok()) {
+    return status;
+  }
   return json;
 }
 
+std::string MessageUtil::getJsonStringFromMessageOrError(const Protobuf::Message& message,
+                                                         bool pretty_print,
+                                                         bool always_print_primitive_fields) {
+  auto json_or_error =
+      getJsonStringFromMessage(message, pretty_print, always_print_primitive_fields);
+  return json_or_error.ok() ? std::move(json_or_error).value()
+                            : fmt::format("Failed to convert protobuf message to JSON string: {}",
+                                          json_or_error.status().ToString());
+}
+
 void MessageUtil::unpackTo(const ProtobufWkt::Any& any_message, Protobuf::Message& message) {
   // If we don't have a type URL match, try an earlier version.
   const absl::string_view any_full_name =
@@ -1004,5 +1019,4 @@ void TimestampUtil::systemClockToTimestamp(const SystemTime system_clock_time,
           .time_since_epoch()
           .count()));
 }
-
 } // namespace Envoy

source/common/protobuf/utility.h

@@ -422,16 +422,51 @@ class MessageUtil {
                                               const bool always_print_primitive_fields = false);
 
   /**
-   * Extract JSON as string from a google.protobuf.Message.
+   * Extract JSON as string from a google.protobuf.Message. Returns an error if the message cannot
+   * be represented as JSON, which can occur if it contains an Any proto with an unrecognized type
+   * URL or invalid data, or if memory cannot be allocated.
+   * @param message message of type type.googleapis.com/google.protobuf.Message.
+   * @param pretty_print whether the returned JSON should be formatted.
+   * @param always_print_primitive_fields whether to include primitive fields set to their default
+   * values, e.g. an int32 set to 0 or a bool set to false.
+   * @return ProtobufUtil::StatusOr<std::string> of formatted JSON object, or an error status if
+   * conversion fails.
+   */
+  static ProtobufUtil::StatusOr<std::string>
+  getJsonStringFromMessage(const Protobuf::Message& message, bool pretty_print = false,
+                           bool always_print_primitive_fields = false);
+
+  /**
+   * Extract JSON as string from a google.protobuf.Message, crashing if the conversion to JSON
+   * fails. This method is safe so long as the message does not contain an Any proto with an
+   * unrecognized type or invalid data.
    * @param message message of type type.googleapis.com/google.protobuf.Message.
    * @param pretty_print whether the returned JSON should be formatted.
    * @param always_print_primitive_fields whether to include primitive fields set to their default
    * values, e.g. an int32 set to 0 or a bool set to false.
    * @return std::string of formatted JSON object.
    */
-  static std::string getJsonStringFromMessage(const Protobuf::Message& message,
-                                              bool pretty_print = false,
-                                              bool always_print_primitive_fields = false);
+  static std::string getJsonStringFromMessageOrDie(const Protobuf::Message& message,
+                                                   bool pretty_print = false,
+                                                   bool always_print_primitive_fields = false) {
+    auto json_or_error =
+        getJsonStringFromMessage(message, pretty_print, always_print_primitive_fields);
+    RELEASE_ASSERT(json_or_error.ok(), json_or_error.status().ToString());
+    return std::move(json_or_error).value();
+  }
+
+  /**
+   * Extract JSON as string from a google.protobuf.Message, returning some error string if the
+   * conversion to JSON fails.
+   * @param message message of type type.googleapis.com/google.protobuf.Message.
+   * @param pretty_print whether the returned JSON should be formatted.
+   * @param always_print_primitive_fields whether to include primitive fields set to their default
+   * values, e.g. an int32 set to 0 or a bool set to false.
+   * @return std::string of formatted JSON object, or an error message if conversion fails.
+   */
+  static std::string getJsonStringFromMessageOrError(const Protobuf::Message& message,
+                                                     bool pretty_print = false,
+                                                     bool always_print_primitive_fields = false);
 
   /**
    * Utility method to create a Struct containing the passed in key/value strings.

source/common/tracing/http_tracer_impl.cc

@@ -352,10 +352,10 @@ void MetadataCustomTag::apply(Span& span, const CustomTagContext& ctx) const {
     span.setTag(tag(), value.string_value());
     return;
   case ProtobufWkt::Value::kListValue:
-    span.setTag(tag(), MessageUtil::getJsonStringFromMessage(value.list_value()));
+    span.setTag(tag(), MessageUtil::getJsonStringFromMessageOrDie(value.list_value()));
     return;
   case ProtobufWkt::Value::kStructValue:
-    span.setTag(tag(), MessageUtil::getJsonStringFromMessage(value.struct_value()));
+    span.setTag(tag(), MessageUtil::getJsonStringFromMessageOrDie(value.struct_value()));
     return;
   default:
     break;

source/common/upstream/health_checker_base_impl.cc

@@ -450,8 +450,9 @@ void HealthCheckEventLoggerImpl::createHealthCheckEvent(
   callback(event);
 
   // Make sure the type enums make it into the JSON
-  const auto json = MessageUtil::getJsonStringFromMessage(event, /* pretty_print */ false,
-                                                          /* always_print_primitive_fields */ true);
+  const auto json =
+      MessageUtil::getJsonStringFromMessageOrError(event, /* pretty_print */ false,
+                                                   /* always_print_primitive_fields */ true);
   file_->write(fmt::format("{}\n", json));
 }
 } // namespace Upstream

source/common/upstream/outlier_detection_impl.cc

@@ -764,8 +764,9 @@ void EventLoggerImpl::logEject(const HostDescriptionConstSharedPtr& host, Detect
     event.mutable_eject_consecutive_event();
   }
 
-  const auto json = MessageUtil::getJsonStringFromMessage(event, /* pretty_print */ false,
-                                                          /* always_print_primitive_fields */ true);
+  const auto json =
+      MessageUtil::getJsonStringFromMessageOrError(event, /* pretty_print */ false,
+                                                   /* always_print_primitive_fields */ true);
   file_->write(fmt::format("{}\n", json));
 }
 
@@ -777,8 +778,9 @@ void EventLoggerImpl::logUneject(const HostDescriptionConstSharedPtr& host) {
 
   event.set_action(envoy::data::cluster::v2alpha::UNEJECT);
 
-  const auto json = MessageUtil::getJsonStringFromMessage(event, /* pretty_print */ false,
-                                                          /* always_print_primitive_fields */ true);
+  const auto json =
+      MessageUtil::getJsonStringFromMessageOrError(event, /* pretty_print */ false,
+                                                   /* always_print_primitive_fields */ true);
   file_->write(fmt::format("{}\n", json));
 }
 

source/common/upstream/subset_lb.cc

@@ -623,7 +623,8 @@ std::string SubsetLoadBalancer::describeMetadata(const SubsetLoadBalancer::Subse
       first = false;
     }
 
-    buf << it.first << "=" << MessageUtil::getJsonStringFromMessage(it.second);
+    const ProtobufWkt::Value& value = it.second;
+    buf << it.first << "=" << MessageUtil::getJsonStringFromMessageOrDie(value);
   }
 
   return buf.str();

source/extensions/common/tap/admin.cc

@@ -126,7 +126,7 @@ void AdminHandler::AdminPerTapSinkHandle::submitTrace(
     switch (format) {
     case envoy::config::tap::v3::OutputSink::JSON_BODY_AS_STRING:
     case envoy::config::tap::v3::OutputSink::JSON_BODY_AS_BYTES:
-      output_string = MessageUtil::getJsonStringFromMessage(*trace, true, true);
+      output_string = MessageUtil::getJsonStringFromMessageOrError(*trace, true, true);
       break;
     default:
       NOT_REACHED_GCOVR_EXCL_LINE;

source/extensions/common/tap/tap_config_base.cc

@@ -209,7 +209,7 @@ void FilePerTapSink::FilePerTapSinkHandle::submitTrace(
     break;
   case envoy::config::tap::v3::OutputSink::JSON_BODY_AS_BYTES:
   case envoy::config::tap::v3::OutputSink::JSON_BODY_AS_STRING:
-    output_file_ << MessageUtil::getJsonStringFromMessage(*trace, true, true);
+    output_file_ << MessageUtil::getJsonStringFromMessageOrError(*trace, true, true);
     break;
   default:
     NOT_REACHED_GCOVR_EXCL_LINE;

source/extensions/filters/http/aws_lambda/aws_lambda_filter.cc

@@ -310,7 +310,7 @@ void Filter::jsonizeRequest(Http::RequestHeaderMap const& headers, const Buffer:
   }
 
   MessageUtil::validate(json_req, ProtobufMessage::getStrictValidationVisitor());
-  const std::string json_data = MessageUtil::getJsonStringFromMessage(
+  const std::string json_data = MessageUtil::getJsonStringFromMessageOrError(
       json_req, false /* pretty_print  */, true /* always_print_primitive_fields */);
   out.add(json_data);
 }

source/extensions/filters/http/squash/squash_filter.cc

@@ -50,7 +50,7 @@ SquashFilterConfig::SquashFilterConfig(
 std::string SquashFilterConfig::getAttachment(const ProtobufWkt::Struct& attachment_template) {
   ProtobufWkt::Struct attachment_json(attachment_template);
   updateTemplateInStruct(attachment_json);
-  return MessageUtil::getJsonStringFromMessage(attachment_json);
+  return MessageUtil::getJsonStringFromMessageOrDie(attachment_json);
 }
 
 void SquashFilterConfig::updateTemplateInStruct(ProtobufWkt::Struct& attachment_template) {

source/extensions/filters/network/dubbo_proxy/config.cc

@@ -139,7 +139,7 @@ void ConfigImpl::registerFilter(const DubboFilterConfig& proto_config) {
   ENVOY_LOG(debug, "    dubbo filter #{}", filter_factories_.size());
   ENVOY_LOG(debug, "      name: {}", string_name);
   ENVOY_LOG(debug, "    config: {}",
-            MessageUtil::getJsonStringFromMessage(proto_config.config(), true));
+            MessageUtil::getJsonStringFromMessageOrError(proto_config.config(), true));
 
   auto& factory =
       Envoy::Config::Utility::getAndCheckFactoryByName<DubboFilters::NamedDubboFilterConfigFactory>(

source/extensions/filters/network/http_connection_manager/config.cc

@@ -516,7 +516,7 @@ void HttpConnectionManagerConfig::processFilter(
       callback, proto_config.name());
   ENVOY_LOG(debug, "      name: {}", filter_config_provider->name());
   ENVOY_LOG(debug, "    config: {}",
-            MessageUtil::getJsonStringFromMessage(
+            MessageUtil::getJsonStringFromMessageOrError(
                 proto_config.has_typed_config()
                     ? static_cast<const Protobuf::Message&>(proto_config.typed_config())
                     : static_cast<const Protobuf::Message&>(

source/extensions/filters/network/rocketmq_proxy/active_message.cc

@@ -209,7 +209,7 @@ void ActiveMessage::onQueryTopicRoute() {
     TopicRouteData topic_route_data(std::move(queue_data_list), std::move(broker_data_list));
     ProtobufWkt::Struct data_struct;
     topic_route_data.encode(data_struct);
-    std::string json = MessageUtil::getJsonStringFromMessage(data_struct);
+    std::string json = MessageUtil::getJsonStringFromMessageOrDie(data_struct);
     ENVOY_LOG(trace, "Serialize TopicRouteData for {} OK:\n{}", cluster_name, json);
     RemotingCommandPtr response = std::make_unique<RemotingCommand>(
         static_cast<int>(ResponseCode::Success), downstreamRequest()->version(),

source/extensions/filters/network/rocketmq_proxy/codec.cc

@@ -391,7 +391,7 @@ void Encoder::encode(const RemotingCommandPtr& command, Buffer::Instance& data)
     (*fields)["extFields"] = ext_fields_v;
   }
 
-  std::string json = MessageUtil::getJsonStringFromMessage(command_struct);
+  std::string json = MessageUtil::getJsonStringFromMessageOrDie(command_struct);
 
   int32_t frame_length = 4;
   int32_t header_length = json.size();

source/extensions/filters/network/rocketmq_proxy/conn_manager.cc

@@ -301,7 +301,7 @@ void ConnectionManager::onGetConsumerListByGroup(RemotingCommandPtr request) {
   RemotingCommandPtr response = std::make_unique<RemotingCommand>(
       enumToSignedInt(ResponseCode::Success), request->version(), request->opaque());
   response->markAsResponse();
-  std::string json = MessageUtil::getJsonStringFromMessage(body_struct);
+  std::string json = MessageUtil::getJsonStringFromMessageOrDie(body_struct);
   response->body().add(json);
   ENVOY_LOG(trace, "GetConsumerListByGroup respond with body: {}", json);
 

source/extensions/filters/network/thrift_proxy/config.cc

@@ -158,7 +158,7 @@ void ConfigImpl::processFilter(
   ENVOY_LOG(debug, "    thrift filter #{}", filter_factories_.size());
   ENVOY_LOG(debug, "      name: {}", string_name);
   ENVOY_LOG(debug, "    config: {}",
-            MessageUtil::getJsonStringFromMessage(
+            MessageUtil::getJsonStringFromMessageOrError(
                 proto_config.has_typed_config()
                     ? static_cast<const Protobuf::Message&>(proto_config.typed_config())
                     : static_cast<const Protobuf::Message&>(

source/extensions/tracers/dynamic_ot/config.cc

@@ -21,7 +21,8 @@ Tracing::HttpTracerSharedPtr DynamicOpenTracingTracerFactory::createHttpTracerTy
     const envoy::config::trace::v3::DynamicOtConfig& proto_config,
     Server::Configuration::TracerFactoryContext& context) {
   const std::string& library = proto_config.library();
-  const std::string config = MessageUtil::getJsonStringFromMessage(proto_config.config());
+  const ProtobufWkt::Struct& config_struct = proto_config.config();
+  const std::string config = MessageUtil::getJsonStringFromMessageOrDie(config_struct);
   Tracing::DriverPtr dynamic_driver = std::make_unique<DynamicOpenTracingDriver>(
       context.serverFactoryContext().scope(), library, config);
   return std::make_shared<Tracing::HttpTracerImpl>(std::move(dynamic_driver),

source/extensions/tracers/xray/daemon_broker.cc

@@ -22,8 +22,8 @@ std::string createHeader(const std::string& format, uint32_t version) {
   source::extensions::tracers::xray::daemon::Header header;
   header.set_format(format);
   header.set_version(version);
-  return MessageUtil::getJsonStringFromMessage(header, false /* pretty_print  */,
-                                               false /* always_print_primitive_fields */);
+  return MessageUtil::getJsonStringFromMessageOrDie(header, false /* pretty_print  */,
+                                                    false /* always_print_primitive_fields */);
 }
 
 } // namespace

source/extensions/tracers/xray/tracer.cc

@@ -92,7 +92,7 @@ void Span::finishSpan() {
     s.mutable_annotations()->insert({item.first, item.second});
   }
 
-  const std::string json = MessageUtil::getJsonStringFromMessage(
+  const std::string json = MessageUtil::getJsonStringFromMessageOrDie(
       s, false /* pretty_print  */, false /* always_print_primitive_fields */);
 
   broker_.send(json);

source/extensions/tracers/zipkin/span_buffer.cc

@@ -80,7 +80,7 @@ std::string JsonV2Serializer::serialize(const std::vector<Span>& zipkin_spans) {
             out, absl::StrJoin(
                      toListOfSpans(zipkin_span, replacements), ",",
                      [&replacement_values](std::string* element, const ProtobufWkt::Struct& span) {
-                       const std::string json = MessageUtil::getJsonStringFromMessage(
+                       const std::string json = MessageUtil::getJsonStringFromMessageOrDie(
                            span, /* pretty_print */ false,
                            /* always_print_primitive_fields */ true);