Harden log4net against the findings of a security audit

.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

AGENTS.md

@@ -10,14 +10,29 @@ This is mandatory: skipping steps leads to duplicate reports and wasted time for
 ### Step 1: Read the security model
 
 Fetch and read the project's security model before evaluating any finding:
-https://raw.githubusercontent.com/apache/logging-site/refs/heads/main-site-pro/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
+https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
 
 Use this to answer:
 - Is this component/behavior in scope at all?
 - Does the project consider this a security boundary?
 
 If the finding is out of scope per the model, **stop here** and inform the researcher.
 
+Two rules from the model account for most of what gets reported wrongly. Read the model itself
+rather than relying on this summary, but know them before you start:
+
+- **Configuration is operator-supplied and trusted**, including configuration URIs and the types,
+  members and converters that configuration names. A finding that needs the attacker to control
+  configuration, a watched configuration file, or the format string of a log statement is out of
+  scope: that is application misuse. Protecting those resources, and transmitting them over a
+  confidential channel, is a deployer responsibility the model states explicitly.
+- **Content is untrusted and must never be rejected**, because rejecting it would turn a malicious
+  value into a denial of service, so it has to be escaped or sanitized instead. Structural
+  identifiers, such as logger names or the identifier fields of a structured log message, are
+  trusted, and there the model allows *rejecting* a malformed value rather than silently altering
+  it. Getting these two the wrong way round produces a plausible-looking report that proposes
+  exactly the wrong fix.
+
 ### Step 2: Check previously disclosed vulnerabilities
 
 Read the project's Vulnerability Disclosure Report to check for duplicates:
@@ -29,7 +44,7 @@ If it overlaps with a known issue, **stop here**, link to the existing advisory
 ### Step 3: Read the Security FAQ
 
 Read the Security FAQ before concluding anything is a vulnerability:
-https://raw.githubusercontent.com/apache/logging-site/refs/heads/main-site-pro/src/site/antora/modules/ROOT/pages/security/faq.adoc
+https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/security/faq.adoc
 
 The FAQ lists behaviors that are **intentional and not vulnerabilities**.
 If the finding matches an FAQ entry, inform the researcher that it is a known non-issue
@@ -52,6 +67,11 @@ Assess the finding:
 
 ## Report quality rules
 
+- **Only call something a vulnerability when it really is one.** Name the adversary, then check that
+  capability against the model. If it needs a misconfiguration, a co-resident local user, or
+  anything the model does not grant, it is a correctness bug, a reliability defect or hardening, and
+  saying so is more useful than a severity. Do not inherit the framing of a scanner report that
+  arrived with severities already attached.
 - Never speculate about impact beyond what you can demonstrate.
 - Reproduction steps must be minimal and self-contained.
 - Do not include unrelated findings in the same report: one issue per report.

CLAUDE.md

@@ -26,33 +26,33 @@ almost always be doing.
   (238 of 244 files in `src/log4net`). Copy it verbatim into new files.
 - File-scoped namespaces (`namespace log4net.Appender;`). Note `.editorconfig` still says
   `csharp_style_namespace_declarations = block_scoped:silent`, but 242 of 244 files are
-  file-scoped — follow the code, not that setting.
+  file-scoped: follow the code, not that setting.
 - `using` directives outside the namespace, in one contiguous block.
 
 ### Language usage
-- **Explicit types, not `var`** — all three `csharp_style_var_*` options are `false`.
+- **Explicit types, not `var`**: all three `csharp_style_var_*` options are `false`.
   Write `StringWriter writer = new(...)`.
 - Target-typed `new()` and collection expressions (`private static readonly char[] _x = [',', ';'];`).
-  Omit the type wherever the target is known — including `return new(…);` and `=> new(…);`, where
+  Omit the type wherever the target is known, including `return new(…);` and `=> new(…);`, where
   the enclosing member's return type supplies it. It cannot be omitted when the target type is an
   interface or abstract class, as in `Func<ISmtpTransport> f = () => new MailKitSmtpTransport();`.
-- Expression-bodied members whenever the body fits on one line — this includes constructors
+- Expression-bodied members whenever the body fits on one line, including constructors
   (`resharper_constructor_or_destructor_body = expression_body`).
 - Braces on `if`/`else` bodies even for a single statement.
 - `LangVersion` is `latest`, and current C# features are welcome and in use: primary
   constructors (`csharp_style_prefer_primary_constructors = true`), the `field` keyword in
   property accessors, list patterns, `switch` expressions.
 - **Wrap long string literals with a multi-line raw string (`"""`), never with `+`
