Skip to content

Commit 71c038c

Browse files
authored
Harden log4net against the findings of a security audit
Appenders: network and transport - 2fb4539 time out writes to stalled TelnetAppender clients - 15d16ee add a listen address to TelnetAppender - bd35fe0 add a TransportSecurity option to the MailKit SmtpAppender Appenders: syslog record integrity - eccb876 escape NUL characters in LocalSyslogAppender messages - 46582e5 report a RemoteSyslogAppender Identity that would split the record - 360a102 fix the lifetime of the LocalSyslogAppender identity AdoNet appender - 9f5c955 contain per-event failures in AdoNetAppender.SendBuffer - 19fdb4a warn when AdoNetAppender executes layout-generated SQL - e80b381 redact the password when reporting a failed database connection Reliability and resource bounds - 3fd97cb bound the waits for the file locking mutexes - 394fd3d bound regular expression matching in the string match filters - 86ecb15 flush TextWriterAppender under the appender lock - 1786b13 keep the impersonated user name when a logging event is fixed Diagnostics - ecd1b8b report the first appender error without log4net.Internal.Debug Build and release infrastructure - 9cc34d2 make the release verification scripts fail closed - 28d411a pin the Maven wrapper and distribution downloads - e203b7c remove the git-broadcast workflow Documentation - 6bc3df3 document that configuration is trusted input - 287fa9c document that format strings are trusted developer input - dc17e1b document the conventions the security scan work relied on (#309) 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.
2 parents e3246a5 + 843ccfc commit 71c038c

60 files changed

Lines changed: 2746 additions & 198 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.github/workflows/git-broadcast.yml

Lines changed: 0 additions & 44 deletions
This file was deleted.

.mvn/wrapper/maven-wrapper.properties

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,10 @@
1616
# under the License.
1717
distributionUrl=https://repo.maven.apache.org/maven2/org/apache/maven/apache-maven/3.9.0/apache-maven-3.9.0-bin.zip
1818
wrapperUrl=https://repo.maven.apache.org/maven2/org/apache/maven/wrapper/maven-wrapper/3.2.0/maven-wrapper-3.2.0.jar
19+
# mvnw and MavenWrapperDownloader refuse to run when a download does not match these.
20+
# distributionSha256Sum is of apache-maven-3.9.0-bin.zip as published on archive.apache.org, whose
21+
# PGP signature verifies against https://downloads.apache.org/maven/KEYS and whose bytes are
22+
# identical to the Maven Central copy above. wrapperSha256Sum is of the maven-wrapper.jar committed
23+
# next to this file, which is identical to the published maven-wrapper-3.2.0.jar.
24+
distributionSha256Sum=68e5a1745a5f5e4b0dfae051f83297e2ea40912b2c3b84d3b7420f463f39260d
25+
wrapperSha256Sum=e63a53cfb9c4d291ebe3c2b0edacb7622bbc480326beaa5a0456e412f52f066a

AGENTS.md

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,14 +10,29 @@ This is mandatory: skipping steps leads to duplicate reports and wasted time for
1010
### Step 1: Read the security model
1111

1212
Fetch and read the project's security model before evaluating any finding:
13-
https://raw.githubusercontent.com/apache/logging-site/refs/heads/main-site-pro/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
13+
https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/_threat-model-common.adoc
1414

1515
Use this to answer:
1616
- Is this component/behavior in scope at all?
1717
- Does the project consider this a security boundary?
1818

1919
If the finding is out of scope per the model, **stop here** and inform the researcher.
2020

21+
Two rules from the model account for most of what gets reported wrongly. Read the model itself
22+
rather than relying on this summary, but know them before you start:
23+
24+
- **Configuration is operator-supplied and trusted**, including configuration URIs and the types,
25+
members and converters that configuration names. A finding that needs the attacker to control
26+
configuration, a watched configuration file, or the format string of a log statement is out of
27+
scope: that is application misuse. Protecting those resources, and transmitting them over a
28+
confidential channel, is a deployer responsibility the model states explicitly.
29+
- **Content is untrusted and must never be rejected**, because rejecting it would turn a malicious
30+
value into a denial of service, so it has to be escaped or sanitized instead. Structural
31+
identifiers, such as logger names or the identifier fields of a structured log message, are
32+
trusted, and there the model allows *rejecting* a malformed value rather than silently altering
33+
it. Getting these two the wrong way round produces a plausible-looking report that proposes
34+
exactly the wrong fix.
35+
2136
### Step 2: Check previously disclosed vulnerabilities
2237

2338
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
2944
### Step 3: Read the Security FAQ
3045

3146
Read the Security FAQ before concluding anything is a vulnerability:
32-
https://raw.githubusercontent.com/apache/logging-site/refs/heads/main-site-pro/src/site/antora/modules/ROOT/pages/security/faq.adoc
47+
https://raw.githubusercontent.com/apache/logging-site/refs/heads/main/src/site/antora/modules/ROOT/pages/security/faq.adoc
3348

3449
The FAQ lists behaviors that are **intentional and not vulnerabilities**.
3550
If the finding matches an FAQ entry, inform the researcher that it is a known non-issue
@@ -52,6 +67,11 @@ Assess the finding:
5267

5368
## Report quality rules
5469

70+
- **Only call something a vulnerability when it really is one.** Name the adversary, then check that
71+
capability against the model. If it needs a misconfiguration, a co-resident local user, or
72+
anything the model does not grant, it is a correctness bug, a reliability defect or hardening, and
73+
saying so is more useful than a severity. Do not inherit the framing of a scanner report that
74+
arrived with severities already attached.
5575
- Never speculate about impact beyond what you can demonstrate.
5676
- Reproduction steps must be minimal and self-contained.
5777
- Do not include unrelated findings in the same report: one issue per report.

CLAUDE.md

Lines changed: 81 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,33 +26,33 @@ almost always be doing.
2626
(238 of 244 files in `src/log4net`). Copy it verbatim into new files.
2727
- File-scoped namespaces (`namespace log4net.Appender;`). Note `.editorconfig` still says
2828
`csharp_style_namespace_declarations = block_scoped:silent`, but 242 of 244 files are
29-
file-scoped follow the code, not that setting.
29+
file-scoped: follow the code, not that setting.
3030
- `using` directives outside the namespace, in one contiguous block.
3131

3232
### Language usage
33-
- **Explicit types, not `var`** all three `csharp_style_var_*` options are `false`.
33+
- **Explicit types, not `var`**: all three `csharp_style_var_*` options are `false`.
3434
Write `StringWriter writer = new(...)`.
3535
- Target-typed `new()` and collection expressions (`private static readonly char[] _x = [',', ';'];`).
36-
Omit the type wherever the target is known including `return new(…);` and `=> new(…);`, where
36+
Omit the type wherever the target is known, including `return new(…);` and `=> new(…);`, where
3737
the enclosing member's return type supplies it. It cannot be omitted when the target type is an
3838
interface or abstract class, as in `Func<ISmtpTransport> f = () => new MailKitSmtpTransport();`.
39-
- Expression-bodied members whenever the body fits on one line — this includes constructors
39+
- Expression-bodied members whenever the body fits on one line, including constructors
4040
(`resharper_constructor_or_destructor_body = expression_body`).
4141
- Braces on `if`/`else` bodies even for a single statement.
4242
- `LangVersion` is `latest`, and current C# features are welcome and in use: primary
4343
constructors (`csharp_style_prefer_primary_constructors = true`), the `field` keyword in
4444
property accessors, list patterns, `switch` expressions.
4545
- **Wrap long string literals with a multi-line raw string (`"""`), never with `+`
46-
concatenation.** This includes attribute arguments see the `[Obsolete(...)]` message on
46+
concatenation.** This includes attribute arguments; see the `[Obsolete(...)]` message on
4747
`log4net.Appender.SmtpAppender`. Raw strings have no line-continuation, so each source line
4848
break really is a `\n` in the value, but that is fine here: compiler diagnostics render those
4949
newlines as spaces, so a wrapped message still reads as one sentence. Raw strings are constant
50-
expressions, so they are legal in attributes, and the feature is purely syntactic it works on
50+
expressions, so they are legal in attributes, and the feature is purely syntactic, so it works on
5151
`net462`/`netstandard2.0` too.
5252
- Private fields are `_camelCase`. Private fields and helper methods are commonly placed
5353
*after* the public surface of the type rather than at the top.
5454

55-
### Nullability the big constraint
55+
### Nullability, the big constraint
5656
- `Nullable` is enabled solution-wide with `WarningsAsErrors=nullable`: **any nullability
5757
warning is a build error**, so it cannot be deferred.
5858
- `log4net` targets `net462;netstandard2.0`. **Neither reference assembly is nullable-annotated**,
@@ -68,7 +68,7 @@ almost always be doing.
6868
- Use the internal `log4net.Util.Log4NetAssert` extensions rather than hand-rolled checks:
6969
`EnsureNotNull()`, `EnsureNotNullOrEmpty()`, `EnsureIs<T>()`. They carry
7070
`[CallerArgumentExpression(nameof(value))]`, so no argument name is passed at the call site.
71-
This includes constructor and property assignments write `_x = x.EnsureNotNull();`,
71+
This includes constructor and property assignments: write `_x = x.EnsureNotNull();`,
7272
not `_x = x ?? throw new ArgumentNullException(nameof(x));`.
7373
- Appenders never let exceptions escape to the caller. The house pattern is
7474
`catch (Exception e) when (!e.IsFatal()) { ErrorHandler.Error("...", e); }`.
@@ -83,20 +83,90 @@ almost always be doing.
8383
requires linking `NotNullAttribute`, `ValidatedNotNullAttribute` and
8484
`CallerArgumentExpressionAttribute`, or you get `CS0122`.
8585
- Analyzers (`Microsoft.CodeAnalysis.NetAnalyzers`, `AnalysisLevel 8`, `src/log4net.globalconfig`)
86-
run on every build. **The solution builds with 0 warnings — keep it that way.**
86+
run on every build. **The solution builds with 0 warnings, keep it that way.**
87+
88+
### Documentation comments
89+
- **Every public and protected member gets an XML doc comment**, in test code as well as production
90+
code: test methods, nested helper classes and hand-written fakes included.
91+
- Use `/// <inheritdoc/>` when the member implements an interface or overrides a base member, and a
92+
real `<summary>` for everything else. `Log4NetTransaction` in the AdoNet test doubles is the
93+
pattern to copy.
94+
- When checking whether a member is documented, remember that `[Test]`, `#pragma` and
95+
`// ReSharper disable` lines legitimately sit between the doc comment and the declaration.
96+
97+
### Writing, in code and everywhere else
98+
- **Never use an em dash (``) or en dash (``).** Use a plain hyphen, or restructure with a colon,
99+
comma or parentheses. This covers comments, XML docs, commit messages, AsciiDoc and chat.
100+
- In AsciiDoc, ` -- ` is also forbidden: Asciidoctor renders a spaced double hyphen as an em dash,
101+
so it breaks the rule even though the source looks like plain hyphens. Grep touched files for
102+
`[—–]` and ` -- ` before presenting a change.
103+
- No underscores in identifiers, including test method names. `AllContainsEveryFlag`, not
104+
`All_ShouldContainAllFlags`. (Private fields are `_camelCase`, which is the one exception.)
87105

88106
### Tests
89107
- NUnit 4, not MSTest, and always the constraint model: `Assert.That(actual, Is.EqualTo(expected))`
90108
(810 uses of `Assert.That`, zero of `Assert.AreEqual`). `[TestFixture]`, `[Test]`, `[TestCase]`,
91109
with `[SetUp]`/`[TearDown]` for per-test state.
92-
- `NUnit.Analyzers` warnings are errors too — e.g. NUnit1032 requires an `IDisposable` fixture
110+
- Use an expression body for a single-statement test: `public void X() => Assert.That(...);`.
111+
- **`log4net` has no `InternalsVisibleTo`**, so private and internal members are exercised through
112+
reflection, not by widening their accessibility. See `SystemInfoTest`, `LevelMappingTest` and
113+
`UserNameFixingTest` for the `BindingFlags.Static | BindingFlags.NonPublic` pattern.
114+
`log4net.Ext.Mail` does grant `InternalsVisibleTo` to its own test project.
115+
- Mark a test `[NonParallelizable]` when it mutates static state (`LogLog.InternalDebugging`, a
116+
static field on a test double, a process-wide native registration).
117+
- Wrap expected internal logging in `LogLog.ExecuteWithoutEmittingInternalMessages(...)` and capture
118+
it with `LogLog.LogReceivedAdapter` rather than letting it reach the console. Appender errors are
119+
emitted by default, so a test that provokes one will otherwise add noise to the suite output.
120+
- Guard platform-specific tests with `[Platform("Win")]` / `[Platform("Linux")]`. A test that only
121+
runs on Windows leaves the behaviour unverified in local Linux runs, so prefer a cross-platform
122+
home for the assertion when one exists.
123+
- `NUnit.Analyzers` warnings are errors too: for example NUnit1032 requires an `IDisposable` fixture
93124
field to be disposed in a `[TearDown]` method.
94125
- For code that talks to the outside world, introduce a narrow interface and hand-write a fake;
95126
there is no mocking library in any test project. See `ISmtpTransport` / `FakeSmtpTransport`.
96127
- Verify with `dotnet build src/log4net.sln` and
97128
`dotnet test src/<project>.Tests/<project>.Tests.csproj`.
98129
- **When inspecting build output, redirect it to a file and read the whole thing; do not pipe
99130
MSBuild through line-oriented tools.** `grep`/`Select-String` cannot match across newlines, and
100-
MSBuild's console logger formats differently when piped than when redirected a multi-line
131+
MSBuild's console logger formats differently when piped than when redirected, so a multi-line
101132
diagnostic message then looks truncated when it is not. Before reporting that the toolchain
102133
mangles something, re-check with `dotnet build … > out.txt 2>&1` and inspect `out.txt`.
134+
135+
## Changelog
136+
137+
Every user-visible change gets an entry in `src/changelog/<unreleased version>/`, named
138+
`<issue>-<kebab-case-slug>.xml`. The format is the log4j changelog schema:
139+
140+
- `type` is one of `added`, `changed`, `fixed`, `removed`, `updated`.
141+
- **Every `<issue>` element requires both `id` and `link`**; the export fails with
142+
`missing attribute: link` otherwise, which is only caught by the Maven site build.
143+
- Put anything that has no issue number, such as an external finding identifier, in the description
144+
text rather than inventing an `<issue>` for it.
145+
- `src/changelog/3.3.2/298-fix-interprocesslock-mutex-leak.xml` shows the shape for a change that
146+
came out of an external audit.
147+
148+
## Documentation site
149+
150+
The manual lives in `src/site/antora/modules/ROOT/pages/`. A new appender page needs three edits,
151+
not one: the page itself, an `xref` line in `nav.adoc` (kept alphabetical), and the appender table
152+
in `manual/configuration/appenders.adoc`.
153+
154+
## Security findings
155+
156+
**[AGENTS.md](AGENTS.md) decides whether something is in scope and whether it is a vulnerability.**
157+
Read it before triaging a report, and describe a finding in commit messages and changelog entries
158+
the way it comes out of that assessment: a correctness bug, a reliability defect or hardening is
159+
none the worse for being called one.
160+
161+
What that leaves for this file is where the answers live in the code:
162+
163+
- When a report is likely to recur on a path the threat model already settles, leave a short comment
164+
at the site with a link to the model rather than changing the code. `XmlConfigurator` and
165+
`XmlHierarchyConfigurator` carry these for the configuration-is-trusted paths, and
166+
`SystemStringFormat` for the format string.
167+
- `LocalSyslogAppender.EscapeNulCharacters` and `RemoteSyslogAppender.ValidateIdentity` are the two
168+
sides of the content and structural-identifier rule: content is escaped and never rejected, a
169+
malformed identifier is reported rather than quietly repaired.
170+
- Deliberate secure-default choices belong in the changelog with their opt-out named, so that an
171+
upgrade surprise is searchable. See the entries for `SendTimeoutMillis`, `MatchTimeoutMillis` and
172+
`LockTimeoutMillis`.

scripts/verify-release.ps1

Lines changed: 54 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,52 +5,82 @@ Param (
55

66
Set-StrictMode -Version Latest
77
$ErrorActionPreference = 'Stop'
8+
# $ErrorActionPreference alone does not apply to native commands: gpg only sets $LASTEXITCODE, so
9+
# without this a failed signature check would still reach the extraction at the end and the script
10+
# would exit 0. Requires PowerShell 7.3+.
11+
$PSNativeCommandUseErrorActionPreference = $true
12+
813
if (!$Directory)
914
{
1015
$Directory = $PSScriptRoot
1116
}
1217

13-
function Verify-Hash
18+
function Assert-Hash
1419
{
1520
param
1621
(
17-
[Parameter(Mandatory=$true, HelpMessage='The file containing the hash.')]
22+
[Parameter(Mandatory=$true, HelpMessage='The artifact to check.')]
1823
[System.IO.FileInfo]$File
19-
)
20-
$Line = @(Get-Content $File.FullName)[0]
21-
$Fields = $Line -split '\s+'
22-
$Hash = $Fields[0].Trim().ToUpper()
23-
$Filename = $Fields[1].Trim()
24-
if ($Filename.StartsWith("*"))
25-
{
26-
$Filename = $Filename.Substring(1).Trim()
27-
}
28-
29-
$ComputedHash = (Get-FileHash -Algorithm 'SHA512' "$($File.DirectoryName)/$Filename").Hash.ToUpperInvariant()
24+
)
3025

31-
if($Hash -eq $ComputedHash)
26+
$HashFile = "$($File.FullName).sha512"
27+
if (!(Test-Path $HashFile))
3228
{
33-
"$($Filename): Passed"
29+
throw "$($File.Name): no $($File.Name).sha512 to check it against"
3430
}
35-
else
31+
32+
$Hash = (@(Get-Content $HashFile)[0] -split '\s+')[0].Trim().ToUpperInvariant()
33+
$ComputedHash = (Get-FileHash -Algorithm 'SHA512' $File.FullName).Hash.ToUpperInvariant()
34+
if ($Hash -ne $ComputedHash)
3635
{
37-
Write-Error "$($Filename): Not Passed" -ErrorAction Continue
38-
Write-Error "Read from file: $Hash" -ErrorAction Continue
39-
Write-Error "Computed: $ComputedHash" -ErrorAction Continue
36+
throw "$($File.Name): SHA-512 mismatch, read $Hash but computed $ComputedHash"
4037
}
38+
39+
"$($File.Name): hash ok"
40+
}
41+
42+
# Everything that is not a hash, a signature or the key file has to be covered by both. Driving the
43+
# checks from the artifacts, rather than from the .sha512 and .asc files that happen to be present,
44+
# is what turns a missing signature into a failure instead of one loop iteration fewer.
45+
$Artifacts = @(Get-ChildItem $Directory -File |
46+
Where-Object { $_.Extension -notin '.asc', '.sha512' -and $_.Name -ne 'KEYS' })
47+
48+
if ($Artifacts.Count -eq 0)
49+
{
50+
throw "No artifacts to verify in $Directory"
4151
}
4252

43-
foreach ($File in Get-ChildItem $Directory *.sha512)
53+
foreach ($Artifact in $Artifacts)
4454
{
45-
Verify-Hash $File
55+
Assert-Hash $Artifact
4656
}
4757

4858
Invoke-WebRequest https://downloads.apache.org/logging/KEYS -OutFile $Directory/KEYS
49-
gpg --import -q $Directory/KEYS
5059

51-
foreach ($File in Get-ChildItem $Directory *.asc)
60+
# A key ring of its own, holding only the downloaded KEYS. Importing into the default key ring
61+
# would accept a signature from any key this machine already has, not only from a key in the
62+
# Logging Services KEYS file.
63+
$KeyringDirectory = New-Item -ItemType Directory -Path (Join-Path ([System.IO.Path]::GetTempPath()) ([guid]::NewGuid()))
64+
try
65+
{
66+
$Keyring = Join-Path $KeyringDirectory 'logging-keys.gpg'
67+
gpg --no-default-keyring --keyring $Keyring --batch --quiet --import $Directory/KEYS
68+
69+
foreach ($Artifact in $Artifacts)
70+
{
71+
$Signature = "$($Artifact.FullName).asc"
72+
if (!(Test-Path $Signature))
73+
{
74+
throw "$($Artifact.Name): no $($Artifact.Name).asc to verify it with"
75+
}
76+
77+
gpg --no-default-keyring --keyring $Keyring --batch --verify $Signature $Artifact.FullName
78+
"$($Artifact.Name): signature ok"
79+
}
80+
}
81+
finally
5282
{
53-
gpg --verify $File
83+
Remove-Item $KeyringDirectory -Recurse -Force -ErrorAction SilentlyContinue
5484
}
5585

5686
Expand-Archive $Directory/*source*.zip -DestinationPath $Directory/src

0 commit comments

Comments
 (0)