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 19fdb4a229de5ea1fe306254aaae7802df638945 Author: Jan Friedrich <[email protected]> AuthorDate: Mon Aug 17 21:31:39 2026 +0200 warn when AdoNetAppender executes layout-generated SQL Without CommandText the appender builds a complete SQL statement per event by rendering the Layout and executes it as it is. Layouts perform no SQL quoting or escaping and offer no way to add it, so anything that reaches a log statement is executed as part of the statement, with the privileges of the appender's connection. ActivateOptions now logs an error naming the appender and pointing at CommandText with AdoNetAppenderParameter bindings, which pass content as database parameters. The mode itself keeps working, so no existing configuration breaks. The manual gained a warning as well, and its claim that BufferSize defaults to 100 is corrected to 512. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../3.4.0/309-warn-about-layout-generated-sql.xml | 13 ++++++ src/log4net.Tests/Appender/AdoNetAppenderTest.cs | 49 ++++++++++++++++++++++ src/log4net/Appender/AdoNetAppender.cs | 30 ++++++++++++- .../configuration/appenders/adonetappender.adoc | 14 ++++++- 4 files changed, 104 insertions(+), 2 deletions(-) diff --git a/src/changelog/3.4.0/309-warn-about-layout-generated-sql.xml b/src/changelog/3.4.0/309-warn-about-layout-generated-sql.xml new file mode 100644 index 00000000..a2010af2 --- /dev/null +++ b/src/changelog/3.4.0/309-warn-about-layout-generated-sql.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="added"> + <issue id="309" link="https://github.com/apache/logging-log4net/pull/309"/> + <description format="asciidoc"> + log an error when `AdoNetAppender` is activated without `CommandText`. In that legacy mode the + rendered `Layout` output is executed as the SQL statement, and because layouts perform no SQL + quoting, logged content becomes part of the statement (CWE-89). Configure `CommandText` with + `AdoNetAppenderParameter` bindings instead (audit 1231d72-f003) + </description> +</entry> diff --git a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs index c42560d4..a07744c8 100644 --- a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs +++ b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs @@ -20,6 +20,7 @@ */ using System; +using System.Collections.Generic; using System.Data; using System.Xml; using log4net.Appender; @@ -264,6 +265,54 @@ public void BufferingWebsiteExample() Assert.That(param.Value, Is.Empty); } + /// <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. + /// </summary> + [Test] + [NonParallelizable] + public void ActivateOptionsWithoutCommandTextWarnsAboutSqlInjection() + { + List<LogLog> messages = []; + LogLog.ExecuteWithoutEmittingInternalMessages(() => + { + using LogLog.LogReceivedAdapter _ = new(messages); + AdoNetAppender adoNetAppender = new() + { + BufferSize = -1, + ConnectionType = typeof(Log4NetConnection).AssemblyQualifiedName! + }; + adoNetAppender.ActivateOptions(); + }); + + Assert.That(messages.ConvertAll(m => m.Message), + Has.Some.Contains("open to SQL injection")); + } + + /// <summary> + /// Configuring CommandText is the supported way to use the appender and must not warn. + /// </summary> + [Test] + [NonParallelizable] + public void ActivateOptionsWithCommandTextDoesNotWarn() + { + List<LogLog> messages = []; + LogLog.ExecuteWithoutEmittingInternalMessages(() => + { + using LogLog.LogReceivedAdapter _ = new(messages); + AdoNetAppender adoNetAppender = new() + { + BufferSize = -1, + ConnectionType = typeof(Log4NetConnection).AssemblyQualifiedName!, + CommandText = "INSERT INTO Log ([Message]) VALUES (@message)" + }; + adoNetAppender.ActivateOptions(); + }); + + Assert.That(messages.ConvertAll(m => m.Message), + Has.None.Contains("open to SQL injection")); + } + /// <summary> /// An event the database rejects must only lose itself. The other events of the flushed /// buffer have already been removed from it and cannot be retried later, so they have to diff --git a/src/log4net/Appender/AdoNetAppender.cs b/src/log4net/Appender/AdoNetAppender.cs index 886c975d..0faf995e 100644 --- a/src/log4net/Appender/AdoNetAppender.cs +++ b/src/log4net/Appender/AdoNetAppender.cs @@ -243,6 +243,15 @@ public AdoNetAppender() /// If this property is not set, the command text is retrieved by invoking /// <see cref="GetLogStatement(LoggingEvent)"/>. /// </para> + /// <para> + /// Setting this property is strongly recommended. Without it every event is turned into + /// a complete SQL statement by the <see cref="AppenderSkeleton.Layout"/> and executed as + /// it is. Layouts perform no SQL quoting or escaping and offer no way to add it, so any + /// content that reaches a log statement - a user name or a request parameter, for + /// example - is executed as part of the statement, with the privileges of the appender's + /// connection. Use this property together with <see cref="AdoNetAppenderParameter"/> + /// bindings, which pass the content as database parameters instead. + /// </para> /// </remarks> public string? CommandText { get; set; } @@ -363,6 +372,17 @@ public override void ActivateOptions() { base.ActivateOptions(); + if (string.IsNullOrWhiteSpace(CommandText)) + { + // Without CommandText every event is turned into a complete SQL statement by the + // Layout and executed as it is. Layouts do not quote or escape anything, so a single + // quote anywhere in the logged content changes the statement that gets executed. + LogLog.Error(_declaringType, + $"AdoNetAppender [{Name}]: CommandText is not configured, so the rendered Layout is executed as the SQL statement. " + + "Layouts perform no SQL quoting, which makes this mode open to SQL injection from logged content. " + + "Configure CommandText together with AdoNetAppenderParameter bindings instead, which pass the content as database parameters."); + } + SecurityContext ??= SecurityContextProvider.DefaultProvider.CreateSecurityContext(this); InitializeDatabaseConnection(); @@ -622,8 +642,16 @@ protected virtual void Prepare(IDbCommand dbCmd) /// </summary> /// <param name="logEvent">The event being logged.</param> /// <remarks> - /// This method can be overridden by subclasses to provide + /// <para> + /// This method can be overridden by subclasses to provide /// more control over the format of the database statement. + /// </para> + /// <para> + /// The returned text is executed as the SQL statement without any quoting, so an override + /// that interpolates event content has to escape that content itself. Prefer configuring + /// <see cref="CommandText"/> with <see cref="AdoNetAppenderParameter"/> bindings over + /// generating statement text here. + /// </para> /// </remarks> /// <returns> /// Text that can be passed to a <see cref="IDbCommand"/>. diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/adonetappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/adonetappender.adoc index 472b3c69..06e91c5b 100644 --- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/adonetappender.adoc +++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/adonetappender.adoc @@ -19,7 +19,7 @@ = AdoNetAppender The `AdoNetAppender` is used to log events directly to a database table. -It writes log events in batches (with a default size of 100, controlled by the `BufferSize` setting). +It writes log events in batches (with a default size of 512, controlled by the `BufferSize` setting). The configuration of `AdoNetAppender` depends on the database provider you're using. Here are the key configuration elements: @@ -28,6 +28,18 @@ Here are the key configuration elements: * `ConnectionString`: The connection string that is specific to the database provider (e.g., SQL Server, MySQL). * `CommandText`: Defines the SQL command to execute. This can either be a prepared statement or a stored procedure. In the examples below, a prepared statement is used. +[WARNING] +==== +Always configure `CommandText` together with `parameter` elements, as every example below does. + +If `CommandText` is omitted, the appender falls back to a legacy mode in which the rendered +`Layout` output *is* the SQL statement that gets executed. +Layouts perform no SQL quoting or escaping and offer no way to add it, so anything that reaches +a log statement -- a user name, a request parameter, an exception message -- is executed as part +of the statement, with the privileges of the appender's connection. +The appender logs an error at startup when it is configured this way. +==== + Each parameter in the prepared statement or stored procedure is defined with: * `Name`: The name of the parameter.
