You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@logging.apache.org by fr...@apache.org on 2026/08/17 22:10:17 UTC
(logging-log4net) branch Feature/security-audit-hardening created (now e203b7c7)
This is an automated email from the ASF dual-hosted git repository.
FreeAndNil pushed a change to branch Feature/security-audit-hardening
in repository https://gitbox.apache.org/repos/asf/logging-log4net.git
at e203b7c7 remove the git-broadcast workflow
This branch includes the following new commits:
new ecd1b8b9 report the first appender error without log4net.Internal.Debug
new 9f5c9557 contain per-event failures in AdoNetAppender.SendBuffer
new 19fdb4a2 warn when AdoNetAppender executes layout-generated SQL
new 2fb4539f time out writes to stalled TelnetAppender clients
new e80b3810 redact the password when reporting a failed database connection
new 6bc3df34 document that configuration is trusted input
new bd35fe0d add a TransportSecurity option to the MailKit SmtpAppender
new eccb876e escape NUL characters in LocalSyslogAppender messages
new 46582e52 report a RemoteSyslogAppender Identity that would split the record
new 394fd3dc bound regular expression matching in the string match filters
new 86ecb152 flush TextWriterAppender under the appender lock
new 1786b139 keep the impersonated user name when a logging event is fixed
new 287fa9cd document that format strings are trusted developer input
new 9cc34d29 make the release verification scripts fail closed
new 360a1026 fix the lifetime of the LocalSyslogAppender identity
new 3fd97cb5 bound the waits for the file locking mutexes
new 15d16eed add a listen address to TelnetAppender
new 28d411a5 pin the Maven wrapper and distribution downloads
new e203b7c7 remove the git-broadcast workflow
The 19 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(logging-log4net) 14/19: make the release verification scripts fail closed
Posted by fr...@apache.org.
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 9cc34d29e31b985e8e7d028220d9235c83fc9c7a
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 23:30:16 2026 +0200
make the release verification scripts fail closed
verify-release.ps1 reported a SHA-512 mismatch with -ErrorAction Continue,
which overrides the script level $ErrorActionPreference, and never checked
the exit code of gpg, because $ErrorActionPreference does not apply to
native commands. A tampered artifact or a broken signature therefore still
reached Expand-Archive and the script exited 0. build-release.ps1 already
sets $PSNativeCommandUseErrorActionPreference with a comment explaining
this, so the trap was known.
Both scripts looped over whichever .asc files happened to be present, so
deleting them left nothing to verify and the scripts succeeded. The .sha512
files travel with the artifacts and can be regenerated, so they add nothing
on their own.
Verification is now driven from the artifacts: everything that is not a
hash, a signature or KEYS must have both, and an empty directory is an
error. That also catches a file added to the release.
Both scripts now import KEYS into a key ring of their own. Importing into
the default one accepted a signature from any key the machine already
trusted rather than only from a key in the Logging Services KEYS file.
Checked against a synthetic release signed with a throwaway key. Before,
the PowerShell script accepted a tampered artifact, a corrupted signature,
a deleted signature, an added unsigned file and a signature from an
untrusted key, and the shell script accepted the last three. After, all of
them are rejected and an untampered release still passes.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
scripts/verify-release.ps1 | 78 +++++++++++++++-------
scripts/verify-release.sh | 50 ++++++++++++--
.../3.4.0/309-verify-release-fails-closed.xml | 16 +++++
3 files changed, 113 insertions(+), 31 deletions(-)
diff --git a/scripts/verify-release.ps1 b/scripts/verify-release.ps1
index f99e4276..46e0ca02 100644
--- a/scripts/verify-release.ps1
+++ b/scripts/verify-release.ps1
@@ -5,52 +5,82 @@ Param (
Set-StrictMode -Version Latest
$ErrorActionPreference = 'Stop'
+# $ErrorActionPreference alone does not apply to native commands: gpg only sets $LASTEXITCODE, so
+# without this a failed signature check would still reach the extraction at the end and the script
+# would exit 0. Requires PowerShell 7.3+.
+$PSNativeCommandUseErrorActionPreference = $true
+
if (!$Directory)
{
$Directory = $PSScriptRoot
}
-function Verify-Hash
+function Assert-Hash
{
param
(
- [Parameter(Mandatory=$true, HelpMessage='The file containing the hash.')]
+ [Parameter(Mandatory=$true, HelpMessage='The artifact to check.')]
[System.IO.FileInfo]$File
- )
- $Line = @(Get-Content $File.FullName)[0]
- $Fields = $Line -split '\s+'
- $Hash = $Fields[0].Trim().ToUpper()
- $Filename = $Fields[1].Trim()
- if ($Filename.StartsWith("*"))
- {
- $Filename = $Filename.Substring(1).Trim()
- }
-
- $ComputedHash = (Get-FileHash -Algorithm 'SHA512' "$($File.DirectoryName)/$Filename").Hash.ToUpperInvariant()
+ )
- if($Hash -eq $ComputedHash)
+ $HashFile = "$($File.FullName).sha512"
+ if (!(Test-Path $HashFile))
{
- "$($Filename): Passed"
+ throw "$($File.Name): no $($File.Name).sha512 to check it against"
}
- else
+
+ $Hash = (@(Get-Content $HashFile)[0] -split '\s+')[0].Trim().ToUpperInvariant()
+ $ComputedHash = (Get-FileHash -Algorithm 'SHA512' $File.FullName).Hash.ToUpperInvariant()
+ if ($Hash -ne $ComputedHash)
{
- Write-Error "$($Filename): Not Passed" -ErrorAction Continue
- Write-Error "Read from file: $Hash" -ErrorAction Continue
- Write-Error "Computed: $ComputedHash" -ErrorAction Continue
+ throw "$($File.Name): SHA-512 mismatch, read $Hash but computed $ComputedHash"
}
+
+ "$($File.Name): hash ok"
+}
+
+# Everything that is not a hash, a signature or the key file has to be covered by both. Driving the
+# checks from the artifacts, rather than from the .sha512 and .asc files that happen to be present,
+# is what turns a missing signature into a failure instead of one loop iteration fewer.
+$Artifacts = @(Get-ChildItem $Directory -File |
+ Where-Object { $_.Extension -notin '.asc', '.sha512' -and $_.Name -ne 'KEYS' })
+
+if ($Artifacts.Count -eq 0)
+{
+ throw "No artifacts to verify in $Directory"
}
-foreach ($File in Get-ChildItem $Directory *.sha512)
+foreach ($Artifact in $Artifacts)
{
- Verify-Hash $File
+ Assert-Hash $Artifact
}
Invoke-WebRequest https://downloads.apache.org/logging/KEYS -OutFile $Directory/KEYS
-gpg --import -q $Directory/KEYS
-foreach ($File in Get-ChildItem $Directory *.asc)
+# A key ring of its own, holding only the downloaded KEYS. Importing into the default key ring
+# would accept a signature from any key this machine already has, not only from a key in the
+# Logging Services KEYS file.
+$KeyringDirectory = New-Item -ItemType Directory -Path (Join-Path ([System.IO.Path]::GetTempPath()) ([guid]::NewGuid()))
+try
+{
+ $Keyring = Join-Path $KeyringDirectory 'logging-keys.gpg'
+ gpg --no-default-keyring --keyring $Keyring --batch --quiet --import $Directory/KEYS
+
+ foreach ($Artifact in $Artifacts)
+ {
+ $Signature = "$($Artifact.FullName).asc"
+ if (!(Test-Path $Signature))
+ {
+ throw "$($Artifact.Name): no $($Artifact.Name).asc to verify it with"
+ }
+
+ gpg --no-default-keyring --keyring $Keyring --batch --verify $Signature $Artifact.FullName
+ "$($Artifact.Name): signature ok"
+ }
+}
+finally
{
- gpg --verify $File
+ Remove-Item $KeyringDirectory -Recurse -Force -ErrorAction SilentlyContinue
}
Expand-Archive $Directory/*source*.zip -DestinationPath $Directory/src
diff --git a/scripts/verify-release.sh b/scripts/verify-release.sh
index d1b3b9b8..70bdd1ac 100644
--- a/scripts/verify-release.sh
+++ b/scripts/verify-release.sh
@@ -1,26 +1,62 @@
#!/bin/bash
-set -e
+set -euo pipefail
if ! which unzip >/dev/null 2>&1; then
echo "The 'unzip' utility is required, but was not found in your path" >&2
exit 1
fi
-TARGET_DIR="$1";
+TARGET_DIR="${1:-}"
if test -z "$TARGET_DIR"; then
TARGET_DIR="$(pwd)"
fi
+cd "$TARGET_DIR"
-sha512sum --check *.sha512
+# Everything that is not a hash, a signature or the key file has to be covered by both. Driving the
+# checks from the artifacts, rather than from the .sha512 and .asc files that happen to be present,
+# is what turns a missing signature into a failure instead of one loop iteration fewer.
+shopt -s nullglob
+artifacts=()
+for file in *; do
+ case "$file" in
+ *.asc|*.sha512|KEYS) continue ;;
+ esac
+ test -f "$file" || continue
+ artifacts+=("$file")
+done
+
+if test ${#artifacts[@]} -eq 0; then
+ echo "No artifacts to verify in $TARGET_DIR" >&2
+ exit 1
+fi
+
+for file in "${artifacts[@]}"; do
+ if test ! -f "$file.sha512"; then
+ echo "$file: no $file.sha512 to check it against" >&2
+ exit 1
+ fi
+ sha512sum --check "$file.sha512"
+done
wget https://downloads.apache.org/logging/KEYS
-gpg --import -q KEYS
-for f in $(find "$TARGET_DIR" -iname '*.asc'); do
- gpg --verify "$f"
+
+# A key ring of its own, holding only the downloaded KEYS. Importing into the default key ring
+# would accept a signature from any key this machine already has, not only from a key in the
+# Logging Services KEYS file.
+keyring_dir="$(mktemp -d)"
+trap 'rm -rf "$keyring_dir"' EXIT
+gpg --no-default-keyring --keyring "$keyring_dir/logging-keys.gpg" --batch --quiet --import KEYS
+
+for file in "${artifacts[@]}"; do
+ if test ! -f "$file.asc"; then
+ echo "$file: no $file.asc to verify it with" >&2
+ exit 1
+ fi
+ gpg --no-default-keyring --keyring "$keyring_dir/logging-keys.gpg" --batch --verify "$file.asc" "$file"
done
mkdir -p src
cd src
unzip -q -o ../*source*.zip
-cd src
\ No newline at end of file
+cd src
diff --git a/src/changelog/3.4.0/309-verify-release-fails-closed.xml b/src/changelog/3.4.0/309-verify-release-fails-closed.xml
new file mode 100644
index 00000000..8f6f02a0
--- /dev/null
+++ b/src/changelog/3.4.0/309-verify-release-fails-closed.xml
@@ -0,0 +1,16 @@
+<?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">
+ make the release verification scripts fail closed. `verify-release.ps1` reported a SHA-512
+ mismatch with `-ErrorAction Continue` and never checked the exit code of `gpg`, so it extracted the
+ archive and exited 0 for a tampered artifact or a broken signature. Both scripts looped over
+ whichever `.asc` files were present, so deleting them left nothing to verify and the scripts
+ succeeded. The checks are now driven from the artifacts, each of which must have a `.sha512` and a
+ `.asc`, and the keys are imported into a key ring of their own so that a signature from any other
+ key this machine trusts is no longer accepted (audit 1231d72-f009, 1231d72-f010)
+ </description>
+</entry>
(logging-log4net) 09/19: report a RemoteSyslogAppender Identity that would split the record
Posted by fr...@apache.org.
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 <fr...@apache.org>
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) <no...@anthropic.com>
---
.../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>
(logging-log4net) 02/19: contain per-event failures in AdoNetAppender.SendBuffer
Posted by fr...@apache.org.
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 9f5c955787ddeedf49792a4dd89156c40af87576
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 21:29:30 2026 +0200
contain per-event failures in AdoNetAppender.SendBuffer
Events are removed from the CyclicBuffer by PopAll before SendBuffer runs,
so they cannot be retried later. Neither ExecuteNonQuery loop contained
per-event failures, and with UseTransactions (the default) a single event
the provider rejects - npgsql refuses U+0000, for example - rolled back
the whole batch of up to 512 events, including the ones logged before it.
An attacker who gets one such byte logged per flush window could suppress
the database audit trail indefinitely.
Without a transaction each event is now reported and skipped individually.
In transaction mode the exception still has to propagate, so the events
are retried one by one after the rollback; only the events the database
actually rejects are lost.
This makes delivery at-least-once: if the batch failed after the database
had already applied some statements, those events are written again.
Duplicates are preferred over losing the whole buffer.
Log4NetTransaction.Dispose threw NotImplementedException, which log4net
swallowed in DoAppend, so no test ever exercised the rollback path. Real
providers roll back on Dispose rather than throwing.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../309-contain-per-event-adonet-failures.xml | 15 +++
.../Appender/AdoNet/Log4NetCommand.cs | 34 ++++++
.../Appender/AdoNet/Log4NetTransaction.cs | 16 +--
src/log4net.Tests/Appender/AdoNetAppenderTest.cs | 67 +++++++++++
src/log4net/Appender/AdoNetAppender.cs | 130 ++++++++++++++++-----
5 files changed, 228 insertions(+), 34 deletions(-)
diff --git a/src/changelog/3.4.0/309-contain-per-event-adonet-failures.xml b/src/changelog/3.4.0/309-contain-per-event-adonet-failures.xml
new file mode 100644
index 00000000..6cef7661
--- /dev/null
+++ b/src/changelog/3.4.0/309-contain-per-event-adonet-failures.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">
+ stop a single logging event that the database rejects from discarding the whole buffer in
+ `AdoNetAppender`. The events have already been removed from the buffer when they are sent, so a
+ rolled back transaction lost up to `BufferSize` unrelated events, which an attacker could trigger
+ repeatedly with content the provider refuses, such as `U+0000` on npgsql (CWE-778). Delivery
+ becomes at-least-once: events already applied by a batch that then failed may be written again
+ (audit 1231d72-f004)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Appender/AdoNet/Log4NetCommand.cs b/src/log4net.Tests/Appender/AdoNet/Log4NetCommand.cs
index 161d4923..fee95a5d 100644
--- a/src/log4net.Tests/Appender/AdoNet/Log4NetCommand.cs
+++ b/src/log4net.Tests/Appender/AdoNet/Log4NetCommand.cs
@@ -20,6 +20,7 @@
*/
using System;
+using System.Collections.Generic;
using System.Data;
namespace log4net.Tests.Appender.AdoNet;
@@ -42,12 +43,45 @@ public void Dispose()
public int ExecuteNonQuery()
{
+ string? payload = null;
+ foreach (object? parameter in Parameters)
+ {
+ if (parameter is IDataParameter { Value: string value })
+ {
+ payload = value;
+ break;
+ }
+ }
+ payload ??= CommandText;
+
+ if (ExceptionTrigger is not null
+ && payload?.IndexOf(ExceptionTrigger, StringComparison.Ordinal) >= 0)
+ {
+ throw new InvalidOperationException($"Simulated database rejection of [{payload}]");
+ }
+
ExecuteNonQueryCount++;
+ if (payload is not null)
+ {
+ ExecutedPayloads.Add(payload);
+ }
return 0;
}
public int ExecuteNonQueryCount { get; private set; }
+ /// <summary>
+ /// When set, <see cref="ExecuteNonQuery"/> throws for every command whose payload
+ /// contains this string, simulating a database that rejects specific content.
+ /// </summary>
+ public static string? ExceptionTrigger { get; set; }
+
+ /// <summary>
+ /// The payload - the first string parameter value, or the command text when there are no
+ /// parameters - of every successful <see cref="ExecuteNonQuery"/> across all instances.
+ /// </summary>
+ public static List<string> ExecutedPayloads { get; } = [];
+
public IDbDataParameter CreateParameter() => new Log4NetParameter();
#pragma warning disable CS8766 // Nullability of reference types in return type doesn't match implicitly implemented member (possibly because of nullability attributes).
diff --git a/src/log4net.Tests/Appender/AdoNet/Log4NetTransaction.cs b/src/log4net.Tests/Appender/AdoNet/Log4NetTransaction.cs
index 27b3bb9c..81afb218 100644
--- a/src/log4net.Tests/Appender/AdoNet/Log4NetTransaction.cs
+++ b/src/log4net.Tests/Appender/AdoNet/Log4NetTransaction.cs
@@ -26,19 +26,21 @@ namespace log4net.Tests.Appender.AdoNet;
internal sealed class Log4NetTransaction : IDbTransaction
{
+ /// <inheritdoc/>
public void Commit()
- {
- // empty
- }
+ { }
+ /// <inheritdoc/>
public void Rollback()
- {
- // empty
- }
+ { }
+ /// <inheritdoc/>
public IDbConnection Connection => throw new NotImplementedException();
+ /// <inheritdoc/>
public IsolationLevel IsolationLevel => throw new NotImplementedException();
- public void Dispose() => throw new NotImplementedException();
+ /// <inheritdoc/>
+ public void Dispose()
+ { }
}
diff --git a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs
index 17b4119e..c42560d4 100644
--- a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs
+++ b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs
@@ -264,6 +264,73 @@ public void BufferingWebsiteExample()
Assert.That(param.Value, Is.Empty);
}
+ /// <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
+ /// be written even though they shared a transaction with the rejected event.
+ /// </summary>
+ [Test]
+ [NonParallelizable]
+ public void RejectedEventDoesNotDiscardTheRestOfTheBuffer()
+ {
+ try
+ {
+ Log4NetCommand.ExceptionTrigger = "POISON";
+ Log4NetCommand.ExecutedPayloads.Clear();
+
+ XmlDocument log4NetConfig = new();
+ log4NetConfig.LoadXml(
+ """
+ <log4net>
+ <appender name="AdoNetAppender" type="log4net.Appender.AdoNetAppender">
+ <bufferSize value="3" />
+ <useTransactions value="true" />
+ <connectionType value="log4net.Tests.Appender.AdoNet.Log4NetConnection" />
+ <connectionString value="data source=[database server]" />
+ <commandText value="INSERT INTO Log ([Message]) VALUES (@message)" />
+ <parameter>
+ <parameterName value="@message" />
+ <dbType value="String" />
+ <size value="4000" />
+ <layout type="log4net.Layout.PatternLayout">
+ <conversionPattern value="%message" />
+ </layout>
+ </parameter>
+ </appender>
+ <root>
+ <level value="ALL" />
+ <appender-ref ref="AdoNetAppender" />
+ </root>
+ </log4net>
+ """);
+
+ ILoggerRepository rep = LogManager.CreateRepository(Guid.NewGuid().ToString());
+ XmlConfigurator.Configure(rep, log4NetConfig["log4net"]!);
+ ILog log = LogManager.GetLogger(rep.Name, "RejectedEventDoesNotDiscardTheRestOfTheBuffer");
+
+ // The appender reports the rejected event through its ErrorHandler; that is expected
+ // here and should not clutter the test output.
+ LogLog.ExecuteWithoutEmittingInternalMessages(() =>
+ {
+ log.Debug("before");
+ log.Debug("a POISON message");
+ log.Debug("after one");
+ // The fourth event overflows the buffer of 3 and flushes all four events.
+ log.Debug("after two");
+ });
+
+ Assert.That(Log4NetCommand.ExecutedPayloads, Has.Member("before"));
+ Assert.That(Log4NetCommand.ExecutedPayloads, Has.Member("after one"));
+ Assert.That(Log4NetCommand.ExecutedPayloads, Has.Member("after two"));
+ Assert.That(Log4NetCommand.ExecutedPayloads, Has.No.Member("a POISON message"));
+ }
+ finally
+ {
+ Log4NetCommand.ExceptionTrigger = null;
+ Log4NetCommand.ExecutedPayloads.Clear();
+ }
+ }
+
[Test]
public void NullPropertyXmlConfig()
{
diff --git a/src/log4net/Appender/AdoNetAppender.cs b/src/log4net/Appender/AdoNetAppender.cs
index 65d3e546..886c975d 100644
--- a/src/log4net/Appender/AdoNetAppender.cs
+++ b/src/log4net/Appender/AdoNetAppender.cs
@@ -394,6 +394,8 @@ protected override void OnClose()
/// </remarks>
protected override void SendBuffer(LoggingEvent[] events)
{
+ events.EnsureNotNull();
+
if (ReconnectOnError && (Connection is null || Connection.State != ConnectionState.Open))
{
LogLog.Debug(_declaringType, $"Attempting to reconnect to database. Current Connection State: {((Connection is null) ? SystemInfo.NullText : Connection.State.ToString())}");
@@ -406,30 +408,45 @@ protected override void SendBuffer(LoggingEvent[] events)
{
if (UseTransactions)
{
+ bool retryPerEvent = false;
+
// Create transaction
// NJC - Do this on 2 lines because it can confuse the debugger
- using IDbTransaction dbTran = Connection.BeginTransaction();
- try
- {
- SendBuffer(dbTran, events);
-
- // commit transaction
- dbTran.Commit();
- }
- catch (Exception ex) when (!ex.IsFatal())
+ using (IDbTransaction dbTran = Connection.BeginTransaction())
{
- // rollback the transaction
try
{
- dbTran.Rollback();
+ SendBuffer(dbTran, events);
+
+ // commit transaction
+ dbTran.Commit();
}
- catch (Exception inner) when (!inner.IsFatal())
+ catch (Exception ex) when (!ex.IsFatal())
{
- // Ignore exception
+ // rollback the transaction
+ try
+ {
+ dbTran.Rollback();
+ }
+ catch (Exception inner) when (!inner.IsFatal())
+ {
+ // Ignore exception
+ }
+
+ // Can't insert into the database. That's a bad thing
+ ErrorHandler.Error("Exception while writing to database", ex);
+
+ retryPerEvent = true;
}
+ }
- // Can't insert into the database. That's a bad thing
- ErrorHandler.Error("Exception while writing to database", ex);
+ // The events have already been removed from the buffer, so a rolled back
+ // transaction would lose all of them - including the events logged before the one
+ // the database rejected. Retry them one by one, outside the failed transaction,
+ // so that only the events the database actually rejects are lost.
+ if (retryPerEvent)
+ {
+ SendBufferPerEvent(events);
}
}
else
@@ -493,15 +510,26 @@ protected virtual void SendBuffer(IDbTransaction? dbTran, LoggingEvent[] events)
// run for all events
foreach (LoggingEvent e in events)
{
- // No need to clear dbCmd.Parameters, just use existing.
- // Set the parameter values
- foreach (AdoNetAppenderParameter param in m_parameters)
+ try
{
- param.FormatValue(dbCmd, e);
- }
+ // No need to clear dbCmd.Parameters, just use existing.
+ // Set the parameter values
+ foreach (AdoNetAppenderParameter param in m_parameters)
+ {
+ param.FormatValue(dbCmd, e);
+ }
- // Execute the query
- dbCmd.ExecuteNonQuery();
+ // Execute the query
+ dbCmd.ExecuteNonQuery();
+ }
+ catch (Exception ex) when (dbTran is null && !ex.IsFatal())
+ {
+ // Without a transaction every event stands alone, so an event the database
+ // rejects must not stop the remaining events from being written. In transaction
+ // mode the exception has to propagate - the transaction is in a failed state -
+ // and SendBuffer retries the events individually after the rollback.
+ ErrorHandler.Error("Exception while writing a logging event to the database. Continuing with the remaining events.", ex);
+ }
}
}
else
@@ -515,13 +543,61 @@ protected virtual void SendBuffer(IDbTransaction? dbTran, LoggingEvent[] events)
// run for all events
foreach (LoggingEvent e in events)
{
- // Get the command text from the Layout
- string logStatement = GetLogStatement(e);
+ try
+ {
+ // Get the command text from the Layout
+ string logStatement = GetLogStatement(e);
- LogLog.Debug(_declaringType, $"LogStatement [{logStatement}]");
+ LogLog.Debug(_declaringType, $"LogStatement [{logStatement}]");
- dbCmd.CommandText = logStatement;
- dbCmd.ExecuteNonQuery();
+ dbCmd.CommandText = logStatement;
+ dbCmd.ExecuteNonQuery();
+ }
+ catch (Exception ex) when (dbTran is null && !ex.IsFatal())
+ {
+ // See the parameterized path above: contain per-event failures outside transactions.
+ ErrorHandler.Error("Exception while writing a logging event to the database. Continuing with the remaining events.", ex);
+ }
+ }
+ }
+ }
+
+ /// <summary>
+ /// Writes each event with its own command, so that an event the database rejects only
+ /// loses itself.
+ /// </summary>
+ /// <param name="events">The events to insert into the database.</param>
+ /// <remarks>
+ /// <para>
+ /// Used as the fallback after a transactional batch failed and was rolled back. The
+ /// events are sent without a transaction and failures are reported to the
+ /// <see cref="AppenderSkeleton.ErrorHandler"/> without affecting the remaining events.
+ /// </para>
+ /// <para>
+ /// Note that this makes delivery at-least-once rather than exactly-once: if the batch
+ /// failed after the database had already applied some of its statements - for example
+ /// when the commit itself failed but the rollback did not take effect - those events are
+ /// written a second time here. Duplicated events are preferred over silently losing the
+ /// whole buffer.
+ /// </para>
+ /// </remarks>
+ private void SendBufferPerEvent(LoggingEvent[] events)
+ {
+ foreach (LoggingEvent e in events)
+ {
+ if (Connection is not { State: ConnectionState.Open })
+ {
+ // The connection failed rather than a single event - nothing more can be written.
+ return;
+ }
+
+ try
+ {
+ SendBuffer(null, [e]);
+ }
+ catch (Exception ex) when (!ex.IsFatal())
+ {
+ ErrorHandler.Error("Exception while writing a logging event to the database. The event has been dropped.", ex);
}
}
}
(logging-log4net) 08/19: escape NUL characters in LocalSyslogAppender messages
Posted by fr...@apache.org.
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 eccb876ea4c07c3f3080835b7141b71077b12321
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 22:23:24 2026 +0200
escape NUL characters in LocalSyslogAppender messages
The rendered message is marshaled to libc as a null-terminated string, so a
NUL character anywhere in it ended the record there and silently dropped
everything the layout rendered after it, including trailing fields and
exception text. Logged content is not trusted and a NUL in it is an
in-scope input, so an attacker who gets one logged could hide the tail of
every record.
Confirmed with the same marshalling the appender uses: for a 24 character
message with a NUL in the middle, libc sees 13 characters.
NUL is now escaped as \0. Other control characters are still passed
through, because syslog(3) encodes them itself and newlines are needed for
the multi-line output an exception layout produces. RemoteSyslogAppender
drops unprintable characters instead, which would lose the stack traces
this appender is expected to carry.
Also switches the single statement tests added for the send timeout to
expression bodies.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../309-escape-nul-in-local-syslog-messages.xml | 14 ++++
.../Appender/LocalSyslogAppenderTest.cs | 78 ++++++++++++++++++++++
src/log4net.Tests/Appender/TelnetAppenderTest.cs | 6 +-
src/log4net/Appender/LocalSyslogAppender.cs | 22 +++++-
4 files changed, 114 insertions(+), 6 deletions(-)
diff --git a/src/changelog/3.4.0/309-escape-nul-in-local-syslog-messages.xml b/src/changelog/3.4.0/309-escape-nul-in-local-syslog-messages.xml
new file mode 100644
index 00000000..1ab3b993
--- /dev/null
+++ b/src/changelog/3.4.0/309-escape-nul-in-local-syslog-messages.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="309" link="https://github.com/apache/logging-log4net/pull/309"/>
+ <description format="asciidoc">
+ stop a NUL character in logged content from truncating `LocalSyslogAppender` records. The
+ message is marshaled to libc as a null-terminated string, so everything the layout rendered after
+ the NUL was silently dropped, including trailing fields and exception text (CWE-158). NUL is now
+ escaped as `\0`; other control characters are still passed through, because `syslog(3)` encodes
+ them and newlines are needed for multi-line exception output (audit 1231d72-f008)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs
new file mode 100644
index 00000000..ff8cb8de
--- /dev/null
+++ b/src/log4net.Tests/Appender/LocalSyslogAppenderTest.cs
@@ -0,0 +1,78 @@
+#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 <see cref="LocalSyslogAppender"/>
+/// </summary>
+/// <remarks>
+/// <para>
+/// The appender itself writes through <c>syslog(3)</c>, whose output cannot be read back from a
+/// test, so these tests cover the message preparation that happens before the native call.
+/// </para>
+/// </remarks>
+[TestFixture]
+public class LocalSyslogAppenderTest
+{
+ /// <summary>
+ /// The message is marshaled to libc as a null-terminated string, so a NUL character in logged
+ /// content would end the record there and drop everything the layout rendered after it.
+ /// </summary>
+ [Test]
+ public void NulCharactersAreEscaped()
+ => Assert.That(EscapeNulCharacters("priority=high\0user=alice"), Is.EqualTo("priority=high\\0user=alice"));
+
+ /// <summary>
+ /// Several NUL characters must all be escaped, not just the first one.
+ /// </summary>
+ [Test]
+ 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.
+ /// </summary>
+ [Test]
+ public void MessagesWithoutNulCharactersAreUnchanged()
+ {
+ const string message = "System.InvalidOperationException: boom\r\n at Program.Main()\tfield=1";
+
+ Assert.That(EscapeNulCharacters(message), Is.EqualTo(message));
+ }
+
+ /// <summary>
+ /// An empty message takes the fast path, which must not turn it into anything else.
+ /// </summary>
+ [Test]
+ public void EmptyMessageIsUnchanged()
+ => Assert.That(EscapeNulCharacters(string.Empty), Is.Empty);
+
+ private static string EscapeNulCharacters(string message)
+ => (string)typeof(LocalSyslogAppender)
+ .GetMethod("EscapeNulCharacters", BindingFlags.Static | BindingFlags.NonPublic)!
+ .Invoke(null, [message])!;
+}
diff --git a/src/log4net.Tests/Appender/TelnetAppenderTest.cs b/src/log4net.Tests/Appender/TelnetAppenderTest.cs
index cde2231d..e5c238c3 100644
--- a/src/log4net.Tests/Appender/TelnetAppenderTest.cs
+++ b/src/log4net.Tests/Appender/TelnetAppenderTest.cs
@@ -135,11 +135,7 @@ void WaitForReceived(string what, string expected)
/// </summary>
[Test]
public void SendTimeoutMillisDefaultsToAFiniteValue()
- {
- TelnetAppender appender = new();
-
- Assert.That(appender.SendTimeoutMillis, Is.EqualTo(5000));
- }
+ => Assert.That(new TelnetAppender().SendTimeoutMillis, Is.EqualTo(5000));
/// <summary>
/// 0 is the documented opt-out that restores blocking indefinitely; a negative timeout has no
diff --git a/src/log4net/Appender/LocalSyslogAppender.cs b/src/log4net/Appender/LocalSyslogAppender.cs
index df481c46..abdbbcb3 100644
--- a/src/log4net/Appender/LocalSyslogAppender.cs
+++ b/src/log4net/Appender/LocalSyslogAppender.cs
@@ -331,13 +331,33 @@ public override void ActivateOptions()
protected override void Append(LoggingEvent loggingEvent)
{
int priority = GeneratePriority(Facility, GetSeverity(loggingEvent.EnsureNotNull().Level));
- string message = RenderLoggingEvent(loggingEvent);
+ string message = EscapeNulCharacters(RenderLoggingEvent(loggingEvent));
// Call the local libc syslog method
// The second argument is a printf style format string
NativeMethods.syslog(priority, "%s", message);
}
+ /// <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>
+ /// Other control characters are left alone: <c>syslog(3)</c> encodes them itself, and newlines
+ /// are needed for the multi-line output an exception layout produces.
+ /// </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>
(logging-log4net) 01/19: report the first appender error without log4net.Internal.Debug
Posted by fr...@apache.org.
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 ecd1b8b9e26c8b4a5414d67c757b1ffc692b1355
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 21:29:19 2026 +0200
report the first appender error without log4net.Internal.Debug
OnlyOnceErrorHandler.FirstError only forwarded to LogLog when
LogLog.InternalDebugging was set, which is off by default. Since every
appender uses this handler by default, an appender that stopped
delivering events did so completely silently: no stderr line and no
LogLog.LogReceived event, and the handler disables itself afterwards.
The condition was redundant anyway: LogLog.Error already checks
LogLog.QuietMode (log4net.Internal.Quiet) and EmitInternalMessages, so
both documented ways of silencing internal messages keep working.
Upgrade note: previously invisible appender errors are now visible, so a
misconfigured appender emits one log4net:ERROR line on stderr.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../309-report-first-appender-error-by-default.xml | 14 +++
src/log4net.Tests/Util/OnlyOnceErrorHandlerTest.cs | 121 +++++++++++++++++++++
src/log4net/Util/OnlyOnceErrorHandler.cs | 9 +-
3 files changed, 140 insertions(+), 4 deletions(-)
diff --git a/src/changelog/3.4.0/309-report-first-appender-error-by-default.xml b/src/changelog/3.4.0/309-report-first-appender-error-by-default.xml
new file mode 100644
index 00000000..6d372d61
--- /dev/null
+++ b/src/changelog/3.4.0/309-report-first-appender-error-by-default.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="changed">
+ <issue id="309" link="https://github.com/apache/logging-log4net/pull/309"/>
+ <description format="asciidoc">
+ report the first error of an appender even when `log4net.Internal.Debug` is off, which is the
+ default. `OnlyOnceErrorHandler` is the default error handler of every appender, so an appender
+ that stopped delivering events previously did so without leaving any trace (CWE-778).
+ `log4net.Internal.Quiet` and `LogLog.EmitInternalMessages` remain the ways to silence internal
+ messages (audit 1231d72-f019)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Util/OnlyOnceErrorHandlerTest.cs b/src/log4net.Tests/Util/OnlyOnceErrorHandlerTest.cs
new file mode 100644
index 00000000..a2731edb
--- /dev/null
+++ b/src/log4net.Tests/Util/OnlyOnceErrorHandlerTest.cs
@@ -0,0 +1,121 @@
+#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.Collections.Generic;
+
+using log4net.Util;
+
+using NUnit.Framework;
+
+namespace log4net.Tests.Util;
+
+/// <summary>
+/// Used for internal unit testing the <see cref="OnlyOnceErrorHandler"/> class.
+/// </summary>
+[TestFixture]
+public class OnlyOnceErrorHandlerTest
+{
+ /// <summary>
+ /// The first error must reach <see cref="LogLog"/> even when
+ /// <see cref="LogLog.InternalDebugging"/> is off, which is the default. An appender that
+ /// stops delivering events would otherwise leave no trace at all.
+ /// </summary>
+ [Test]
+ [NonParallelizable]
+ public void FirstErrorIsEmittedWithoutInternalDebugging()
+ {
+ bool internalDebugging = LogLog.InternalDebugging;
+ LogLog.InternalDebugging = false;
+ try
+ {
+ List<LogLog> messages = [];
+ LogLog.ExecuteWithoutEmittingInternalMessages(() =>
+ {
+ using LogLog.LogReceivedAdapter _ = new(messages);
+ new OnlyOnceErrorHandler("TestAppender").Error("Something went wrong");
+ });
+
+ Assert.That(messages, Has.Count.EqualTo(1));
+ Assert.That(messages[0].Message, Does.Contain("Something went wrong"));
+ }
+ finally
+ {
+ LogLog.InternalDebugging = internalDebugging;
+ }
+ }
+
+ /// <summary>
+ /// Only the first error is reported: the handler disables itself afterwards so that a
+ /// repeatedly failing appender cannot flood the internal log.
+ /// </summary>
+ [Test]
+ [NonParallelizable]
+ public void OnlyTheFirstErrorIsEmitted()
+ {
+ bool internalDebugging = LogLog.InternalDebugging;
+ LogLog.InternalDebugging = false;
+ try
+ {
+ List<LogLog> messages = [];
+ OnlyOnceErrorHandler handler = new("TestAppender");
+ LogLog.ExecuteWithoutEmittingInternalMessages(() =>
+ {
+ using LogLog.LogReceivedAdapter _ = new(messages);
+ handler.Error("First failure");
+ handler.Error("Second failure");
+ handler.Error("Third failure");
+ });
+
+ Assert.That(messages, Has.Count.EqualTo(1));
+ Assert.That(messages[0].Message, Does.Contain("First failure"));
+ Assert.That(handler.IsEnabled, Is.False);
+ }
+ finally
+ {
+ LogLog.InternalDebugging = internalDebugging;
+ }
+ }
+
+ /// <summary>
+ /// <see cref="LogLog.QuietMode"/> (the <c>log4net.Internal.Quiet</c> setting) remains the
+ /// documented way to silence internal messages, including appender errors.
+ /// </summary>
+ [Test]
+ [NonParallelizable]
+ public void QuietModeSuppressesTheError()
+ {
+ bool quietMode = LogLog.QuietMode;
+ LogLog.QuietMode = true;
+ try
+ {
+ List<LogLog> messages = [];
+ LogLog.ExecuteWithoutEmittingInternalMessages(() =>
+ {
+ using LogLog.LogReceivedAdapter _ = new(messages);
+ new OnlyOnceErrorHandler("TestAppender").Error("Something went wrong");
+ });
+
+ Assert.That(messages, Is.Empty);
+ }
+ finally
+ {
+ LogLog.QuietMode = quietMode;
+ }
+ }
+}
diff --git a/src/log4net/Util/OnlyOnceErrorHandler.cs b/src/log4net/Util/OnlyOnceErrorHandler.cs
index d0c9ebe2..909529b2 100644
--- a/src/log4net/Util/OnlyOnceErrorHandler.cs
+++ b/src/log4net/Util/OnlyOnceErrorHandler.cs
@@ -114,10 +114,11 @@ public virtual void FirstError(string message, Exception? e, ErrorCode errorCode
ErrorMessage = message;
IsEnabled = false;
- if (LogLog.InternalDebugging && !LogLog.QuietMode)
- {
- LogLog.Error(_declaringType, "[" + _prefix + "] ErrorCode: " + errorCode.ToString() + ". " + message, e);
- }
+ // Emit the first error unconditionally so that an appender which silently stopped
+ // delivering events leaves a trace in a default configuration. LogLog.Error already
+ // honors LogLog.QuietMode (log4net.Internal.Quiet) and LogLog.EmitInternalMessages,
+ // which remain the documented ways to silence internal messages.
+ LogLog.Error(_declaringType, "[" + _prefix + "] ErrorCode: " + errorCode.ToString() + ". " + message, e);
}
/// <summary>
(logging-log4net) 10/19: bound regular expression matching in the string match filters
Posted by fr...@apache.org.
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 394fd3dc8316de38bf013b23b86fa07fbfdf0e71
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 22:41:45 2026 +0200
bound regular expression matching in the string match filters
RegexToMatch was compiled with Regex.InfiniteMatchTimeout, and the match
runs while the appender lock is held, so a pattern that backtracks could
stall everything logging through the appender on some inputs.
Matching now stops after MatchTimeoutMillis, 1000 by default, and 0 restores
the previous unbounded behaviour. An abandoned match counts as no match, so
the event is left to the rest of the filter chain rather than having its
decision changed.
The pattern comes from configuration and is trusted, so this is hardening
against a pattern that turns out to be expensive, not protection against
untrusted input.
StringMatchFilter and PropertyFilter both matched the regex themselves, so
the handling lives in one protected IsRegexMatch used by both, which also
covers MdcFilter and NdcFilter. It reports an abandoned match once per
filter rather than once per event, since a warning per event would be a
problem of its own.
regexToMatch was missing from the manual entirely and is documented now.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../3.4.0/309-bound-filter-regex-matching.xml | 17 +++
src/log4net.Tests/Filter/StringMatchFilterTest.cs | 128 +++++++++++++++++++++
src/log4net/Filter/PropertyFilter.cs | 2 +-
src/log4net/Filter/StringMatchFilter.cs | 87 +++++++++++++-
.../ROOT/pages/manual/configuration/filters.adoc | 23 ++++
5 files changed, 252 insertions(+), 5 deletions(-)
diff --git a/src/changelog/3.4.0/309-bound-filter-regex-matching.xml b/src/changelog/3.4.0/309-bound-filter-regex-matching.xml
new file mode 100644
index 00000000..5b721997
--- /dev/null
+++ b/src/changelog/3.4.0/309-bound-filter-regex-matching.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="changed">
+ <issue id="309" link="https://github.com/apache/logging-log4net/pull/309"/>
+ <description format="asciidoc">
+ give `RegexToMatch` matching a deadline in `StringMatchFilter` and the filters deriving from it,
+ `PropertyFilter`, `MdcFilter` and `NdcFilter`. The pattern was matched with
+ `Regex.InfiniteMatchTimeout` while the appender lock was held, so a pattern that backtracks could
+ stall everything logging through the appender on some inputs. Matching now stops after
+ `MatchTimeoutMillis`, 1000 by default, and an abandoned match is reported once and leaves the event
+ to the rest of the filter chain; 0 restores unbounded matching. The pattern comes from
+ configuration and is trusted, so this is hardening rather than a vulnerability fix
+ (audit 1231d72-f013)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Filter/StringMatchFilterTest.cs b/src/log4net.Tests/Filter/StringMatchFilterTest.cs
new file mode 100644
index 00000000..ff3eced6
--- /dev/null
+++ b/src/log4net.Tests/Filter/StringMatchFilterTest.cs
@@ -0,0 +1,128 @@
+#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;
+using System.Collections.Generic;
+using System.Diagnostics;
+
+using log4net.Core;
+using log4net.Filter;
+using log4net.Util;
+
+using NUnit.Framework;
+
+namespace log4net.Tests.Filter;
+
+/// <summary>
+/// Tests for <see cref="StringMatchFilter"/>
+/// </summary>
+[TestFixture]
+public class StringMatchFilterTest
+{
+ /// <summary>
+ /// A pattern that backtracks, with an input that makes it do so. Matching this without a deadline
+ /// runs for longer than any test would wait.
+ /// </summary>
+ private const string CatastrophicPattern = "^(a+)+$";
+
+ private const string CraftedMessage = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa!";
+
+ /// <summary>
+ /// The match runs while the appender lock is held, so one that backtracks has to be abandoned
+ /// rather than holding the thread. The event is then left to the rest of the filter chain.
+ /// </summary>
+ [Test]
+ public void AMatchThatBacktracksIsAbandoned()
+ {
+ StringMatchFilter filter = new() { RegexToMatch = CatastrophicPattern, MatchTimeoutMillis = 100 };
+ filter.ActivateOptions();
+
+ Stopwatch stopwatch = Stopwatch.StartNew();
+ FilterDecision decision = FilterDecision.Accept;
+ LogLog.ExecuteWithoutEmittingInternalMessages(() => decision = filter.Decide(CreateEvent(CraftedMessage)));
+ stopwatch.Stop();
+
+ Assert.That(decision, Is.EqualTo(FilterDecision.Neutral));
+ Assert.That(stopwatch.Elapsed, Is.LessThan(TimeSpan.FromSeconds(30)));
+ }
+
+ /// <summary>
+ /// Abandoning the match must not be silent, but it must also not report once per event: the
+ /// condition repeats for every event that reaches the filter.
+ /// </summary>
+ [Test]
+ [NonParallelizable]
+ public void AnAbandonedMatchIsReportedOnce()
+ {
+ StringMatchFilter filter = new() { RegexToMatch = CatastrophicPattern, MatchTimeoutMillis = 100 };
+ filter.ActivateOptions();
+
+ List<LogLog> messages = [];
+ LogLog.ExecuteWithoutEmittingInternalMessages(() =>
+ {
+ using LogLog.LogReceivedAdapter _ = new(messages);
+ filter.Decide(CreateEvent(CraftedMessage));
+ filter.Decide(CreateEvent(CraftedMessage));
+ filter.Decide(CreateEvent(CraftedMessage));
+ });
+
+ Assert.That(
+ messages.ConvertAll(m => m.Message).FindAll(m => m.IndexOf("was abandoned", StringComparison.Ordinal) >= 0),
+ Has.Count.EqualTo(1));
+ }
+
+ /// <summary>
+ /// A pattern that does not backtrack has to keep working, with the decision unchanged.
+ /// </summary>
+ [Test]
+ public void AMatchingPatternStillDecides()
+ {
+ StringMatchFilter filter = new() { RegexToMatch = "cat", AcceptOnMatch = true };
+ filter.ActivateOptions();
+
+ Assert.That(filter.Decide(CreateEvent("the cat sat")), Is.EqualTo(FilterDecision.Accept));
+ Assert.That(filter.Decide(CreateEvent("the dog sat")), Is.EqualTo(FilterDecision.Neutral));
+ }
+
+ /// <summary>
+ /// The deadline has to be finite by default, so that an expensive pattern cannot hold the
+ /// appender lock indefinitely without the operator having opted into that.
+ /// </summary>
+ [Test]
+ public void MatchTimeoutMillisDefaultsToAFiniteValue()
+ => Assert.That(new StringMatchFilter().MatchTimeoutMillis, Is.EqualTo(1000));
+
+ /// <summary>
+ /// 0 is the documented opt-out that restores unbounded matching; a negative deadline has no
+ /// meaning and is rejected rather than being reinterpreted.
+ /// </summary>
+ [Test]
+ public void MatchTimeoutMillisRejectsNegativeValuesButAllowsZero()
+ {
+ StringMatchFilter filter = new();
+
+ Assert.That(() => filter.MatchTimeoutMillis = -1, Throws.TypeOf<ArgumentOutOfRangeException>());
+
+ filter.MatchTimeoutMillis = 0;
+ Assert.That(filter.MatchTimeoutMillis, Is.EqualTo(0));
+ }
+
+ private static LoggingEvent CreateEvent(string message)
+ => new(new LoggingEventData { Level = Level.Info, Message = message, LoggerName = "TestLogger" });
+}
diff --git a/src/log4net/Filter/PropertyFilter.cs b/src/log4net/Filter/PropertyFilter.cs
index cef49c75..c487682b 100644
--- a/src/log4net/Filter/PropertyFilter.cs
+++ b/src/log4net/Filter/PropertyFilter.cs
@@ -95,7 +95,7 @@ public override FilterDecision Decide(LoggingEvent loggingEvent)
if (m_regexToMatch is not null)
{
// Check the regex
- if (m_regexToMatch.Match(msg).Success == false)
+ if (!IsRegexMatch(msg))
{
// No match, continue processing
return FilterDecision.Neutral;
diff --git a/src/log4net/Filter/StringMatchFilter.cs b/src/log4net/Filter/StringMatchFilter.cs
index e7819913..d0c79c42 100644
--- a/src/log4net/Filter/StringMatchFilter.cs
+++ b/src/log4net/Filter/StringMatchFilter.cs
@@ -17,6 +17,7 @@
//
#endregion
+using System;
using System.Text.RegularExpressions;
using log4net.Core;
@@ -53,14 +54,92 @@ public class StringMatchFilter : FilterSkeleton
/// <see cref="ActivateOptions"/> must be called again.
/// </para>
/// </remarks>
- public override void ActivateOptions()
+ public override void ActivateOptions()
{
if (RegexToMatch is not null)
{
- m_regexToMatch = new(RegexToMatch, RegexOptions.Compiled);
+ m_regexToMatch = new(RegexToMatch, RegexOptions.Compiled,
+ _matchTimeoutMillis == 0
+ ? Regex.InfiniteMatchTimeout
+ : TimeSpan.FromMilliseconds(_matchTimeoutMillis));
}
}
+ /// <summary>
+ /// Gets or sets the time, in milliseconds, that matching <see cref="RegexToMatch"/> against a
+ /// single event may take before the match is abandoned.
+ /// </summary>
+ /// <value>
+ /// A positive number of milliseconds, or 0 to let a match run for as long as it takes.
+ /// </value>
+ /// <remarks>
+ /// <para>
+ /// A regular expression that backtracks can take a very long time on some inputs. The pattern is
+ /// matched while the appender lock is held, so an unbounded match would stall everything logging
+ /// through the appender, and matching is therefore given a deadline. A match that reaches it is
+ /// treated as no match, leaving the rest of the filter chain to decide.
+ /// </para>
+ /// <para>
+ /// The pattern comes from configuration and is trusted, so this is a guard against a pattern that
+ /// turns out to be expensive rather than protection against untrusted input.
+ /// </para>
+ /// <para>
+ /// The default value is 1000 (one second). Setting the value to 0 restores unbounded matching and
+ /// is not recommended. Changing it takes effect when <see cref="ActivateOptions"/> is called.
+ /// </para>
+ /// </remarks>
+ /// <exception cref="ArgumentOutOfRangeException">The value specified is negative.</exception>
+ public int MatchTimeoutMillis
+ {
+ get => _matchTimeoutMillis;
+ set
+ {
+ if (value < 0)
+ {
+ throw SystemInfo.CreateArgumentOutOfRangeException(nameof(value), value,
+ "The value specified for MatchTimeoutMillis is negative.");
+ }
+ _matchTimeoutMillis = value;
+ }
+ }
+
+ private int _matchTimeoutMillis = 1000;
+ private bool _matchTimeoutReported;
+
+ /// <summary>
+ /// Matches <paramref name="value"/> against <see cref="m_regexToMatch"/>.
+ /// </summary>
+ /// <param name="value">The text to match.</param>
+ /// <returns>
+ /// <see langword="true"/> when the pattern matches, and <see langword="false"/> when it does not
+ /// or when matching took longer than <see cref="MatchTimeoutMillis"/>.
+ /// </returns>
+ protected bool IsRegexMatch(string value)
+ {
+ try
+ {
+ return m_regexToMatch!.IsMatch(value);
+ }
+ catch (RegexMatchTimeoutException)
+ {
+ if (!_matchTimeoutReported)
+ {
+ // Once per filter. The condition repeats for every event that reaches it, and a warning
+ // per event would be a denial of service of its own.
+ _matchTimeoutReported = true;
+ LogLog.Warn(_declaringType,
+ $"Matching the pattern [{RegexToMatch}] took longer than {MatchTimeoutMillis}ms and was abandoned, so the event was not filtered by it. "
+ + "A pattern that backtracks can take arbitrarily long on some inputs; consider rewriting it or raising MatchTimeoutMillis.");
+ }
+ return false;
+ }
+ }
+
+ /// <summary>
+ /// The fully qualified type of the <see cref="StringMatchFilter"/> class.
+ /// </summary>
+ private static readonly Type _declaringType = typeof(StringMatchFilter);
+
/// <summary>
/// <see cref="FilterDecision.Accept"/> when matching <see cref="StringToMatch"/> or <see cref="RegexToMatch"/>
/// </summary>
@@ -144,11 +223,11 @@ public override FilterDecision Decide(LoggingEvent loggingEvent)
if (m_regexToMatch is not null)
{
// Check the regex
- if (m_regexToMatch.Match(msg).Success == false)
+ if (!IsRegexMatch(msg))
{
// No match, continue processing
return FilterDecision.Neutral;
- }
+ }
// we've got a match
if (AcceptOnMatch)
diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/filters.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/filters.adoc
index 5f2e07a6..0c34f84c 100644
--- a/src/site/antora/modules/ROOT/pages/manual/configuration/filters.adoc
+++ b/src/site/antora/modules/ROOT/pages/manual/configuration/filters.adoc
@@ -94,3 +94,26 @@ The following filters are defined in the log4net package:
|log4net.Filter.StringMatchFilter
|Matches events containing a specific substring in the message.
|===
+
+[#filters-regex-timeout]
+== Matching with a regular expression
+
+`StringMatchFilter`, `PropertyFilter`, `MdcFilter` and `NdcFilter` accept a `regexToMatch` instead
+of a `stringToMatch`.
+
+A regular expression that backtracks can take a very long time on some inputs, and the match runs
+while the appender lock is held.
+Matching is therefore given a deadline of one second, configurable with `matchTimeoutMillis`.
+A match that reaches the deadline is abandoned, the filter reports it once and returns `Neutral`,
+and the remaining filters decide the event.
+Prefer a pattern that cannot backtrack; the deadline is a safety net, not a substitute.
+
+[source,xml]
+----
+<filter type="log4net.Filter.StringMatchFilter">
+ <regexToMatch value="user=[a-z0-9._-]+@example\.com" />
+ <matchTimeoutMillis value="1000" />
+</filter>
+----
+
+Setting `matchTimeoutMillis` to `0` lets a match run for as long as it takes and is not recommended.
(logging-log4net) 04/19: time out writes to stalled TelnetAppender clients
Posted by fr...@apache.org.
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 2fb4539f5c78f7037061265346f3992f54bcad67
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 21:42:58 2026 +0200
time out writes to stalled TelnetAppender clients
Clients are written to synchronously while the appender lock is held and no
Socket.SendTimeout was set anywhere, so a client that connects and then
stops reading let TCP flow control fill its receive window and the server
send buffer. The next write blocked forever and every thread logging
through the appender queued behind it. The existing eviction only fires on
a thrown exception, and a blocked write never throws.
Accepted sockets now get a finite SendTimeout, configurable through the new
SendTimeoutMillis property and defaulting to 5000. A timed-out write throws
and the client is evicted like any other dead connection. Setting the
property to 0 restores the previous unbounded behavior.
SocketHandler gained a (port, sendTimeoutMillis) overload rather than an
optional parameter, so the existing (port) signature keeps working for
subclasses; it maps to 0 to preserve its old semantics.
Writes stay synchronous under the appender lock, so several stalled clients
still cost up to the timeout each. Moving the sends to a bounded per-client
queue would remove that entirely and is left as a follow-up.
TelnetAppender had no page in the manual, which is added here, including
that it is a diagnostic tool for trusted networks and that the connecting
client is trusted, like any other appender destination.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../3.4.0/309-telnet-appender-send-timeout.xml | 15 ++++
src/log4net.Tests/Appender/TelnetAppenderTest.cs | 31 ++++++++
src/log4net/Appender/TelnetAppender.cs | 74 +++++++++++++++++-
src/site/antora/modules/ROOT/nav.adoc | 1 +
.../ROOT/pages/manual/configuration/appenders.adoc | 3 +-
.../configuration/appenders/telnetappender.adoc | 88 ++++++++++++++++++++++
6 files changed, 209 insertions(+), 3 deletions(-)
diff --git a/src/changelog/3.4.0/309-telnet-appender-send-timeout.xml b/src/changelog/3.4.0/309-telnet-appender-send-timeout.xml
new file mode 100644
index 00000000..e5813cc7
--- /dev/null
+++ b/src/changelog/3.4.0/309-telnet-appender-send-timeout.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">
+ stop a Telnet client that connects and then stops reading from suspending all logging.
+ `TelnetAppender` writes to its clients while the appender lock is held and set no
+ `Socket.SendTimeout`, so once TCP flow control filled the client's receive window the next write
+ blocked forever and every thread logging through the appender queued behind it (CWE-833). Writes
+ now time out after `SendTimeoutMillis` (5000 by default) and the client is disconnected; 0
+ restores the previous unbounded behavior (audit 1231d72-f001)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Appender/TelnetAppenderTest.cs b/src/log4net.Tests/Appender/TelnetAppenderTest.cs
index e8cf95ec..cde2231d 100644
--- a/src/log4net.Tests/Appender/TelnetAppenderTest.cs
+++ b/src/log4net.Tests/Appender/TelnetAppenderTest.cs
@@ -129,6 +129,37 @@ void WaitForReceived(string what, string expected)
}
}
+ /// <summary>
+ /// Writes to a client block while the appender lock is held, so the send timeout has to be
+ /// finite by default - otherwise a client that stops reading suspends all logging.
+ /// </summary>
+ [Test]
+ public void SendTimeoutMillisDefaultsToAFiniteValue()
+ {
+ TelnetAppender appender = new();
+
+ Assert.That(appender.SendTimeoutMillis, Is.EqualTo(5000));
+ }
+
+ /// <summary>
+ /// 0 is the documented opt-out that restores blocking indefinitely; a negative timeout has no
+ /// meaning for <see cref="Socket.SendTimeout"/> and is rejected instead of being silently
+ /// reinterpreted.
+ /// </summary>
+ [Test]
+ public void SendTimeoutMillisRejectsNegativeValuesButAllowsZero()
+ {
+ TelnetAppender appender = new();
+
+ Assert.That(() => appender.SendTimeoutMillis = -1, Throws.TypeOf<ArgumentOutOfRangeException>());
+
+ appender.SendTimeoutMillis = 0;
+ Assert.That(appender.SendTimeoutMillis, Is.EqualTo(0));
+
+ appender.SendTimeoutMillis = 250;
+ Assert.That(appender.SendTimeoutMillis, Is.EqualTo(250));
+ }
+
/// <summary>
/// Asks the OS for a currently unused TCP port - a fixed port would collide with
/// other tests or processes on the build machine.
diff --git a/src/log4net/Appender/TelnetAppender.cs b/src/log4net/Appender/TelnetAppender.cs
index adb4cdae..e62e9346 100644
--- a/src/log4net/Appender/TelnetAppender.cs
+++ b/src/log4net/Appender/TelnetAppender.cs
@@ -40,6 +40,13 @@ namespace log4net.Appender;
/// <para>
/// The default <see cref="Port"/> is 23 (the telnet port).
/// </para>
+/// <para>
+/// This appender is a diagnostic tool for trusted networks. As with any other appender
+/// destination, the connecting client is trusted: enabling the appender declares that whoever can
+/// reach the port may read the application's log, so no authentication is performed and the stream
+/// is not encrypted. Keeping untrusted parties away from the port is the operator's
+/// responsibility, exactly as it is for a log file.
+/// </para>
/// </remarks>
/// <author>Keith Long</author>
/// <author>Nicko Cadell</author>
@@ -47,6 +54,7 @@ public class TelnetAppender : AppenderSkeleton
{
private SocketHandler? _handler;
private int _listeningPort = 23;
+ private int _sendTimeoutMillis = 5_000;
/// <summary>
/// The fully qualified type of the TelnetAppender class.
@@ -85,6 +93,42 @@ public int Port
}
}
+ /// <summary>
+ /// Gets or sets the time, in milliseconds, that a write to a client may block before that
+ /// client is treated as dead and disconnected.
+ /// </summary>
+ /// <value>
+ /// A positive number of milliseconds, or 0 to block indefinitely.
+ /// </value>
+ /// <remarks>
+ /// <para>
+ /// Clients are written to synchronously while the appender lock is held, so a client that
+ /// connects and then stops reading lets TCP flow control fill its receive window and the
+ /// server send buffer. Without a timeout the next write blocks forever and suspends every
+ /// thread that logs through this appender.
+ /// </para>
+ /// <para>
+ /// The default value is 5000 (5 seconds). A write that exceeds it fails with a
+ /// <see cref="SocketException"/>, and the client is then disconnected like any other dead
+ /// connection. Setting the value to 0 restores the previous behavior of blocking
+ /// indefinitely and is not recommended.
+ /// </para>
+ /// </remarks>
+ /// <exception cref="ArgumentOutOfRangeException">The value specified is negative.</exception>
+ public int SendTimeoutMillis
+ {
+ get => _sendTimeoutMillis;
+ set
+ {
+ if (value < 0)
+ {
+ throw SystemInfo.CreateArgumentOutOfRangeException(nameof(value), value,
+ "The value specified for SendTimeoutMillis is negative.");
+ }
+ _sendTimeoutMillis = value;
+ }
+ }
+
/// <summary>
/// Overrides the parent method to close the socket handler
/// </summary>
@@ -115,7 +159,7 @@ public override void ActivateOptions()
try
{
LogLog.Debug(_declaringType, $"Creating SocketHandler to listen on port [{_listeningPort}]");
- _handler = new SocketHandler(_listeningPort);
+ _handler = new SocketHandler(_listeningPort, _sendTimeoutMillis);
}
catch (Exception ex)
{
@@ -151,6 +195,7 @@ protected class SocketHandler : IDisposable
private const int MaxConnections = 20;
private readonly Socket _serverSocket;
+ private readonly int _sendTimeoutMillis;
private readonly List<SocketClient> _clients = [];
private readonly object _syncRoot = new();
private bool _wasDisposed;
@@ -238,11 +283,28 @@ public void Dispose()
/// <param name="port">the local port to listen on for connections</param>
/// <remarks>
/// <para>
- /// Creates a socket handler on the specified local server port.
+ /// Creates a socket handler on the specified local server port, blocking indefinitely on
+ /// clients that stop reading. Prefer <see cref="SocketHandler(int, int)"/>.
/// </para>
/// </remarks>
public SocketHandler(int port)
+ : this(port, 0)
+ { }
+
+ /// <summary>
+ /// Opens a new server port on <paramref ref="port"/>
+ /// </summary>
+ /// <param name="port">the local port to listen on for connections</param>
+ /// <param name="sendTimeoutMillis">the time, in milliseconds, that a write to a client may
+ /// block before that client is disconnected, or 0 to block indefinitely</param>
+ /// <remarks>
+ /// <para>
+ /// Creates a socket handler on the specified local server port.
+ /// </para>
+ /// </remarks>
+ public SocketHandler(int port, int sendTimeoutMillis)
{
+ _sendTimeoutMillis = sendTimeoutMillis;
_serverSocket = new(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
_serverSocket.Bind(new IPEndPoint(IPAddress.Any, port));
_serverSocket.Listen(5);
@@ -332,6 +394,14 @@ private void OnConnect(IAsyncResult asyncResult)
// Block until a client connects
Socket socket = _serverSocket.EndAccept(asyncResult);
LogLog.Debug(_declaringType, $"Accepting connection from [{socket.RemoteEndPoint}]");
+ if (_sendTimeoutMillis > 0)
+ {
+ // Bound how long a write to this client can block. Clients are written to while the
+ // appender lock is held, so without a timeout a client that stops reading suspends
+ // every thread that logs. A timed-out write throws and the client is then evicted
+ // like any other dead connection.
+ socket.SendTimeout = _sendTimeoutMillis;
+ }
SocketClient client = new(socket);
// clients.Count is an atomic read that can be done outside the lock.
diff --git a/src/site/antora/modules/ROOT/nav.adoc b/src/site/antora/modules/ROOT/nav.adoc
index 8aadc37d..c6f993ee 100644
--- a/src/site/antora/modules/ROOT/nav.adoc
+++ b/src/site/antora/modules/ROOT/nav.adoc
@@ -38,6 +38,7 @@
**** xref:manual/configuration/appenders/rollingfileappender.adoc[]
**** xref:manual/configuration/appenders/smtpappender.adoc[]
**** xref:manual/configuration/appenders/smtppickupdirappender.adoc[]
+**** xref:manual/configuration/appenders/telnetappender.adoc[]
**** xref:manual/configuration/appenders/traceappender.adoc[]
**** xref:manual/configuration/appenders/udpappender.adoc[]
*** xref:manual/configuration/filters.adoc[]
diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders.adoc
index e2e0c18a..d5cf4bae 100644
--- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders.adoc
+++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders.adoc
@@ -97,8 +97,9 @@ The MailKit based appender from the `log4net.Ext.Mail` package is recommended; t
|xref:manual/configuration/appenders/smtppickupdirappender.adoc[]
|Sends logging events to an email address but writes the emails to a configurable directory rather than sending them directly via SMTP.
-|TelnetAppender
+|xref:manual/configuration/appenders/telnetappender.adoc[]
|*Clients* connect via Telnet to receive logging events.
+The connection is unauthenticated and unencrypted.
|xref:manual/configuration/appenders/traceappender.adoc[]
|Writes logging events to the .NET trace system (https://web.archive.org/web/20240907024634/https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.trace?view=net-8.0[System.Diagnostics.Trace]).
diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc
new file mode 100644
index 00000000..27891b57
--- /dev/null
+++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc
@@ -0,0 +1,88 @@
+////
+ 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.
+////
+
+[#telnetappender]
+= TelnetAppender
+
+The `TelnetAppender` listens for incoming TCP connections and streams rendered log events to
+every connected client, so that a running application's log can be watched over a socket with a
+telnet client.
+Unlike every other appender, it does not write to a destination you configure: it accepts
+connections from clients that reach it.
+It is intended for diagnostic use on trusted networks -- see <<telnetappender-trust>>.
+
+At most 20 clients may be connected at the same time; further connection attempts are answered
+with a message and closed.
+
+The following example configures the appender to listen on port 8023.
+
+[source,xml]
+----
+<appender name="TelnetAppender" type="log4net.Appender.TelnetAppender">
+ <port value="8023" />
+ <sendTimeoutMillis value="5000" />
+ <layout type="log4net.Layout.PatternLayout">
+ <conversionPattern value="%date %-5level %logger - %message%newline" />
+ </layout>
+</appender>
+----
+
+[#telnetappender-settings]
+== Settings
+
+`port`::
+The TCP port to listen on.
+The default is `23`, the telnet port.
+
+`sendTimeoutMillis`::
+How long, in milliseconds, a write to a client may block before that client is treated as dead
+and disconnected.
+The default is `5000`.
++
+Clients are written to synchronously while the appender lock is held, so a client that connects
+and then stops reading lets TCP flow control fill its receive window.
+A finite timeout bounds how long that client can hold up the threads that log through this
+appender.
+Setting the value to `0` restores blocking indefinitely and is not recommended.
+
+[#telnetappender-trust]
+== Intended use and trust model
+
+This appender is a *diagnostic tool for trusted networks*.
+It is meant for watching the log of a running application during development or while
+investigating a problem, not as a general-purpose logging destination.
+
+Like every other appender destination, the connecting client is *trusted*: by enabling the
+appender the operator declares that whoever can reach the port is allowed to read the
+application's log.
+The appender therefore performs no authentication of its own.
+
+[WARNING]
+====
+The connection is *unauthenticated* and *unencrypted*, and the appender listens on *all network
+interfaces*.
+There is no option to restrict the listen address, require a credential, or enable TLS.
+
+Any client that can reach the port receives the full rendered log stream, including whatever the
+layout renders -- user names, session identifiers, request parameters, stack traces.
+Keeping untrusted parties away from the port is the operator's responsibility, exactly as it is
+for a log file:
+
+* Only enable this appender on a trusted network.
+* Restrict access to the port with a host firewall or network policy.
+* Prefer it for local or short-lived diagnostics rather than as a permanent logging destination.
+====
(logging-log4net) 18/19: pin the Maven wrapper and distribution downloads
Posted by fr...@apache.org.
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 28d411a53b7f61b0da7f9324181e6a23f926cd39
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Tue Aug 18 00:02:34 2026 +0200
pin the Maven wrapper and distribution downloads
mvnw and MavenWrapperDownloader both refuse to run when a download does not
match a checksum, but neither wrapperSha256Sum nor distributionSha256Sum was
set, so the enforcement never ran and whatever the URLs returned was
executed.
wrapperSha256Sum is of the maven-wrapper.jar committed next to the
properties, which is byte identical to the published maven-wrapper-3.2.0.jar.
distributionSha256Sum is of apache-maven-3.9.0-bin.zip from
archive.apache.org, whose SHA-512 matches the published value, which is byte
identical to the copy on Maven Central, and whose PGP signature verifies
against https://downloads.apache.org/maven/KEYS.
Both enforcement paths were exercised: with the checksums correct mvnw runs
Maven 3.9.0, and with either one altered it refuses.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.mvn/wrapper/maven-wrapper.properties | 7 +++++++
src/changelog/3.4.0/309-pin-maven-wrapper-checksums.xml | 14 ++++++++++++++
2 files changed, 21 insertions(+)
diff --git a/.mvn/wrapper/maven-wrapper.properties b/.mvn/wrapper/maven-wrapper.properties
index 08ea486a..09c1213c 100644
--- a/.mvn/wrapper/maven-wrapper.properties
+++ b/.mvn/wrapper/maven-wrapper.properties
@@ -16,3 +16,10 @@
# under the License.
distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.0/apache-maven-3.9.0-bin.zip
wrapperUrl=https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.2.0/maven-wrapper-3.2.0.jar
+# mvnw and MavenWrapperDownloader refuse to run when a download does not match these.
+# distributionSha256Sum is of apache-maven-3.9.0-bin.zip as published on archive.apache.org, whose
+# PGP signature verifies against https://downloads.apache.org/maven/KEYS and whose bytes are
+# identical to the Maven Central copy above. wrapperSha256Sum is of the maven-wrapper.jar committed
+# next to this file, which is identical to the published maven-wrapper-3.2.0.jar.
+distributionSha256Sum=68e5a1745a5f5e4b0dfae051f83297e2ea40912b2c3b84d3b7420f463f39260d
+wrapperSha256Sum=e63a53cfb9c4d291ebe3c2b0edacb7622bbc480326beaa5a0456e412f52f066a
diff --git a/src/changelog/3.4.0/309-pin-maven-wrapper-checksums.xml b/src/changelog/3.4.0/309-pin-maven-wrapper-checksums.xml
new file mode 100644
index 00000000..b4ce40cb
--- /dev/null
+++ b/src/changelog/3.4.0/309-pin-maven-wrapper-checksums.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="added">
+ <issue id="309" link="https://github.com/apache/logging-log4net/pull/309"/>
+ <description format="asciidoc">
+ pin the Maven wrapper and the Maven distribution it downloads with
+ `wrapperSha256Sum` and `distributionSha256Sum`. `mvnw` and `MavenWrapperDownloader` already refuse
+ to run when a download does not match, but neither property was set, so whatever the URLs returned
+ was executed (CWE-494). Both values were taken from artifacts whose PGP signature verifies against
+ the Apache Maven KEYS (audit 1231d72-f023)
+ </description>
+</entry>
(logging-log4net) 11/19: flush TextWriterAppender under the appender lock
Posted by fr...@apache.org.
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 86ecb1526ff9fbdb94b823a27f4e8f49cf250952
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 22:50:58 2026 +0200
flush TextWriterAppender under the appender lock
Flush synchronized on a private object while Append runs under the lock
taken by DoAppend, so a flush could run concurrently with a write to the
same QuietTextWriter, which is not thread safe. The comment claiming the
lock blocked any Append was left over from when it locked on this.
All four lock sites in the class now take the inherited LockObj and the
private object is gone, so no ordering between two locks remains. Taking
LockObj in OnClose is safe because Close already holds it and Monitor is
reentrant.
Flush also returned true whatever happened. QuietTextWriter routes failing
writes to the ErrorHandler but does not override Flush, so a failure from
the underlying writer escaped to the caller. It is now reported with
ErrorCode.FlushFailure and Flush returns false.
The AdoNet test doubles gained the doc comments the rest of the test code
has, matching Log4NetTransaction.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../3.4.0/309-flush-uses-the-appender-lock.xml | 15 +++
.../Appender/AdoNet/Log4NetCommand.cs | 24 +++++
.../Appender/AdoNet/Log4NetConnection.cs | 17 +++
.../Appender/TextWriterAppenderTest.cs | 116 +++++++++++++++++++++
src/log4net/Appender/TextWriterAppender.cs | 25 +++--
5 files changed, 189 insertions(+), 8 deletions(-)
diff --git a/src/changelog/3.4.0/309-flush-uses-the-appender-lock.xml b/src/changelog/3.4.0/309-flush-uses-the-appender-lock.xml
new file mode 100644
index 00000000..90565811
--- /dev/null
+++ b/src/changelog/3.4.0/309-flush-uses-the-appender-lock.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">
+ make `TextWriterAppender` and the appenders deriving from it, including `FileAppender` and
+ `RollingFileAppender`, flush under the appender lock. `Flush` synchronized on a private object while
+ `Append` runs under the lock taken by `DoAppend`, so a flush could run concurrently with a write to
+ the same `QuietTextWriter`, which is not thread safe, and interleave or lose output. `Flush` also
+ returned `true` whatever happened and let a failure from the underlying writer escape to its
+ caller; it now reports through the `ErrorHandler` and returns `false` (audit 1231d72-f020)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Appender/AdoNet/Log4NetCommand.cs b/src/log4net.Tests/Appender/AdoNet/Log4NetCommand.cs
index fee95a5d..7a44dea2 100644
--- a/src/log4net.Tests/Appender/AdoNet/Log4NetCommand.cs
+++ b/src/log4net.Tests/Appender/AdoNet/Log4NetCommand.cs
@@ -27,6 +27,9 @@ namespace log4net.Tests.Appender.AdoNet;
internal sealed class Log4NetCommand : IDbCommand
{
+ /// <summary>
+ /// Initializes a new instance and records it as the <see cref="MostRecentInstance"/>.
+ /// </summary>
public Log4NetCommand()
{
MostRecentInstance = this;
@@ -34,13 +37,16 @@ public Log4NetCommand()
Parameters = new Log4NetParameterCollection();
}
+ /// <inheritdoc/>
public void Dispose()
{
// empty
}
+ /// <inheritdoc/>
public IDbTransaction? Transaction { get; set; }
+ /// <inheritdoc/>
public int ExecuteNonQuery()
{
string? payload = null;
@@ -68,6 +74,9 @@ public int ExecuteNonQuery()
return 0;
}
+ /// <summary>
+ /// The number of successful <see cref="ExecuteNonQuery"/> calls on this instance.
+ /// </summary>
public int ExecuteNonQueryCount { get; private set; }
/// <summary>
@@ -82,43 +91,58 @@ public int ExecuteNonQuery()
/// </summary>
public static List<string> ExecutedPayloads { get; } = [];
+ /// <inheritdoc/>
public IDbDataParameter CreateParameter() => new Log4NetParameter();
#pragma warning disable CS8766 // Nullability of reference types in return type doesn't match implicitly implemented member (possibly because of nullability attributes).
+ /// <inheritdoc/>
public string? CommandText { get; set; }
#pragma warning restore CS8766
+ /// <inheritdoc/>
public CommandType CommandType { get; set; }
+ /// <inheritdoc/>
public void Prepare()
{
// empty
}
+ /// <inheritdoc/>
public IDataParameterCollection Parameters { get; }
+ /// <summary>
+ /// The most recently constructed instance, so that a test can inspect what the appender used.
+ /// </summary>
public static Log4NetCommand? MostRecentInstance { get; private set; }
+ /// <inheritdoc/>
public void Cancel() => throw new NotImplementedException();
+ /// <inheritdoc/>
public IDataReader ExecuteReader() => throw new NotImplementedException();
+ /// <inheritdoc/>
public IDataReader ExecuteReader(CommandBehavior behavior) => throw new NotImplementedException();
+ /// <inheritdoc/>
public object ExecuteScalar() => throw new NotImplementedException();
+ /// <inheritdoc/>
public IDbConnection? Connection
{
get => throw new NotImplementedException();
set => throw new NotImplementedException();
}
+ /// <inheritdoc/>
public int CommandTimeout
{
get => throw new NotImplementedException();
set => throw new NotImplementedException();
}
+ /// <inheritdoc/>
public UpdateRowSource UpdatedRowSource
{
get => throw new NotImplementedException();
diff --git a/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs b/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs
index 43364c51..f5324ea3 100644
--- a/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs
+++ b/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs
@@ -30,20 +30,29 @@ internal sealed class Log4NetConnection : IDbConnection
{
private bool _open;
+ /// <summary>
+ /// Initializes a new instance and records it as the <see cref="MostRecentInstance"/>.
+ /// </summary>
public Log4NetConnection() => MostRecentInstance = this;
+ /// <inheritdoc/>
public void Close() => _open = false;
+ /// <inheritdoc/>
public ConnectionState State => _open ? ConnectionState.Open : ConnectionState.Closed;
#pragma warning disable CS8766 // Nullability of reference types in return type doesn't match implicitly implemented member (possibly because of nullability attributes).
+ /// <inheritdoc/>
public string? ConnectionString { get; set; }
#pragma warning restore CS8766
+ /// <inheritdoc/>
public IDbTransaction BeginTransaction() => new Log4NetTransaction();
+ /// <inheritdoc/>
public IDbCommand CreateCommand() => new Log4NetCommand();
+ /// <inheritdoc/>
public void Open()
{
if (FailOnOpen)
@@ -58,15 +67,23 @@ public void Open()
/// </summary>
public static bool FailOnOpen { get; set; }
+ /// <summary>
+ /// The most recently constructed instance, so that a test can inspect what the appender used.
+ /// </summary>
public static Log4NetConnection? MostRecentInstance { get; private set; }
+ /// <inheritdoc/>
public IDbTransaction BeginTransaction(IsolationLevel il) => throw new NotImplementedException();
+ /// <inheritdoc/>
public void ChangeDatabase(string databaseName) => throw new NotImplementedException();
+ /// <inheritdoc/>
public int ConnectionTimeout => throw new NotImplementedException();
+ /// <inheritdoc/>
public string Database => throw new NotImplementedException();
+ /// <inheritdoc/>
public void Dispose() => throw new NotImplementedException();
}
diff --git a/src/log4net.Tests/Appender/TextWriterAppenderTest.cs b/src/log4net.Tests/Appender/TextWriterAppenderTest.cs
new file mode 100644
index 00000000..e9c387c0
--- /dev/null
+++ b/src/log4net.Tests/Appender/TextWriterAppenderTest.cs
@@ -0,0 +1,116 @@
+#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;
+using System.IO;
+
+using log4net.Appender;
+using log4net.Core;
+using log4net.Layout;
+
+using NUnit.Framework;
+
+namespace log4net.Tests.Appender;
+
+/// <summary>
+/// Tests for <see cref="TextWriterAppender"/>
+/// </summary>
+[TestFixture]
+public class TextWriterAppenderTest
+{
+ /// <summary>
+ /// A writer that accepts everything written to it but cannot be flushed, standing in for a full
+ /// disk or a broken stream.
+ /// </summary>
+ private sealed class UnflushableWriter : StringWriter
+ {
+ /// <inheritdoc/>
+ public override void Flush() => throw new IOException("Simulated failure to flush");
+ }
+
+ /// <summary>
+ /// Swallows what the appender reports, so that the tests observe the return value rather than
+ /// internal logging.
+ /// </summary>
+ private sealed class SilentErrorHandler : IErrorHandler
+ {
+ /// <inheritdoc/>
+ public void Error(string message, Exception? e, ErrorCode errorCode)
+ { }
+
+ /// <inheritdoc/>
+ public void Error(string message, Exception e)
+ { }
+
+ /// <inheritdoc/>
+ public void Error(string message)
+ { }
+ }
+
+ /// <summary>
+ /// Flush reported success whatever happened. Its contract is to say whether the events were
+ /// flushed, and a caller such as a shutdown hook relies on that.
+ /// </summary>
+ [Test]
+ public void FlushReportsFailure()
+ => Assert.That(CreateAppender(new UnflushableWriter()).Flush(1000), Is.False);
+
+ /// <summary>
+ /// A writer that flushes cleanly still has to report success.
+ /// </summary>
+ [Test]
+ public void FlushReportsSuccess()
+ => Assert.That(CreateAppender(new StringWriter()).Flush(1000), Is.True);
+
+ /// <summary>
+ /// A failing flush must not escape to the caller. QuietTextWriter routes failing writes to the
+ /// ErrorHandler but does not override Flush, so the appender has to catch it.
+ /// </summary>
+ [Test]
+ public void FlushDoesNotThrow()
+ => Assert.That(() => CreateAppender(new UnflushableWriter()).Flush(1000), Throws.Nothing);
+
+ /// <summary>
+ /// With ImmediateFlush there is nothing buffered, so the writer is never touched.
+ /// </summary>
+ [Test]
+ public void FlushIsANoOpWhenImmediateFlushIsSet()
+ {
+ TextWriterAppender appender = CreateAppender(new UnflushableWriter());
+ appender.ImmediateFlush = true;
+
+ Assert.That(appender.Flush(1000), Is.True);
+ }
+
+ private static TextWriterAppender CreateAppender(TextWriter writer)
+ {
+ PatternLayout layout = new("%message%newline");
+ layout.ActivateOptions();
+
+ TextWriterAppender appender = new()
+ {
+ Layout = layout,
+ ImmediateFlush = false,
+ ErrorHandler = new SilentErrorHandler(),
+ Writer = writer
+ };
+ appender.ActivateOptions();
+ return appender;
+ }
+}
diff --git a/src/log4net/Appender/TextWriterAppender.cs b/src/log4net/Appender/TextWriterAppender.cs
index 046476c5..7fba8d21 100644
--- a/src/log4net/Appender/TextWriterAppender.cs
+++ b/src/log4net/Appender/TextWriterAppender.cs
@@ -89,7 +89,7 @@ public class TextWriterAppender : AppenderSkeleton
get => QuietWriter;
set
{
- lock (_syncRoot)
+ lock (LockObj)
{
Reset();
if (value is not null)
@@ -204,7 +204,7 @@ protected override void Append(LoggingEvent[] loggingEvents)
/// </remarks>
protected override void OnClose()
{
- lock (_syncRoot)
+ lock (LockObj)
{
Reset();
}
@@ -222,7 +222,7 @@ public override IErrorHandler ErrorHandler
get => base.ErrorHandler;
set
{
- lock (_syncRoot)
+ lock (LockObj)
{
if (value is null)
{
@@ -357,8 +357,6 @@ protected virtual void PrepareWriter()
/// </remarks>
protected QuietTextWriter? QuietWriter { get; set; }
- private readonly object _syncRoot = new();
-
/// <summary>
/// The fully qualified type of the TextWriterAppender class.
/// </summary>
@@ -381,10 +379,21 @@ public override bool Flush(int millisecondsTimeout)
return true;
}
- // lock(this) will block any Appends while the buffer is flushed.
- lock (_syncRoot)
+ // Taking the appender lock blocks any Append while the buffer is flushed. QuietTextWriter is
+ // not thread safe, and Append holds this same lock through DoAppend.
+ lock (LockObj)
{
- QuietWriter?.Flush();
+ try
+ {
+ QuietWriter?.Flush();
+ }
+ catch (Exception e) when (!e.IsFatal())
+ {
+ // QuietTextWriter routes failing writes to the ErrorHandler but does not override Flush,
+ // so a failure here would otherwise escape to the caller of Flush.
+ ErrorHandler.Error($"Failed to flush appender [{Name}].", e, ErrorCode.FlushFailure);
+ return false;
+ }
}
return true;
(logging-log4net) 15/19: fix the lifetime of the LocalSyslogAppender identity
Posted by fr...@apache.org.
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 <fr...@apache.org>
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) <no...@anthropic.com>
---
.../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
(logging-log4net) 06/19: document that configuration is trusted input
Posted by fr...@apache.org.
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 6bc3df34ba304ec9db6f7c361bf3c1c7279af2eb
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 21:54:28 2026 +0200
document that configuration is trusted input
Security scans regularly report the configuration paths as cleartext
transport, credential exposure, unrestricted type loading and unverified
reload. None of them is a vulnerability, so record why, with a link to the
threat model, at the places a scan actually flags:
- InternalConfigure(ILoggerRepository, Uri) neither restricts the URI scheme
nor withholds the process credentials. The endpoint is named by the
configuration and is trusted for the same reason an appender destination
is; transmitting configuration confidentially is a deployer
responsibility. Nothing runs until an operator supplies a URI, either by
calling Configure(Uri) or through the log4net.Config appSetting.
- ParseAppender instantiates the types the configuration names, and
SetParameter reaches non-public members, both by design.
- ConfigureAndWatchHandler reloads a replaced file without re-checking its
origin; keeping the watched file writable only by the operator is a
deployer responsibility.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
src/log4net/Config/XmlConfigurator.cs | 18 ++++++++++++++++++
.../Repository/Hierarchy/XmlHierarchyConfigurator.cs | 5 +++++
2 files changed, 23 insertions(+)
diff --git a/src/log4net/Config/XmlConfigurator.cs b/src/log4net/Config/XmlConfigurator.cs
index e7ffd2db..ece21ae4 100644
--- a/src/log4net/Config/XmlConfigurator.cs
+++ b/src/log4net/Config/XmlConfigurator.cs
@@ -491,6 +491,20 @@ private static void InternalConfigure(ILoggerRepository repository, Uri? configU
}
else
{
+ // The URI is not restricted to a particular scheme and the request below is sent with the
+ // process credentials. Both are intentional and are not vulnerabilities:
+ //
+ // Configuration is operator-supplied and therefore trusted, and the endpoint named here is
+ // part of that configuration - it is trusted for the same reason an appender destination
+ // is. Ensuring that configuration is transmitted only over a confidential channel, and
+ // that the endpoint is one the credentials may be presented to, is a deployer
+ // responsibility. Nothing here is reachable until an operator supplies a URI, either by
+ // calling Configure(Uri) or through the log4net.Config appSetting.
+ //
+ // See the Apache Logging Services common threat model, sections "Configuration
+ // (operator-controlled)" and "Adversary capabilities":
+ // https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
+
// NETCF dose not support WebClient
WebRequest? configRequest = null;
@@ -749,6 +763,10 @@ private static void InternalConfigureAndWatch(ILoggerRepository repository, File
/// </remarks>
private sealed class ConfigureAndWatchHandler : IDisposable
{
+ // The replaced file is reloaded without re-checking its origin. Keeping the watched file
+ // writable only by the operator is a deployer responsibility, see
+ // https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
+
/// <summary>
/// Holds the FileInfo used to configure the XmlConfigurator
/// </summary>
diff --git a/src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs b/src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs
index c65b719b..32df0c2d 100644
--- a/src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs
+++ b/src/log4net/Repository/Hierarchy/XmlHierarchyConfigurator.cs
@@ -270,6 +270,9 @@ public void Configure(XmlElement? element)
/// </remarks>
protected IAppender? ParseAppender(XmlElement appenderElement)
{
+ // Configuration names the types to load and instantiate. This is by design: configuration is
+ // trusted input, see
+ // https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
string appenderName = appenderElement.EnsureNotNull().GetAttribute(NameAttr);
string typeName = appenderElement.GetAttribute(TypeAttr);
@@ -564,6 +567,8 @@ protected void SetParameter(XmlElement element, object target)
MethodInfo? methInfo = null;
// Try to find a writable property
+ // Non-public members are reachable on purpose: configuration is trusted input, see
+ // https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
PropertyInfo? propInfo = targetType.GetProperty(name, BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.IgnoreCase);
if (propInfo is not null && propInfo.CanWrite)
{
(logging-log4net) 19/19: remove the git-broadcast workflow
Posted by fr...@apache.org.
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 e203b7c7424496595b004e0ae62147bc3f43fde0
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Tue Aug 18 00:02:45 2026 +0200
remove the git-broadcast workflow
Its push and pull request triggers had been commented out, leaving it
dispatched by hand only, so nothing depends on it.
What remained was worth removing rather than pinning: it ran
npx git-broadcast@beta in a job that checks out with a token able to push to
this repository, using mutable action tags and with no permissions block.
The beta dist-tag resolves to 0.45.7 from 2024, older than the 0.50.0 that
latest points at, so it was not tracking newer code either; it was simply a
pointer that can be moved to any published version.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.github/workflows/git-broadcast.yml | 44 ----------------------
.../3.4.0/309-remove-git-broadcast-workflow.xml | 13 +++++++
2 files changed, 13 insertions(+), 44 deletions(-)
diff --git a/.github/workflows/git-broadcast.yml b/.github/workflows/git-broadcast.yml
deleted file mode 100644
index 513b8c11..00000000
--- a/.github/workflows/git-broadcast.yml
+++ /dev/null
@@ -1,44 +0,0 @@
-#
-# 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.
-#
-name: Broadcast master updates to satellites
-
-on:
- workflow_dispatch:
-# Temporarily disabled, uncomment if needed.
-# push:
-# branches: [ master ]
-# pull_request:
-# branches: [ master ]
-
-concurrency:
- group: git-broadcast
-
-jobs:
- main:
- runs-on: ubuntu-latest
- steps:
- - uses: actions/checkout@v2
- - uses: actions/setup-node@v2-beta
- with:
- node-version: '16'
- - name: broadcast master changes to satellite branches
- env:
- RUN_NUMBER: ${{ github.run_number }}
- run: |
- git config --global user.name "Git Broadcast"
- git config --global user.email "git-broadcast@no-reply.com"
- npx git-broadcast@beta --ignore abandoned-develop --from master --push --pretty --suppress-log-prefixes --prefix-logs-with $GITHUB_REPOSITORY
diff --git a/src/changelog/3.4.0/309-remove-git-broadcast-workflow.xml b/src/changelog/3.4.0/309-remove-git-broadcast-workflow.xml
new file mode 100644
index 00000000..1b7b5265
--- /dev/null
+++ b/src/changelog/3.4.0/309-remove-git-broadcast-workflow.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="removed">
+ <issue id="309" link="https://github.com/apache/logging-log4net/pull/309"/>
+ <description format="asciidoc">
+ remove the `git-broadcast` workflow. Its push and pull request triggers had been commented out,
+ leaving it dispatched by hand only, while it still ran `npx git-broadcast@beta`, a dist-tag that can
+ be repointed at any published version, in a job that checks out with a token able to push to this
+ repository, using mutable action tags and no `permissions` block (CWE-1357) (audit 1231d72-f011)
+ </description>
+</entry>
(logging-log4net) 12/19: keep the impersonated user name when a logging event is fixed
Posted by fr...@apache.org.
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 1786b139aad01148120f981cba80336c2c505f6e
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 23:06:40 2026 +0200
keep the impersonated user name when a logging event is fixed
UserName resolved the identity of whichever thread read it, so with a
buffering appender the buffered events were attributed to the thread
flushing the buffer rather than the one that logged them.
Only impersonation makes that wrong. Without it the name is the process
identity, which is the same on every thread and stays resolvable, so it is
still resolved lazily and nothing changes for those applications.
An event logged while impersonating now takes its user name with it when it
is fixed, because that is the last point at which the identity is known.
This happens whatever Fix asks for, and outside the block that fixes the
requested fields, since that block is skipped when there is nothing to fix
while the cache is locked all the same. FixFlags.UserName stays unset, so
the flags keep reporting what the caller requested.
Reading the property on a thread that is impersonating no longer reports
that thread's user; the not available text is used instead.
The impersonation check only queries the thread token. Resolving the name
behind it is the expensive part and is unchanged.
Also renames FixingTest.All_ShouldContainAllFlags and documents its members.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../3.4.0/309-username-honours-the-fix-gate.xml | 16 ++++
src/log4net.Tests/Core/FixingTest.cs | 99 +++++++++++++++++++++-
src/log4net.Tests/Core/UserNameFixingTest.cs | 43 ++++++++++
src/log4net/Core/LoggingEvent.cs | 68 ++++++++++++++-
4 files changed, 222 insertions(+), 4 deletions(-)
diff --git a/src/changelog/3.4.0/309-username-honours-the-fix-gate.xml b/src/changelog/3.4.0/309-username-honours-the-fix-gate.xml
new file mode 100644
index 00000000..ceffb9e1
--- /dev/null
+++ b/src/changelog/3.4.0/309-username-honours-the-fix-gate.xml
@@ -0,0 +1,16 @@
+<?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">
+ stop `LoggingEvent.UserName` reporting the wrong user for an event logged while impersonating.
+ The property resolved the identity of whichever thread read it, so with a buffering appender the
+ buffered events were attributed to the thread flushing the buffer rather than the one that logged
+ them (CWE-282). An event logged while impersonating now takes its user name with it when it is
+ fixed, and an event read on an impersonating thread reports the not available text instead of that
+ thread's user. Without impersonation the name is the process identity, which is the same on every
+ thread, so it is still resolved lazily and nothing changes (audit 1231d72-f014)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Core/FixingTest.cs b/src/log4net.Tests/Core/FixingTest.cs
index 3efe326b..840431fc 100644
--- a/src/log4net.Tests/Core/FixingTest.cs
+++ b/src/log4net.Tests/Core/FixingTest.cs
@@ -22,17 +22,28 @@
using System.Threading;
using log4net.Core;
+using log4net.Util;
using NUnit.Framework;
namespace log4net.Tests.Core;
+/// <summary>
+/// Tests for <see cref="LoggingEvent.Fix"/> and the fields it captures.
+/// </summary>
[TestFixture]
[System.Diagnostics.CodeAnalysis.SuppressMessage("Usage", "CA2201:Do not raise reserved exception types")]
public class FixingTest
{
- const string TestRepository = "Test Repository";
+ /// <summary>
+ /// The name of the repository the events under test belong to.
+ /// </summary>
+ private const string TestRepository = "Test Repository";
+ /// <summary>
+ /// Creates the repository the events under test belong to, and names the thread, so that
+ /// <see cref="LoggingEvent.ThreadName"/> has something stable to capture.
+ /// </summary>
[OneTimeSetUp]
public void CreateRepository()
{
@@ -61,8 +72,12 @@ public void CreateRepository()
}
}
+ /// <summary>
+ /// <see cref="FixFlags.All"/> has to contain every other flag, so that fixing everything does not
+ /// quietly leave a field out when a new flag is added.
+ /// </summary>
[Test]
- public void All_ShouldContainAllFlags()
+ public void AllContainsEveryFlag()
{
// Arrange
// Act
@@ -76,6 +91,9 @@ public void All_ShouldContainAllFlags()
}
}
+ /// <summary>
+ /// A newly created event has nothing fixed yet.
+ /// </summary>
[Test]
public void TestUnfixedValues()
{
@@ -95,6 +113,9 @@ public void TestUnfixedValues()
Assert.That(loggingEvent.Fix, Is.EqualTo(FixFlags.None), "Fixed Fields is incorrect");
}
+ /// <summary>
+ /// Fixing with <see cref="FixFlags.All"/> reports every field as fixed.
+ /// </summary>
[Test]
public void TestAllFixedValues()
{
@@ -116,6 +137,9 @@ public void TestAllFixedValues()
Assert.That(loggingEvent.Fix, Is.EqualTo(FixFlags.LocationInfo | FixFlags.UserName | FixFlags.Identity | FixFlags.Partial | FixFlags.Message | FixFlags.ThreadName | FixFlags.Exception | FixFlags.Domain | FixFlags.Properties), "Fixed Fields is incorrect");
}
+ /// <summary>
+ /// Fixing with <see cref="FixFlags.None"/> leaves the event unfixed.
+ /// </summary>
[Test]
public void TestNoFixedValues()
{
@@ -137,6 +161,71 @@ public void TestNoFixedValues()
Assert.That(loggingEvent.Fix, Is.EqualTo(FixFlags.None), "Fixed Fields is incorrect");
}
+ /// <summary>
+ /// Without impersonation the user name is the process identity, which is the same whichever
+ /// thread asks, so resolving it after the event has been fixed still gives the right answer. An
+ /// event fixed without <see cref="FixFlags.UserName"/> must therefore keep reporting it.
+ /// </summary>
+ [Test]
+ public void UserNameIsStillResolvedAfterFixingWithoutImpersonation()
+ {
+ string expected = CreateEvent().UserName;
+ LoggingEvent loggingEvent = CreateEvent();
+
+ // Partial deliberately leaves UserName out, being the documented setting for avoiding its cost.
+ loggingEvent.Fix = FixFlags.Partial;
+
+ Assert.That(loggingEvent.Fix & FixFlags.UserName, Is.EqualTo(FixFlags.None));
+ Assert.That(loggingEvent.UserName, Is.EqualTo(expected));
+ Assert.That(loggingEvent.UserName, Is.Not.EqualTo(SystemInfo.NotAvailableText));
+ }
+
+ /// <summary>
+ /// Fixing with <see cref="FixFlags.None"/> skips the whole of FixVolatileData but still locks the
+ /// cache, so the user name has to survive that path too.
+ /// </summary>
+ [Test]
+ public void UserNameIsStillResolvedAfterFixingNothing()
+ {
+ string expected = CreateEvent().UserName;
+ LoggingEvent loggingEvent = CreateEvent();
+
+ loggingEvent.Fix = FixFlags.None;
+
+ Assert.That(loggingEvent.UserName, Is.EqualTo(expected));
+ }
+
+ /// <summary>
+ /// Fixing an event with UserName still has to capture it, on the thread that logged the event.
+ /// </summary>
+ [Test]
+ public void UserNameIsCapturedWhenItIsFixed()
+ {
+ LoggingEvent loggingEvent = CreateEvent();
+
+ string expected = loggingEvent.UserName;
+ loggingEvent.Fix = FixFlags.All;
+
+ Assert.That(loggingEvent.UserName, Is.EqualTo(expected));
+ Assert.That(loggingEvent.UserName, Is.Not.EqualTo(SystemInfo.NotAvailableText));
+ }
+
+ /// <summary>
+ /// Creates an event in the test repository.
+ /// </summary>
+ /// <returns>A new, unfixed event.</returns>
+ private static LoggingEvent CreateEvent()
+ => new(typeof(FixingTest),
+ LogManager.GetRepository(TestRepository),
+ typeof(FixingTest).FullName,
+ Level.Warn,
+ "Logging event works",
+ null);
+
+ /// <summary>
+ /// Builds the event data the tests compare against.
+ /// </summary>
+ /// <returns>Event data with every field set to a known value.</returns>
private static LoggingEventData BuildStandardEventData()
{
LoggingEventData loggingEventData = new()
@@ -154,6 +243,12 @@ private static LoggingEventData BuildStandardEventData()
return loggingEventData;
}
+ /// <summary>
+ /// Asserts that <paramref name="loggingEvent"/> carries the values of
+ /// <paramref name="loggingEventData"/>.
+ /// </summary>
+ /// <param name="loggingEvent">The event to check.</param>
+ /// <param name="loggingEventData">The expected values.</param>
private static void AssertExpectedLoggingEvent(LoggingEvent loggingEvent, LoggingEventData loggingEventData)
{
Assert.That(loggingEventData.Domain, Is.EqualTo("ReallySimpleApp"), "Domain is incorrect");
diff --git a/src/log4net.Tests/Core/UserNameFixingTest.cs b/src/log4net.Tests/Core/UserNameFixingTest.cs
index 25dfe1ec..50536c5d 100644
--- a/src/log4net.Tests/Core/UserNameFixingTest.cs
+++ b/src/log4net.Tests/Core/UserNameFixingTest.cs
@@ -95,6 +95,49 @@ public void UserNameIsResolvedWhileImpersonating()
Assert.That(actual, Is.EqualTo(expected));
}
+ /// <summary>
+ /// An event logged while impersonating takes the user name with it when it is fixed, even though
+ /// <see cref="FixFlags.Partial"/> does not ask for it. Fixing is the last point at which the
+ /// identity that logged the event is known: a buffering appender reads the property later, from
+ /// the thread flushing the buffer.
+ /// </summary>
+ [Test]
+ public void UserNameIsCapturedWhenFixingAnImpersonatedEvent()
+ {
+ using WindowsIdentity identity = WindowsIdentity.GetCurrent();
+ string expected = identity.Name;
+
+ LoggingEvent loggingEvent = WindowsIdentity.RunImpersonated(identity.AccessToken, () =>
+ {
+ LoggingEvent impersonatedEvent = CreateEvent();
+ impersonatedEvent.Fix = FixFlags.Partial;
+ return impersonatedEvent;
+ });
+
+ // Read outside the impersonation, as a buffering appender would.
+ Assert.That(loggingEvent.UserName, Is.EqualTo(expected));
+ }
+
+ /// <summary>
+ /// The same holds when nothing at all is fixed, which skips most of FixVolatileData but still
+ /// locks the cache.
+ /// </summary>
+ [Test]
+ public void UserNameIsCapturedWhenFixingNothingOnAnImpersonatedEvent()
+ {
+ using WindowsIdentity identity = WindowsIdentity.GetCurrent();
+ string expected = identity.Name;
+
+ LoggingEvent loggingEvent = WindowsIdentity.RunImpersonated(identity.AccessToken, () =>
+ {
+ LoggingEvent impersonatedEvent = CreateEvent();
+ impersonatedEvent.Fix = FixFlags.None;
+ return impersonatedEvent;
+ });
+
+ Assert.That(loggingEvent.UserName, Is.EqualTo(expected));
+ }
+
/// <summary>
/// The process identity name may only be resolved on a thread that is not impersonating.
/// Seeding it from an impersonating thread would report that user for every later event in
diff --git a/src/log4net/Core/LoggingEvent.cs b/src/log4net/Core/LoggingEvent.cs
index 016473fa..4f7c852e 100644
--- a/src/log4net/Core/LoggingEvent.cs
+++ b/src/log4net/Core/LoggingEvent.cs
@@ -735,8 +735,63 @@ private static string ReviseThreadName(string? threadName)
/// rather than the Windows account the request happens to run as.
/// </para>
/// </remarks>
- public string UserName =>
- _data.UserName ??= TryGetCurrentUserName() ?? SystemInfo.NotAvailableText;
+ public string UserName
+ {
+ get
+ {
+ if (_data.UserName is null)
+ {
+ // Resolving late gives the process identity, which is the same on every thread. On an
+ // impersonating thread it would give whoever is reading the event instead, so nothing is
+ // reported; FixVolatileData captures that case up front.
+ if (_cacheUpdatable || !IsImpersonating())
+ {
+ _data.UserName = TryGetCurrentUserName() ?? SystemInfo.NotAvailableText;
+ }
+ }
+
+ return _data.UserName ?? SystemInfo.NotAvailableText;
+ }
+ }
+
+ /// <summary>
+ /// Whether the calling thread is impersonating another identity.
+ /// </summary>
+ /// <returns>
+ /// <see langword="true"/> when the thread runs as an impersonated identity rather than as the
+ /// process identity, and <see langword="false"/> when it does not or when that cannot be known.
+ /// </returns>
+ /// <remarks>
+ /// <para>
+ /// Only queries the thread token. Resolving the name behind it is the expensive part and is left
+ /// to <see cref="TryGetCurrentUserName"/>.
+ /// </para>
+ /// </remarks>
+ private static bool IsImpersonating()
+ {
+ try
+ {
+ if (_windowsIdentityUnavailable)
+ {
+ return false;
+ }
+
+ if (!IsWindowsIdentitySupported())
+ {
+ _windowsIdentityUnavailable = true;
+ return false;
+ }
+
+ using WindowsIdentity? impersonated = WindowsIdentity.GetCurrent(ifImpersonating: true);
+ return impersonated is not null;
+ }
+ catch (Exception e) when (!e.IsFatal())
+ {
+ // As in TryGetCurrentUserName: an unreadable identity must not break logging.
+ _windowsIdentityUnavailable = true;
+ return false;
+ }
+ }
private static string? TryGetCurrentUserName()
{
@@ -1156,6 +1211,15 @@ protected virtual void FixVolatileData(FixFlags flags)
}
}
+ // Last point at which the identity that logged the event is known, so grab it even when it was
+ // not asked for. Outside the block above, which is skipped when there is nothing to fix while
+ // the cache is locked all the same. FixFlags.UserName stays unset: the flags report what the
+ // caller requested.
+ if (_data.UserName is null && IsImpersonating())
+ {
+ _data.UserName = TryGetCurrentUserName() ?? SystemInfo.NotAvailableText;
+ }
+
// Finally lock everything we've cached.
_cacheUpdatable = false;
}
(logging-log4net) 03/19: warn when AdoNetAppender executes layout-generated SQL
Posted by fr...@apache.org.
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 <fr...@apache.org>
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) <no...@anthropic.com>
---
.../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.
(logging-log4net) 16/19: bound the waits for the file locking mutexes
Posted by fr...@apache.org.
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 3fd97cb511b2a99b3b4b6957ea606a2e5a6ab804
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 23:52:21 2026 +0200
bound the waits for the file locking mutexes
InterProcessLock.AcquireLock carried a "TODO: add timeout?" and waited
without one, as did RollingFileAppender when deciding whether to roll. Both
waits happen while the appender lock is held, so a mutex nobody releases
suspended every thread logging through the appender.
Both now wait at most LockTimeoutMillis, 10000 by default, with
Timeout.Infinite restoring the previous behaviour. An event that cannot get
the file lock is reported and dropped; one that cannot get the rolling lock
is written to the current file without the roll check, because rolling
without the lock would race another process renaming the same files.
Two more problems turned up on the way:
AbandonedMutexException was unhandled although it means the wait succeeded
and this thread owns the mutex. It propagated out of AcquireLock before
_recursiveWatch was incremented, so ReleaseLock never released it and the
mutex stayed held for good. That needs no attacker, only a process dying
mid-write.
AdjustFileBeforeAppend released the rolling mutex in a finally without
checking that it had been taken. Harmless while the wait could only succeed,
but a bounded wait makes it throw, so it is guarded now.
The mutex names are left alone. They are derived from the log file path so
that separate processes agree on them, and making them unpredictable would
break the cross-process coordination they exist for.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
src/changelog/3.4.0/309-bound-file-lock-waits.xml | 17 +++++
src/log4net.Tests/Appender/FileAppenderTest.cs | 80 ++++++++++++++++++++++
src/log4net/Appender/FileAppender.cs | 55 ++++++++++++++-
src/log4net/Appender/RollingFileAppender.cs | 64 ++++++++++++++++-
.../configuration/appenders/fileappender.adoc | 34 ++++++++-
5 files changed, 244 insertions(+), 6 deletions(-)
diff --git a/src/changelog/3.4.0/309-bound-file-lock-waits.xml b/src/changelog/3.4.0/309-bound-file-lock-waits.xml
new file mode 100644
index 00000000..fb80fad5
--- /dev/null
+++ b/src/changelog/3.4.0/309-bound-file-lock-waits.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="changed">
+ <issue id="309" link="https://github.com/apache/logging-log4net/pull/309"/>
+ <description format="asciidoc">
+ bound the waits for the named mutexes used by `FileAppender.InterProcessLock` and by
+ `RollingFileAppender` when it decides whether to roll. Both waited without a timeout while the
+ appender lock was held, so a mutex nobody released suspended every thread logging through the
+ appender. The wait now stops after `LockTimeoutMillis`, 10000 by default, reporting through the
+ error handler and dropping the event or skipping the roll check; `Timeout.Infinite` restores the
+ previous behaviour. `AbandonedMutexException` is handled as the successful acquisition it is,
+ rather than leaving the mutex held, and the rolling lock is no longer released when it was never
+ taken (audit 1231d72-f015)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Appender/FileAppenderTest.cs b/src/log4net.Tests/Appender/FileAppenderTest.cs
index 8537f80d..7fa095c5 100644
--- a/src/log4net.Tests/Appender/FileAppenderTest.cs
+++ b/src/log4net.Tests/Appender/FileAppenderTest.cs
@@ -30,7 +30,9 @@
using System.IO;
using System.Linq;
using System.Text;
+using System.Threading;
using System.Threading.Tasks;
+using System.Diagnostics;
namespace log4net.Tests.Appender;
@@ -183,4 +185,82 @@ public void InterProcessLock_AcquireLock_ReleasesMutexWhenStreamIsNull()
File.Delete(tempFile);
}
}
+
+ /// <summary>
+ /// The wait for the inter process lock happens while the appender lock is held, so it has to be
+ /// bounded by default: a lock nobody releases would otherwise suspend all logging for good.
+ /// </summary>
+ [Test]
+ public void LockTimeoutMillisDefaultsToAFiniteValue()
+ => Assert.That(new FileAppender.InterProcessLock().LockTimeoutMillis, Is.EqualTo(10000));
+
+ /// <summary>
+ /// Timeout.Infinite restores waiting indefinitely and 0 gives up at once; any other negative
+ /// value has no meaning and is rejected rather than reinterpreted.
+ /// </summary>
+ [Test]
+ public void LockTimeoutMillisRejectsNegativeValuesExceptInfinite()
+ {
+ FileAppender.InterProcessLock lockingModel = new();
+
+ Assert.That(() => lockingModel.LockTimeoutMillis = -2, Throws.TypeOf<ArgumentOutOfRangeException>());
+
+ lockingModel.LockTimeoutMillis = Timeout.Infinite;
+ Assert.That(lockingModel.LockTimeoutMillis, Is.EqualTo(Timeout.Infinite));
+
+ lockingModel.LockTimeoutMillis = 0;
+ Assert.That(lockingModel.LockTimeoutMillis, Is.EqualTo(0));
+ }
+
+ /// <summary>
+ /// When something else holds the lock and does not let go, the event is dropped rather than the
+ /// logging thread being blocked forever.
+ /// </summary>
+ [Test]
+ [NonParallelizable]
+ public void AcquireLockGivesUpWhenTheLockIsHeldTooLong()
+ {
+ const string appenderFile = "log4net_lock_timeout_test";
+ string tempFile = Path.GetTempFileName();
+ FileAppender appender = new() { File = appenderFile };
+ FileAppender.InterProcessLock lockingModel = new()
+ {
+ CurrentAppender = appender,
+ LockTimeoutMillis = 200
+ };
+ lockingModel.ActivateOptions();
+ lockingModel.OpenFile(tempFile, false, Encoding.UTF8);
+
+ using ManualResetEventSlim held = new();
+ using ManualResetEventSlim release = new();
+ // A mutex has to be taken and released on one thread, so the holder does both.
+ Task holder = Task.Run(() =>
+ {
+ using Mutex contender = new(false, appenderFile);
+ contender.WaitOne();
+ held.Set();
+ release.Wait();
+ contender.ReleaseMutex();
+ });
+
+ try
+ {
+ Assert.That(held.Wait(TimeSpan.FromSeconds(10)), Is.True, "the contending thread never took the mutex");
+
+ Stream? stream = null;
+ Stopwatch stopwatch = Stopwatch.StartNew();
+ LogLog.ExecuteWithoutEmittingInternalMessages(() => stream = lockingModel.AcquireLock());
+ stopwatch.Stop();
+
+ Assert.That(stream, Is.Null, "the lock was reported as acquired while another thread held it");
+ Assert.That(stopwatch.Elapsed, Is.LessThan(TimeSpan.FromSeconds(10)));
+ }
+ finally
+ {
+ release.Set();
+ holder.Wait(TimeSpan.FromSeconds(10));
+ lockingModel.OnClose();
+ File.Delete(tempFile);
+ }
+ }
}
\ No newline at end of file
diff --git a/src/log4net/Appender/FileAppender.cs b/src/log4net/Appender/FileAppender.cs
index 993e2974..5887fbfe 100644
--- a/src/log4net/Appender/FileAppender.cs
+++ b/src/log4net/Appender/FileAppender.cs
@@ -576,6 +576,42 @@ public class InterProcessLock : LockingModelBase
private Mutex? _mutex;
private Stream? _stream;
private int _recursiveWatch;
+ private int _lockTimeoutMillis = 10_000;
+
+ /// <summary>
+ /// Gets or sets the time, in milliseconds, to wait for the lock before giving up on an event.
+ /// </summary>
+ /// <value>
+ /// A number of milliseconds, 0 to fail immediately when the lock is held, or
+ /// <see cref="Timeout.Infinite"/> to wait for as long as it takes.
+ /// </value>
+ /// <remarks>
+ /// <para>
+ /// The wait happens while the appender lock is held, so a lock nobody releases would otherwise
+ /// suspend every thread logging through this appender. An event that cannot get the lock in
+ /// time is reported and dropped instead.
+ /// </para>
+ /// <para>
+ /// The default value is 10000. Raise it when the file is on storage where the lock is
+ /// legitimately slow to obtain, such as a network share.
+ /// </para>
+ /// </remarks>
+ /// <exception cref="ArgumentOutOfRangeException">
+ /// The value specified is negative and is not <see cref="Timeout.Infinite"/>.
+ /// </exception>
+ public int LockTimeoutMillis
+ {
+ get => _lockTimeoutMillis;
+ set
+ {
+ if (value < 0 && value != Timeout.Infinite)
+ {
+ throw SystemInfo.CreateArgumentOutOfRangeException(nameof(value), value,
+ $"The value specified for LockTimeoutMillis is negative and is not {nameof(Timeout)}.{nameof(Timeout.Infinite)}.");
+ }
+ _lockTimeoutMillis = value;
+ }
+ }
/// <summary>
/// Open the file specified and prepare for logging.
@@ -641,8 +677,23 @@ public override void CloseFile()
{
if (_mutex is not null)
{
- // TODO: add timeout?
- _mutex.WaitOne();
+ bool acquired;
+ try
+ {
+ acquired = _mutex.WaitOne(LockTimeoutMillis);
+ }
+ catch (AbandonedMutexException)
+ {
+ // Previous owner died without releasing; the wait succeeded and we own the mutex.
+ acquired = true;
+ }
+
+ if (!acquired)
+ {
+ CurrentAppender?.ErrorHandler.Error(
+ $"Timeout after {LockTimeoutMillis}ms waiting for the inter process lock on the log file, so the logging event was not written.");
+ return null;
+ }
// increment recursive watch
_recursiveWatch++;
diff --git a/src/log4net/Appender/RollingFileAppender.cs b/src/log4net/Appender/RollingFileAppender.cs
index 55b30078..a15e8f97 100644
--- a/src/log4net/Appender/RollingFileAppender.cs
+++ b/src/log4net/Appender/RollingFileAppender.cs
@@ -518,10 +518,32 @@ protected override void Append(LoggingEvent[] loggingEvents)
protected virtual void AdjustFileBeforeAppend()
{
// reuse the file appenders locking model to lock the rolling
+ bool acquired = false;
try
{
// if rolling should be locked, acquire the lock
- _mutexForRolling?.WaitOne();
+ if (_mutexForRolling is not null)
+ {
+ try
+ {
+ acquired = _mutexForRolling.WaitOne(LockTimeoutMillis);
+ }
+ catch (AbandonedMutexException)
+ {
+ // Previous owner died without releasing; the wait succeeded and we own the mutex.
+ acquired = true;
+ }
+
+ if (!acquired)
+ {
+ // Rolling without the lock would race another process renaming the same files, so append
+ // to the current file instead of waiting, which would suspend every logging thread.
+ ErrorHandler.Error(
+ $"Timeout after {LockTimeoutMillis}ms waiting for the lock on rolling {File}, so this event was written without checking whether the file should roll.");
+ return;
+ }
+ }
+
if (_rollDate)
{
DateTime n = DateTimeStrategy.Now;
@@ -541,8 +563,11 @@ protected virtual void AdjustFileBeforeAppend()
}
finally
{
- // if rolling should be locked, release the lock
- _mutexForRolling?.ReleaseMutex();
+ // Only when the wait succeeded: releasing a mutex this thread does not own throws.
+ if (acquired)
+ {
+ _mutexForRolling!.ReleaseMutex();
+ }
}
}
@@ -1516,6 +1541,39 @@ protected static DateTime NextCheckDate(DateTime currentDateTime, RollPoint roll
/// </summary>
private Mutex? _mutexForRolling;
+ private int _lockTimeoutMillis = 10_000;
+
+ /// <summary>
+ /// Gets or sets the time, in milliseconds, to wait for the rolling lock before appending without
+ /// checking whether the file should roll.
+ /// </summary>
+ /// <value>
+ /// A number of milliseconds, 0 to give up immediately when the lock is held, or
+ /// <see cref="Timeout.Infinite"/> to wait for as long as it takes.
+ /// </value>
+ /// <remarks>
+ /// <para>
+ /// The wait happens while the appender lock is held, so a lock nobody releases would otherwise
+ /// suspend every thread logging through this appender. The default value is 10000.
+ /// </para>
+ /// </remarks>
+ /// <exception cref="ArgumentOutOfRangeException">
+ /// The value specified is negative and is not <see cref="Timeout.Infinite"/>.
+ /// </exception>
+ public int LockTimeoutMillis
+ {
+ get => _lockTimeoutMillis;
+ set
+ {
+ if (value < 0 && value != Timeout.Infinite)
+ {
+ throw SystemInfo.CreateArgumentOutOfRangeException(nameof(value), value,
+ $"The value specified for LockTimeoutMillis is negative and is not {nameof(Timeout)}.{nameof(Timeout.Infinite)}.");
+ }
+ _lockTimeoutMillis = value;
+ }
+ }
+
/// <summary>
/// The 1st of January 1970 in UTC
/// </summary>
diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/fileappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/fileappender.adoc
index e05d8d3e..a64571d0 100644
--- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/fileappender.adoc
+++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/fileappender.adoc
@@ -63,4 +63,36 @@ This example shows how to configure the appender to use the minimal locking mode
<conversionPattern value="%date [%thread] %-5level %logger %message%newline" />
</layout>
</appender>
-----
\ No newline at end of file
+----
+
+[#fileappender-lock-timeout]
+== Waiting for the lock
+
+`InterProcessLock` coordinates several processes writing to one file through a named mutex.
+The wait for it happens while the appender lock is held, so a lock nobody releases would suspend
+every thread logging through the appender.
+
+The wait is therefore bounded by `lockTimeoutMillis`, 10000 by default.
+An event that cannot get the lock in time is reported through the error handler and dropped, and the
+appender carries on with the next one.
+Raise the value when the file is on storage where the lock is legitimately slow to obtain, such as a
+network share, or set it to `-1` to wait for as long as it takes.
+
+[source,xml]
+----
+<appender name="FileAppender" type="log4net.Appender.FileAppender">
+ <file value="MyApp.log" />
+ <appendToFile value="true" />
+ <lockingModel type="log4net.Appender.FileAppender+InterProcessLock">
+ <lockTimeoutMillis value="10000" />
+ </lockingModel>
+ <layout type="log4net.Layout.PatternLayout">
+ <conversionPattern value="%date [%thread] %-5level %logger %message%newline" />
+ </layout>
+</appender>
+----
+
+`RollingFileAppender` takes a second mutex around the decision to roll and has its own
+`lockTimeoutMillis` for it, with the same default.
+An event that cannot get that lock is written to the current file without checking whether it should
+roll first, rather than waiting.
\ No newline at end of file
(logging-log4net) 13/19: document that format strings are trusted developer input
Posted by fr...@apache.org.
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 287fa9cd044f15bf8ccd5ef8e04caa748e91fb59
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 23:16:27 2026 +0200
document that format strings are trusted developer input
Composite formatting honours the alignment of a format item before anything
can reject it, so a format such as "{0,2000000000}" allocates a buffer of
that size, and the OutOfMemoryException is fatal and escapes the catch that
otherwise turns a bad format into an error string. Security scans report
this, so record why it is not guarded against, with a link to the threat
model.
Format strings are developer-controlled and trusted, and routing user data
into one is application misuse.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
src/log4net/Util/SystemStringFormat.cs | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/log4net/Util/SystemStringFormat.cs b/src/log4net/Util/SystemStringFormat.cs
index 0d19571e..68121368 100644
--- a/src/log4net/Util/SystemStringFormat.cs
+++ b/src/log4net/Util/SystemStringFormat.cs
@@ -82,7 +82,10 @@ public sealed class SystemStringFormat(IFormatProvider? provider, string format,
return format;
}
- // Try to format the string
+ // An alignment such as "{0,2000000000}" allocates before anything can reject it, and the
+ // OutOfMemoryException is fatal and escapes the catch below. Not guarded against: the format
+ // string is trusted developer input, see
+ // https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
return string.Format(provider, format, args);
}
catch (Exception e) when (!e.IsFatal())
(logging-log4net) 07/19: add a TransportSecurity option to the MailKit SmtpAppender
Posted by fr...@apache.org.
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 bd35fe0d5d9d83150fed88e63fb7eb4e887174ea
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 22:15:15 2026 +0200
add a TransportSecurity option to the MailKit SmtpAppender
EnableSsl mapped to MailKit's SecureSocketOptions.Auto, which is
opportunistic on every port other than 465. An attacker able to strip
STARTTLS from the EHLO response silently downgraded the session to
plaintext, taking the credentials passed to Authenticate and the log
content with it, while the operator had asked for an encrypted connection.
EnableSsl now requires transport security: implicit TLS on port 465 and
mandatory STARTTLS elsewhere, so connecting fails when the server offers no
TLS, as System.Net.Mail.SmtpClient.EnableSsl does. The appender ships for
the first time in this release, so no configuration changes behaviour.
Opportunistic STARTTLS is still reachable, but only by asking for it. The
new TransportSecurity option carries the full set of modes and EnableSsl
became a shorthand for it, so the two cannot disagree. TransportSecurity
also covers a server expecting implicit TLS on a port other than 465, which
neither Auto nor the legacy appender could reach.
The option uses its own enum rather than MailKit's SecureSocketOptions,
which is not CLS compliant and would have needed CLSCompliant(false) on the
primary TLS setting.
The remarks on EnableSsl offered a custom ISmtpTransport for finer control,
which no caller can supply because both the interface and the constructor
taking it are internal. Removed, since TransportSecurity is the answer now.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
.../309-require-tls-when-enablessl-is-set.xml | 15 ++++
.../Appender/SmtpAppenderTest.cs | 76 ++++++++++++++++++++-
src/log4net.Ext.Mail/Appender/SmtpAppender.cs | 75 ++++++++++++++++----
.../Appender/SmtpTransportSecurity.cs | 79 ++++++++++++++++++++++
.../configuration/appenders/adonetappender.adoc | 2 +-
.../configuration/appenders/smtpappender.adoc | 58 +++++++++++++++-
.../configuration/appenders/telnetappender.adoc | 4 +-
7 files changed, 289 insertions(+), 20 deletions(-)
diff --git a/src/changelog/3.4.0/309-require-tls-when-enablessl-is-set.xml b/src/changelog/3.4.0/309-require-tls-when-enablessl-is-set.xml
new file mode 100644
index 00000000..0e279930
--- /dev/null
+++ b/src/changelog/3.4.0/309-require-tls-when-enablessl-is-set.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="added">
+ <issue id="309" link="https://github.com/apache/logging-log4net/pull/309"/>
+ <description format="asciidoc">
+ add a `TransportSecurity` option to the `log4net.Ext.Mail` `SmtpAppender` and make `EnableSsl`
+ a shorthand for it. `EnableSsl` requires transport security, selecting implicit TLS on port 465 and
+ mandatory `STARTTLS` elsewhere, so connecting fails when the server offers no TLS instead of
+ silently continuing in plaintext, as `System.Net.Mail.SmtpClient.EnableSsl` does. Opportunistic
+ `STARTTLS` remains available, but only by asking for it with
+ `TransportSecurity=StartTlsWhenAvailable` (audit 1231d72-f007)
+ </description>
+</entry>
diff --git a/src/log4net.Ext.Mail.Tests/Appender/SmtpAppenderTest.cs b/src/log4net.Ext.Mail.Tests/Appender/SmtpAppenderTest.cs
index bb23df02..33810395 100644
--- a/src/log4net.Ext.Mail.Tests/Appender/SmtpAppenderTest.cs
+++ b/src/log4net.Ext.Mail.Tests/Appender/SmtpAppenderTest.cs
@@ -207,15 +207,87 @@ public void EnableSslOffConnectsWithoutTransportSecurity()
Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.None));
}
+ /// <summary>
+ /// SecureSocketOptions.Auto is opportunistic away from port 465, so an attacker who strips
+ /// STARTTLS from the EHLO response downgrades the session to plaintext. Asking for SSL has to
+ /// mean mandatory STARTTLS.
+ /// </summary>
[Test]
- public void EnableSslOnNegotiatesTransportSecurity()
+ public void EnableSslOnRequiresStartTls()
{
SmtpAppender appender = CreateAppender();
appender.EnableSsl = true;
Append(appender);
- Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.Auto));
+ Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.StartTls));
+ }
+
+ /// <summary>
+ /// Port 465 is implicit TLS: the session is encrypted before the SMTP greeting, so STARTTLS is
+ /// never offered there and must not be demanded.
+ /// </summary>
+ [Test]
+ public void EnableSslOnPort465UsesImplicitTls()
+ {
+ SmtpAppender appender = CreateAppender();
+ appender.EnableSsl = true;
+ appender.Port = 465;
+
+ Append(appender);
+
+ Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.SslOnConnect));
+ }
+
+ /// <summary>
+ /// A server doing implicit TLS on a port other than 465 cannot be reached with Required, which
+ /// would try to negotiate STARTTLS there.
+ /// </summary>
+ [Test]
+ public void ImplicitTlsUsesSslOnConnectRegardlessOfThePort()
+ {
+ SmtpAppender appender = CreateAppender();
+ appender.TransportSecurity = SmtpTransportSecurity.ImplicitTls;
+ appender.Port = 8465;
+
+ Append(appender);
+
+ Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.SslOnConnect));
+ }
+
+ /// <summary>
+ /// Opportunistic transport security stays available, but only for an operator who asks for it.
+ /// </summary>
+ [Test]
+ public void StartTlsWhenAvailableIsOpportunistic()
+ {
+ SmtpAppender appender = CreateAppender();
+ appender.TransportSecurity = SmtpTransportSecurity.StartTlsWhenAvailable;
+
+ Append(appender);
+
+ Assert.That(_transport.SecureSocketOptions, Is.EqualTo(SecureSocketOptions.StartTlsWhenAvailable));
+ }
+
+ /// <summary>
+ /// EnableSsl and TransportSecurity are the same setting, so they cannot disagree.
+ /// </summary>
+ [Test]
+ public void EnableSslIsAShorthandForTransportSecurity()
+ {
+ SmtpAppender appender = CreateAppender();
+
+ Assert.That(appender.TransportSecurity, Is.EqualTo(SmtpTransportSecurity.None));
+ Assert.That(appender.EnableSsl, Is.False);
+
+ appender.EnableSsl = true;
+ Assert.That(appender.TransportSecurity, Is.EqualTo(SmtpTransportSecurity.Required));
+
+ appender.TransportSecurity = SmtpTransportSecurity.StartTlsWhenAvailable;
+ Assert.That(appender.EnableSsl, Is.True);
+
+ appender.EnableSsl = false;
+ Assert.That(appender.TransportSecurity, Is.EqualTo(SmtpTransportSecurity.None));
}
[Test]
diff --git a/src/log4net.Ext.Mail/Appender/SmtpAppender.cs b/src/log4net.Ext.Mail/Appender/SmtpAppender.cs
index 54ce6a91..e2d631a4 100644
--- a/src/log4net.Ext.Mail/Appender/SmtpAppender.cs
+++ b/src/log4net.Ext.Mail/Appender/SmtpAppender.cs
@@ -69,6 +69,12 @@ namespace log4net.Ext.Mail.Appender;
/// </remarks>
public class SmtpAppender : BufferingAppenderSkeleton
{
+ /// <summary>
+ /// The port reserved for SMTP over implicit TLS, where TLS starts before the SMTP greeting
+ /// rather than being negotiated with <c>STARTTLS</c>.
+ /// </summary>
+ private const int ImplicitTlsPort = 465;
+
private readonly Func<ISmtpTransport> _transportFactory;
/// <summary>
@@ -239,18 +245,40 @@ internal SmtpAppender(Func<ISmtpTransport> transportFactory)
/// </summary>
/// <remarks>
/// <para>
- /// When <see langword="true"/>, <see cref="SecureSocketOptions.Auto"/> is used, which
- /// negotiates implicit TLS or <c>STARTTLS</c> depending on the <see cref="Port"/> and
- /// what the server advertises. When <see langword="false"/> the connection is not
- /// encrypted at all (<see cref="SecureSocketOptions.None"/>), matching the behaviour of
- /// <see cref="System.Net.Mail.SmtpClient.EnableSsl"/>.
+ /// This is a shorthand for <see cref="TransportSecurity"/>: setting it to
+ /// <see langword="true"/> selects <see cref="SmtpTransportSecurity.Required"/> and setting it to
+ /// <see langword="false"/> selects <see cref="SmtpTransportSecurity.None"/>. The two properties
+ /// are the same setting, so the one assigned last wins.
/// </para>
/// <para>
- /// Use <see cref="SecureSocketOptions"/> directly via a custom
- /// <see cref="ISmtpTransport"/> if you need finer control.
+ /// When <see langword="true"/>, transport security is required: implicit TLS on port 465 and
+ /// <c>STARTTLS</c> on every other port. Connecting fails if the server does not offer TLS,
+ /// rather than continuing unencrypted, which matches the behaviour of
+ /// <see cref="System.Net.Mail.SmtpClient.EnableSsl"/>. Use <see cref="TransportSecurity"/> when
+ /// the server needs something else.
+ /// </para>
+ /// </remarks>
+ public bool EnableSsl
+ {
+ get => TransportSecurity != SmtpTransportSecurity.None;
+ set => TransportSecurity = value ? SmtpTransportSecurity.Required : SmtpTransportSecurity.None;
+ }
+
+ /// <summary>
+ /// Gets or sets how the connection to the SMTP server is secured.
+ /// </summary>
+ /// <value>
+ /// One of the <see cref="SmtpTransportSecurity"/> values. The default is
+ /// <see cref="SmtpTransportSecurity.None"/>.
+ /// </value>
+ /// <remarks>
+ /// <para>
+ /// <see cref="EnableSsl"/> is a shorthand for this property and covers the usual cases; set this
+ /// one when the server expects implicit TLS on a port other than 465, or when only opportunistic
+ /// <c>STARTTLS</c> is possible.
/// </para>
/// </remarks>
- public bool EnableSsl { get; set; }
+ public SmtpTransportSecurity TransportSecurity { get; set; } = SmtpTransportSecurity.None;
/// <summary>
/// Gets or sets the reply-to e-mail address.
@@ -315,6 +343,32 @@ protected override void SendBuffer(LoggingEvent[] events)
/// </summary>
protected override bool RequiresLayout => true;
+ /// <summary>
+ /// Translates <see cref="TransportSecurity"/> into the mail library's own representation.
+ /// </summary>
+ /// <returns>The transport security to connect with.</returns>
+ /// <remarks>
+ /// <para>
+ /// <see cref="SecureSocketOptions.Auto"/> is deliberately never used: away from port 465 it is
+ /// opportunistic, so an attacker who strips <c>STARTTLS</c> from the EHLO response silently
+ /// downgrades the session to plaintext, taking the credentials and the log content with it.
+ /// Opportunistic behavior is available, but only by asking for it with
+ /// <see cref="SmtpTransportSecurity.StartTlsWhenAvailable"/>.
+ /// </para>
+ /// </remarks>
+ private SecureSocketOptions ResolveSecureSocketOptions() => TransportSecurity switch
+ {
+ SmtpTransportSecurity.None => SecureSocketOptions.None,
+ SmtpTransportSecurity.Required => Port == ImplicitTlsPort
+ ? SecureSocketOptions.SslOnConnect
+ : SecureSocketOptions.StartTls,
+ SmtpTransportSecurity.ImplicitTls => SecureSocketOptions.SslOnConnect,
+ SmtpTransportSecurity.StartTls => SecureSocketOptions.StartTls,
+ SmtpTransportSecurity.StartTlsWhenAvailable => SecureSocketOptions.StartTlsWhenAvailable,
+ _ => throw SystemInfo.CreateArgumentOutOfRangeException(nameof(TransportSecurity), TransportSecurity,
+ $"The value specified for TransportSecurity is not one of the {nameof(SmtpTransportSecurity)} values.")
+ };
+
/// <summary>
/// Send the email message
/// </summary>
@@ -324,10 +378,7 @@ protected virtual void SendEmail(string messageBody)
using MimeMessage message = CreateMessage(messageBody);
using ISmtpTransport transport = _transportFactory().EnsureNotNull();
- transport.Connect(
- SmtpHost.EnsureNotNullOrEmpty(),
- Port,
- EnableSsl ? SecureSocketOptions.Auto : SecureSocketOptions.None);
+ transport.Connect(SmtpHost.EnsureNotNullOrEmpty(), Port, ResolveSecureSocketOptions());
try
{
switch (Authentication)
diff --git a/src/log4net.Ext.Mail/Appender/SmtpTransportSecurity.cs b/src/log4net.Ext.Mail/Appender/SmtpTransportSecurity.cs
new file mode 100644
index 00000000..a6ec293b
--- /dev/null
+++ b/src/log4net.Ext.Mail/Appender/SmtpTransportSecurity.cs
@@ -0,0 +1,79 @@
+#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.Ext.Mail.Appender;
+
+/// <summary>
+/// How <see cref="SmtpAppender"/> secures its connection to the SMTP server.
+/// </summary>
+/// <remarks>
+/// <para>
+/// This mirrors the transport security modes of the underlying mail library without exposing its
+/// types, so that the appender's configuration surface stays CLS compliant.
+/// </para>
+/// </remarks>
+public enum SmtpTransportSecurity
+{
+ /// <summary>
+ /// The connection is not encrypted.
+ /// </summary>
+ None,
+
+ /// <summary>
+ /// Transport security is required, and the mechanism follows the port: implicit TLS on port 465,
+ /// <c>STARTTLS</c> on every other port.
+ /// </summary>
+ /// <remarks>
+ /// <para>
+ /// Connecting fails when the server does not offer transport security, rather than continuing
+ /// unencrypted. This is what <see cref="SmtpAppender.EnableSsl"/> selects and is the right
+ /// choice unless the server does something unusual.
+ /// </para>
+ /// </remarks>
+ Required,
+
+ /// <summary>
+ /// The session is encrypted before the SMTP greeting, without <c>STARTTLS</c>.
+ /// </summary>
+ /// <remarks>
+ /// <para>
+ /// Use this for a server that expects implicit TLS on a port other than 465, which
+ /// <see cref="Required"/> would try to negotiate with <c>STARTTLS</c> instead.
+ /// </para>
+ /// </remarks>
+ ImplicitTls,
+
+ /// <summary>
+ /// <c>STARTTLS</c> is required, whatever the port.
+ /// </summary>
+ StartTls,
+
+ /// <summary>
+ /// <c>STARTTLS</c> is used when the server advertises it, and the connection continues
+ /// unencrypted when it does not.
+ /// </summary>
+ /// <remarks>
+ /// <para>
+ /// This is opportunistic: an attacker who can modify the traffic can remove the server's
+ /// <c>STARTTLS</c> advertisement and the session then proceeds in plaintext, exposing the
+ /// credentials and the log content. Only choose it for a network where that is acceptable.
+ /// </para>
+ /// </remarks>
+ StartTlsWhenAvailable
+}
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 06e91c5b..37a43137 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
@@ -35,7 +35,7 @@ Always configure `CommandText` together with `parameter` elements, as every exam
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
+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.
====
diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtpappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtpappender.adoc
index 2f5ca1ee..0c33679b 100644
--- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtpappender.adoc
+++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/smtpappender.adoc
@@ -190,7 +190,12 @@ This example authenticates against a mail server that requires an encrypted conn
|The port the SMTP server listens on. Defaults to `25`.
|`enableSsl`
-|Whether to secure the connection. Defaults to `false`. See xref:#mailkit-smtpappender-differences[].
+|Whether to require transport security. Defaults to `false`.
+Shorthand for `transportSecurity`: `true` selects `Required`, `false` selects `None`.
+
+|`transportSecurity`
+|How the connection is secured. One of `None` (default), `Required`, `ImplicitTls`, `StartTls` or `StartTlsWhenAvailable`.
+See xref:#mailkit-smtpappender-transport-security[].
|`authentication`
|One of `None` (default), `Basic` or `Ntlm`. `Basic` and `Ntlm` both require `username` and `password`.
@@ -222,17 +227,64 @@ This example authenticates against a mail server that requires an encrypted conn
`to`, `cc` and `bcc` also accept semicolons as separators, and quoted display names such as `"Doe, John" <jo...@example.com>`.
+[#mailkit-smtpappender-transport-security]
+=== Transport security
+
+`enableSsl` covers the usual cases and behaves as it does in the legacy appender: `true` requires
+transport security, so connecting fails when the server does not offer it rather than continuing
+unencrypted.
+
+`transportSecurity` is the same setting expressed precisely, for servers that need something else.
+The two properties cannot disagree: whichever is assigned last wins.
+
+[cols="Value,Description"]
+|===
+|Value |Description
+
+|`None`
+|The connection is not encrypted. Equivalent to `enableSsl` set to `false`, and the default.
+
+|`Required`
+|Transport security is required and the mechanism follows the port: implicit TLS on port 465,
+`STARTTLS` on every other port. Equivalent to `enableSsl` set to `true`.
+
+|`ImplicitTls`
+|The session is encrypted before the SMTP greeting, whatever the port.
+Use this for a server expecting implicit TLS on a port other than 465, which `Required` would try
+to negotiate with `STARTTLS` instead.
+
+|`StartTls`
+|`STARTTLS` is required, whatever the port.
+
+|`StartTlsWhenAvailable`
+|`STARTTLS` is used when the server advertises it, and the connection continues unencrypted when it
+does not.
+|===
+
+[WARNING]
+====
+`StartTlsWhenAvailable` is opportunistic.
+An attacker able to modify the traffic can remove the server's `STARTTLS` advertisement, and the
+session then proceeds in plaintext, exposing the credentials sent by `authentication` and the log
+content itself.
+Only choose it for a network where that is acceptable, and prefer `Required`.
+====
+
[#mailkit-smtpappender-differences]
=== Differences from the legacy appender
The options above are named exactly as in the legacy appender, but a few behave differently:
* `smtpHost` is *required*. MailKit has no notion of a machine-wide default SMTP server, so there is nothing to fall back on when the option is omitted.
-* `enableSsl` set to `true` negotiates transport security automatically: implicit TLS on port 465, otherwise `STARTTLS` when the server advertises it.
-Set to `false`, the connection is not encrypted at all.
* `authentication` set to `Ntlm` requires `username` and `password`.
MailKit cannot reuse the Windows logon session of the current thread or process, which the legacy appender did.
* Semicolon-delimited recipient lists in `to`, `cc` and `bcc` are parsed correctly.
+* `transportSecurity` has no counterpart in the legacy appender, which offers only `enableSsl`.
+
+`enableSsl` keeps its meaning, so a migrated configuration secures the connection exactly as before.
+If the legacy appender reached your server with `enableSsl` set to `true`, so does this one.
+The new `transportSecurity` option is only needed for a server that the legacy appender could not
+reach either, such as one expecting implicit TLS on a port other than 465.
[#legacy-smtpappender]
== Built-in SmtpAppender (deprecated)
diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc
index 27891b57..6ce35769 100644
--- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc
+++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc
@@ -23,7 +23,7 @@ every connected client, so that a running application's log can be watched over
telnet client.
Unlike every other appender, it does not write to a destination you configure: it accepts
connections from clients that reach it.
-It is intended for diagnostic use on trusted networks -- see <<telnetappender-trust>>.
+It is intended for diagnostic use on trusted networks; see <<telnetappender-trust>>.
At most 20 clients may be connected at the same time; further connection attempts are answered
with a message and closed.
@@ -78,7 +78,7 @@ interfaces*.
There is no option to restrict the listen address, require a credential, or enable TLS.
Any client that can reach the port receives the full rendered log stream, including whatever the
-layout renders -- user names, session identifiers, request parameters, stack traces.
+layout renders: user names, session identifiers, request parameters, stack traces.
Keeping untrusted parties away from the port is the operator's responsibility, exactly as it is
for a log file:
(logging-log4net) 17/19: add a listen address to TelnetAppender
Posted by fr...@apache.org.
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 15d16eedf5804c5b441db52096d4ee018a4e5028
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 23:56:00 2026 +0200
add a listen address to TelnetAppender
The listening socket was bound to IPAddress.Any with no way to scope it, so
an operator who only wanted to watch the log from the machine itself still
got a listener on every interface.
ListenAddress fills that gap. The default is unchanged, so nothing moves
unless it is set: the connecting client is trusted, as the manual now
states, and flipping the default would break every remote monitoring setup
on upgrade.
The listening socket now takes its family from the address rather than
always being InterNetwork, so an IPv6 address works too.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
src/changelog/3.4.0/309-telnet-listen-address.xml | 13 ++++++
src/log4net.Tests/Appender/TelnetAppenderTest.cs | 52 ++++++++++++++++++++++
src/log4net/Appender/TelnetAppender.cs | 47 +++++++++++++++++--
.../configuration/appenders/telnetappender.adoc | 16 +++++--
4 files changed, 121 insertions(+), 7 deletions(-)
diff --git a/src/changelog/3.4.0/309-telnet-listen-address.xml b/src/changelog/3.4.0/309-telnet-listen-address.xml
new file mode 100644
index 00000000..05853417
--- /dev/null
+++ b/src/changelog/3.4.0/309-telnet-listen-address.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">
+ add `ListenAddress` to `TelnetAppender`. The listening socket was bound to `IPAddress.Any` with
+ no way to scope it, so an operator who only wanted to watch the log from the machine itself still
+ got a listener on every interface. The default is unchanged, and the listening socket now follows
+ the address family, so an IPv6 address works too (audit 1231d72-f002)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Appender/TelnetAppenderTest.cs b/src/log4net.Tests/Appender/TelnetAppenderTest.cs
index e5c238c3..5d8f37cd 100644
--- a/src/log4net.Tests/Appender/TelnetAppenderTest.cs
+++ b/src/log4net.Tests/Appender/TelnetAppenderTest.cs
@@ -27,6 +27,7 @@
using log4net.Appender;
using log4net.Config;
using log4net.Core;
+using log4net.Layout;
using log4net.Repository;
using log4net.Tests.Appender.Internal;
using NUnit.Framework;
@@ -156,6 +157,57 @@ public void SendTimeoutMillisRejectsNegativeValuesButAllowsZero()
Assert.That(appender.SendTimeoutMillis, Is.EqualTo(250));
}
+ /// <summary>
+ /// The appender accepts connections on every interface unless told otherwise, which is the
+ /// behaviour it has always had.
+ /// </summary>
+ [Test]
+ public void ListenAddressDefaultsToEveryInterface()
+ => Assert.That(new TelnetAppender().ListenAddress, Is.EqualTo(IPAddress.Any));
+
+ /// <summary>
+ /// Binding to the loopback address has to keep the port unreachable from other machines, which
+ /// is what an operator asking for it wants.
+ /// </summary>
+ [Test]
+ [NonParallelizable]
+ public void ListenAddressBindsOnlyThatAddress()
+ {
+ int port = FindFreeTcpPort();
+ TelnetAppender appender = new()
+ {
+ Port = port,
+ ListenAddress = IPAddress.Loopback,
+ Layout = new PatternLayout("%message%newline")
+ };
+ appender.ActivateOptions();
+ try
+ {
+ // The loopback listener accepts a loopback connection.
+ using (Socket loopback = new(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp))
+ {
+ loopback.Connect(new IPEndPoint(IPAddress.Loopback, port));
+ Assert.That(loopback.Connected, Is.True);
+ }
+
+ // and nothing is listening on the machine's other addresses
+ IPAddress? routable = Array.Find(
+ Dns.GetHostAddresses(Dns.GetHostName()),
+ address => address.AddressFamily == AddressFamily.InterNetwork && !IPAddress.IsLoopback(address));
+ if (routable is null)
+ {
+ Assert.Ignore("no non-loopback IPv4 address on this machine to test against");
+ }
+
+ using Socket external = new(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
+ Assert.That(() => external.Connect(new IPEndPoint(routable!, port)), Throws.TypeOf<SocketException>());
+ }
+ finally
+ {
+ appender.Close();
+ }
+ }
+
/// <summary>
/// Asks the OS for a currently unused TCP port - a fixed port would collide with
/// other tests or processes on the build machine.
diff --git a/src/log4net/Appender/TelnetAppender.cs b/src/log4net/Appender/TelnetAppender.cs
index e62e9346..a0b04e13 100644
--- a/src/log4net/Appender/TelnetAppender.cs
+++ b/src/log4net/Appender/TelnetAppender.cs
@@ -55,6 +55,28 @@ public class TelnetAppender : AppenderSkeleton
private SocketHandler? _handler;
private int _listeningPort = 23;
private int _sendTimeoutMillis = 5_000;
+ private IPAddress _listenAddress = IPAddress.Any;
+
+ /// <summary>
+ /// Gets or sets the address to listen on.
+ /// </summary>
+ /// <value>
+ /// The local address to accept connections on. The default is <see cref="IPAddress.Any"/>, every
+ /// interface of the machine.
+ /// </value>
+ /// <remarks>
+ /// <para>
+ /// Set this to <see cref="IPAddress.Loopback"/> to accept connections only from the machine the
+ /// application runs on, which is what the diagnostic use this appender is meant for usually
+ /// needs.
+ /// </para>
+ /// </remarks>
+ /// <exception cref="ArgumentNullException">The value specified is <see langword="null"/>.</exception>
+ public IPAddress ListenAddress
+ {
+ get => _listenAddress;
+ set => _listenAddress = value.EnsureNotNull();
+ }
/// <summary>
/// The fully qualified type of the TelnetAppender class.
@@ -158,8 +180,8 @@ public override void ActivateOptions()
base.ActivateOptions();
try
{
- LogLog.Debug(_declaringType, $"Creating SocketHandler to listen on port [{_listeningPort}]");
- _handler = new SocketHandler(_listeningPort, _sendTimeoutMillis);
+ LogLog.Debug(_declaringType, $"Creating SocketHandler to listen on [{_listenAddress}]:[{_listeningPort}]");
+ _handler = new SocketHandler(_listenAddress, _listeningPort, _sendTimeoutMillis);
}
catch (Exception ex)
{
@@ -303,10 +325,27 @@ public SocketHandler(int port)
/// </para>
/// </remarks>
public SocketHandler(int port, int sendTimeoutMillis)
+ : this(IPAddress.Any, port, sendTimeoutMillis)
+ { }
+
+ /// <summary>
+ /// Opens a new server port on <paramref ref="port"/> of <paramref ref="listenAddress"/>
+ /// </summary>
+ /// <param name="listenAddress">the local address to accept connections on</param>
+ /// <param name="port">the local port to listen on for connections</param>
+ /// <param name="sendTimeoutMillis">the time, in milliseconds, that a write to a client may
+ /// block before that client is disconnected, or 0 to block indefinitely</param>
+ /// <remarks>
+ /// <para>
+ /// Creates a socket handler on the specified local address and server port.
+ /// </para>
+ /// </remarks>
+ public SocketHandler(IPAddress listenAddress, int port, int sendTimeoutMillis)
{
_sendTimeoutMillis = sendTimeoutMillis;
- _serverSocket = new(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
- _serverSocket.Bind(new IPEndPoint(IPAddress.Any, port));
+ // The address decides the family, so that an IPv6 address does not end up on an IPv4 socket.
+ _serverSocket = new(listenAddress.EnsureNotNull().AddressFamily, SocketType.Stream, ProtocolType.Tcp);
+ _serverSocket.Bind(new IPEndPoint(listenAddress, port));
_serverSocket.Listen(5);
AcceptConnection();
}
diff --git a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc
index 6ce35769..6751ca0c 100644
--- a/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc
+++ b/src/site/antora/modules/ROOT/pages/manual/configuration/appenders/telnetappender.adoc
@@ -33,6 +33,7 @@ The following example configures the appender to listen on port 8023.
[source,xml]
----
<appender name="TelnetAppender" type="log4net.Appender.TelnetAppender">
+ <listenAddress value="127.0.0.1" />
<port value="8023" />
<sendTimeoutMillis value="5000" />
<layout type="log4net.Layout.PatternLayout">
@@ -48,6 +49,14 @@ The following example configures the appender to listen on port 8023.
The TCP port to listen on.
The default is `23`, the telnet port.
+`listenAddress`::
+The local address to accept connections on.
+The default is `0.0.0.0`, every interface of the machine.
++
+Set it to `127.0.0.1` to accept connections only from the machine the application runs on, which is
+what diagnostic use usually needs.
+An IPv6 address may be given instead, and the listening socket follows its family.
+
`sendTimeoutMillis`::
How long, in milliseconds, a write to a client may block before that client is treated as dead
and disconnected.
@@ -73,15 +82,16 @@ The appender therefore performs no authentication of its own.
[WARNING]
====
-The connection is *unauthenticated* and *unencrypted*, and the appender listens on *all network
-interfaces*.
-There is no option to restrict the listen address, require a credential, or enable TLS.
+The connection is *unauthenticated* and *unencrypted*, and by default the appender listens on *all
+network interfaces*.
+There is no option to require a credential or to enable TLS.
Any client that can reach the port receives the full rendered log stream, including whatever the
layout renders: user names, session identifiers, request parameters, stack traces.
Keeping untrusted parties away from the port is the operator's responsibility, exactly as it is
for a log file:
+* Set `listenAddress` to `127.0.0.1` unless clients on other machines really have to connect.
* Only enable this appender on a trusted network.
* Restrict access to the port with a host firewall or network policy.
* Prefer it for local or short-lived diagnostics rather than as a permanent logging destination.
(logging-log4net) 05/19: redact the password when reporting a failed database connection
Posted by fr...@apache.org.
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 e80b3810acb3a2c9cb9788dab1ef1b9e4aedcf42
Author: Jan Friedrich <fr...@apache.org>
AuthorDate: Mon Aug 17 21:54:18 2026 +0200
redact the password when reporting a failed database connection
InitializeDatabaseConnection named the resolved connection string in full
when it could not open the connection, and the documented examples embed
Password=... The message goes through the ErrorHandler, so it is what an
operator sees while diagnosing exactly this failure.
Password-bearing keywords are now replaced with *****. The rest of the
connection string is kept, so the message stays useful for spotting a typo
in the server name or catalog. If the string cannot be parsed - likely,
given that it just failed to connect - all of it is redacted.
This matters more since appender errors became visible without
log4net.Internal.Debug: the password would otherwise have reached stderr in
a default configuration.
Co-Authored-By: Claude Opus 5 (1M context) <no...@anthropic.com>
---
...redact-password-in-connection-string-errors.xml | 13 +++++
.../Appender/AdoNet/Log4NetConnection.cs | 14 +++++-
src/log4net.Tests/Appender/AdoNetAppenderTest.cs | 40 ++++++++++++++++
src/log4net/Appender/AdoNetAppender.cs | 56 +++++++++++++++++++++-
4 files changed, 121 insertions(+), 2 deletions(-)
diff --git a/src/changelog/3.4.0/309-redact-password-in-connection-string-errors.xml b/src/changelog/3.4.0/309-redact-password-in-connection-string-errors.xml
new file mode 100644
index 00000000..bcc07ebb
--- /dev/null
+++ b/src/changelog/3.4.0/309-redact-password-in-connection-string-errors.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="309" link="https://github.com/apache/logging-log4net/pull/309"/>
+ <description format="asciidoc">
+ stop `AdoNetAppender` from repeating the password when it reports a connection it could not
+ open. The message named the resolved connection string in full, and the documented examples embed
+ `Password=...` (CWE-532). Password-bearing keywords are now replaced with `*****`, while the rest
+ of the connection string is kept so the message stays useful (audit 1231d72-f018)
+ </description>
+</entry>
diff --git a/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs b/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs
index e11411b0..43364c51 100644
--- a/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs
+++ b/src/log4net.Tests/Appender/AdoNet/Log4NetConnection.cs
@@ -44,7 +44,19 @@ internal sealed class Log4NetConnection : IDbConnection
public IDbCommand CreateCommand() => new Log4NetCommand();
- public void Open() => _open = true;
+ public void Open()
+ {
+ if (FailOnOpen)
+ {
+ throw new InvalidOperationException("Simulated failure to open the connection");
+ }
+ _open = true;
+ }
+
+ /// <summary>
+ /// When set, <see cref="Open"/> throws, simulating a connection that cannot be established.
+ /// </summary>
+ public static bool FailOnOpen { get; set; }
public static Log4NetConnection? MostRecentInstance { get; private set; }
diff --git a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs
index a07744c8..3e391586 100644
--- a/src/log4net.Tests/Appender/AdoNetAppenderTest.cs
+++ b/src/log4net.Tests/Appender/AdoNetAppenderTest.cs
@@ -265,6 +265,46 @@ public void BufferingWebsiteExample()
Assert.That(param.Value, Is.Empty);
}
+ /// <summary>
+ /// The message reporting a failed connection must not repeat the password from the connection
+ /// string. The appender reports the failure through its ErrorHandler, so this message is what
+ /// an operator sees on stderr in a default configuration.
+ /// </summary>
+ [Test]
+ [NonParallelizable]
+ public void FailedConnectionDoesNotReportThePassword()
+ {
+ const string password = "H0rseBatteryStaple";
+ List<LogLog> messages = [];
+ try
+ {
+ Log4NetConnection.FailOnOpen = true;
+ LogLog.ExecuteWithoutEmittingInternalMessages(() =>
+ {
+ using LogLog.LogReceivedAdapter _ = new(messages);
+ AdoNetAppender adoNetAppender = new()
+ {
+ BufferSize = -1,
+ ConnectionType = typeof(Log4NetConnection).AssemblyQualifiedName!,
+ ConnectionString = $"data source=someserver;initial catalog=somedb;User ID=someuser;Password={password}",
+ CommandText = "INSERT INTO Log ([Message]) VALUES (@message)"
+ };
+ adoNetAppender.ActivateOptions();
+ });
+
+ string reported = string.Join(Environment.NewLine, messages.ConvertAll(m => m.Message));
+
+ Assert.That(reported, Does.Not.Contain(password));
+ Assert.That(reported, Does.Contain("Could not open database connection"));
+ // The rest of the connection string survives, so the message stays useful for diagnosis.
+ Assert.That(reported, Does.Contain("someserver"));
+ }
+ finally
+ {
+ Log4NetConnection.FailOnOpen = false;
+ }
+ }
+
/// <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.
diff --git a/src/log4net/Appender/AdoNetAppender.cs b/src/log4net/Appender/AdoNetAppender.cs
index 0faf995e..b4cd2209 100644
--- a/src/log4net/Appender/AdoNetAppender.cs
+++ b/src/log4net/Appender/AdoNetAppender.cs
@@ -21,6 +21,7 @@
using System.Collections.Generic;
using System.Configuration;
using System.Data;
+using System.Data.Common;
using System.IO;
using log4net.Util;
@@ -788,12 +789,65 @@ private void InitializeDatabaseConnection()
catch (Exception e) when (!e.IsFatal())
{
// Sadly, your connection string is bad.
- ErrorHandler.Error($"Could not open database connection [{resolvedConnectionString}]. Connection string context [{connectionStringContext}].", e);
+ ErrorHandler.Error($"Could not open database connection [{RedactConnectionString(resolvedConnectionString)}]. Connection string context [{connectionStringContext}].", e);
Connection = null;
}
}
+ /// <summary>
+ /// Replaces the values of password-bearing keywords in a connection string with
+ /// <see cref="RedactedValue"/>, so that it can be named in a diagnostic message.
+ /// </summary>
+ /// <param name="connectionString">The connection string to redact.</param>
+ /// <returns>
+ /// The connection string with every password value replaced, or <see cref="RedactedValue"/> if it
+ /// could not be parsed.
+ /// </returns>
+ private static string RedactConnectionString(string connectionString)
+ {
+ if (string.IsNullOrEmpty(connectionString))
+ {
+ return connectionString;
+ }
+
+ try
+ {
+ DbConnectionStringBuilder builder = new() { ConnectionString = connectionString };
+
+ List<string> keys = [];
+ foreach (string key in builder.Keys)
+ {
+ keys.Add(key);
+ }
+
+ foreach (string key in keys)
+ {
+ // Providers spell the secret differently - Password, PWD, User Password - so match on
+ // the keyword rather than on a fixed list.
+ if (key.IndexOf("password", StringComparison.OrdinalIgnoreCase) >= 0
+ || key.Equals("pwd", StringComparison.OrdinalIgnoreCase))
+ {
+ builder[key] = RedactedValue;
+ }
+ }
+
+ return builder.ConnectionString;
+ }
+ catch (Exception e) when (!e.IsFatal())
+ {
+ // The connection string could not be parsed - which is likely, given that it just failed
+ // to connect - so redact all of it rather than risk echoing a password.
+ LogLog.Debug(_declaringType, "Could not parse the connection string in order to redact it", e);
+ return RedactedValue;
+ }
+ }
+
+ /// <summary>
+ /// Stands in for a password in diagnostic messages.
+ /// </summary>
+ private const string RedactedValue = "*****";
+
/// <summary>
/// Cleanup the existing connection.
/// </summary>