Fix Authenticode signing time extraction and split signature validity rules

[Giulia Stocco (gfs)]

Why

PR #763 reworked binary signature expiry detection to compare the signing time against the certificate's validity period, rather than comparing "now" against NotAfter. That is the correct model: a binary that was properly signed and timestamped should stay valid after its signing certificate expires.

The intent was right, but the implementation never actually recovered a signing time for modern binaries, so the new check inverted into a false-positive generator. PR #763 could not be tested on Windows at the time it was written. This PR fixes it, and adds the test coverage that would have caught it.

What was broken

Signing time was never extracted for modern Authenticode signatures. Microsoft's Authenticode RFC 3161 timestamps are carried in the unsigned attribute 1.3.6.1.4.1.311.3.3.1 (szOID_RFC3161_counterSign), not the standard 1.2.840.113549.1.9.16.2.14 that was being checked. Those tokens also carry no PKCS#9 signingTime attribute at all; the authoritative value is the genTime of the encapsulated TSTInfo. Only legacy PKCS#9 countersigned binaries were handled, so SigningTime came back null for nearly every signed binary. Because IsTimeValid returns false when the signing time is unknown, each of those binaries was reported as a warning.

Measured over a 505 file sweep of System32, dotnet, and PowerShell 7: signing times were recovered for only 2 of 323 authenticode-valid files, and the change produced 278 net new false positives.

IsTimeValid compared mixed time bases. Signing times are recovered as Kind=Utc, while certificate validity comes back from X509Certificate2 as Kind=Local. The comparison silently shifted by the machine's UTC offset, so results varied by timezone.

The tests masked both bugs. All 11 tests added by #763 used vcruntime140d.dll, the single legacy-countersigned fixture in the repo, which happens to be the one code path that worked. That is why the suite was green while the feature was broken.

Approach

WindowsFileSystemUtils now matches both timestamp OIDs and parses TSTInfo.genTime out of the RFC 3161 token using AsnReader, falling back to a PKCS#9 time on the token signer if a TSA does supply one. The legacy countersigner path is preserved and still tried first, so nothing regresses for older binaries.

Signature.IsTimeValid normalizes all three operands to UTC before comparing.

analyses.json splits the single ambiguous rule into two rules with distinct severities, since the two situations it conflated are very different in practice:

• Binaries signed outside certificate validity period stays WARNING. This is genuinely suspicious.
• Binaries with an undeterminable signing time becomes INFORMATION. This is expected for catalog-signed binaries and unusual signers, and is not by itself an integrity problem.

Both variants are defined for FILE and FILEMONITOR. The discriminator is a SigningTime IsNull clause (inverted for the warning rule), which was verified to route correctly through OAT.

SignatureTests grows from 11 to 24 tests, covering RFC 3161 timestamped binaries, DateTime.Kind normalization, and rule routing through the real analyzer against the embedded rule set. The RFC 3161 test asserts that its fixture has zero PKCS#9 countersigners, so it cannot silently stop covering the new path if the fixture is ever swapped.

Validation

All run on Windows.

Check	Result
Build, net8.0 and net9.0	0 errors
SignatureTests	24/24 pass
Full CI filter TestCategory=PipelineSafeTests	77/77 on net8.0, 77/77 on net9.0
Negative control: fixes stashed, new tests against old lib	7 of 24 fail, as expected
505 file sweep, signing times recovered	2/323 -> 323/323
505 file sweep, false positives	278 -> 0
EnumerateRuleIssues on embedded rules	0 issues
End-to-end asa collect + export-collect	SigningTime round-trips through SQLite and JSON export

End-to-end spot check: Microsoft.Data.Sqlite.dll (certificate expired 2026-06-17, signed 2025-09-26) and vcruntime140d.dll (certificate expired 2020, signed 2019) both now resolve to IsTimeValid=True with zero rules flagged, which is exactly the scenario #763 set out to fix. Unsigned binaries are still correctly caught by the pre-existing "Unsigned binaries" rule.

Notes for reviewers

Runs collected before this change have no SigningTime stored, so comparing a new run against an older baseline will match every previously-collected signed binary against the "undeterminable signing time" rule. This is inherent to adding a new collected field. The severity split keeps that noise at INFORMATION instead of WARNING, which was part of the motivation for splitting the rules.

