escape invalid host name

Signed-off-by: tyxia <tyxia@google.com> Signed-off-by: Boteng Yao <boteng@google.com> Signed-off-by: Ryan Northey <ryan@synca.io>
envoyproxy · Sep 19, 2024 · 30c7d6c · 30c7d6c
1 parent 285a408
commit 30c7d6c
Show file tree

Hide file tree

Showing 6 changed files with 94 additions and 8 deletions.
diff --git a/changelogs/current.yaml b/changelogs/current.yaml
@@ -98,6 +98,10 @@ minor_behavior_changes:
   change: |
     Changes the default value of ``envoy.reloadable_features.http2_use_oghttp2`` to ``false``. This changes the codec used for HTTP/2
     requests and responses to address to address stability concerns. This behavior can be reverted by setting the feature to ``true``.
+- area: access_log
+  change: |
+    Sanitize SNI for potential log injection. The invalid character will be replaced by ``_`` with an ``invalid:`` marker. If runtime
+    flag ``envoy.reloadable_features.sanitize_sni_in_access_log`` is set to ``false``, the sanitize behavior is disabled.
 
 bug_fixes:
 # *Changes expected to improve the state of the world and are unlikely to have negative effects*

diff --git a/source/common/common/utility.cc b/source/common/common/utility.cc
@@ -586,6 +586,23 @@ void StringUtil::escapeToOstream(std::ostream& os, absl::string_view view) {
   }
 }
 
