Fix the low-severity findings from the 2026-08-25 security audit #17

Merged

hannah-vernon merged 1 commit from fix/security-audit-low into dev

2026-08-25 19:06:48 -05:00

hannah-vernon commented

2026-08-25 19:05:58 -05:00

Owner

What does this PR do?

Second batch from the 2026-08-25 security audit: the Low/Info findings (the four Medium findings landed in #16).

MDaemon bounded tail reads. One scan now consumes at most MaxScanBytes (new option, default 8 MiB, validated floor 4096) instead of buffering the entire unread backlog (previously up to 2 GB in one allocation). Capped reads end at the last complete line and the remainder arrives on subsequent scans; a single line larger than the cap is discarded with a warning so the file cannot stall every future scan. Offsets and payload references track actual consumption rather than the file length (also fixes a payload-reference uniqueness collision that partial scans would have introduced).
IMAP UID wrap guard. A stored offset at uint.MaxValue skips the folder sweep with a warning instead of wrapping lastUid + 1 to UID 0 and issuing an overbroad search.
Log-injection hardening. New Viegard.Application.Logging.LogSanitizer replaces control characters with spaces in attacker-influenced strings before they reach ILogger: MDaemon file names and IMAP folder names. Syslog logging was reviewed and needs no change - it logs only parsed IPAddress values and operator configuration.
Queue stats clamp. GetStatsAsync clamps long counts instead of casting, so a flooded queue cannot wrap the reported depth/in-flight/dead-letter numbers negative.
Dependency patch. Microsoft.Extensions.Hosting 10.0.10 -> 10.0.11 (the one project a patch behind). The test-tooling major-version jumps flagged by the audit (coverlet 6->10, Test.Sdk 17->18, xunit.runner 3->4) were deliberately not taken in a security batch; they can ride a normal maintenance PR.

Also records the schema-change data-migration request in TODO.md (from tonight''s live cutover: an explicit opt-in ALTER SCHEMA ... RENAME design with fail-closed detection of stranded data; needs a design decision).

Deferred audit recommendations (revisit at Phase 9, tracked in TODO.md): audit-record exception text, LISTEN/NOTIFY wakeup spam, secret string zeroization, in-memory ledger doc note.

How was this tested?

dotnet build - 0 errors, 0 warnings
dotnet test - 279 passed, 9 skipped (live-PostgreSQL integration tests; local WSL Docker unavailable)
New tests: LogSanitizer theory covering CR/LF forgery, escapes, NUL, and Unicode pass-through; MaxScanBytes validator floor

Checklist

I have read the Contributing Guide
Changes are focused - one logical change per PR
Documentation updated (README.md / DECISIONS.md / TODO.md / AGENT-README.md, if applicable)
New dependencies are MIT/Apache-2.0/BSD, security-vetted, and recorded in THIRD-PARTY-NOTICES.md (none added; one patch bump)
No secrets, credentials, or real infrastructure identifiers in the diff
No commented-out code or debug leftovers

## What does this PR do? Second batch from the 2026-08-25 security audit: the **Low/Info** findings (the four Medium findings landed in #16). 1. **MDaemon bounded tail reads.** One scan now consumes at most `MaxScanBytes` (new option, default 8 MiB, validated floor 4096) instead of buffering the entire unread backlog (previously up to 2 GB in one allocation). Capped reads end at the last complete line and the remainder arrives on subsequent scans; a single line larger than the cap is discarded with a warning so the file cannot stall every future scan. Offsets and payload references track actual consumption rather than the file length (also fixes a payload-reference uniqueness collision that partial scans would have introduced). 2. **IMAP UID wrap guard.** A stored offset at `uint.MaxValue` skips the folder sweep with a warning instead of wrapping `lastUid + 1` to UID 0 and issuing an overbroad search. 3. **Log-injection hardening.** New `Viegard.Application.Logging.LogSanitizer` replaces control characters with spaces in attacker-influenced strings before they reach `ILogger`: MDaemon file names and IMAP folder names. Syslog logging was reviewed and needs no change - it logs only parsed `IPAddress` values and operator configuration. 4. **Queue stats clamp.** `GetStatsAsync` clamps `long` counts instead of casting, so a flooded queue cannot wrap the reported depth/in-flight/dead-letter numbers negative. 5. **Dependency patch.** `Microsoft.Extensions.Hosting` 10.0.10 -> 10.0.11 (the one project a patch behind). The test-tooling major-version jumps flagged by the audit (coverlet 6->10, Test.Sdk 17->18, xunit.runner 3->4) were deliberately not taken in a security batch; they can ride a normal maintenance PR. Also records the schema-change data-migration request in TODO.md (from tonight''s live cutover: an explicit opt-in `ALTER SCHEMA ... RENAME` design with fail-closed detection of stranded data; needs a design decision). Deferred audit recommendations (revisit at Phase 9, tracked in TODO.md): audit-record exception text, LISTEN/NOTIFY wakeup spam, secret string zeroization, in-memory ledger doc note. ## How was this tested? - [x] `dotnet build` - 0 errors, 0 warnings - [x] `dotnet test` - 279 passed, 9 skipped (live-PostgreSQL integration tests; local WSL Docker unavailable) - [x] New tests: LogSanitizer theory covering CR/LF forgery, escapes, NUL, and Unicode pass-through; MaxScanBytes validator floor ## Checklist - [x] I have read the [Contributing Guide](../CONTRIBUTING.md) - [x] Changes are focused - one logical change per PR - [x] Documentation updated (README.md / DECISIONS.md / TODO.md / AGENT-README.md, if applicable) - [x] New dependencies are MIT/Apache-2.0/BSD, security-vetted, and recorded in THIRD-PARTY-NOTICES.md (none added; one patch bump) - [x] No secrets, credentials, or real infrastructure identifiers in the diff - [x] No commented-out code or debug leftovers

hannah-vernon added 1 commit

2026-08-25 19:05:58 -05:00

Fix the low-severity findings from the 2026-08-25 security audit 3c0bca41f5

Second remediation batch (Low/Info findings):

- MDaemon bounded tail reads: one scan consumes at most MaxScanBytes
  (new option, default 8 MiB, validated floor 4096), ending at the
  last complete line; the remainder arrives on subsequent scans.  A
  single line larger than the cap is discarded with a warning so the
  file cannot stall.  Offsets and payload references now track actual
  consumption instead of the file length.
- IMAP UID wrap guard: a stored offset at uint.MaxValue skips the
  folder sweep instead of wrapping the search range to UID 0.
- Log-injection hardening: new LogSanitizer replaces control
  characters in attacker-influenced strings (MDaemon file names, IMAP
  folder names) before they reach ILogger; syslog logging reviewed
  and already safe (parsed IPAddress values and operator config only).
- Queue stats clamp: long counts are clamped, not cast, so a flooded
  queue cannot wrap the reported depth negative.
- Microsoft.Extensions.Hosting 10.0.10 -> 10.0.11 (last project one
  patch behind); test tooling majors deliberately not taken.

Also records Hannah's schema-change data-migration request in TODO.md
(explicit opt-in ALTER SCHEMA RENAME design, needs decision) after the
live schema cutover stranded the pre-viegard-schema rows in public.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>