[DRAFT][WIP] SecurityPkg: DxeImageValidationLib rewrite

[Javagedes]

Description

This is a complete rewrite of DxeImageValidationLib.

Please review the Scenario scoped tests found in GoogleTest/DxeImageVerificationLibGoogleTest.cpp.

Notable differences from original implementation:

1. Policy management has been simplified (almost completely removed). Policy now dictates that if the image comes from an FV, it should be ALWAYS_EXECUTE. Anything else is set to DENY_EXECUTE_ON_SECURITY_VIOLATION and must go through the handler. There are no longer PCDs to configure Policy for certain scenarios.
2. PE image parsing is delegated to PeCoffLib.
3. Hashing is delegated to BaseCryptLib.
4. AuditMode has been removed. Secureboot is either enabled or disabled and that logic comes from the existing SecureBootVariableLib.
5. Multi-signed images only require one signer to fully validate. The previous implementation requires all signers fully validate.
6. Added X509 cert hash support in the DB
7. Global data, including digests, have been removed. A Cache system has been created to reduce duplicate hashing.
8. Functionality has been compartmentalized into small testable functions that do not rely on global state
9. Test coverage is around 95%

PQC Code First Status (TODOs)

Link	Description	Implemented?	Status
12405	Add EFI_CERT_X509_SHAxxx support for DB	✅	IsCertAuthorized implements this.
12406	Mark sha1 as deprecated	❌	Support removed without deprecation text
12525	Remove Audit and Deployed Mode	✅	DxeImageValidationLib does not consider these modes. It relies on IsSecureBootEnabled from SecurityManagementLib
12526	First validated signature validates image	✅	First signature validated top-down is what is recorded in PCR[7]
12527	Remove EFI_CERT_X509_GUID support from DBX	❌	Fully supported. Can be removed with a few lines of code
12541	Remove DBT support	✅	DBT support removed
12574	Add V2 structures that remove SignatureOwner	❌	Support for both types can be handled by updating mHashAlgorithms and an abstraction (or union) to extract the data we care about from both.

[mu-automation]

⌛ QEMU Validation Pending

QEMU validation is pending on successful CI completion.

Note: Any previous results are available in this comment's edit history.

This comment was automatically generated by the Mu QEMU PR Validation workflow.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 0% with 23 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (release/202511@29d32ac). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
MdePkg/Library/BasePeCoffLib/BasePeCoff.c	0.00%	23 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##             release/202511    #1809   +/-   ##
=================================================
  Coverage                  ?    2.32%           
=================================================
  Files                     ?     1477           
  Lines                     ?   383030           
  Branches                  ?     4876           
=================================================
  Hits                      ?     8900           
  Misses                    ?   374046           
  Partials                  ?       84           

Flag	Coverage Δ
FmpDevicePkg	9.53% <ø> (?)
MdeModulePkg	1.65% <ø> (?)
MdePkg	5.44% <0.00%> (?)
NetworkPkg	0.55% <ø> (?)
PolicyServicePkg	30.42% <ø> (?)
UefiCpuPkg	4.78% <ø> (?)
UnitTestFrameworkPkg	11.70% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[makubacki]

I wasn't sure about adding the AI skills to this repo. But in any case, it shouldn't be included in this PR.

[jyao1]

I checked the code with AI.

It seems the code does not handle 2 cases:

1. intermediate cert in DBX.
   Thinking of a case that: Chain leaf→intermediate→root, db trusts root, dbx lists the intermediate:
   I think the expectation is to revoke the image, right? But it seems allow.
   Is there a test to cover that?

2. signed image hash in DBX
   It seems the code only check unsigned image hash in DBX, not signed image hash.
   Is there a test to cover that?

I have not got chance to review all the code yet. Maybe I am wrong.

But I feel we have better have a test to cover that.

[Javagedes]

I checked the code with AI.

It seems the code does not handle 2 cases:

1. intermediate cert in DBX.
   Thinking of a case that: Chain leaf→intermediate→root, db trusts root, dbx lists the intermediate:
   I think the expectation is to revoke the image, right? But it seems allow.
   Is there a test to cover that?
2. signed image hash in DBX
   It seems the code only check unsigned image hash in DBX, not signed image hash.
   Is there a test to cover that?

I have not got chance to review all the code yet. Maybe I am wrong.

But I feel we have better have a test to cover that.

@jyao1 - For (1), the current implementation does not support this from my understanding. It uses Pkcs7GetSigners, but I don't believe that returns intermediates, correct? Do we have a working implementation of this for me to integrate, or a PQC Code First item requesting this feature that I can base any work off of, for this implementation?

For (2), We do support checking signed images in the DBX. The flow is:

