Rfc3161TimestampToken.TryDecode does not work for timestamps in Windows OS libraries Authenticode signatures

[bartonjs]

If you take the Authenticode signature payload from (e.g.) ntdll.dll, decode that as a SignedCms, and try to interpret cms.SignerInfos[0].UnsignedAttributes[0].Values[0] (the only instance of the only unsigned attribute of the only signer) as an Rfc3161TimeStampToken, TryDecode returns false.

Stepping through, the first failure occurs because the TimestampToken contains a certificate of a type other than X.509 public key certificates (namely, an X.509 attribute certificate, v1).

After confirming Windows behaviors in the CryptMsg* APIs for how unsupported certificate formats are treated, I made the changes locally to get past this point. The branch https://github.com/bartonjs/runtime/tree/authenticode_rfc3161 contains those changes (and some unrelated test refactoring changes in earlier commits).

Unfortunately, that doesn't solve the issue.

RFC 3161, section 2.3 says

2.3. Identification of the TSA

   The TSA MUST sign each time-stamp message with a key reserved
   specifically for that purpose.  A TSA MAY have distinct private keys,
   e.g., to accommodate different policies, different algorithms,
   different private key sizes or to increase the performance.  The
   corresponding certificate MUST contain only one instance of the
   extended key usage field extension as defined in [RFC2459] Section
   4.2.1.13 with KeyPurposeID having value:

   id-kp-timeStamping.  This extension MUST be critical.

   The following object identifier identifies the KeyPurposeID having
   value id-kp-timeStamping.

   id-kp-timeStamping OBJECT IDENTIFIER ::= {iso(1)
                   identified-organization(3) dod(6)
                   internet(1) security(5) mechanisms(5) pkix(7)
                   kp (3) timestamping (8)}

(specifically: This extension MUST be critical.)

And the relevant TSA certificate is non-conforming, the EKU extension is not marked critical.

Since the spec said it, we check for it:

runtime/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Rfc3161TimestampToken.cs

Lines 259 to 262 in b83fee9

	if (!ekuExt.Critical)
	{
	return false;
	}

The fact that Windows considers the timestamp valid suggests that the OS must not care about that particular sentence, and must not perform that particular check. So maybe we should relax that check for industry conformance (or come up with some other mode/opt-in). (FWIW, I haven't independently verified that the OS thinks the timestamp is valid, just taken it on word of mouth and faith.)

Leaving the notes here so others have the option of weighing in on how we handle the non-conforming TSA cert.

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

If you take the Authenticode signature payload from (e.g.) ntdll.dll, decode that as a SignedCms, and try to interpret cms.SignerInfos[0].UnsignedAttributes[0].Values[0] (the only instance of the only unsigned attribute of the only signer) as an Rfc3161TimeStampToken, TryDecode returns false.

Stepping through, the first failure occurs because the TimestampToken contains a certificate of a type other than X.509 public key certificates (namely, an X.509 attribute certificate, v1).

After confirming Windows behaviors in the CryptMsg* APIs for how unsupported certificate formats are treated, I made the changes locally to get past this point. The branch https://github.com/bartonjs/runtime/tree/authenticode_rfc3161 contains those changes (and some unrelated test refactoring changes in earlier commits).

Unfortunately, that doesn't solve the issue.

RFC 3161, section 2.3 says

2.3. Identification of the TSA

   The TSA MUST sign each time-stamp message with a key reserved
   specifically for that purpose.  A TSA MAY have distinct private keys,
   e.g., to accommodate different policies, different algorithms,
   different private key sizes or to increase the performance.  The
   corresponding certificate MUST contain only one instance of the
   extended key usage field extension as defined in [RFC2459] Section
   4.2.1.13 with KeyPurposeID having value:

   id-kp-timeStamping.  This extension MUST be critical.

   The following object identifier identifies the KeyPurposeID having
   value id-kp-timeStamping.

   id-kp-timeStamping OBJECT IDENTIFIER ::= {iso(1)
                   identified-organization(3) dod(6)
                   internet(1) security(5) mechanisms(5) pkix(7)
                   kp (3) timestamping (8)}

(specifically: This extension MUST be critical.)

And the relevant TSA certificate is non-conforming, the EKU extension is not marked critical.

Since the spec said it, we check for it:

runtime/src/libraries/System.Security.Cryptography.Pkcs/src/System/Security/Cryptography/Pkcs/Rfc3161TimestampToken.cs

Lines 259 to 262 in b83fee9

	if (!ekuExt.Critical)
	{
	return false;
	}

The fact that Windows considers the timestamp valid suggests that the OS must not care about that particular sentence, and must not perform that particular check. So maybe we should relax that check for industry conformance (or come up with some other mode/opt-in). (FWIW, I haven't independently verified that the OS thinks the timestamp is valid, just taken it on word of mouth and faith.)

Leaving the notes here so others have the option of weighing in on how we handle the non-conforming TSA cert.

Author:	bartonjs
Assignees:	-
Labels:	area-System.Security
Milestone:	7.0.0

[vcsjones]

Leaving the notes here so others have the option of weighing in on how we handle the non-conforming TSA cert.

In addition to the RFC, the code signing baseline requirements from CA/B say

The value id-kp-timeStamping [RFC5280] MUST be present and MUST be marked critical

See page 40, paragraph F.

Perhaps some useful info, BouncyCastle from the Java ecosystem also requires the extension be marked as critical:

https://github.com/bcgit/bc-java/blob/1ef1da015e116c1c2d09719674dfdd437971077f/pkix/src/main/java/org/bouncycastle/tsp/TSPUtil.java#L168-L171

Indeed the TST in your branch causes this:

The timestamping certificate is non-compliant according to a standard and non-compliant according to a governing body of certificate authorities. Another popular library in the Java ecosystem also enforces this rule. I would find it unfortunate if we deviated from the spec when Windows permitting it seems to be the outlier.

[vcsjones]

FWIW, I haven't independently verified that the OS thinks the timestamp is valid, just taken it on word of mouth and faith

Is there any chance Windows is doing something "clever"? I thought Windows components were catalog signed, anyway.

[GrabYourPitchforks]

If the larger ecosystem (both us + third-party code) is blowing up when parsing this cert, I think PRSS would be very interested in hearing this. They've been more amenable recently to being good participants within the ecosystem. It's possible they have a good reason for their current cert construction that we simply don't yet know about.

[bartonjs]

Thanks for doing some research while I was out, @vcsjones. I've passed your information on and am currently attempting to hold to a spec-conformant-enforcing implementation (e.g. "please generate a new conformant TSA cert"). We'll still need the rest of the PR to ignore X.509 attribute certificates, of course.

[bartonjs]

At this point we're not convinced that there's justification in being lax regarding the EKU extension verification. The operational error has been passed on to the TSA that timestamped ntdll.dll, hopefully they fix it.