Sign the fwupmgr bootloader #10

ericonr · 2020-06-23T21:35:01Z

Hey there! I know the verify command checks files that are in the ESP partition, but there are other pieces of software that need to be signed and aren't in the ESP. The example I have is the fwupdmgr bootloader, which lives in /usr/libexec/fwupd/efi/fwupdx64.efi (on Void, I don't remember where it was on Arch). The issue is that, when trying to check for updates, fwupdmgr fails in the system firmware part on a secure boot protected device, because it can't find the signed version of the bootloader.

$ fwupdmgr get-devices
[...]
├─System Firmware:
│     Device ID:           xxxx
│     Current version:     xxxx
│     Minimum Version:     xxxx
│     Vendor:              Dell Inc. (DMI:Dell Inc.)
│     Update Error:        missing signed bootloader for secure boot: /usr/libexec/fwupd/efi/fwupdx64.efi.signed cannot be found
│     GUID:                xxxx
│     Device Flags:        • Internal device
│                          • Requires AC power
│                          • Needs a reboot after installation
│                          • Cryptographic hash verification is available
[...]

The part about it failing is probably a bug on their part, which I opened an issue about (fwupd/fwupd#2219), but I don't think it makes sense for them to implement some way of signing the bootloader. Do you think this is something that could be included in sbctl? If yes, what would the mechanism for it be? Some system wide configuration file / configuration directory, or hard coded behavior?

Thanks!

The text was updated successfully, but these errors were encountered:

Foxboron · 2020-06-23T21:39:49Z

sbctl sign -s /usr/libexec/fwupd/efi/fwupdx64.efi -o /usr/libexec/fwupd/efi/fwupdx64.efi.signed

Should solve all of it. sbctl would track fwupdx64.efi for changes and write out fwupdx64.efi.signed when you sign.

ericonr · 2020-06-23T21:48:40Z

Indeed. Thanks for the quick answer!

Actually, it's there in the README, no idea how I missed it at first.

Foxboron · 2020-06-23T22:13:57Z

I did actually intentionally add it to the README.md for this reason :) It's a nice feature and demonstrates the flexibility.

ericonr · 2020-06-24T05:53:32Z

Ok, I'm fairly sure this has some bug in functionality now...

$ cd /usr/libexec/fwupd/efi
$ sudo sbctl -s fwupdx64.efi -o fwupdx64.efi.signed
-> Signing /usr/libexec/fwupd/efi/fwupdx64.efi...
$ sudo sbctl verify
==> Verifying file database and EFI images in /boot/efi...
  -> /boot/efi/EFI/refind/refind_x64.efi is signed
  -> /boot/efi/EFI/void/vmlinuz-1.0_1 is signed
  -> /boot/efi/EFI/void/vmlinuz-1.0_1.EFI is signed
  -> /boot/efi/EFI/void/vmlinuz-1.1_1 is signed
panic: runtime error: slice bounds out of range [2:1]

goroutine 1 [running]:
github.com/foxboron/sbctl.VerifyESP()
	/home/ericonr/mypro/sbctl/sbctl.go:52 +0x486
main.verifyCmd.func1(0xc000135080, 0x8d06d8, 0x0, 0x0)
	/home/ericonr/mypro/sbctl/cmd/sbctl/main.go:109 +0x20
github.com/spf13/cobra.(*Command).execute(0xc000135080, 0x8d06d8, 0x0, 0x0, 0xc000135080, 0x8d06d8)
	/home/ericonr/.cache/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:846 +0x29d
github.com/spf13/cobra.(*Command).ExecuteC(0x8a2860, 0xc000055f70, 0x1, 0x1)
	/home/ericonr/.cache/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:950 +0x349
github.com/spf13/cobra.(*Command).Execute(...)
	/home/ericonr/.cache/go/pkg/mod/github.com/spf13/cobra@v1.0.0/command.go:887
main.main()
	/home/ericonr/mypro/sbctl/cmd/sbctl/main.go:290 +0x4a7
