Pkcs7 cert chain check fix - Revoke by hash of any cert in signing chain#562
Merged
Conversation
Add tests using a 2-cert PKCS#7 chain (leaf signer issued by the root CA)
to cover the case where the signer would be allowed by db but a chain
certificate is revoked in dbx, so the image must be rejected:
- Chain_LeafInDbAllowsSigner: baseline, leaf signer cert hash in db
trusts the chain-signed data.
- Chain_LeafCertHashRevokedByDbx: leaf signer cert hash in dbx revokes.
- Chain_RootCertRevokedByDbx: the root certificate as a full
EFI_CERT_X509 entry in dbx revokes (the signature chains to it).
- Chain_RootInDbxOverridesDbAllow: leaf allowed by db, root revoked in
dbx; P7CheckRevocation (run before P7CheckTrust by the protocol)
reports revoked, so the image is rejected.
Note: Pkcs7GetSigners() returns only the signer (leaf) certificate, so a
non-signer chain certificate (intermediate/root) is matched by the
full-certificate revocation path rather than the certificate-hash path.
The new 2-cert vector reuses the existing test key/cert material
(TestCert2 issued by TestCACert) and is documented for reproduction.
All 29 cases pass under the VS2022 host build with AddressSanitizer.
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
Per UEFI Spec 32.5.3.3 rule B, a dbx EFI_CERT_X509_SHAxxx entry forbids the image if its hash reflects the To-Be-Signed hash of ANY certificate in the signing chain - the leaf signer and every issuer up to the root. The certificate-hash revocation path only inspected the leaf signer returned by Pkcs7GetSigners() (OpenSSL CMS_get0_signers returns only the signer), so a revoked intermediate or root certificate hash in dbx did not reject the image. Add IsSignerCertChainRevokedByHash(), which retrieves the full signing chain via Pkcs7GetCertificatesList() and checks each certificate's TBS hash against the revocation database. If the chain cannot be retrieved, it fails safe and reports the signedData as revoked (rather than checking only a subset of the chain). P7CheckRevocation() and P7CheckRevocationByHash() now use it. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
…tion
Add a 3-level certificate chain (root -> intermediate -> leaf, all
embedded) and tests confirming that a non-signer chain certificate
revoked in dbx by EFI_CERT_X509_SHAxxx hash rejects the image:
- Chain3_LeafInDbAllowsSigner: baseline, leaf hash in db trusts.
- Chain3_IntermediateCertHashRevokedByDbx: intermediate hash in dbx
revokes (V1 and V2 variants).
- Chain3_RootCertHashRevokedByDbx: root hash in dbx revokes.
- Chain3_UnrelatedCertHashNotRevoked: unrelated hash does not revoke.
The 3-cert chain reuses the existing root CA (TestCACert) and is built
with openssl; reproduction is documented next to the vector.
All 34 cases pass under the VS2022 host build with AddressSanitizer.
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
Per UEFI Spec 32.5.3.3 rule B, a dbx EFI_CERT_X509_SHAxxx entry forbids the image if its hash reflects the To-Be-Signed hash of ANY certificate in the signing chain - the leaf signer and every issuer up to the root. IsForbiddenByDbx() only inspected the leaf signer returned by Pkcs7GetSigners() (OpenSSL CMS_get0_signers returns only the signer), so a revoked intermediate or root certificate hash in dbx did not forbid the image. Retrieve the full signing chain via Pkcs7GetCertificatesList() and check every certificate's TBS hash against dbx. If the chain cannot be retrieved, fail safe and treat the image as forbidden (rather than checking only a subset of the chain). Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
…sh dbx
Add a 3-level certificate chain (root -> intermediate -> leaf, all
embedded) and IsForbiddenByDbx() tests confirming that a non-signer
chain certificate revoked in dbx by EFI_CERT_X509_SHAxxx hash forbids
the image. The dbx variable is provided by mocking gRT->GetVariable:
- Chain_IntermediateCertHashRevoked: intermediate hash in dbx forbids
(V1 and V2 variants).
- Chain_RootCertHashRevoked: root hash in dbx forbids.
- Chain_LeafCertHashRevoked: leaf signer hash in dbx forbids.
- Chain_UnrelatedCertHashNotForbidden: unrelated hash does not forbid.
All 23 cases pass under the VS2022 host build with AddressSanitizer.
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
Replace the ASSERT(FALSE) stub of Pkcs7GetCertificatesList in the MbedTLS backend with a working implementation. It locates the signer certificate, walks the issuer chain (matching issuer_raw to a candidate's subject_raw) to build SignerChainCerts in EFI_CERT_STACK format, and collects the remaining embedded certificates into UnchainCerts. Two static helpers, IsCertInCertStack() and MbedTlsAppendCertToStack(), support buffer construction. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
…atesList d2i_CMS_ContentInfo() advances the pointer it is given past the consumed DER, so after decoding NewP7Data no longer points at the start of the buffer. The cleanup path then calls free() on an interior pointer in the non-wrapped case, which is undefined behavior. Decode through a temporary pointer and keep NewP7Data pointing at the buffer start for free(). Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
Add the TestVerifyPkcs7GetCertificatesList host test, registered for both the OpenSSL and MbedTLS suites. It signs with TestCert2 (leaf) embedding TestCACert (root) via a constructed X509 stack and asserts the returned chain contains both certificates. All 69 cases pass under the VS2022 host build with AddressSanitizer on both backends. Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref tianocore/edk2#12107
ref microsoft/mu_basecore#1809 (comment)