-  concatenation.** This includes attribute arguments — see the `[Obsolete(...)]` message on
+  concatenation.** This includes attribute arguments; see the `[Obsolete(...)]` message on
   `log4net.Appender.SmtpAppender`. Raw strings have no line-continuation, so each source line
   break really is a `\n` in the value, but that is fine here: compiler diagnostics render those
   newlines as spaces, so a wrapped message still reads as one sentence. Raw strings are constant
-  expressions, so they are legal in attributes, and the feature is purely syntactic — it works on
+  expressions, so they are legal in attributes, and the feature is purely syntactic, so it works on
   `net462`/`netstandard2.0` too.
 - Private fields are `_camelCase`. Private fields and helper methods are commonly placed
   *after* the public surface of the type rather than at the top.
 
-### Nullability — the big constraint
+### Nullability, the big constraint
 - `Nullable` is enabled solution-wide with `WarningsAsErrors=nullable`: **any nullability
   warning is a build error**, so it cannot be deferred.
 - `log4net` targets `net462;netstandard2.0`. **Neither reference assembly is nullable-annotated**,
@@ -68,7 +68,7 @@ almost always be doing.
 - Use the internal `log4net.Util.Log4NetAssert` extensions rather than hand-rolled checks:
   `EnsureNotNull()`, `EnsureNotNullOrEmpty()`, `EnsureIs<T>()`. They carry
   `[CallerArgumentExpression(nameof(value))]`, so no argument name is passed at the call site.
-  This includes constructor and property assignments — write `_x = x.EnsureNotNull();`,
+  This includes constructor and property assignments: write `_x = x.EnsureNotNull();`,
   not `_x = x ?? throw new ArgumentNullException(nameof(x));`.
 - Appenders never let exceptions escape to the caller. The house pattern is
   `catch (Exception e) when (!e.IsFatal()) { ErrorHandler.Error("...", e); }`.
@@ -83,20 +83,90 @@ almost always be doing.
   requires linking `NotNullAttribute`, `ValidatedNotNullAttribute` and
   `CallerArgumentExpressionAttribute`, or you get `CS0122`.
 - Analyzers (`Microsoft.CodeAnalysis.NetAnalyzers`, `AnalysisLevel 8`, `src/log4net.globalconfig`)
-  run on every build. **The solution builds with 0 warnings — keep it that way.**
+  run on every build. **The solution builds with 0 warnings, keep it that way.**
+
+### Documentation comments
+- **Every public and protected member gets an XML doc comment**, in test code as well as production
+  code: test methods, nested helper classes and hand-written fakes included.
+- Use `/// <inheritdoc/>` when the member implements an interface or overrides a base member, and a
+  real `<summary>` for everything else. `Log4NetTransaction` in the AdoNet test doubles is the
+  pattern to copy.
+- When checking whether a member is documented, remember that `[Test]`, `#pragma` and
+  `// ReSharper disable` lines legitimately sit between the doc comment and the declaration.
+
+### Writing, in code and everywhere else
+- **Never use an em dash (`—`) or en dash (`–`).** Use a plain hyphen, or restructure with a colon,
+  comma or parentheses. This covers comments, XML docs, commit messages, AsciiDoc and chat.
+- In AsciiDoc, ` -- ` is also forbidden: Asciidoctor renders a spaced double hyphen as an em dash,
+  so it breaks the rule even though the source looks like plain hyphens. Grep touched files for
+  `[—–]` and ` -- ` before presenting a change.
+- No underscores in identifiers, including test method names. `AllContainsEveryFlag`, not
+  `All_ShouldContainAllFlags`. (Private fields are `_camelCase`, which is the one exception.)
 
 ### Tests
 - NUnit 4, not MSTest, and always the constraint model: `Assert.That(actual, Is.EqualTo(expected))`
   (810 uses of `Assert.That`, zero of `Assert.AreEqual`). `[TestFixture]`, `[Test]`, `[TestCase]`,
   with `[SetUp]`/`[TearDown]` for per-test state.
