efi: add CheckImageSignatureIsValidForHost function by zyga · Pull Request #532 · canonical/secboot

zyga · 2026-04-01T15:06:53Z

The new function allows a update manager to look if the given image is signed with keys that are allowed by the host secure boot stack. This way an update manager can detect that an asset signature is unlikely to pass verification by the boot firmware before writing or updating the asset in persistent storage.

Copilot

Pull request overview

Adds an EFI helper to preflight whether a PE image’s Authenticode signing chain appears compatible with the host’s Secure Boot trust anchors, enabling update managers to detect likely-to-fail boot assets before writing them to persistent storage.

Changes:

Introduces CheckImageSignatureIsValidForHost to validate that an image has at least one signature chaining to a CA in the host’s UEFI db.
Adds a comprehensive unit test suite covering success and common failure modes.
Adds a test helper to mock CheckImageSignatureIsValidForHost.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File	Description
`efi/image_trust.go`	Implements the new host-`db` trust check for image signatures.
`efi/image_trust_test.go`	Adds unit tests for the new trust-check behavior.
`efi/export_test.go`	Adds a mock helper for the new trust-check entry point.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

This should prevent writing boot shim that is only signed by the new Microsoft key, on a machine that doesn't have that key in their UEFI DB. By the nature of the check we also prevent writing things that have entries in DBX that would equally fail to boot. This requires canonical/secboot#532 Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

The new function allows a update manager to look if the given image is signed with keys that are allowed by the host secure boot stack. This way an update manager can detect that an asset signature is unlikely to pass verification by the boot firmware before writing or updating the asset in persistent storage. Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-02T09:56:42Z

+// CheckImageSignatureIsValidForHost checks whether the supplied image has at
+// least one Authenticode signature that is authorized by the host's authorized
+// signature database (the UEFI "db" variable).
+//
+// The image must have at least one Authenticode signature. It is not possible
+// to authorize an unsigned image based solely on its digest being present in db.
+//
+// The image is authorized if:
+//   - At least one of its Authenticode signatures chains to an X.509 certificate
+//     authority that is enrolled in db, OR


The function doc comment is out of sync with the current behavior: (1) the implementation rejects unsigned images (len(sigs)==0) even if a digest exists in db, but the comment doesn’t mention this requirement; and (2) digest-based authorization compares the PE image Authenticode digest computed using the hash implied by the EFI_SIGNATURE_LIST type (e.g. SHA384 list uses SHA384 digest), not “the digest of one of its Authenticode signatures”. Please update the comment so callers don’t rely on incorrect semantics.

Copilot · 2026-04-02T09:56:42Z

+//   - The host's dbx variable cannot be read (eg, if EFI variables are unavailable).
+//   - The image is revoked by a certificate or digest entry in the host's dbx.
+//   - No signature on the image is authorized by the host's db.
+func CheckImageSignatureIsValidForHost(ctx context.Context, image Image) error {


The error wrapping here can produce duplicated context because openPeImage already returns errors prefixed with "cannot open image" (see efi/pe.go). Consider either returning the error as-is, or wrapping with a more specific message (e.g. "cannot open PE image") to avoid messages like "cannot open image: cannot open image: ...".

Copilot · 2026-04-02T09:56:42Z

+				}
+			}
+		} else {
+			// Skip unrecognized signature list types since we cannot use them for verification


This inline comment is misleading: the continue happens for any non-matching digest as well as unrecognized signature list types. Please reword it to reflect the actual condition (e.g. “no digest match (or unrecognized type)”).

Suggested change

// Skip unrecognized signature list types since we cannot use them for verification

// Skip signature lists with no digest match (or unrecognized type)

Copilot · 2026-04-02T09:56:43Z

+	for _, sig := range sigs {
+		if certIsForbiddenByDbx(sig, dbx) {
+			return errors.New("secure boot signature is forbidden by the current host's signature databases")
+		}
+	}
+
+	return nil
+}
+
+var errNoTrustedSignature = errors.New("cannot find any secure boot signature that is trusted by the current host's authorized signature database")


There’s no test exercising the error path where db exists but cannot be read (the cannot read authorized signature database: %w branch in checkDbAuthorization). Since this function is intended for update preflight decisions, it would be good to add a unit test that injects a vars backend read error for db and asserts the returned error matches that message.

valentindavid · 2026-04-07T07:14:38Z

