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 46582e5204e004bf26569242597acd8896752919 Author: Jan Friedrich <[email protected]> AuthorDate: Mon Aug 17 22:30:49 2026 +0200 report a RemoteSyslogAppender Identity that would split the record The Identity becomes the TAG of the syslog record and was appended verbatim, two lines before the message part goes through AppendMessage's filtering. A carriage return or line feed in the TAG ends the record, so the text after it is read as a record of its own with its own facility and severity. The TAG is a structural identifier and is expected to be a constant rather than a pattern rendering event data, so a malformed one is a configuration error. It is now reported through the ErrorHandler instead of being repaired quietly. The control characters are removed rather than the event being dropped. An Identity pattern that does render event data would otherwise give control over whether a record survives at all. Only control characters are removed. Identity defaults to the application friendly name, which may contain a space, and a space cannot split the record. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> --- .../3.4.0/309-report-malformed-syslog-identity.xml | 15 +++++ .../Appender/RemoteSyslogAppenderTest.cs | 71 ++++++++++++++++++++-- src/log4net/Appender/RemoteSyslogAppender.cs | 60 +++++++++++++++++- 3 files changed, 140 insertions(+), 6 deletions(-) diff --git a/src/changelog/3.4.0/309-report-malformed-syslog-identity.xml b/src/changelog/3.4.0/309-report-malformed-syslog-identity.xml new file mode 100644 index 00000000..6c57d4f6 --- /dev/null +++ b/src/changelog/3.4.0/309-report-malformed-syslog-identity.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"> + report a `RemoteSyslogAppender` `Identity` that renders control characters, and remove them, so + that it cannot split the record. The identity becomes the TAG of the syslog record and was appended + verbatim, while the message part is filtered, so a line feed in it let the text that followed be + read as a record of its own with an attacker chosen facility and severity. The TAG is a structural + identifier and expected to be a constant, so this is reported as the configuration error it is + rather than being repaired quietly (audit 1231d72-f016) + </description> +</entry> diff --git a/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs b/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs index 99b9c309..a9f6ed44 100644 --- a/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs +++ b/src/log4net.Tests/Appender/RemoteSyslogAppenderTest.cs @@ -26,6 +26,7 @@ using log4net.Core; using log4net.Layout; using log4net.Tests.Appender.Internal; +using log4net.Util; using NUnit.Framework; namespace log4net.Tests.Appender; @@ -117,15 +118,75 @@ public void RemoteSyslogNewLineHandlingSplitTest() Assert.That(Encoding.ASCII.GetString(sentBytes[1]), Is.EqualTo(expectedData1)); } + /// <summary> + /// The Identity becomes the TAG of the record. A control character in it would end the record, + /// so that the rest is read as a second record with its own facility and severity. + /// </summary> + [Test] + public void IdentityCannotSplitTheRecord() + { + List<byte[]> sentBytes = []; + // The malformed Identity is reported, which is expected here and should not clutter the output. + LogLog.ExecuteWithoutEmittingInternalMessages( + () => sentBytes = ExecuteAppend("Test message", identity: "app\r\n<34>sshd")); + + Assert.That(sentBytes, Has.Count.EqualTo(1)); + const string expectedData = "<14>app<34>sshd: INFO - Test message"; + Assert.That(Encoding.ASCII.GetString(sentBytes[0]), Is.EqualTo(expectedData)); + } + + /// <summary> + /// Removing the characters is not enough on its own: a malformed structural identifier is a + /// configuration error and has to be reported rather than quietly repaired. + /// </summary> + [Test] + [NonParallelizable] + public void IdentityWithControlCharactersIsReported() + { + List<LogLog> messages = []; + LogLog.ExecuteWithoutEmittingInternalMessages(() => + { + using LogLog.LogReceivedAdapter _ = new(messages); + ExecuteAppend("Test message", identity: "app\r\n<34>sshd"); + }); + + Assert.That(messages.ConvertAll(m => m.Message), + Has.Some.Contains("Identity of appender")); + } + + /// <summary> + /// An Identity without control characters has to reach the record untouched, including a space, + /// which the application friendly name used by default may well contain. + /// </summary> + [Test] + [NonParallelizable] + public void IdentityWithoutControlCharactersIsUnchangedAndNotReported() + { + List<LogLog> messages = []; + List<byte[]> sentBytes = []; + LogLog.ExecuteWithoutEmittingInternalMessages(() => + { + using LogLog.LogReceivedAdapter _ = new(messages); + sentBytes = ExecuteAppend("Test message", identity: "My App"); + }); + + Assert.That(sentBytes, Has.Count.EqualTo(1)); + const string expectedData = "<14>My App: INFO - Test message"; + Assert.That(Encoding.ASCII.GetString(sentBytes[0]), Is.EqualTo(expectedData)); + Assert.That(messages.ConvertAll(m => m.Message), Has.None.Contains("Identity of appender")); + } + private static List<byte[]> ExecuteAppend(string message, - RemoteSyslogAppender.SyslogNewLineHandling newLineHandling = default) + RemoteSyslogAppender.SyslogNewLineHandling newLineHandling = default, + string? identity = null) { System.Net.IPAddress ipAddress = new([127, 0, 0, 1]); - RemoteAppender appender = new() - { - RemoteAddress = ipAddress, + RemoteAppender appender = new() + { + RemoteAddress = ipAddress, Layout = new PatternLayout("%-5level - %message"), - NewLineHandling = newLineHandling + NewLineHandling = newLineHandling, + Identity = identity is null ? null : new PatternLayout(identity) }; appender.ActivateOptions(); LoggingEvent loggingEvent = new(new() diff --git a/src/log4net/Appender/RemoteSyslogAppender.cs b/src/log4net/Appender/RemoteSyslogAppender.cs index 8b5b21ea..3d30c526 100644 --- a/src/log4net/Appender/RemoteSyslogAppender.cs +++ b/src/log4net/Appender/RemoteSyslogAppender.cs @@ -364,7 +364,7 @@ protected override void Append(LoggingEvent loggingEvent) int priority = GeneratePriority(Facility, GetSeverity(loggingEvent.Level)); // Identity - string? identity = Identity?.Format(loggingEvent) ?? loggingEvent.Domain; + string? identity = ValidateIdentity(Identity?.Format(loggingEvent) ?? loggingEvent.Domain); // Message. The message goes after the tag/identity string message = RenderLoggingEvent(loggingEvent); @@ -400,6 +400,64 @@ protected override void Append(LoggingEvent loggingEvent) } } + /// <summary> + /// Checks that <paramref name="identity"/> is usable as the TAG part of a syslog record, and + /// reports it through the <see cref="AppenderSkeleton.ErrorHandler"/> when it is not. + /// </summary> + /// <param name="identity">The formatted <see cref="Identity"/>.</param> + /// <returns> + /// The identity, with any control character removed. + /// </returns> + /// <remarks> + /// <para> + /// The TAG is a structural identifier and therefore expected to be a constant chosen by the + /// developer or operator, not something derived from a logging event. A malformed one is a + /// configuration error rather than untrusted input, so it is reported instead of being altered + /// silently: a carriage return or line feed in the TAG would split the record and let the text + /// after it be read as a second record with its own facility and severity. + /// </para> + /// <para> + /// The offending characters are removed rather than the event being dropped, so that an + /// <see cref="Identity"/> pattern which does contain event data cannot be used to suppress + /// records. + /// </para> + /// </remarks> + private string? ValidateIdentity(string? identity) + { + if (identity is null) + { + return null; + } + + StringBuilder? sanitized = null; + for (int i = 0; i < identity.Length; i++) + { + // Control characters only. A carriage return or line feed ends the record, so the text + // after it is read as a record of its own. Printable characters are left alone, including + // the space that an application friendly name may contain, because they cannot break the + // record apart. + if (identity[i] is >= ' ' and not (char)127) + { + sanitized?.Append(identity[i]); + } + else + { + sanitized ??= new StringBuilder(identity.Length).Append(identity, 0, i); + } + } + + if (sanitized is null) + { + return identity; + } + + ErrorHandler.Error( + $"The Identity of appender [{Name}] rendered control characters, which would have split the syslog record, and they were removed. " + + "Identity is a structural identifier and is expected to be a constant rather than a pattern that renders logging event data."); + + return sanitized.ToString(); + } + /// <summary> /// Appends the rendered message to the buffer /// </summary>