-- `NUnit.Analyzers` warnings are errors too — e.g. NUnit1032 requires an `IDisposable` fixture
+- Use an expression body for a single-statement test: `public void X() => Assert.That(...);`.
+- **`log4net` has no `InternalsVisibleTo`**, so private and internal members are exercised through
+  reflection, not by widening their accessibility. See `SystemInfoTest`, `LevelMappingTest` and
+  `UserNameFixingTest` for the `BindingFlags.Static | BindingFlags.NonPublic` pattern.
+  `log4net.Ext.Mail` does grant `InternalsVisibleTo` to its own test project.
+- Mark a test `[NonParallelizable]` when it mutates static state (`LogLog.InternalDebugging`, a
+  static field on a test double, a process-wide native registration).
+- Wrap expected internal logging in `LogLog.ExecuteWithoutEmittingInternalMessages(...)` and capture
+  it with `LogLog.LogReceivedAdapter` rather than letting it reach the console. Appender errors are
+  emitted by default, so a test that provokes one will otherwise add noise to the suite output.
+- Guard platform-specific tests with `[Platform("Win")]` / `[Platform("Linux")]`. A test that only
+  runs on Windows leaves the behaviour unverified in local Linux runs, so prefer a cross-platform
+  home for the assertion when one exists.
+- `NUnit.Analyzers` warnings are errors too: for example NUnit1032 requires an `IDisposable` fixture
   field to be disposed in a `[TearDown]` method.
 - For code that talks to the outside world, introduce a narrow interface and hand-write a fake;
   there is no mocking library in any test project. See `ISmtpTransport` / `FakeSmtpTransport`.
 - Verify with `dotnet build src/log4net.sln` and
   `dotnet test src/<project>.Tests/<project>.Tests.csproj`.
 - **When inspecting build output, redirect it to a file and read the whole thing; do not pipe
   MSBuild through line-oriented tools.** `grep`/`Select-String` cannot match across newlines, and
-  MSBuild's console logger formats differently when piped than when redirected — a multi-line
+  MSBuild's console logger formats differently when piped than when redirected, so a multi-line
   diagnostic message then looks truncated when it is not. Before reporting that the toolchain
   mangles something, re-check with `dotnet build … > out.txt 2>&1` and inspect `out.txt`.
+
+## Changelog
+
+Every user-visible change gets an entry in `src/changelog/<unreleased version>/`, named
+`<issue>-<kebab-case-slug>.xml`. The format is the log4j changelog schema:
+
+- `type` is one of `added`, `changed`, `fixed`, `removed`, `updated`.
+- **Every `<issue>` element requires both `id` and `link`**; the export fails with
+  `missing attribute: link` otherwise, which is only caught by the Maven site build.
+- Put anything that has no issue number, such as an external finding identifier, in the description
+  text rather than inventing an `<issue>` for it.
+- `src/changelog/3.3.2/298-fix-interprocesslock-mutex-leak.xml` shows the shape for a change that
+  came out of an external audit.
+
+## Documentation site
+
+The manual lives in `src/site/antora/modules/ROOT/pages/`. A new appender page needs three edits,
+not one: the page itself, an `xref` line in `nav.adoc` (kept alphabetical), and the appender table
+in `manual/configuration/appenders.adoc`.
+
+## Security findings
+
+**[AGENTS.md](AGENTS.md) decides whether something is in scope and whether it is a vulnerability.**
+Read it before triaging a report, and describe a finding in commit messages and changelog entries
+the way it comes out of that assessment: a correctness bug, a reliability defect or hardening is
+none the worse for being called one.
+
+What that leaves for this file is where the answers live in the code:
+
+- When a report is likely to recur on a path the threat model already settles, leave a short comment
+  at the site with a link to the model rather than changing the code. `XmlConfigurator` and
+  `XmlHierarchyConfigurator` carry these for the configuration-is-trusted paths, and
+  `SystemStringFormat` for the format string.
+- `LocalSyslogAppender.EscapeNulCharacters` and `RemoteSyslogAppender.ValidateIdentity` are the two
+  sides of the content and structural-identifier rule: content is escaped and never rejected, a
+  malformed identifier is reported rather than quietly repaired.
+- Deliberate secure-default choices belong in the changelog with their opt-out named, so that an
+  upgrade surprise is searchable. See the entries for `SendTimeoutMillis`, `MatchTimeoutMillis` and
+  `LockTimeoutMillis`.

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