1. Check DBX for Authenticode Digest match
2. If security directory: For each cert, attempt a validation (Handle success if any 1 validates)
3. Check DB for Authenticode Digest match

I will also be starting work on a UEFI_APPLICATION test app that will run through a list of scenarios regarding with different image signature states and DB / DBX states.

[jyao1]

I think we should design the test cases to ensure we cover all path.

For DB and DBX, we have 5 possible value (N/A, ImageHash, RootCert(Hash), InterCert(Hash), LeafCert(Hash)) for an image.

Image (unsigned/signed)	(DB) N/A	(DB) ImageHash	(DB) RootCert(Hash)	(DB) InterCert(Hash)	(DB) LeafCert(Hash)
(DBX) N/A	reject(1)	allow(3)	allow(4)	allow(4)	allow(4)
(DBX) ImageHash	reject(1)	reject(2)	reject(2)	reject(2)	reject(2)
(DBX) RootCertHash	reject(1)	allow(3)	reject(5)	allow (4)	allow (4)
(DBX) InterCertHash	reject(1)	allow(3)	reject(5)	reject(5)	allow (4)
(DBX) LeafCertHash	reject(1)	allow(3)	reject(5)	reject(5)	reject(5)

NOTE:

• An image could be signed or unsgined.
• A signed image have multiple cert chain with different algo. (RSA, ECDSA, MLDSA)
• DB/DBX could have multiple ImageHash with different algo and multiple Cert(Hash) with different algo.
• DB may include ImageHash, Cert or CertHash. (CertHash in DB is new).
• DBX may include ImageHash, CertHash, but NOT Cert. (Cert in DBX is deprecated).
• DB/DBX Cert or CertHash may be partial of the cert chain, e.g. RootCert, InterCert, or LeafCert.
• DB/DBX may be V1 format or V2 format.

The rule is below in order:

1. Nothing in DB -> reject
2. Any DBX ImageHash match -> reject
3. Any DB ImageHash match -> allow
4. For each cert chain in the image, if (one cert chain component(hash) in DB) and (none of the this or lower component(hash) in DBX) -> allow /*NOTE: Upper component is ignored, even if it is in DBX.*/
5. Else /*for all cert chain in the image, if (any of the cert chain component(hash) in DB) and (one of this or lower component(hash) in DBX)*/ -> reject

Additional rule for multiple signature:

1. Firmware shall evaluate each signature sequentially in the order they appear in the image's certificate table.
2. A signature is accepted only if (its digital signature verification succeeds) and (it is authorized by db and not forbidden by dbx). If a signature is accepted, the image is accepted and the remaining signatures are not evaluated. If a signature is not accepted, the firmware shall proceed to evaluate the next signature. If all signatures have been evaluated and none is accepted, the image is rejected.

Additional rule for one signer:

1. PE image multiple signature

PE/COFF image: the three "multiple signer" mechanisms

There are three structurally distinct ways a PE/COFF image can carry more than one
signature. They live at different layers of the format:

  PE Attribute Certificate Table  (DataDirectory[SECURITY])
  │
  ├─ WIN_CERTIFICATE#1 ──► PKCS#7 SignedData
  │                          ├─ SignerInfo#1 ─┐
  │                          ├─ SignerInfo#2 ─┤  (3) multiple SignerInfos
  │                          └─ SignerInfo#1's unsigned attrs
  │                              └─ nested PKCS#7  (2) nested signature
  ├─ WIN_CERTIFICATE#2 ──► PKCS#7 SignedData    (1) multiple WIN_CERTIFICATE entries
  └─ WIN_CERTIFICATE#3 ──► ...

• (1) Multiple WIN_CERTIFICATE entries — SUPPORTED.
• (2) Nested signature — SUPPORTED, but it is fundamentally single-signer.
• (3) Multiple SignerInfos in one SignedData — NOT supported.

Reference:

• UEFI Specification, driver signing. "Multiple signatures are allowed to exist in the binary’s certificate table (as per the “Attribute Certificate Table” section of the Microsoft PE/COFF Specification)".
• PE Authenticode Specification. "signerInfos — This field contains a set of SignerInfo structures, which contains information about the signatures. Because Authenticode supports only one signer, only one SignerInfo structure is in signerInfos.". Also "digestAlgorithms — Because Authenticode signatures support only one signer, digestAlgorithms must contain only one digestAlgorithmIdentifier structure and the structure must match the value set in the SignerInfo structure's digestAlgorithm field. If not, the signature has been tampered with."

2. Authenticated Variable, only one signer is allowed. Multi-SignerInfo in PKCS#7 blob will be rejected.

BTW:
I updated jyao1/UEFI-Specification-Release#1 and jyao1/UEFI-Specification-Release#15 to make it clear.

