Escaping special characters in multiple appenders

CLAUDE.md

@@ -229,9 +229,15 @@ What that leaves for this file is where the answers live in the code:
   at the site with a link to the model rather than changing the code. `XmlConfigurator` and
   `XmlHierarchyConfigurator` carry these for the configuration-is-trusted paths, and
   `SystemStringFormat` for the format string.
-- `LocalSyslogAppender.EscapeNulCharacters` and `RemoteSyslogAppender.ValidateIdentity` are the two
+- `log4net.Appender.Internal.ContentEscape` and `RemoteSyslogAppender.ValidateIdentity` are the two
   sides of the content and structural-identifier rule: content is escaped and never rejected, a
   malformed identifier is reported rather than quietly repaired.
+- **A sink that cannot carry a character escapes it visibly, and never drops the character, the
+  rest of the record, or the event.** The escapes already in use are `\0` for NUL, `\r` and `\n`
+  for newlines, and `\uXXXX` for anything else, in `ContentEscape` and in
+  `RemoteSyslogAppender.AppendMessage`. Put new ones in `ContentEscape` rather than in the
+  appender: four appenders have needed the same two so far. Escaping before a length limit, not
+  after, since an escape is longer than what it replaces.
 - Deliberate secure-default choices belong in the changelog with their opt-out named, so that an
   upgrade surprise is searchable. See the entries for `SendTimeoutMillis`, `MatchTimeoutMillis` and
   `LockTimeoutMillis`.

src/changelog/3.5.0/315-eventlog-nul.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="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    escape NUL characters in `EventLogAppender` content. `ReportEventW` takes a null terminated
+ string, so a NUL in logged content ended the stored record there and silently dropped whatever
+ the layout rendered after it, exception text and trailing fields included (CWE-158). `WriteEntry`
+ raises nothing, so the record simply stored short. Measured on Windows 11 build 26200: of a 45
+ character message with a NUL at 23, the 23 character prefix was stored and the rest was gone
+ (audit da18b6fd-f007)
+  </description>
+</entry>

src/changelog/3.5.0/315-eventlog-size-budget.xml

@@ -0,0 +1,17 @@
+<?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">
+    stop `EventLogAppender` truncating to a size the event log then discards. The limit is a whole
+ record budget that the log name, the source and the machine name are spent from, so the fixed
+ 31837 was above the real ceiling: measured on Windows 11 build 26200, a record is stored while
+ `message + logName + applicationName` stays within 31736 characters, and one character beyond
+ that the service stores nothing and reports nothing. The whole event was lost rather than
+ shortened, and `applicationName` defaults to the app domain name, so a consumer with a long
+ assembly name lost more. The limit is now computed, and a truncation is reported through the
+ error handler, which is the only signal available (audit da18b6fd-f030)
+  </description>
+</entry>

src/changelog/3.5.0/315-local-syslog-newlines.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="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    escape the newlines in logged content in `LocalSyslogAppender`, which passed them to
+ `syslog(3)` unchanged. A daemon that writes the message through to a line oriented log then
+ records everything after the newline as its own entry, so content could forge an authentic
+ looking record (CWE-117). `NewLineHandling` mirrors the option of the same name on
+ `RemoteSyslogAppender`, which already escaped by default; set it to `Keep` for the previous
+ behaviour (audit da18b6fd-f008)
+  </description>
+</entry>

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/changelog/3.5.0/315-pickup-dir-unencodable-content.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">
+    stop one logging event destroying a whole `SmtpPickupDirAppender` batch. `File.CreateText`
+ throws on content it cannot encode, such as an unpaired surrogate, which abandoned every buffered
+ event and left a truncated mail in the pickup directory for the service to send. Such content is
+ now written as a `\uXXXX` escape (audit da18b6fd-f011)
+  </description>
+</entry>

