Harden log4net against the findings of a security audit - #310
Merged
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
fluffynuts
approved these changes
Aug 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Appenders: network and transport
Appenders: syslog record integrity
AdoNet appender
Reliability and resource bounds
Diagnostics
Build and release infrastructure
Documentation
Notes for reviewers
Several commits introduce secure defaults with a named opt-out (SendTimeoutMillis, MatchTimeoutMillis, LockTimeoutMillis); those are behaviour changes on upgrade and are recorded in the changelog. The documentation commits deliberately settle recurring reports in the threat model rather than changing code.