Tests/TestData/ExportTests/TestGenerateSarifLog/rules.json still contains the original pre-#763 rule name. It is a frozen SARIF input snapshot that is deliberately decoupled from analyses.json, so it is intentionally left unchanged.

The ASN.1 reader uses BER rather than DER and deliberately does not call ThrowIfNotEmpty, because the trailing TSTInfo fields after genTime are optional.

[Giulia Stocco (gfs)]

Note

This is automated output from the COMPASS SDL Security Review tooling, posted for the record. It is not a personal review opinion, and the findings have not been independently triaged yet.

SDL Security Review

Scope reviewed: Lib/Collectors/WindowsFileSystemUtils.cs, Lib/Objects/Signature.cs, analyses.json, Tests/SignatureTests.cs

---

🟡 Lib/Collectors/WindowsFileSystemUtils.cs

Insufficient verification of data authenticity (CWE-345) — timestamp token is decoded but never cryptographically verified

var tokenCms = new SignedCms();
tokenCms.Decode(tokenData);

if (string.Equals(tokenCms.ContentInfo.ContentType?.Value, TstInfoContentTypeOid, StringComparison.Ordinal))
{
    var genTime = GetTstInfoGenTime(tokenCms.ContentInfo.Content);

SignedCms.Decode() parses without validating. CheckSignature() is never called on the timestamp token, and nothing binds the token's messageImprint back to the outer SignerInfo.SignatureValue. Combined with the source of the data — signerInfo.UnsignedAttributes, which by definition is not covered by the Authenticode signature — the recovered genTime is attacker-controllable on any binary.

Attack vector: take a binary that is genuinely Authenticode-valid but was signed outside its certificate's validity window (the stolen-key-used-after-cert-expiry case this rule exists to catch). Replace or append the 1.3.6.1.4.1.311.3.3.1 unsigned attribute with a self-generated TSTInfo whose genTime lands inside NotBefore..NotAfter. IsAuthenticodeValid still returns true because unsigned attributes sit outside the signed blob, IsTimeValid flips to true, and the WARNING rule is silently suppressed. Omitting the timestamp entirely yields an INFORMATION finding, so forging is strictly better for an attacker than omitting.

Impact: detection evasion. The WARNING rule's description asserts more than the code can prove ("so the signature was not applied while the certificate was valid").

Remediation — the BCL already covers this in System.Security.Cryptography.Pkcs, which is imported here:

if (Rfc3161TimestampToken.TryDecode(tokenData, out var token, out _) &&
    token.VerifySignatureForSignerInfo(signerInfo, out var tsaCert))
{
    return token.TokenInfo.Timestamp.UtcDateTime;
}

VerifySignatureForSignerInfo verifies the token signature and that the message imprint matches the signer's signature value.

This is not a regression introduced by this PR — the legacy CounterSignerInfos path has the identical gap, and the code replaced here was equally unverified. It is marked 🟡 rather than 🔴 because this PR strictly improves on both main and #763, and the field is defensive telemetry rather than an access-control decision.

---

🟢 Lib/Collectors/WindowsFileSystemUtils.cs — informational

GetTstInfoGenTime runs AsnEncodingRules.BER over attacker-controlled bytes and calls reader.ReadInteger() twice for values it discards. A hostile PE can embed an oversized INTEGER, which ReadInteger materializes into a BigInteger. Amplification is roughly 1x over the already-resident certificate blob so this is not a practical DoS, but ReadIntegerBytes() avoids the allocation when the value is unused.

---

Reviewed and clear

• Signature.IsTimeValid UTC normalization — correct. SerializableCertificate round-trips through Newtonsoft with DateTimeZoneHandling.RoundtripKind, so Kind survives serialization and ToUniversalTime() behaves on both the collected and deserialized paths.
• analyses.json — the three states (IsTimeValid true / signed outside validity / signing time null) are mutually exclusive and exhaustive under IsAuthenticodeValid IsTrue. No detection gap is opened; unsigned binaries remain covered by the pre-existing "Unsigned binaries" rule.
• Error handling — the narrow CryptographicException / AsnContentException catches sit under a logging catch-all in GetSigningTime, and no security-critical path swallows silently. Logged exception data contains no secrets or PII.
• Tests — fixture strings ("thumbprint", "key", "pkcs7") are inert placeholders, not credentials. No hardcoded secrets anywhere in the diff.
• No injection, path traversal, SSRF, deserialization, crypto-primitive, cookie, or IAM changes in scope.

---

APPROVE — no blocking findings.