[Enhancement]: About - VerifyAuthenticode accepts a binary signed by any publisher

[laurentiu021]

Problem

UpdateService.VerifyAuthenticode (Services/UpdateService.cs:410-438) extracts the signer certificate and returns true after only logging cert.Subject (lines 421-422). There is no subject/thumbprint pinning and no chain build. So once SysManager is signed, a binary signed by ANY certificate - including an attacker's own self-issued one - passes this gate identically to a legitimately signed build. The same codebase already contains the correct pattern for a third-party binary: SpeedTestService.VerifyOoklaSignature (Services/SpeedTestService.cs:443-484) pins the subject (line 452: !cert.Subject.Contains("Ookla") -> delete + throw), then explicitly builds an X509Chain with RevocationMode.Online (lines 463-471) because, in its own comment at line 466-467, "Subject alone is forgeable (anyone can issue a self-signed 'Ookla' cert)". The asymmetry is stark: the downloaded Ookla CLI gets pinned-subject + full chain + revocation + fail-closed deletion, while SysManager's own self-replacing, potentially-elevated update binary gets a log line. Grepped for any pinning constant (expectedSigner|signerSubject|CN=laurentiu|pinnedSubject|ExpectedSubject) -> no matches. Grepped UpdateServiceAuthenticodeTests.cs for Subject|signer|publisher -> no matches; its four tests only cover unsigned/tiny/empty/MZ-header files.

Proposed solution

Restructure VerifyAuthenticode into a two-state contract now, while it is cheap: (a) no embedded signature -> allowed, log it, as today (correct while builds are unsigned); (b) signature present -> pin it the way VerifyOoklaSignature does - build an X509Chain with RevocationMode.Online and compare the signer against a pinned expected subject/thumbprint constant, failing closed on mismatch. Introduce the pinned value as a single named constant that is empty/inactive until a certificate exists, so switching signing on is a one-line change rather than a security redesign under time pressure. Add the missing negative tests to SysManager.Tests/UpdateServiceAuthenticodeTests.cs: a signed-but-wrong-publisher file must be rejected. Reference SpeedTestService.cs:443-484 in the code comment so the two paths stay visibly symmetric.

Rationale

This is the one place where the trust posture is pre-wired to fail the moment the maintainer's biggest planned trust upgrade lands. Today the exposure is bounded, because SHA256 (verified over a held deny-write handle) is the real gate and the no-signature branch is the live one. But the day a certificate arrives, the natural assumption will be "we sign now, so the signature check protects us" - and it will not, silently. Fixing it before signing ships costs one function and two tests; fixing it after means auditing a security control while also managing a certificate rollout. The codebase already proves the maintainer knows the correct pattern, so this is closing an internal inconsistency, not inventing a subsystem.

Evidence

Read Services/UpdateService.cs:391-438 in full: line 396 private const int CryptENoMatch = unchecked((int)0x80092009);, line 421 Serilog.Log.Information("Update binary is Authenticode-signed: {Subject}", cert.Subject); immediately followed by return true; at 422 - no subject comparison, no X509Chain anywhere in the method. Read Services/SpeedTestService.cs:438-484 for contrast: subject pin at 452, chain build at 463-471, TryDeleteExe + throw on both failure paths. Grepped for pinning identifiers across SysManager/ excluding obj/ -> exit 1 (none). Read SysManager.Tests/UpdateServiceAuthenticodeTests.cs in full: four tests, all path-shape cases (UnsignedFile->true, TinyUnsignedFile->true, EmptyFile->false, RandomBinaryContent->true); grep for Subject/signer/publisher -> exit 1.

Risk / trade-off

Adding a chain build with online revocation introduces a network dependency and latency into the install path, and could fail-closed on an offline machine - so the chain check must apply only in the signature-present branch (never blocking today's unsigned flow) and needs a considered offline policy. Over-tightening could brick self-update for users who legitimately obtained a build signed under a future, rotated certificate, so the pinned value should tolerate certificate renewal (pin on subject/organization rather than a single thumbprint, or maintain a small allowed set).

Affected area

About

---

Effort: M | priority: value 3/5, fit 4/5
Identified during the trust, distribution and reach audit audit.

[laurentiu021]

Fixed in v1.60.3 (PR #1762).

Confirmed against current main: VerifyAuthenticode extracted the signer certificate, logged cert.Subject, and returned true — no subject comparison, no X509Chain anywhere in the method. So once SysManager is signed, a binary signed by any certificate, including an attacker's self-issued one, would have passed identically to a legitimate build.

Your framing of why this matters now rather than later is exactly right, and it's the reason I took it ahead of larger items: the exposure today is bounded (SHA256 is the live gate, the no-signature branch is the one that runs), but the day a certificate arrives the assumption "we sign now, so that check protects us" would have been silently false.

Implemented as proposed, with the trade-offs you flagged resolved

• Two-state contract: no signature → allowed and logged, exactly as before. Signature present → pinned subject plus an X509Chain with RevocationMode.Online, failing closed.
• ExpectedSignerSubject is a single const, empty today. Turning signing on is a one-line change rather than a security redesign under release pressure — your point about not auditing a control during a certificate rollout.
• Pinned on the organization substring, not a thumbprint. You flagged renewal: a thumbprint pin would brick self-update for everyone the day the certificate rotates. Subject survives renewal.
• The chain build runs only in the signature-present branch, so today's unsigned flow never touches the network and can't fail closed on an offline machine — the other risk you raised.
• The code comment references VerifyOoklaSignature explicitly, so the two paths stay visibly symmetric. That asymmetry was the sharpest part of the report: the downloaded third-party CLI got pinned subject + chain + revocation + fail-closed deletion, while our own self-replacing, potentially-elevated update binary got a log line.

Verification

Harness against unfixed main: 6 failures, 4 passes. All four passes are controls, and they matter more than usual — an unsigned binary is still accepted, a larger unsigned PE-ish file is still accepted, an empty/unreadable file is still rejected, and SHA256 is still the documented integrity gate. Tightening the signed branch must not block every current install; "every update aborting with invalid digital signature" is the precise bug UpdateServiceAuthenticodeTests was originally written for. Branch: 10/10.

The one limitation, stated plainly

The signed-but-wrong-publisher case is asserted against the shipped source, not executed. Producing a genuinely signed binary with a controllable publisher needs a test certificate and signtool, which the unit-test project doesn't have. What is pinned mechanically: the signed branch consults the pin and builds a chain with online revocation (either alone is bypassable), and the const exists, is a literal, and is currently empty.

That last assertion is a deliberate tripwire — the day a certificate arrives it fails, and it points straight at the tests that will then need real signed fixtures. Which is the "add the missing negative tests" half of your proposal, deferred to the moment it can actually be satisfied rather than faked.

SECURITY.md described this as "not a publisher check" — accurate before, stale now. It documents the pin, the chain policy, why the pin is empty today, and why SHA256 remains the integrity gate rather than a fallback.