src/changelog/3.5.0/315-syslog-newline-handling-type.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="changed">
+  <issue id="315" link="https://github.com/apache/logging-log4net/pull/315"/>
+  <description format="asciidoc">
+    move `SyslogNewLineHandling` out of `RemoteSyslogAppender` to `log4net.Appender`, now that
+ `LocalSyslogAppender` uses it too. Configuration files are unaffected, they bind the value by
+ name, but code naming `RemoteSyslogAppender.SyslogNewLineHandling` has to drop the prefix
+ (implemented by @FreeAndNil)
+  </description>
+</entry>

src/changelog/3.5.0/315-syslog-non-ascii.xml

@@ -0,0 +1,14 @@
+<?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 the characters `RemoteSyslogAppender` cannot send instead of deleting them. RFC 3164
+ allows only the visible ASCII characters and space, and everything else was dropped silently, so
+ `Sch&#246;nwetter &#20320;&#22909;` reached the collector as `Schnwetter ` and a tab disappeared
+ from between its neighbours. Such characters are now written as a `\uXXXX` escape, which keeps
+ the record inside the allowed range and readable (audit da18b6fd-f035)
+  </description>
+</entry>

src/changelog/3.5.0/315-telnet-unencodable-content.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">
+    stop one logging event disconnecting every `TelnetAppender` client. The default writer encoding
+ throws on content it cannot encode, such as an unpaired surrogate, and `Send` reads any failure as
+ a client that hung up. Unpaired surrogates are now written as a `\uXXXX` escape, as elsewhere
+ (audit da18b6fd-f013)
+  </description>
+</entry>

src/log4net.Tests/Appender/ContentEscapeTest.cs

@@ -0,0 +1,73 @@
+#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
+
+using System.Reflection;
+
+using log4net.Appender;
+
+using NUnit.Framework;
+
+namespace log4net.Tests.Appender;
+
+/// <summary>
+/// Tests for the internal <c>ContentEscape</c> helper.
+/// </summary>
+[TestFixture]
+public class ContentEscapeTest
+{
+  /// <summary>
+  /// An unpaired surrogate cannot be encoded, and an encoder that throws costs the event. The
+  /// input is built here rather than in the attribute: an attribute argument lives in metadata as
+  /// UTF-8, so the compiler would replace the surrogate with U+FFFD before the test ran.
+  /// </summary>
+  [TestCase(0xd800)]
+  [TestCase(0xdbff)]
+  [TestCase(0xdc00)]
+  [TestCase(0xdfff)]
+  public void UnpairedSurrogatesAreEscaped(int surrogate)
+  {
+    string input = "before" + (char)surrogate + "after";
+
+    Assert.That(EscapeUnpairedSurrogates(input), Is.EqualTo($@"before\u{surrogate:x4}after"));
+  }
+
+  /// <summary>Every one of them, not just the first.</summary>
+  [Test]
+  public void EveryUnpairedSurrogateIsEscaped()
+    => Assert.That(EscapeUnpairedSurrogates("a" + (char)0xd800 + "b" + (char)0xdc00 + "c"),
+      Is.EqualTo(@"a\ud800b\udc00c"));
+
+  /// <summary>A valid pair is one character and must survive untouched.</summary>
+  [Test]
+  public void ValidSurrogatePairsAreLeftAlone()
+    => Assert.That(EscapeUnpairedSurrogates("emoji \U0001F600 here"), Is.EqualTo("emoji \U0001F600 here"));
+
+  /// <summary>The common case takes a fast path that must not alter anything.</summary>
+  [TestCase("")]
+  [TestCase("plain ascii")]
+  [TestCase("Schönwetter 你好")]
+  public void MessagesWithoutSurrogatesAreUnchanged(string message)
+    => Assert.That(EscapeUnpairedSurrogates(message), Is.EqualTo(message));
+
+  private static string EscapeUnpairedSurrogates(string message)
+    => (string)typeof(TelnetAppender).Assembly
+      .GetType("log4net.Appender.Internal.ContentEscape")!
+      .GetMethod("EscapeUnpairedSurrogates", BindingFlags.Static | BindingFlags.NonPublic)!
+      .Invoke(null, [message])!;
+}

src/log4net.Tests/Appender/EventLogAppenderTest.cs

