Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

efi: Make use of the grub's prefix for detection #268

Merged

Conversation

chrisccoulson
Copy link
Collaborator

Avoid relying on the signature in order to detect an Ubuntu production
grub binary for binaries without a SBAT section, as this breaks test cases
in snapd where grub is re-signed. Instead, obtain the prefix from the
"mods" section (which is set to "/EFI/ubuntu" in Ubuntu).

@chrisccoulson chrisccoulson force-pushed the efi-use-grub-prefix-for-detection branch from 73d2efa to 3a7fd0b Compare October 27, 2023 12:30
chrisccoulson added a commit to chrisccoulson/secboot that referenced this pull request Nov 28, 2023
Grub detection will be fixed separately in canonical#268
@chrisccoulson chrisccoulson force-pushed the efi-use-grub-prefix-for-detection branch 2 times, most recently from 40fbab3 to 12a3c33 Compare November 28, 2023 19:34
chrisccoulson added a commit that referenced this pull request Dec 1, 2023
…aries

efi: Add support for shim binaries used during snapd spread tests.

Snapd re-signs production shim and grub binaries (previously signed by Microsoft and Canonical) with a snakeoil key, so recognize this as part of the Microsoft hierarchy during testing.

Some tests also recompile our patched shim v15, which we use an image digest match to recognize it as having patches backported from 15.2. Recognize any shim v15 signed with the snakeoil key as being supported during testing.

#268 will contain changes required for grub.
Avoid relying on the signature in order to detect an Ubuntu production
grub binary for binaries without a SBAT section, as this breaks test cases
in snapd where grub is re-signed. Instead, obtain the prefix from the
"mods" section (which is set to "/EFI/ubuntu" in Ubuntu).
@chrisccoulson chrisccoulson force-pushed the efi-use-grub-prefix-for-detection branch from 12a3c33 to 8cace2b Compare December 1, 2023 17:11
@chrisccoulson chrisccoulson marked this pull request as ready for review December 1, 2023 17:11
@chrisccoulson chrisccoulson force-pushed the efi-use-grub-prefix-for-detection branch from 3ab72af to 421f90a Compare December 1, 2023 17:33
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you, couple small comments and questions

efi/pe.go Show resolved Hide resolved
Comment on lines +109 to +120
case pe.IMAGE_FILE_MACHINE_ARM, pe.IMAGE_FILE_MACHINE_I386, pe.IMAGE_FILE_MACHINE_RISCV32:
var info grubModuleInfo32
if err := binary.Read(section, binary.LittleEndian, &info); err != nil {
return nil, fmt.Errorf("cannot obtain modules info: %w", err)
}
if info.Magic != grubModuleMagic {
return nil, errors.New("invalid module magic")
}
if info.Size < info.Offset {
return nil, errors.New("invalid modules size")
}
r = io.NewSectionReader(section, int64(info.Offset), int64(info.Size)-int64(info.Offset))
Copy link
Collaborator

Choose a reason for hiding this comment

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

this case is not tested

@@ -163,7 +163,7 @@ func makeMicrosoftUEFICASecureBootNamespaceRules() *secureBootNamespaceRules {
),
imageMatchesAll(
imageSectionExists("mods"),
imageSignedByOrganization("Canonical Ltd."),
grubHasPrefix("/EFI/ubuntu"),
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we still need all the changes from the snakeoil PR?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do

Copy link
Contributor

@valentindavid valentindavid left a comment

Choose a reason for hiding this comment

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

Just one minor thing.

efi/grub.go Outdated
if info.Magic != grubModuleMagic {
return nil, errors.New("invalid modules magic")
}
if info.Size > math.MaxInt64 {
Copy link
Contributor

Choose a reason for hiding this comment

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

info.Offset?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was addressed with 421f90a

@chrisccoulson chrisccoulson force-pushed the efi-use-grub-prefix-for-detection branch from 2b485a2 to 8e2219e Compare December 11, 2023 23:45
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you

@chrisccoulson chrisccoulson merged commit 7619639 into canonical:master Dec 12, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants