escape NUL characters in OutputDebugStringAppender content #315

OutputDebugStringW takes a null terminated string, so a NUL in logged content
ended the record there and dropped whatever the layout rendered after it.

The escape LocalSyslogAppender already had is now shared by both, since
EventLogAppender is the same shape and will want it too. Its tests moved onto the
shared helper with it.

The appender level test only runs on Windows: Append refuses to run elsewhere.

Author: FreeAndNil

src/changelog/3.5.0/315-outputdebugstring-nul.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="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    escape NUL characters in `OutputDebugStringAppender` content. `OutputDebugStringW` takes a null
+ terminated string, so a NUL in logged content ended the record there and silently dropped whatever
+ the layout rendered after it, exception text and trailing fields included (CWE-158). The escape
+ `LocalSyslogAppender` already applied is now shared between the two (audit da18b6fd-f009)
+  </description>
+</entry>

src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs

@@ -156,7 +156,8 @@ private static string[] SplitLines(string message)
       .Invoke(null, [message])!;
 
   private static string EscapeNulCharacters(string message)
-    => (string)typeof(LocalSyslogAppender)
+    => (string)typeof(LocalSyslogAppender).Assembly
+      .GetType("log4net.Appender.Internal.NativeStringEscape")!
       .GetMethod("EscapeNulCharacters", BindingFlags.Static | BindingFlags.NonPublic)!
       .Invoke(null, [message])!;
 }

src/log4net.Tests/Appender/OutputDebugAppenderTest.cs

@@ -58,6 +58,29 @@ public void AppendShouldNotCauseAnyErrors()
     log.Debug(DebugMessage);
     Assert.That(lastDebugString, Is.Not.Null.And.Contains(DebugMessage));
   }
+
+  /// <summary>
+  /// OutputDebugStringW takes a null terminated string, so a NUL in content would end the record
+  /// there and drop whatever the layout rendered after it.
+  /// </summary>
+  [Test]
+  public void NulCharactersAreEscapedBeforeTheNativeCall()
+  {
+    ILoggerRepository rep = LogManager.CreateRepository(Guid.NewGuid().ToString());
+    string? lastDebugString = null;
+    OutputAppender appender = new(value => lastDebugString = value)
+    {
+      Layout = new SimpleLayout(),
+      ErrorHandler = new FailOnError()
+    };
+    appender.ActivateOptions();
+    BasicConfigurator.Configure(rep, appender);
+
+    LogManager.GetLogger(rep.Name, GetType()).Debug("before\0after");
+
+    Assert.That(lastDebugString, Does.Contain("before\\0after"));
+    Assert.That(lastDebugString, Does.Not.Contain("\0"));
+  }
 }
 
 file sealed class OutputAppender(Action<string> outputDebugString)

src/log4net/Appender/Internal/NativeStringEscape.cs

@@ -0,0 +1,41 @@
+#region Apache License
+//
+// Licensed to the Apache Software Foundation (ASF) under one or more 
+// contributor license agreements. See the NOTICE file distributed with
+// this work for additional information regarding copyright ownership. 
+// The ASF licenses this file to you under the Apache License, Version 2.0
+// (the "License"); you may not use this file except in compliance with 
+// the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//
+#endregion
+
+namespace log4net.Appender.Internal;
+
+/// <summary>
+/// Prepares rendered content for a sink that takes a null terminated string.
+/// </summary>
+internal static class NativeStringEscape
+{
+  /// <summary>
+  /// Replaces NUL characters with a visible <c>\0</c> escape.
+  /// </summary>
+  /// <param name="message">The rendered message.</param>
+  /// <returns>The message with every NUL character escaped.</returns>
+  /// <remarks>
+  /// <para>
+  /// A NUL ends the string for the native sink, dropping everything the layout rendered after it,
+  /// trailing fields and exception text included. Logged content is not trusted and may well
+  /// contain a NUL, so the character is escaped rather than passed through.
+  /// </para>
+  /// </remarks>
+  internal static string EscapeNulCharacters(string message)
+    => message.IndexOf('\0') < 0 ? message : message.Replace("\0", "\\0");
+}

src/log4net/Appender/LocalSyslogAppender.cs

@@ -20,6 +20,7 @@
 using System;
 using System.Runtime.InteropServices;
 
+using log4net.Appender.Internal;
 using log4net.Core;
 using log4net.Util;
 
@@ -353,7 +354,7 @@ public override void ActivateOptions()
   protected override void Append(LoggingEvent loggingEvent)
   {
     int priority = GeneratePriority(Facility, GetSeverity(loggingEvent.EnsureNotNull().Level));
-    string message = EscapeNulCharacters(RenderLoggingEvent(loggingEvent));
+    string message = NativeStringEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent));
 
     // The second argument is a printf style format string.
     if (NewLineHandling == SyslogNewLineHandling.Split)
@@ -405,26 +406,6 @@ private static string[] SplitLines(string message)
 
   private static readonly string[] _newLines = ["\r\n", "\n", "\r"];
 
-  /// <summary>
-  /// Replaces NUL characters with a visible <c>\0</c> escape.
-  /// </summary>
-  /// <param name="message">The rendered message.</param>
-  /// <returns>The message with every NUL character escaped.</returns>
-  /// <remarks>
-  /// <para>
-  /// The message is marshaled to libc as a null-terminated string, so a NUL character anywhere in
-  /// it would end the record there and silently drop everything the layout rendered after it,
-  /// including trailing fields and exception text. Logged content is not trusted and may well
-  /// contain a NUL, so the character is escaped rather than passed through.
-  /// </para>
-  /// <para>
-  /// Newlines are handled separately, see <see cref="NewLineHandling"/>. Other control characters
-  /// are passed through.
-  /// </para>
-  /// </remarks>
-  private static string EscapeNulCharacters(string message)
-    => message.IndexOf('\0') < 0 ? message : message.Replace("\0", "\\0");
-
   /// <summary>
   /// Close the syslog when the appender is closed
   /// </summary>

src/log4net/Appender/OutputDebugStringAppender.cs

@@ -18,6 +18,7 @@
 #endregion
 
 using System;
+using log4net.Appender.Internal;
 using log4net.Core;
 using log4net.Util;
 
@@ -59,7 +60,7 @@ protected override void Append(LoggingEvent loggingEvent)
     }
 #endif
 
-    _outputDebugString(RenderLoggingEvent(loggingEvent));
+    _outputDebugString(NativeStringEscape.EscapeNulCharacters(RenderLoggingEvent(loggingEvent)));
   }
 
   /// <summary>