$ sudo sbctl sign-all
==> /boot/efi/EFI/void/vmlinuz-1.0_1.EFI has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.2_1.EFI has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.2_1 has been signed...
==> /boot/efi/EFI/voidb/vmlinuz-1.2_1 has been signed...
  -> Signing /usr/libexec/fwupd/efi/fwupdx64.efi...
==> /boot/efi/EFI/refind/refind_x64.efi has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.0_1 has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.1_1 has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.1_1.EFI has been signed...

Removing the files with remove-file seems to be enough to stop getting errors. Perhaps it's necessary to mention that files need to have absolute paths? Or make the path logic when using -s more complex.

EDIT: when running sign-all, it creates the file in my current directory, which makes sense, but doesn't feel like reasonable behavior for something that you're saving. I think it makes sense to require absolute paths when saving something.

Doing it with absolute paths seems to work fine, but has weird stuff in sign-all.

$ sudo sbctl sign -s /usr/libexec/fwupd/efi/fwupdx64.efi -o /usr/libexec/fwupd/efi/fwupdx64.efi.signed
  -> Signing /usr/libexec/fwupd/efi/fwupdx64.efi...
$ sudo sbctl sign-all
==> /boot/efi/EFI/void/vmlinuz-1.0_1.EFI has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.2_1.EFI has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.2_1 has been signed...
==> /boot/efi/EFI/voidb/vmlinuz-1.2_1 has been signed...
  -> Signing /usr/libexec/fwupd/efi/fwupdx64.efi...
==> /boot/efi/EFI/refind/refind_x64.efi has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.0_1 has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.1_1 has been signed...
==> /boot/efi/EFI/void/vmlinuz-1.1_1.EFI has been signed...

It is only regenerating the signed version, not touching the original one, so it isn't a huge bug, but it is always regenerating the signed version.

As a PS, disabling color output when piping into something and/or with a command line option would be a welcome feature, but I can try to implement it later.

Foxboron · 2020-06-24T07:05:04Z

Ah, i see the issue. In an attempt at not verifying files twice I did some basic caching of the checked files.

sbctl/sbctl.go

Line 52 in 7de0523

normalized := strings.Join(strings.Split(file.OutputFile, "/")[2:], "/")

It assumes there is some path to the output, and can probably be made more clever. But we should probably verify or have a warning when people add files without absolute paths.

It is only regenerating the signed version, not touching the original one, so it isn't a huge bug, but it is always regenerating the signed version.

Right, because we don't really know when the file has changed when we save it with -o. We could store a checksum of the original file?

Foxboron · 2020-06-24T07:06:27Z

As a PS, disabling color output when piping into something and/or with a command line option would be a welcome feature, but I can try to implement it later.

The entire logging code is a giant hack as I wanted to have the same output format as efi-roller. Mostly because I didn't want to spend time inventing a new formatting to the output. Suggestions for something more sane, and or help redesigning the output would be welcome

https://github.com/Foxboron/sbctl/blob/master/log.go

ericonr · 2020-06-24T18:14:40Z

It assumes there is some path to the output, and can probably be made more clever. But we should probably verify or have a warning when people add files without absolute paths.

Intuitively, I think it would work like:

path: either relative or absolute

sbctl sign <path>: replaces file
sbctl sign -s <path>: replaces file, saves the absolute path
sbctl sign <path> -o <path>: generates new file
sbctl sign -s <path> -o <path>: generates new file, saves the absolute path for both original file and output file

The 2nd and 4th options can instead be errors if used without an absolute path.

Right, because we don't really know when the file has changed when we save it with -o. We could store a checksum of the original file?

I think that could work, but why not store checksums for both files? Do you think a user could have some other program make changes to the signed file and still want to use it?

The entire logging code is a giant hack as I wanted to have the same output format as efi-roller. Mostly because I didn't want to spend time inventing a new formatting to the output. Suggestions for something more sane, and or help redesigning the output would be welcome

I will take a look at it later!