[jyao1]

Another question: When re-write, can you write a common ImageValidation() function, which can be used by both DxeImageVerificationLib, and Pkcs7DxeVerify driver?

[Javagedes]

@jyao1 I have added a generic UEFI application that can be compiled and ran on any platform (It hooks GetVariable to provide custom DB / DBX, and has self-contained signed images). These are the current scenarios. They all pass with the implementation in this PR. Please let me know if you have any questions or want to see other scenarios.

Image Type	DB	DBX	Expected Result
UNSIGNED	EMPTY	EMPTY	EFI_ACCESS_DENIED
UNSIGNED	IMAGE_DIGEST	EMPTY	EFI_SUCCESS
UNSIGNED	IMAGE_DIGEST	IMAGE_DIGEST	EFI_ACCESS_DENIED
SIGNED	EMPTY	EMPTY	EFI_ACCESS_DENIED
SIGNED	SIGNER1_LEAF_CERT	EMPTY	EFI_SUCCESS
SIGNED	SIGNER1_INTERMEDIATE	EMPTY	EFI_SUCCESS
SIGNED	SIGNER1_ROOT	EMPTY	EFI_SUCCESS
SIGNED	IMAGE_DIGEST	EMPTY	EFI_SUCCESS
SIGNED	SIGNER1_LEAF	SIGNER1_LEAF	EFI_ACCESS_DENIED
SIGNED	SIGNER1_LEAF	SIGNER1_INTERMEDIATE	EFI_ACCESS_DENIED
SIGNED	SIGNER1_LEAF	SIGNER1_ROOT	EFI_ACCESS_DENIED
SIGNED	SIGNER1_INTERMEDIATE	SIGNER1_LEAF	EFI_ACCESS_DENIED
SIGNED	SIGNER1_INTERMEDIATE	SIGNER1_INTERMEDIATE	EFI_ACCESS_DENIED
SIGNED	SIGNER1_INTERMEDIATE	SIGNER1_ROOT	EFI_ACCESS_DENIED
SIGNED	SIGNER1_ROOT	SIGNER1_LEAF	EFI_ACCESS_DENIED
SIGNED	SIGNER1_ROOT	SIGNER1_INTERMEDIATE	EFI_ACCESS_DENIED
SIGNED	SIGNER1_ROOT	SIGNER1_ROOT	EFI_ACCESS_DENIED
SIGNED	IMAGE_DIGEST	SIGNER1_LEAF	EFI_SUCCESS
SIGNED	IMAGE_DIGEST|SIGNER1_LEAF	SIGNER1_LEAF	EFI_SUCCESS
SIGNED	IMAGE_DIGEST	SIGNER1_INTERMEDIATE	EFI_SUCCESS
SIGNED	IAMGE_DIGEST|SIGNER1_INTERMEDIATE	SIGNER1_INTERMEDIATE	EFI_SUCCESS
SIGNED	IMAGE_DIGEST	SIGNER_ROOT	EFI_SUCCESS
SIGNED	IMAGE_DIGEST|SIGNER1_ROOT	SIGNER_ROOT	EFI_SUCCESS
SIGNED	SIGNER1_LEAF	IMAGE_DIGEST	EFI_ACCESS_DENIED
SIGNED	SIGNER1_INTERMEDIATE	IMAGE_DIGEST	EFI_ACCESS_DENIED
SIGNED	SIGNER1_ROOT	IMAGE_DIGEST	EFI_ACCESS_DENIED
SIGNED	IMAGE_DIGEST	IMAGE_DIGEST	EFI_ACCESS_DENIED
DUAL_SIGNED	EMPTY	EMPTY	EFI_ACCESS_DENIED
DUAL_SIGNED	SIGNER1_LEAF	EMPTY	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_INTERMEDIATE	EMPTY	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_ROOT	EMPTY	EFI_SUCCESS
DUAL_SIGNED	SIGNER2	EMPTY	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_LEAF	SIGNER2	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_LEAF|SIGNER2	SIGNER1_LEAF	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_LEAF|SIGNER2	SIGNER2	EFI_SUCCESS
DUAL_SIGNED	EMPTY	SIGNER2	EFI_ACCESS_DENIED
DUAL_SIGNED	EMPTY	SIGNER1_LEAF	EFI_ACCESS_DENIED
DUAL_SIGNED	IMAGE_DIGEST	EMPTY	EFI_SUCCESS
DUAL_SIGNED	IMAGE_DIGEST	IMAGE_DIGEST	EFI_ACCESS_DENIED
DUAL_SIGNED	IMAGE_DIGEST	SIGNER1_LEAF|SIGNER2	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_LEAF	IMAGE_DIGEST	EFI_ACCESS_DENIED
DUAL_SIGNED	SIGNER1_INTERMEDIATE	IMAGE_DIGEST	EFI_ACCESS_DENIED
DUAL_SIGNED	SIGNER1_ROOT	IMAGE_DIGEST	EFI_ACCESS_DENIED
DUAL_SIGNED	SIGNER2	IMAGE_DIGEST	EFI_ACCESS_DENIED
DUAL_SIGNED	SIGNER1_LEAF|SIGNER2	IMAGE_DIGEST	EFI_ACCESS_DENIED
DUAL_SIGNED	SIGNER1_INTERMEDIATE|SIGNER2	SIGNER2	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_ROOT|SIGNER2	SIGNER2	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_INTERMEDIATE|SIGNER2	SIGNER1_INTERMEDIATE	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_ROOT|SIGNER2	SIGNER1_ROOT	EFI_SUCCESS
DUAL_SIGNED	SIGNER1_INTERMEDIATE|SIGNER2	SIGNER1_INTERMEDIATE|SIGNER2	EFI_ACCESS_DENIED
DUAL_SIGNED	SIGNER1_ROOT|SIGNER2	SIGNER1_ROOT|SIGNER2	EFI_ACCESS_DENIED
DUAL_SIGNED	IMAGE_DIGEST|SIGNER2	SIGNER2	EFI_SUCCESS
DUAL_SIGNED	IMAGE_DIGEST|SIGNER1_INTERMEDIATE	SIGNER1_INTERMEDIATE	EFI_SUCCESS
DUAL_SIGNED	IMAGE_DIGEST|SIGNER1_ROOT	SIGNER1_ROOT	EFI_SUCCESS