+
+	digests := newImageDigestCache(pei)
+
+	for _, sigList := range db {


Iterating through db then through signatures seems wrong. I think the spec specifies the way the loop is and it is the opposite. We should check. While the result will be the same, the certificate that will used to validate might be different, and if this function evolves for example to return which certificate would be measured... that will be wrong.

I have been looking through the multiple specifications. And I actually do not find anything about it. I really thought measurements in PCR 7 had to be predictable. But I am not so sure anymore.

Anyway, I do think we should have the same kind of loop as in secureBootPolicyMixin.DetermineAuthority

This is also what edk2 does. And it seems that edk2 also verifies first signatures, then the digest.

valentindavid · 2026-04-07T07:15:35Z

+					return nil
+				}
+			}
+		} else {


Something I would like to check the specs. Is something signed never checked for its digest?

Images are tested against enrolled digests whether they are signed or not.

chrisccoulson

Thanks for working on this. I've done an initial pass and left some comments.

chrisccoulson · 2026-04-07T17:44:48Z

+
+	// Check for a trusted signature in DB.
+	return checkDbAuthorization(ctx, pei, sigs)
+}


Should this function return true if secure boot is disabled?

Probably yes

chrisccoulson · 2026-04-07T17:45:51Z

+		}
+		// If DB doesn't exist, then there are no authorized signatures, so the image cannot be
+		// authorized.
+		return errNoTrustedSignature


I don't know whether I would handle the efi.ErrVarNotExist error case explicitly here - the variable not existing is unexpected.

chrisccoulson · 2026-04-07T18:41:42Z

+
+	if len(sigs) == 0 {
+		return errors.New("image has no secure boot signatures")
+	}


It's odd that an image is tested against enrolled digests only if it is signed. An unsigned image would still authenticate ok if it matched an enrolled digest, even if there are no signatures.

chrisccoulson · 2026-04-07T18:42:40Z

+				}
+
+				for _, imageSig := range imageSigs {
+					if !imageSig.CertWithIDLikelyTrustAnchor(efi.NewX509CertIDFromCertificate(cert)) {


Would

if imageSig.CertWithIDLikelyTrustAnchor(efi.NewX509CertIDFromCertificate(cert)) { return nil }

be neater here?

chrisccoulson · 2026-04-07T18:46:10Z

+	alg := efiSignatureListTypeToDigestAlg(sigList.Type)
+	if alg == crypto.Hash(0) {
+		return false, nil
+	}


Does this make the earlier call to efiSignatureListTypeToDigestAlg in checkDbAuthorizations redundant?

chrisccoulson · 2026-04-07T18:48:05Z

+	return false, nil
+}
+
+func efiSignatureListTypeToDigestAlg(guid efi.GUID) crypto.Hash {


This function doesn't take a signature list, so I wonder if there could be a better name for it.

chrisccoulson · 2026-04-07T19:06:48Z

+	}
+
+	return false
+}


There are some cert types that this doesn't handle - EFI_CERT_X509_SHA{256,384,512}_GUID. These contain TBS digests of revoked certificates and a revocation time, and a match of any certificate in the signing chain marks a signature as forbidden if the image signature has no timestamp signature, or if there is a timestamp signature but the timestamp is before the revocation time (on systems that support timestamp revocation).

Adding this adds a lot of complexity but without it, the result could indicate an image. I'm not sure whether checking revocations is worth the complexity, given the case we most care about is upgrades where we're not expected to upgrade from an image that is not revoked to one that is.

chrisccoulson · 2026-04-07T19:08:46Z

 	}
 }

+func MockCheckImageSignatureIsValidForHost(fn func(context.Context, Image) error) (restore func()) {


This isn't used anywhere

chrisccoulson · 2026-04-07T19:09:37Z

+// signature database (the UEFI "db" variable).
+//
+// The image must have at least one Authenticode signature. It is not possible
+// to authorize an unsigned image based solely on its digest being present in db.


The firmware will authorize an image in this way regardless of the presence of a signature.

chrisccoulson · 2026-04-07T19:12:44Z

+					return nil
+				}
+			}
+		} else {


Images are tested against enrolled digests whether they are signed or not.

zyga · 2026-04-08T11:46:39Z

@chrisccoulson thank you for the review. I need to spend a moment to iterate on the changes.

Copilot AI review requested due to automatic review settings April 1, 2026 15:06

Copilot started reviewing on behalf of zyga April 1, 2026 15:07 View session