Foxboron · 2020-06-24T20:44:36Z

The 2nd and 4th options can instead be errors if used without an absolute path.

Hmm, this seems complicated. It would be better to enforce absolute paths across the board and be consistent I think?

I think that could work, but why not store checksums for both files? Do you think a user could have some other program make changes to the signed file and still want to use it?

Good point. They could very well sign with multiple db keys, and currently we would just override it. So maybe we should have some logic if the outfile is defined? Ensure we have signed it if it exists? and only create it if it doesn't exist?

ericonr · 2020-06-24T23:03:51Z

Hmm, this seems complicated. It would be better to enforce absolute paths across the board and be consistent I think?

I don't see a problem with it. Would just make the usecase of generating a signed file to use elsewhere a bit trickier, but no big deal.

Ensure we have signed it if it exists? and only create it if it doesn't exist?

We'd need proper diagnostics messages for these, I think. But yeah, that sounds ok.

ericonr · 2020-06-25T07:35:14Z

If the original file ceases existing, it bugs out as well.

Foxboron · 2020-07-09T13:27:12Z

So local code now runs filesystem.Abs on the output files with sign, and create-bundle which is the one we store in the database. I think this should ensure we are always having absolute paths.

I'll fix the missing file issue as well.

Foxboron · 2020-07-09T14:20:10Z

I think the latest commits fixes several of the issues mentioned in this issue. Please verify :)

ericonr · 2020-07-09T18:30:28Z

I think you forgot to commit the ChecksumFile function.

Foxboron · 2020-07-09T22:53:18Z

Woopsie, that is correct! fixed :)

ericonr · 2020-07-10T02:53:48Z

Found two issues, fixed in #15

ericonr · 2020-07-10T18:38:07Z

I think this can be closed, thanks!

Foxboron · 2020-07-11T10:45:41Z

Poke me if #11 is OK to merge after this or if there are anything missing :)

ericonr closed this as completed Jun 23, 2020

ericonr reopened this Jun 24, 2020

ericonr mentioned this issue Jun 24, 2020

Update docs #11

Merged

Foxboron closed this as completed Jul 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sign the fwupmgr bootloader #10

Sign the fwupmgr bootloader #10

ericonr commented Jun 23, 2020

Foxboron commented Jun 23, 2020 •

edited

Loading

ericonr commented Jun 23, 2020

Foxboron commented Jun 23, 2020

ericonr commented Jun 24, 2020 •

edited

Loading

Foxboron commented Jun 24, 2020

Foxboron commented Jun 24, 2020

ericonr commented Jun 24, 2020

Foxboron commented Jun 24, 2020

ericonr commented Jun 24, 2020

ericonr commented Jun 25, 2020

Foxboron commented Jul 9, 2020

Foxboron commented Jul 9, 2020

ericonr commented Jul 9, 2020

Foxboron commented Jul 9, 2020

ericonr commented Jul 10, 2020

ericonr commented Jul 10, 2020

Foxboron commented Jul 11, 2020

Sign the fwupmgr bootloader #10

Sign the fwupmgr bootloader #10

Comments

ericonr commented Jun 23, 2020

Foxboron commented Jun 23, 2020 • edited Loading

ericonr commented Jun 23, 2020

Foxboron commented Jun 23, 2020

ericonr commented Jun 24, 2020 • edited Loading

Foxboron commented Jun 24, 2020

Foxboron commented Jun 24, 2020

ericonr commented Jun 24, 2020

Foxboron commented Jun 24, 2020

ericonr commented Jun 24, 2020

ericonr commented Jun 25, 2020

Foxboron commented Jul 9, 2020

Foxboron commented Jul 9, 2020

ericonr commented Jul 9, 2020

Foxboron commented Jul 9, 2020

ericonr commented Jul 10, 2020

ericonr commented Jul 10, 2020

Foxboron commented Jul 11, 2020

Foxboron commented Jun 23, 2020 •

edited

Loading

ericonr commented Jun 24, 2020 •

edited

Loading