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 <[email protected]> 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) <[email protected]> --- .../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. +====
