This is an automated email from the ASF dual-hosted git repository. FreeAndNil pushed a commit to branch Feature/security-audit-hardening in repository https://gitbox.apache.org/repos/asf/logging-log4net.git
commit e80b3810acb3a2c9cb9788dab1ef1b9e4aedcf42 Author: Jan Friedrich <[email protected]> AuthorDate: Mon Aug 17 21:54:18 2026 +0200 redact the password when reporting a failed database connection InitializeDatabaseConnection named the resolved connection string in full when it could not open the connection, and the documented examples embed Password=... The message goes through the ErrorHandler, so it is what an operator sees while diagnosing exactly this failure. Password-bearing keywords are now replaced with *****. The rest of the connection string is kept, so the message stays useful for spotting a typo in the server name or catalog. If the string cannot be parsed - likely, given that it just failed to connect - all of it is redacted. This matters more since appender errors became visible without log4net.Internal.Debug: the password would otherwise have reached stderr in a default configuration. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- ...redact-password-in-connection-string-errors.xml | 13 +++++ .../Appender/AdoNet/Log4NetConnection.cs | 14 +++++- src/log4net.Tests/Appender/AdoNetAppenderTest.cs | 40 ++++++++++++++++ src/log4net/Appender/AdoNetAppender.cs | 56 +++++++++++++++++++++- 4 files changed, 121 insertions(+), 2 deletions(-) diff --git a/src/changelog/3.4.0/309-redact-password-in-connection-string-errors.xml b/src/changelog/3.4.0/309-redact-password-in-connection-string-errors.xml new file mode 100644 index 00000000..bcc07ebb --- /dev/null +++ b/src/changelog/3.4.0/309-redact-password-in-connection-string-errors.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="309" link="https://github.com/apache/logging-log4net/pull/309"/> + <description format="asciidoc"> + stop `AdoNetAppender` from repeating the password when it reports a connection it could not + open. The message named the resolved connection string in full, and the documented examples embed + `Password=...` (CWE-532). Password-bearing keywords are now replaced with `*****`, while the rest + of the connection string is kept so the message stays useful (audit 1231d72-f018) + </description> +</entry> diff --git a/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs b/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs index e11411b0..43364c51 100644 --- a/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs +++ b/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs @@ -44,7 +44,19 @@ internal sealed class Log4NetConnection : IDbConnection public IDbCommand CreateCommand() => new Log4NetCommand(); - public void Open() => _open = true; + public void Open() + { + if (FailOnOpen) + { + throw new InvalidOperationException("Simulated failure to open the connection"); + } + _open = true; + } + + /// <summary> + /// When set, <see cref="Open"/> throws, simulating a connection that cannot be established. + /// </summary> + public static bool FailOnOpen { get; set; } public static Log4NetConnection? MostRecentInstance { get; private set; } diff --git a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs index a07744c8..3e391586 100644 --- a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs +++ b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs @@ -265,6 +265,46 @@ public void BufferingWebsiteExample() Assert.That(param.Value, Is.Empty); } + /// <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. + /// </summary> + [Test] + [NonParallelizable] + public void FailedConnectionDoesNotReportThePassword() + { + const string password = "H0rseBatteryStaple"; + List<LogLog> messages = []; + try + { + Log4NetConnection.FailOnOpen = true; + LogLog.ExecuteWithoutEmittingInternalMessages(() => + { + using LogLog.LogReceivedAdapter _ = new(messages); + AdoNetAppender adoNetAppender = new() + { + BufferSize = -1, + ConnectionType = typeof(Log4NetConnection).AssemblyQualifiedName!, + ConnectionString = $"data source=someserver;initial catalog=somedb;User ID=someuser;Password={password}", + CommandText = "INSERT INTO Log ([Message]) VALUES (@message)" + }; + adoNetAppender.ActivateOptions(); + }); + + string reported = string.Join(Environment.NewLine, messages.ConvertAll(m => m.Message)); + + Assert.That(reported, Does.Not.Contain(password)); + Assert.That(reported, Does.Contain("Could not open database connection")); + // The rest of the connection string survives, so the message stays useful for diagnosis. + Assert.That(reported, Does.Contain("someserver")); + } + finally + { + Log4NetConnection.FailOnOpen = false; + } + } + /// <summary> /// Without CommandText the rendered Layout is executed as the SQL statement, which is open /// to SQL injection from logged content. Activation has to say so. diff --git a/src/log4net/Appender/AdoNetAppender.cs b/src/log4net/Appender/AdoNetAppender.cs index 0faf995e..b4cd2209 100644 --- a/src/log4net/Appender/AdoNetAppender.cs +++ b/src/log4net/Appender/AdoNetAppender.cs @@ -21,6 +21,7 @@ using System.Collections.Generic; using System.Configuration; using System.Data; +using System.Data.Common; using System.IO; using log4net.Util; @@ -788,12 +789,65 @@ private void InitializeDatabaseConnection() catch (Exception e) when (!e.IsFatal()) { // Sadly, your connection string is bad. - ErrorHandler.Error($"Could not open database connection [{resolvedConnectionString}]. Connection string context [{connectionStringContext}].", e); + ErrorHandler.Error($"Could not open database connection [{RedactConnectionString(resolvedConnectionString)}]. Connection string context [{connectionStringContext}].", e); Connection = null; } } + /// <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. + /// </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> + private static string RedactConnectionString(string connectionString) + { + if (string.IsNullOrEmpty(connectionString)) + { + return connectionString; + } + + try + { + DbConnectionStringBuilder builder = new() { ConnectionString = connectionString }; + + List<string> keys = []; + foreach (string key in builder.Keys) + { + keys.Add(key); + } + + 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)) + { + builder[key] = RedactedValue; + } + } + + return builder.ConnectionString; + } + catch (Exception e) when (!e.IsFatal()) + { + // The connection string could not be parsed - which is likely, given that it just failed + // to connect - so redact all of it rather than risk echoing a password. + LogLog.Debug(_declaringType, "Could not parse the connection string in order to redact it", e); + return RedactedValue; + } + } + + /// <summary> + /// Stands in for a password in diagnostic messages. + /// </summary> + private const string RedactedValue = "*****"; + /// <summary> /// Cleanup the existing connection. /// </summary>