@@ -21,6 +21,7 @@
 #if NET462_OR_GREATER
 
 using System.Diagnostics;
+using System.Reflection;
 
 using log4net.Appender;
 using log4net.Core;
@@ -81,6 +82,79 @@ public void ActivateOptionsDisablesAppenderIfSourceDoesntExist()
     eventAppender.ActivateOptions();
     Assert.That(eventAppender.Threshold, Is.EqualTo(Level.Off));
   }
+
+  /// <summary>
+  /// ReportEventW takes a null terminated string, so a NUL in content ends the stored record
+  /// there and silently drops whatever the layout rendered after it. Measured on Windows 11
+  /// 26200: WriteEntry does not throw, and only the prefix is stored.
+  /// </summary>
+  [Test]
+  public void NulCharactersAreEscaped()
+    => Assert.That(PrepareEventText("before\0after", 100), Is.EqualTo("before\\0after"));
+
+  /// <summary>
+  /// The escape doubles each NUL, so it has to happen before the limit is applied. Escaping
+  /// afterwards would push a message near the limit back over it.
+  /// </summary>
+  [Test]
+  public void EscapingHappensBeforeTheLimitIsApplied()
+  {
+    const int maxSize = 4;
+
+    string prepared = PrepareEventText("\0\0\0", maxSize);
+
+    // Equality is ordinal, and pins the length and the absence of a NUL in one go. Escaping the
+    // three NULs gives six characters, so the limit has to cut it back to four.
+    Assert.That(prepared, Is.EqualTo(@"\0\0"));
+  }
+
+  /// <summary>A message within the limit and without a NUL comes through untouched.</summary>
+  [Test]
+  public void MessagesWithinTheLimitAreUnchanged()
+    => Assert.That(PrepareEventText("field=1\tfield=2", 100), Is.EqualTo("field=1\tfield=2"));
+
+  private static string PrepareEventText(string rendered, int maxSize)
+    => (string)typeof(EventLogAppender)
+      .GetMethod("PrepareEventText", BindingFlags.Static | BindingFlags.NonPublic)!
+      .Invoke(null, [rendered, maxSize])!;
+
+  /// <summary>
+  /// The limit is a whole record budget: the source is spent from it one character for one, so a
+  /// longer ApplicationName has to leave less room for the message.
+  /// </summary>
+  [Test]
+  public void TheSourceNameIsSpentFromTheMessageBudget()
+  {
+    const int difference = 44;
+    int shortSource = GetMaxMessageSize(new() { LogName = "Application", ApplicationName = "abc" });
+    int longSource = GetMaxMessageSize(new() { LogName = "Application", ApplicationName = new('a', 3 + difference) });
+
+    Assert.That(shortSource - longSource, Is.EqualTo(difference));
+  }
+
+  /// <summary>
+  /// And so is the log name, which is the half of the budget that was not expected.
+  /// </summary>
+  [Test]
+  public void TheLogNameIsSpentFromTheMessageBudgetToo()
+  {
+    const int difference = 7;
+    int shortLog = GetMaxMessageSize(new() { LogName = "Application", ApplicationName = "abc" });
+    int longLog = GetMaxMessageSize(new() { LogName = new('L', 11 + difference), ApplicationName = "abc" });
+
+    Assert.That(shortLog - longLog, Is.EqualTo(difference));
+  }
+
+  /// <summary>Names long enough to exhaust the budget must not produce a negative length.</summary>
+  [Test]
+  public void TheLimitNeverGoesBelowZero()
+    => Assert.That(GetMaxMessageSize(new() { LogName = new('L', 40000), ApplicationName = "abc" }),
+      Is.EqualTo(0));
+
+  private static int GetMaxMessageSize(EventLogAppender appender)
+    => (int)typeof(EventLogAppender)
+      .GetMethod("GetMaxMessageSize", BindingFlags.Instance | BindingFlags.NonPublic)!
+      .Invoke(appender, [])!;
 }
 
-#endif // NET462_OR_GREATER
+#endif // NET462_OR_GREATER

src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs

