Fix binary signature expiry check to validate signing time against certificate validity period#763
Fix binary signature expiry check to validate signing time against certificate validity period#763Copilot wants to merge 7 commits intorelease/v2.3from
Conversation
Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
…ficate validity period - Add SigningTime property to Signature class to capture when the binary was signed - Update IsTimeValid to check if signing time was within certificate validity period instead of checking against DateTime.Now - Extract signing timestamp from PE file PKCS7 data in WindowsFileSystemUtils - Update analyses.json rules to check IsTimeValid instead of IsExpired on NotAfter - Add unit tests for the new IsTimeValid logic Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
- Restore nuget.config with original Azure DevOps feed configuration - Use >= and <= for certificate validity boundary comparisons - Fix OID for RFC 3161 timestamp token (1.2.840.113549.1.9.16.2.14) - Add boundary condition tests for signing at exact NotBefore/NotAfter times Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in binary signature validation. Previously, the system incorrectly flagged binaries as having "expired signatures" if the signing certificate was currently expired, even if the binary was signed while the certificate was valid. The fix correctly validates that the binary was signed during the certificate's validity period.
Changes:
- Added
SigningTimeproperty to theSignatureclass and updatedIsTimeValidto validate signing time against certificate validity period instead of current time - Implemented
GetSigningTime()method to extract Authenticode timestamps from PE file PKCS#7 data, supporting multiple timestamp formats (countersigner info, RFC 3161 timestamp tokens, and signed attributes) - Updated analyses.json rules to use
IsTimeValidproperty instead of directly checking certificate expiration - Added comprehensive unit tests covering all edge cases including null values, boundary conditions, and the key scenario where a certificate is expired now but was valid when signing occurred
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| nuget.config | Removed BOM (Byte Order Mark) character from file header |
| Lib/Objects/Signature.cs | Added SigningTime property and fixed IsTimeValid to compare signing time against certificate validity period |
| Lib/Collectors/WindowsFileSystemUtils.cs | Added GetSigningTime() method to extract timestamps from PE files, checking countersigners, RFC 3161 tokens, and signed attributes |
| analyses.json | Updated "Binaries with expired signatures" rules to check IsTimeValid property instead of certificate expiration |
| Tests/SignatureTests.cs | Added 8 comprehensive unit tests covering all edge cases and scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…l OID parsing Replace manual OID string comparisons and nested CMS decoding with .NET's built-in Pkcs9SigningTime type checking. The SignedCms decoder automatically creates typed Pkcs9SigningTime instances, so we can use 'val is Pkcs9SigningTime' pattern matching instead of raw OID checks. This removes: - All manual OID string comparisons - Manual RFC 3161 timestamp token CMS decoding - Inner try-catch for CMS structure parsing - Unused System.Linq import The method goes from ~84 lines with 5 levels of nesting to ~40 lines with a clean helper method. Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
analyses.json:1589
- Same concern as the FILE rule above: this FILEMONITOR rule will also match binaries where
FileSystemObject.SignatureStatus.SigningTimeis missing/unextractable, but the rule text implies a known signing time outside the cert validity window. Consider requiring...SigningTimeto be present (or updating the description) to avoid misleading results.
"Name": "Binaries signed outside certificate validity period",
"Description": "These binaries were signed when the signing certificate was not yet valid or had already expired.",
"Flag": "WARNING",
"ResultType": "FILEMONITOR",
"ChangeTypes": [
"MODIFIED",
"CREATED"
],
"Clauses": [
{
"Field": "FileSystemObject.SignatureStatus.IsTimeValid",
"Operation": "IsTrue",
"Invert": true
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Runtime.InteropServices; | ||
| using System.Security.Cryptography; |
There was a problem hiding this comment.
using System.Security.Cryptography; appears unused in this file (all referenced types come from System.Security.Cryptography.Pkcs). Consider removing it to avoid dead imports and keep analyzers/noise down.
| using System.Security.Cryptography; |
There was a problem hiding this comment.
This import is actually required — CryptographicAttributeObjectCollection (used by GetPkcs9SigningTime) and CryptographicException (used in the RFC 3161 catch) are in the System.Security.Cryptography namespace. Removing it causes build error CS0246. Verified by test build.
| foreach (var signerInfo in signedCms.SignerInfos) | ||
| { | ||
| // Check counter-signers for the Authenticode timestamp | ||
| foreach (var counterSigner in signerInfo.CounterSignerInfos) | ||
| { | ||
| var time = GetPkcs9SigningTime(counterSigner.SignedAttributes); | ||
| if (time.HasValue) | ||
| return time; | ||
| } | ||
|
|
||
| // Fallback: check the signer's own signed attributes | ||
| var signerTime = GetPkcs9SigningTime(signerInfo.SignedAttributes); | ||
| if (signerTime.HasValue) | ||
| return signerTime; | ||
| } |
There was a problem hiding this comment.
GetSigningTime currently only looks for Pkcs9SigningTime on countersigners and the signer's signed attributes. Authenticode signatures that use RFC3161 timestamps typically store the trusted timestamp in the signatureTimeStampToken unsigned attribute (OID 1.2.840.113549.1.9.16.2.14), which this code no longer inspects, so SigningTime can become null even when a valid RFC3161 timestamp exists. This will cause IsTimeValid to be false and may produce false positives in the “signed outside certificate validity” rules. Consider reintroducing minimal handling for the RFC3161 timestamp token (or otherwise documenting/handling the null case).
There was a problem hiding this comment.
Good catch. Added RFC 3161 timestamp token handling back in 8788376 — it decodes each unsigned attribute value as a SignedCms and uses the same GetPkcs9SigningTime helper to find Pkcs9SigningTime instances, keeping the type-checking approach. Invalid CMS structures are caught with a narrow CryptographicException.
| } | ||
| catch (Exception e) | ||
| { | ||
| Log.Verbose("Failed to extract signing time: {0}", e.Message); |
There was a problem hiding this comment.
In the exception handler, only e.Message is logged, which drops the exception type and stack trace that can be important when diagnosing signature parsing failures. Consider using the Serilog overload that includes the exception (e.g., passing e as the first argument) and/or logging e.GetType() for consistency with other logging in this file.
| Log.Verbose("Failed to extract signing time: {0}", e.Message); | |
| Log.Verbose(e, "Failed to extract signing time ({0}:{1})", e.GetType(), e.Message); |
There was a problem hiding this comment.
Fixed in 8788376 — now uses the Serilog overload that includes the exception object and logs the type, consistent with the rest of the file.
analyses.json
Outdated
| @@ -1566,14 +1566,15 @@ | |||
| ], | |||
| "Clauses": [ | |||
| { | |||
| "Field": "SignatureStatus.SigningCertificate.NotAfter", | |||
| "Operation": "IsExpired" | |||
| "Field": "SignatureStatus.IsTimeValid", | |||
| "Operation": "IsTrue", | |||
| "Invert": true | |||
| } | |||
There was a problem hiding this comment.
This rule will match when SignatureStatus.IsTimeValid is false, which includes the case where SigningTime could not be extracted (the property returns false when SigningTime is null). In that situation, the new name/description (“signed outside certificate validity period”) can be misleading because the signing time is unknown rather than proven invalid. Consider adding an explicit clause requiring SignatureStatus.SigningTime to be non-null, or adjusting the rule name/description to reflect the “unknown/unverifiable signing time” case.
This issue also appears on line 1576 of the same file.
There was a problem hiding this comment.
@copilot we do want to highlight the cases where there is a signature but the signature is not within the validity period in some way, so we can't just exclude cases where the time stamp couldn't be determined but we should tighten this up a bit at least in the phrasing around what the rule is detecting.
There was a problem hiding this comment.
Updated in 8788376. Rule name is now "Binaries with unverified signature validity" and description clarifies both cases: "The signing time may be outside the validity period or could not be determined." This keeps the rule catching both scenarios as requested while being accurate about what it detects.
…nd rule descriptions - Add RFC 3161 timestamp token extraction from unsigned attributes, still using Pkcs9SigningTime type checking instead of manual OID parsing - Use narrow CryptographicException catch for invalid CMS structures - Improve exception logging to include exception object and type - Update rule names/descriptions to accurately reflect both cases: signing time outside validity period OR signing time undetermined - Keep using System.Security.Cryptography import (required for CryptographicAttributeObjectCollection and CryptographicException) Co-authored-by: gfs <98900+gfs@users.noreply.github.com>
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| /// <summary> | ||
| /// Extracts the signing timestamp from a PE file's Authenticode signature. | ||
| /// The timestamp is obtained from the countersigner info in the PKCS#7 data. |
There was a problem hiding this comment.
The XML doc for GetSigningTime says the timestamp is obtained from the countersigner info, but the implementation also checks RFC3161 tokens in UnsignedAttributes and falls back to the signer's own SignedAttributes. Please update the summary to reflect the actual extraction logic so the documentation remains accurate.
| /// The timestamp is obtained from the countersigner info in the PKCS#7 data. | |
| /// The method first attempts to use the countersigner info in the PKCS#7 data, | |
| /// then checks for RFC3161 timestamp tokens in the signer's UnsignedAttributes, | |
| /// and finally falls back to the signer's own SignedAttributes if needed. |
| foreach (var val in attr.Values) | ||
| { | ||
| try | ||
| { | ||
| var tokenCms = new SignedCms(); | ||
| tokenCms.Decode(val.RawData); | ||
| foreach (var tokenSigner in tokenCms.SignerInfos) | ||
| { | ||
| var time = GetPkcs9SigningTime(tokenSigner.SignedAttributes); | ||
| if (time.HasValue) | ||
| return time; | ||
| } | ||
| } | ||
| catch (CryptographicException) | ||
| { | ||
| // Not a valid CMS structure, skip | ||
| } |
There was a problem hiding this comment.
In the RFC3161 path you attempt to decode every unsigned attribute value as SignedCms and rely on catching CryptographicException to skip non-timestamp attributes. Since this block is specifically for RFC3161 tokens, consider filtering by the attribute OID (e.g., 1.2.840.113549.1.9.16.2.14 / signatureTimeStampToken) before decoding to reduce unnecessary work and exception-driven control flow.
| foreach (var val in attr.Values) | |
| { | |
| try | |
| { | |
| var tokenCms = new SignedCms(); | |
| tokenCms.Decode(val.RawData); | |
| foreach (var tokenSigner in tokenCms.SignerInfos) | |
| { | |
| var time = GetPkcs9SigningTime(tokenSigner.SignedAttributes); | |
| if (time.HasValue) | |
| return time; | |
| } | |
| } | |
| catch (CryptographicException) | |
| { | |
| // Not a valid CMS structure, skip | |
| } | |
| // RFC 3161 timestamp token OID: 1.2.840.113549.1.9.16.2.14 (signatureTimeStampToken) | |
| if (string.Equals(attr.Oid?.Value, "1.2.840.113549.1.9.16.2.14", StringComparison.Ordinal)) | |
| { | |
| foreach (var val in attr.Values) | |
| { | |
| try | |
| { | |
| var tokenCms = new SignedCms(); | |
| tokenCms.Decode(val.RawData); | |
| foreach (var tokenSigner in tokenCms.SignerInfos) | |
| { | |
| var time = GetPkcs9SigningTime(tokenSigner.SignedAttributes); | |
| if (time.HasValue) | |
| return time; | |
| } | |
| } | |
| catch (CryptographicException) | |
| { | |
| // Not a valid CMS structure, skip | |
| } | |
| } |
GetSigningTimeis too complex with manual OID checking and nested CMS parsingSignedCmsdecoder automatically types values asPkcs9SigningTime(confirmed:val is Pkcs9SigningTimeworks)GetSigningTimeinWindowsFileSystemUtils.csusing System.Security.Cryptographyis NOT unused — it's required forCryptographicAttributeObjectCollectionandCryptographicExceptionGetSigningTime(unsigned attributes)✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.