Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions efi/export_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,14 @@ func MockOpenPeImage(fn func(Image) (peImageHandle, error)) (restore func()) {
}
}

func MockCheckImageSignatureIsValidForHost(fn func(context.Context, Image) error) (restore func()) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anywhere

orig := mockedCheckImageSignatureIsValidForHost
mockedCheckImageSignatureIsValidForHost = fn
return func() {
mockedCheckImageSignatureIsValidForHost = orig
}
}

func MockSnapdenvTesting(testing bool) (restore func()) {
orig := snapdenvTesting
snapdenvTesting = func() bool { return testing }
Expand Down
296 changes: 296 additions & 0 deletions efi/image_trust.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,296 @@
// -*- Mode: Go; indent-tabs-mode: t -*-

/*
* Copyright (C) 2026 Canonical Ltd
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 3 as
* published by the Free Software Foundation.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*
*/

package efi

import (
"bytes"
"context"
"crypto"
"crypto/x509"
"errors"

efi "github.com/canonical/go-efilib"
"golang.org/x/xerrors"
)

var mockedCheckImageSignatureIsValidForHost func(ctx context.Context, image Image) error

// 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.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

//
// 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
Comment on lines +35 to +44
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
// - The PE image digest matches a digest entry in db, where the PE image digest
// is computed for the hash algorithm implied by the EFI_SIGNATURE_LIST type
// (e.g., CertSHA256Guid implies SHA256, CertSHA384Guid implies SHA384).
//
// For digest-based authorization, the function computes the PE image digest for
// each digest algorithm present in db and checks for a match against the
// corresponding signature list entries. This allows an image to be verified
// against digest entries regardless of the hash algorithm used by its
// Authenticode signature.
//
// This is intended to be used during boot asset updates to verify that a new
// image will actually be loadable, when secure boot is enforced, by the host's
// firmware before it is installed.
//
Comment thread
zyga marked this conversation as resolved.
// For example, a shim binary signed only by a newer Microsoft UEFI CA will not
// be loadable on older hardware whose db only contains the older CA. Similarly,
// an image whose digest is not in db (if digest-based authorization is used) will
// not be loadable. An unsigned image cannot be loadable even if its digest is
// enrolled in db.
//
// The context must provide access to the EFI variable backend via go-efilib's
// context mechanism. In general, pass the result of
// [HostEnvironment.VarContext] or [efi.DefaultVarContext].
//
// Possible error conditions:
// - The image cannot be opened or is not a valid PE binary.
// - The image has no secure boot signatures.
// - The host's db variable cannot be read (eg, if EFI variables are unavailable).
// - 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 {
Comment on lines +73 to +76
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
if mockedCheckImageSignatureIsValidForHost != nil {
return mockedCheckImageSignatureIsValidForHost(ctx, image)
}

// Extract signatures from the image, and check for the presence of at
// least one signature before doing any further work, to give a more
// specific error if the image is not signed at all.
pei, err := openPeImage(image)
if err != nil {
return xerrors.Errorf("cannot open image: %w", err)
}

defer pei.Close()

sigs, err := pei.SecureBootSignatures()
if err != nil {
return xerrors.Errorf("cannot obtain secure boot signatures for image: %w", err)
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// Check for forbidden signatures in DBX first, to give a more specific
// error if the image is actually signed by a trusted CA but is revoked
// by DBX.
if err := checkDbxRevocation(ctx, pei, sigs); err != nil {
return err
}

// Check for a trusted signature in DB.
return checkDbAuthorization(ctx, pei, sigs)
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this function return true if secure boot is disabled?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yes


func checkDbxRevocation(ctx context.Context, pei peImageHandle, sigs []*efi.WinCertificateAuthenticode) error {
dbx, err := efi.ReadSignatureDatabaseVariable(ctx, Dbx)
if err != nil {
if !errors.Is(err, efi.ErrVarNotExist) {
return xerrors.Errorf("cannot read forbidden signature database: %w", err)
}

// If DBX doesn't exist, then there are no forbidden signatures, so we can
// just treat it as empty and allow the image to be authorized by DB.
return nil
}
Comment thread
zyga marked this conversation as resolved.

digests := newImageDigestCache(pei)
revokedByDigest, err := imageDigestIsForbiddenByDbx(digests, dbx)
if err != nil {
return err
}

if revokedByDigest {
return errors.New("secure boot signature is forbidden by the current host's signature databases")
}

// Per UEFI secure boot semantics, any match in DBX is sufficient to
// revoke an image, even if it has additional valid signatures.
for _, sig := range sigs {
if certIsForbiddenByDbx(sig, dbx) {
return errors.New("secure boot signature is forbidden by the current host's signature databases")
}
Comment thread
zyga marked this conversation as resolved.
}

return nil
}

var errNoTrustedSignature = errors.New("cannot find any secure boot signature that is trusted by the current host's authorized signature database")
Comment on lines +135 to +144
Copy link

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

func checkDbAuthorization(ctx context.Context, pei peImageHandle, imageSigs []*efi.WinCertificateAuthenticode) error {
db, err := efi.ReadSignatureDatabaseVariable(ctx, Db)
if err != nil {
if !errors.Is(err, efi.ErrVarNotExist) {
return xerrors.Errorf("cannot read authorized signature database: %w", err)
}
// If DB doesn't exist, then there are no authorized signatures, so the image cannot be
// authorized.
return errNoTrustedSignature
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

digests := newImageDigestCache(pei)

for _, sigList := range db {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Check for X.509 certificate-based authorization
if sigList.Type == efi.CertX509Guid {
for _, sigEntry := range sigList.Signatures {
cert, err := x509.ParseCertificate(sigEntry.Data)
if err != nil {
continue
}

for _, imageSig := range imageSigs {
if !imageSig.CertWithIDLikelyTrustAnchor(efi.NewX509CertIDFromCertificate(cert)) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would

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

be neater here?

continue
}

// If the signature chains to a trusted certificate, then the image is authorized.
return nil
}
}
} else {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copilot AI Apr 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
alg := efiSignatureListTypeToDigestAlg(sigList.Type)
if alg == crypto.Hash(0) {
continue
}

// Check for digest-based authorization
match, err := imageDigestMatchesDbSignatureList(digests, sigList)
if err != nil {
return err
}

if match {
return nil
}
}
}

return errNoTrustedSignature
}

func certIsForbiddenByDbx(sig *efi.WinCertificateAuthenticode, dbx efi.SignatureDatabase) bool {
for _, sigList := range dbx {
if sigList.Type != efi.CertX509Guid {
continue
}

for _, sigEntry := range sigList.Signatures {
revokedCert, err := x509.ParseCertificate(sigEntry.Data)
if err != nil {
continue
}

if sig.CertWithIDLikelyTrustAnchor(efi.NewX509CertIDFromCertificate(revokedCert)) {
return true
}
}
}

return false
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


type imageDigestCache struct {
pei peImageHandle
digests map[crypto.Hash][]byte
}

func newImageDigestCache(pei peImageHandle) *imageDigestCache {
return &imageDigestCache{
pei: pei,
digests: make(map[crypto.Hash][]byte),
}
}

func (c *imageDigestCache) digestForAlg(alg crypto.Hash) ([]byte, error) {
if digest, exists := c.digests[alg]; exists {
return digest, nil
}

digest, err := c.pei.ImageDigest(alg)
if err != nil {
return nil, xerrors.Errorf("cannot compute image digest with %v: %w", alg, err)
}

c.digests[alg] = digest
return digest, nil
}

func imageDigestMatchesDbSignatureList(digests *imageDigestCache, sigList *efi.SignatureList) (bool, error) {
alg := efiSignatureListTypeToDigestAlg(sigList.Type)
if alg == crypto.Hash(0) {
return false, nil
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


digest, err := digests.digestForAlg(alg)
if err != nil {
return false, err
}

for _, sigEntry := range sigList.Signatures {
if bytes.Equal(sigEntry.Data, digest) {
return true, nil
}
}

return false, nil
}

func imageDigestIsForbiddenByDbx(digests *imageDigestCache, dbx efi.SignatureDatabase) (bool, error) {
for _, sigList := range dbx {
match, err := imageDigestMatchesDbSignatureList(digests, sigList)
if err != nil {
return false, err
}

if match {
return true, nil
}
}

return false, nil
}

func efiSignatureListTypeToDigestAlg(guid efi.GUID) crypto.Hash {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

switch guid {
case efi.CertSHA1Guid:
return crypto.SHA1
case efi.CertSHA224Guid:
return crypto.SHA224
case efi.CertSHA256Guid:
return crypto.SHA256
case efi.CertSHA384Guid:
return crypto.SHA384
case efi.CertSHA512Guid:
return crypto.SHA512
default:
return crypto.Hash(0)
}
}
Loading
Loading