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 360a10268bc6e977a3b8e5c3db9231b33aed7562 Author: Jan Friedrich <[email protected]> AuthorDate: Mon Aug 17 23:39:09 2026 +0200 fix the lifetime of the LocalSyslogAppender identity openlog keeps the pointer it is given rather than a copy of the string, and registers it for the process rather than for an appender. ActivateOptions allocated a new buffer and overwrote the handle to the previous one without freeing it, so every re-activation leaked a buffer. The handle is now replaced under a lock, and the old buffer is freed only once openlog points at the new string. A failing openlog frees the new one. The handle became static, which is what the registration already was: a second appender replaces the identity of the first rather than adding one. OnClose no longer frees it. The buffer belongs to a process wide registration that outlives the appender, and another instance may still be logging through it. That closelog ends the connection for every instance is now documented as well. The use-after-free the audit describes does not apply to the libcs log4net targets: closelog runs before the free and glibc clears its stored pointer, while musl copies the ident. Only the leak and the shared lifetime are fixed here. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../3.4.0/309-syslog-identity-lifetime.xml | 15 ++++++ .../Appender/LocalSyslogAppenderTest.cs | 43 ++++++++++++++++ src/log4net/Appender/LocalSyslogAppender.cs | 58 +++++++++++++++++----- 3 files changed, 104 insertions(+), 12 deletions(-) diff --git a/src/changelog/3.4.0/309-syslog-identity-lifetime.xml b/src/changelog/3.4.0/309-syslog-identity-lifetime.xml new file mode 100644 index 00000000..707f323c --- /dev/null +++ b/src/changelog/3.4.0/309-syslog-identity-lifetime.xml @@ -0,0 +1,15 @@ +<?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"> + fix the lifetime of the `LocalSyslogAppender` identity. `openlog` keeps the pointer it is given + rather than a copy of the string, and registers it for the process rather than for an appender. + Each `ActivateOptions` allocated a new buffer and forgot the previous one, leaking it (CWE-401), + while `OnClose` freed a buffer that another instance may still have been logging through. The + handle is now shared, replaced under a lock once `openlog` points at the new string, and left + allocated when an appender closes (audit 1231d72-f017) + </description> +</entry> diff --git a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs index ff8cb8de..be09b7f9 100644 --- a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs +++ b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs @@ -17,9 +17,11 @@ // #endregion +using System; using System.Reflection; using log4net.Appender; +using log4net.Layout; using NUnit.Framework; @@ -71,6 +73,47 @@ public void MessagesWithoutNulCharactersAreUnchanged() public void EmptyMessageIsUnchanged() => Assert.That(EscapeNulCharacters(string.Empty), Is.Empty); + /// <summary> + /// <c>openlog</c> registers the identity for the process rather than for an appender, so the + /// handle to it has to be shared instead of being kept per instance. + /// </summary> + [Test] + public void TheIdentityHandleBelongsToTheProcess() + { + FieldInfo field = typeof(LocalSyslogAppender) + .GetField("_handleToIdentity", BindingFlags.Static | BindingFlags.NonPublic) + ?? throw new InvalidOperationException("LocalSyslogAppender._handleToIdentity is missing"); + + Assert.That(field.IsStatic, Is.True); + } + + /// <summary> + /// Activating twice has to replace the registered identity rather than allocate another one and + /// forget the first, which leaked a buffer per call. + /// </summary> + [Test] + [Platform("Linux")] + [NonParallelizable] + public void ActivatingTwiceReplacesTheIdentity() + { + LocalSyslogAppender appender = new() { Identity = "log4net-test-first", Layout = new PatternLayout("%message") }; + appender.ActivateOptions(); + IntPtr first = CurrentIdentityHandle(); + + appender.Identity = "log4net-test-second"; + appender.ActivateOptions(); + IntPtr second = CurrentIdentityHandle(); + + Assert.That(first, Is.Not.EqualTo(IntPtr.Zero)); + Assert.That(second, Is.Not.EqualTo(IntPtr.Zero)); + Assert.That(second, Is.Not.EqualTo(first)); + } + + private static IntPtr CurrentIdentityHandle() + => (IntPtr)typeof(LocalSyslogAppender) + .GetField("_handleToIdentity", BindingFlags.Static | BindingFlags.NonPublic)! + .GetValue(null)!; + private static string EscapeNulCharacters(string message) => (string)typeof(LocalSyslogAppender) .GetMethod("EscapeNulCharacters", BindingFlags.Static | BindingFlags.NonPublic)! diff --git a/src/log4net/Appender/LocalSyslogAppender.cs b/src/log4net/Appender/LocalSyslogAppender.cs index abdbbcb3..6c946a1b 100644 --- a/src/log4net/Appender/LocalSyslogAppender.cs +++ b/src/log4net/Appender/LocalSyslogAppender.cs @@ -308,10 +308,32 @@ public override void ActivateOptions() // create the native heap ansi string. Note this is a copy of our string // so we do not need to hold on to the string itself, holding on to the // handle will keep the heap ansi string alive. - _handleToIdentity = Marshal.StringToHGlobalAnsi(identString); + IntPtr identity = Marshal.StringToHGlobalAnsi(identString); - // open syslog - NativeMethods.openlog(_handleToIdentity, 1, Facility); + lock (_syslogSyncRoot) + { + IntPtr replaced = _handleToIdentity; + try + { + // open syslog + NativeMethods.openlog(identity, 1, Facility); + } + catch + { + Marshal.FreeHGlobal(identity); + throw; + } + + _handleToIdentity = identity; + + // Only now that openlog points at the new string. Freeing it before would leave libc + // dereferencing it for every record, and not freeing it at all leaked one buffer per call, + // which ActivateOptions may be called repeatedly. + if (replaced != IntPtr.Zero) + { + Marshal.FreeHGlobal(replaced); + } + } } /// <summary> @@ -363,7 +385,8 @@ private static string EscapeNulCharacters(string message) /// </summary> /// <remarks> /// <para> - /// Close the syslog when the appender is closed + /// <c>closelog</c> applies to the process rather than to this instance, so closing one appender + /// ends the syslog connection for every other instance as well. A later record reopens it. /// </para> /// </remarks> [System.Security.SecuritySafeCritical] @@ -381,11 +404,9 @@ protected override void OnClose() // Ignore dll not found at this point } - if (_handleToIdentity != IntPtr.Zero) - { - // free global ident - Marshal.FreeHGlobal(_handleToIdentity); - } + // The identity is deliberately not freed. openlog registered it for the whole process, so it + // outlives this appender: another instance may still be logging through it. ActivateOptions + // replaces it rather than letting them accumulate. } /// <summary> @@ -452,11 +473,24 @@ private static int GeneratePriority(SyslogFacility facility, SyslogSeverity seve => ((int)facility * 8) + (int)severity; /// <summary> - /// Marshaled handle to the identity string. We have to hold on to the - /// string as the <c>openlog</c> and <c>syslog</c> APIs just hold the + /// Marshaled handle to the identity string currently registered with <c>openlog</c>. + /// </summary> + /// <remarks> + /// <para> + /// We have to hold on to the string as the <c>openlog</c> and <c>syslog</c> APIs just hold the /// pointer to the ident and dereference it for each log message. + /// </para> + /// <para> + /// The registration belongs to the process rather than to an instance, so this is static: a + /// second appender replaces the identity of the first instead of adding one. + /// </para> + /// </remarks> + private static IntPtr _handleToIdentity = IntPtr.Zero; + + /// <summary> + /// Guards <see cref="_handleToIdentity"/> against two appenders being activated at once. /// </summary> - private IntPtr _handleToIdentity = IntPtr.Zero; + private static readonly object _syslogSyncRoot = new(); /// <summary> /// Mapping from level object to syslog severity