+std::string StringUtil::sanitizeInvalidHostname(const absl::string_view source) {
+  std::string ret_str = std::string(source);
+  bool sanitized = false;
+  for (size_t i = 0; i < ret_str.size(); ++i) {
+    if (absl::ascii_isalnum(ret_str[i]) || ret_str[i] == '.' || ret_str[i] == '-') {
+      continue;
+    }
+    sanitized = true;
+    ret_str[i] = '_';
+  }
+
+  if (sanitized) {
+    ret_str = absl::StrCat("invalid:", ret_str);
+  }
+  return ret_str;
+}
+
 const std::string& getDefaultDateFormat(bool local_time) {
   if (local_time) {
     CONSTRUCT_ON_FIRST_USE(std::string, "%Y-%m-%dT%H:%M:%E3S%z");

diff --git a/source/common/common/utility.h b/source/common/common/utility.h
@@ -490,6 +490,15 @@ class StringUtil {
    */
   static void escapeToOstream(std::ostream& os, absl::string_view view);
 
+  /**
+   * Sanitize host name strings for logging purposes. Replace invalid hostname characters (anything
+   * that's not alphanumeric, hyphen, or period) with underscore. The sanitized string is not a
+   * valid host name.
+   * @param source supplies the string to sanitize.
+   * @return sanitized string.
+   */
+  static std::string sanitizeInvalidHostname(const absl::string_view source);
+
   /**
    * Provide a default value for a string if empty.
    * @param s string.

diff --git a/source/common/formatter/stream_info_formatter.cc b/source/common/formatter/stream_info_formatter.cc
@@ -1353,8 +1353,14 @@ const StreamInfoFormatterProviderLookupTable& getKnownStreamInfoFormatterProvide
                   [](const StreamInfo::StreamInfo& stream_info) {
                     absl::optional<std::string> result;
                     if (!stream_info.downstreamAddressProvider().requestedServerName().empty()) {
-                      result = std::string(
-                          stream_info.downstreamAddressProvider().requestedServerName());
+                      if (Runtime::runtimeFeatureEnabled(
+                              "envoy.reloadable_features.sanitize_sni_in_access_log")) {
+                        result = StringUtil::sanitizeInvalidHostname(
+                            stream_info.downstreamAddressProvider().requestedServerName());
+                      } else {
+                        result = std::string(
+                            stream_info.downstreamAddressProvider().requestedServerName());
+                      }
                     }
                     return result;
                   });

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -86,6 +86,7 @@ RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_socket_use_address_cache_f
 RUNTIME_GUARD(envoy_reloadable_features_reject_invalid_yaml);
 RUNTIME_GUARD(envoy_reloadable_features_report_stream_reset_error_code);
 RUNTIME_GUARD(envoy_reloadable_features_sanitize_http2_headers_without_nghttp2);
+RUNTIME_GUARD(envoy_reloadable_features_sanitize_sni_in_access_log);
 RUNTIME_GUARD(envoy_reloadable_features_sanitize_te);
 RUNTIME_GUARD(envoy_reloadable_features_send_local_reply_when_no_buffer_and_upstream_request);
 RUNTIME_GUARD(envoy_reloadable_features_skip_dns_lookup_for_proxied_requests);

diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc
@@ -973,20 +973,69 @@ TEST(SubstitutionFormatterTest, streamInfoFormatter) {
 
   {
     StreamInfoFormatter upstream_format("REQUESTED_SERVER_NAME");
-    std::string requested_server_name = "stub_server";
+    std::string requested_server_name;
     stream_info.downstream_connection_info_provider_->setRequestedServerName(requested_server_name);
-    EXPECT_EQ("stub_server", upstream_format.formatWithContext({}, stream_info));
+    EXPECT_EQ(absl::nullopt, upstream_format.formatWithContext({}, stream_info));
     EXPECT_THAT(upstream_format.formatValueWithContext({}, stream_info),
-                ProtoEq(ValueUtil::stringValue("stub_server")));
+                ProtoEq(ValueUtil::nullValue()));
   }
 
   {
     StreamInfoFormatter upstream_format("REQUESTED_SERVER_NAME");
-    std::string requested_server_name;
+    std::string requested_server_name = "stub-server";
     stream_info.downstream_connection_info_provider_->setRequestedServerName(requested_server_name);
-    EXPECT_EQ(absl::nullopt, upstream_format.formatWithContext({}, stream_info));
+    EXPECT_EQ("stub-server", upstream_format.formatWithContext({}, stream_info));
     EXPECT_THAT(upstream_format.formatValueWithContext({}, stream_info),
-                ProtoEq(ValueUtil::nullValue()));
+                ProtoEq(ValueUtil::stringValue("stub-server")));
+  }
+
+  {
+    StreamInfoFormatter upstream_format("REQUESTED_SERVER_NAME");
+    std::string requested_server_name = "stub_server\n";
+    stream_info.downstream_connection_info_provider_->setRequestedServerName(requested_server_name);
+    EXPECT_EQ("invalid:stub_server_", upstream_format.formatWithContext({}, stream_info));
+    EXPECT_THAT(upstream_format.formatValueWithContext({}, stream_info),
+                ProtoEq(ValueUtil::stringValue("invalid:stub_server_")));
+  }
+
+  {
+    StreamInfoFormatter upstream_format("REQUESTED_SERVER_NAME");
+    std::string requested_server_name = "\e[0;34m\n$(echo -e $blue)end<script>alert()</script>";
+    stream_info.downstream_connection_info_provider_->setRequestedServerName(requested_server_name);
+    EXPECT_EQ("invalid:__0_34m___echo_-e__blue_end_script_alert____script_",
+              upstream_format.formatWithContext({}, stream_info));
+    EXPECT_THAT(upstream_format.formatValueWithContext({}, stream_info),
+                ProtoEq(ValueUtil::stringValue(
+                    "invalid:__0_34m___echo_-e__blue_end_script_alert____script_")));
+  }
+
+  {
+    StreamInfoFormatter upstream_format("REQUESTED_SERVER_NAME");
+    std::string invalid_utf8_string("prefix");
+    invalid_utf8_string.append(1, char(0xc3));
+    invalid_utf8_string.append(1, char(0xc7));
+    invalid_utf8_string.append("valid_middle");
+    invalid_utf8_string.append(1, char(0xc4));
+    invalid_utf8_string.append("valid_suffix");
+    stream_info.downstream_connection_info_provider_->setRequestedServerName(invalid_utf8_string);
+    EXPECT_EQ("invalid:prefix__valid_middle_valid_suffix",
+              upstream_format.formatWithContext({}, stream_info));
+    EXPECT_THAT(upstream_format.formatValueWithContext({}, stream_info),
+                ProtoEq(ValueUtil::stringValue("invalid:prefix__valid_middle_valid_suffix")));
+  }
+
+  {
+    TestScopedRuntime scoped_runtime;
+    scoped_runtime.mergeValues({
+        {"envoy.reloadable_features.sanitize_sni_in_access_log", "false"},
+    });
+
+    StreamInfoFormatter upstream_format("REQUESTED_SERVER_NAME");
+    std::string requested_server_name = "stub_server\n";
+    stream_info.downstream_connection_info_provider_->setRequestedServerName(requested_server_name);
+    EXPECT_EQ("stub_server\n", upstream_format.formatWithContext({}, stream_info));
+    EXPECT_THAT(upstream_format.formatValueWithContext({}, stream_info),
+                ProtoEq(ValueUtil::stringValue("stub_server\n")));
   }
 
   {