[jyao1]

According to the previous discussion, I think we need to change below cases:

Image Type	DB	DBX	Expected Result
SIGNED	SIGNER1_LEAF	SIGNER1_INTERMEDIATE	EFI_ACCESS_DENIED => EFI_SUCCESS
SIGNED	SIGNER1_LEAF	SIGNER1_ROOT	EFI_ACCESS_DENIED => EFI_SUCCESS
SIGNED	SIGNER1_INTERMEDIATE	SIGNER1_ROOT	EFI_ACCESS_DENIED => EFI_SUCCESS

[Javagedes]

According to the previous discussion, I think we need to change below cases:

Image Type DB DBX Expected Result
SIGNED | SIGNER1_LEAF | SIGNER1_INTERMEDIATE | EFI_ACCESS_DENIED => EFI_SUCCESS
SIGNED | SIGNER1_LEAF | SIGNER1_ROOT | EFI_ACCESS_DENIED => EFI_SUCCESS
SIGNED | SIGNER1_INTERMEDIATE | SIGNER1_ROOT | EFI_ACCESS_DENIED => EFI_SUCCESS

@jyao1 I am getting my self confused between the statement you just made (above) and the statement you made on June 9th (below)

Thinking of a case that: Chain leaf→intermediate→root, db trusts root, dbx lists the intermediate:
I think the expectation is to revoke the image, right? But it seems allow.
Is there a test to cover that?

In terms of test cases, is this your expectation? If so, this seems to be a drastic change from the status quo. The current implementation (and this change) currently does not track the relative-ness of two certs in a cert chain of one auth data blob. We call AuthenticodeVerify to check if the contents of a DB / DBX entry validate the particular set of auth data, but we do not know where in the chain that entry from the DB / DBX is at.

DB	DBX	RESULT	DESCRIPTION
SIGNER1_LEAF_CERT	SIGNER1_INTERMEDIATE_CERT	EFI_SUCCESS	Signed Image where cert in DB is lower in chain than cert in DBX is approved
SIGNER1_LEAF_CERT	SIGNER1_ROOT_CERT	EFI_SUCCESS	Signed Image where cert in DB is lower in chain than cert in DBX is approved
SIGNER1_INTERMEDIATE_CERT	SIGNER1_ROOT_CERT	EFI_SUCCESS	Signed Image where cert in DB is lower in chain than cert in DBX is approved
SIGNER1_ROOT_CERT	SIGNER1_INTERMEDIATE_CERT	EFI_ACCESS_DENIED	Signed Image where cert in DB is higher in chain than cert in DBX is denied
SIGNER1_ROOT_CERT	SIGNER1_LEAF_CERT	EFI_ACCESS_DENIED	Signed Image where cert in DB is higher in chain than cert in DBX is denied
SIGNER1_INTERMEDIATE_CERT	SIGNER1_LEAF_CERT	EFI_ACCESS_DENIED	Signed Image where cert in DB is higher in chain than cert in DBX is denied