This is an automated email from the ASF dual-hosted git repository. FreeAndNil pushed a commit to branch Feature/312-more-hardening in repository https://gitbox.apache.org/repos/asf/logging-log4net.git
commit 711e9b908f48c90609d56941ea78925ab58feb4e Author: Jan Friedrich <[email protected]> AuthorDate: Tue Sep 1 19:12:00 2026 +0200 redact connection strings by keyword allowlist Hiding password-bearing keywords missed Extended Properties, which nests a whole connection string the parser returns as one opaque value, and keywords such as AccessToken. Only keywords naming the server and account are kept. The old test only covered a flat Password=, which is why this survived. --- .../312-redact-connection-string-allowlist.xml | 13 +++++++++ src/log4net.Tests/Appender/AdoNetAppenderTest.cs | 20 ++++++++------ src/log4net/Appender/AdoNetAppender.cs | 32 ++++++++++++++-------- 3 files changed, 46 insertions(+), 19 deletions(-) diff --git a/src/changelog/3.4.1/312-redact-connection-string-allowlist.xml b/src/changelog/3.4.1/312-redact-connection-string-allowlist.xml new file mode 100644 index 00000000..5ff9f585 --- /dev/null +++ b/src/changelog/3.4.1/312-redact-connection-string-allowlist.xml @@ -0,0 +1,13 @@ +<?xml version="1.0" encoding="UTF-8"?> +<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xmlns="https://logging.apache.org/xml/ns"; + xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"; + type="fixed"> + <issue id="312" link="https://github.com/apache/logging-log4net/pull/312"/> + <description format="asciidoc"> + keep secrets out of the `AdoNetAppender` message for a connection it could not open. Hiding + password-bearing keywords missed `Extended Properties`, which nests a whole connection string, and + keywords such as `AccessToken` (CWE-532). Only keywords naming the server and account are kept now + (audit da18b6fd-f028) + </description> +</entry> diff --git a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs index 3e391586..af339681 100644 --- a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs +++ b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs @@ -266,15 +266,19 @@ public void BufferingWebsiteExample() } /// <summary> - /// The message reporting a failed connection must not repeat the password from the connection - /// string. The appender reports the failure through its ErrorHandler, so this message is what - /// an operator sees on stderr in a default configuration. + /// A secret need not sit under a password-like keyword: Extended Properties nests a whole + /// connection string, and tokens have keywords of their own. /// </summary> [Test] [NonParallelizable] - public void FailedConnectionDoesNotReportThePassword() + [TestCase("Extended Properties=\"Driver={SQL Server};Server=someserver;UID=someuser;PWD=H0rseBatteryStaple\"")] + [TestCase("AccessToken=H0rseBatteryStaple")] + [TestCase("SharedAccessSignature=H0rseBatteryStaple")] + [TestCase("Password=H0rseBatteryStaple")] + [TestCase("PWD=H0rseBatteryStaple")] + public void FailedConnectionDoesNotReportASecret(string secretBearingKeyword) { - const string password = "H0rseBatteryStaple"; + const string secret = "H0rseBatteryStaple"; List<LogLog> messages = []; try { @@ -286,7 +290,7 @@ public void FailedConnectionDoesNotReportThePassword() { BufferSize = -1, ConnectionType = typeof(Log4NetConnection).AssemblyQualifiedName!, - ConnectionString = $"data source=someserver;initial catalog=somedb;User ID=someuser;Password={password}", + ConnectionString = $"data source=someserver;initial catalog=somedb;{secretBearingKeyword}", CommandText = "INSERT INTO Log ([Message]) VALUES (@message)" }; adoNetAppender.ActivateOptions(); @@ -294,9 +298,9 @@ public void FailedConnectionDoesNotReportThePassword() string reported = string.Join(Environment.NewLine, messages.ConvertAll(m => m.Message)); - Assert.That(reported, Does.Not.Contain(password)); + Assert.That(reported, Does.Not.Contain(secret)); Assert.That(reported, Does.Contain("Could not open database connection")); - // The rest of the connection string survives, so the message stays useful for diagnosis. + // The server is still named, which is what makes the message worth printing. Assert.That(reported, Does.Contain("someserver")); } finally diff --git a/src/log4net/Appender/AdoNetAppender.cs b/src/log4net/Appender/AdoNetAppender.cs index b4cd2209..c3fd833a 100644 --- a/src/log4net/Appender/AdoNetAppender.cs +++ b/src/log4net/Appender/AdoNetAppender.cs @@ -796,14 +796,16 @@ private void InitializeDatabaseConnection() } /// <summary> - /// Replaces the values of password-bearing keywords in a connection string with - /// <see cref="RedactedValue"/>, so that it can be named in a diagnostic message. + /// Reduces a connection string to the keywords that identify the server, for a diagnostic message. /// </summary> /// <param name="connectionString">The connection string to redact.</param> - /// <returns> - /// The connection string with every password value replaced, or <see cref="RedactedValue"/> if it - /// could not be parsed. - /// </returns> + /// <returns>The other values replaced, or <see cref="RedactedValue"/> if it could not be parsed.</returns> + /// <remarks> + /// <para> + /// An allowlist, because hiding known secret keywords misses `Extended Properties`: it nests a + /// whole connection string that the parser returns as one opaque value. + /// </para> + /// </remarks> private static string RedactConnectionString(string connectionString) { if (string.IsNullOrEmpty(connectionString)) @@ -823,10 +825,7 @@ private static string RedactConnectionString(string connectionString) foreach (string key in keys) { - // Providers spell the secret differently - Password, PWD, User Password - so match on - // the keyword rather than on a fixed list. - if (key.IndexOf("password", StringComparison.OrdinalIgnoreCase) >= 0 - || key.Equals("pwd", StringComparison.OrdinalIgnoreCase)) + if (!DiagnosticKeywords.Contains(key)) { builder[key] = RedactedValue; } @@ -844,7 +843,18 @@ private static string RedactConnectionString(string connectionString) } /// <summary> - /// Stands in for a password in diagnostic messages. + /// Keywords whose values are kept: they name the server and account, not the credentials. + /// </summary> + private static readonly HashSet<string> DiagnosticKeywords = new(StringComparer.OrdinalIgnoreCase) + { + "provider", "driver", "data source", "server", "address", "addr", "network address", + "initial catalog", "database", "port", "user id", "uid", "user", "username", + "integrated security", "trusted_connection", "encrypt", "timeout", "connect timeout", + "connection timeout", "application name", "workstation id", "pooling", + }; + + /// <summary> + /// Stands in for a value withheld from a diagnostic message. /// </summary> private const string RedactedValue = "*****";