@@ -55,8 +55,7 @@ public void EveryNulCharacterIsEscaped()
     => Assert.That(EscapeNulCharacters("a\0b\0c"), Is.EqualTo("a\\0b\\0c"));
 
   /// <summary>
-  /// A message without a NUL character has to come through untouched, including the newlines an
-  /// exception layout produces: <c>syslog(3)</c> deals with those itself.
+  /// This escape is only about NUL. Newlines are <see cref="LocalSyslogAppender.NewLineHandling"/>.
   /// </summary>
   [Test]
   public void MessagesWithoutNulCharactersAreUnchanged()
@@ -114,8 +113,51 @@ private static IntPtr CurrentIdentityHandle()
       .GetField("_handleToIdentity", BindingFlags.Static | BindingFlags.NonPublic)!
       .GetValue(null)!;
 
-  private static string EscapeNulCharacters(string message)
+  /// <summary>
+  /// A newline ends the record for a daemon that writes the message through to a line oriented
+  /// log, so content could otherwise forge a second entry. glibc does not escape it.
+  /// </summary>
+  [Test]
+  public void NewLinesAreEscaped()
+    => Assert.That(EscapeNewLines("value\r\nJan  1 00:00:00 host sshd[1]: forged"),
+      Is.EqualTo("value\\r\\nJan  1 00:00:00 host sshd[1]: forged"));
+
+  /// <summary>Both characters count, on their own as well as paired.</summary>
+  [TestCase("a\rb", "a\\rb")]
+  [TestCase("a\nb", "a\\nb")]
+  [TestCase("a\n\nb", "a\\n\\nb")]
+  public void EveryNewLineIsEscaped(string message, string expected)
+    => Assert.That(EscapeNewLines(message), Is.EqualTo(expected));
+
+  /// <summary>A message without newlines takes the fast path and comes through untouched.</summary>
+  [Test]
+  public void MessagesWithoutNewLinesAreUnchanged()
+    => Assert.That(EscapeNewLines("field=1\tfield=2"), Is.EqualTo("field=1\tfield=2"));
+
+  /// <summary>Escaping is the default, because a daemon that splits the record is the common case.</summary>
+  [Test]
+  public void NewLineHandlingDefaultsToEscape()
+    => Assert.That(new LocalSyslogAppender().NewLineHandling,
+      Is.EqualTo(SyslogNewLineHandling.Escape));
+
+  /// <summary>One record per line, and a blank line is no record at all.</summary>
+  [Test]
+  public void SplittingDropsTheEmptyLines()
+    => Assert.That(SplitLines("first\r\nsecond\n\nthird\r"), Is.EqualTo(new[] { "first", "second", "third" }));
+
+  private static string EscapeNewLines(string message)
     => (string)typeof(LocalSyslogAppender)
+      .GetMethod("EscapeNewLines", BindingFlags.Static | BindingFlags.NonPublic)!
+      .Invoke(null, [message])!;
+
+  private static string[] SplitLines(string message)
+    => (string[])typeof(LocalSyslogAppender)
+      .GetMethod("SplitLines", BindingFlags.Static | BindingFlags.NonPublic)!
+      .Invoke(null, [message])!;
+
+  private static string EscapeNulCharacters(string message)
+    => (string)typeof(LocalSyslogAppender).Assembly
+      .GetType("log4net.Appender.Internal.ContentEscape")!
       .GetMethod("EscapeNulCharacters", BindingFlags.Static | BindingFlags.NonPublic)!
       .Invoke(null, [message])!;
 }

src/log4net.Tests/Appender/OutputDebugAppenderTest.cs

@@ -58,6 +58,31 @@ 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");
+
+    // Ordinal throughout: a culture sensitive comparison treats NUL as ignorable, so it reports a
+    // match in a string that has none.
+    Assert.That(lastDebugString, Contains.Substring("before\\0after").Using(StringComparison.Ordinal));
+    Assert.That(lastDebugString, !Contains.Substring("\0").Using(StringComparison.Ordinal));
+  }
 }
 
 file sealed class OutputAppender(Action<string> outputDebugString)