diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 8f9f288dbea8..4fc915462b40 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -13,6 +13,10 @@ behavior_changes: minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* +- 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 index 144d57f69a12..66fbebd4723d 100644 --- a/source/common/common/utility.cc +++ b/source/common/common/utility.cc @@ -500,6 +500,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() { CONSTRUCT_ON_FIRST_USE(std::string, "%Y-%m-%dT%H:%M:%E3SZ"); } diff --git a/source/common/common/utility.h b/source/common/common/utility.h index 4a07b8c4467a..c3d160e12e35 100644 --- a/source/common/common/utility.h +++ b/source/common/common/utility.h @@ -429,6 +429,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 index 1aa75193225f..1ad04c818159 100644 --- a/source/common/formatter/stream_info_formatter.cc +++ b/source/common/formatter/stream_info_formatter.cc @@ -1293,8 +1293,14 @@ const StreamInfoFormatterProviderLookupTable& getKnownStreamInfoFormatterProvide [](const StreamInfo::StreamInfo& stream_info) { absl::optional 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 index 41723d9021a2..ba27431b7b24 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -85,6 +85,7 @@ RUNTIME_GUARD(envoy_reloadable_features_quic_fix_filter_manager_uaf); // @danzh2010 or @RyanTheOptimist before removing. RUNTIME_GUARD(envoy_reloadable_features_quic_send_server_preferred_address_to_all_clients); RUNTIME_GUARD(envoy_reloadable_features_quic_upstream_reads_fixed_number_packets); +RUNTIME_GUARD(envoy_reloadable_features_sanitize_sni_in_access_log); RUNTIME_GUARD(envoy_reloadable_features_sanitize_te); RUNTIME_GUARD(envoy_reloadable_features_send_header_raw_value); RUNTIME_GUARD(envoy_reloadable_features_send_local_reply_when_no_buffer_and_upstream_request); diff --git a/test/common/formatter/substitution_formatter_test.cc b/test/common/formatter/substitution_formatter_test.cc index 72d5b4a929ff..263eecac741c 100644 --- a/test/common/formatter/substitution_formatter_test.cc +++ b/test/common/formatter/substitution_formatter_test.cc @@ -846,20 +846,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"; + 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"))); } {