[systemd-boot] added shim signature/MOK validation by mxre · Pull Request #5702 · systemd/systemd

mxre · 2017-04-05T01:06:20Z

I have written a shim/MOK protocol handler that installs a security/security2 policy in EFI

This enables systemd-boot in a Secure Boot environment to validate a self signed linux kernel (with a PE stub) to validate against the shim[1] prebootloader. Currently only GRUB2 (patched by Fedora) and rEFInd[2] can handle this.

The code I committed is currently just a proof of concept based largely on code from rEFInd and shim.

[1] https://github.com/rhinstaller/shim
[2] https://github.com/falstaff84/rEFInd

poettering · 2017-04-05T10:15:26Z

Hmm, not grokking this? Why precisely is this needed? So far the idea was that kernels are signed using pesign and the firmware validates that. What precisely does your work do differently and why would that be a good thing? Can you elaborate?

(I didn't look closely at the code yet, but the coding style appears quite off, i.e. 8ch indenting and stuff, please have a look at CODING_STYLE and reindent)

mxre · 2017-04-05T11:47:40Z

The firmware can only validate Microsofts key (and most kernels are not signed with that) The firmware can be setup in user mode to use your own certificates, but this is not always easy (firmware is quite difficult to set this up). Hence for Linux there exist two tools (that are signed by Microsoft and can be run on any Secure Boot machine) Preloader.efi (by the Linux foundation) and Shim (by RedHat/Fedora)

PreLoader hashes every EFI-binary (i.e. a kernel) that fails to load with the default security policy and stores the hash in NVRAM, later if the same kernel is run again it is checked against the stored hash.

Shim allows the user to install a custom Machine Owner Key in NVRAM and every loaded signed EFI-binary is checked with this key, or the default policy is used, allowing to load Microsoft signed EFI-executables (i.e. bootmgfw.efi)

Shim does not (in contrast to PreLoader) install a security policy in the EFI Bootservices table, it relies on the boot loader to do this. This path does exactly that.

See also: http://www.rodsbooks.com/efi-bootloaders/secureboot.html for a nice overview of Secure Boot / Linux

I'm sorry that I violated your coding style, I try to fix that. I still see this as a hack job by me and further feed back is welcome. Especially testing, I only use it on my laptop, and qemu (using tiano-core) which both have SecurityPolicy2 support. I try to get my hands on some older hardware with SecurityPolicy1 support, because I've not tested that yet and suspect there might be hiding some bugs there

dvdhrm · 2017-04-18T13:43:11Z

Why can't shim install its security policy? This seems like a major pain just to support buggy firmware. Why don't we require all binaries either to be signed by an installed key, or the pre-loader to install its own security policy?

If a firmware cannot properly implement UEFI, why do we bother supporting Secure Boot on those firmwares? Or is there another reason why shim does not override the security policy?

poettering · 2017-04-18T14:34:51Z

@msekletar opinion on this one?

mxre · 2017-04-18T14:45:32Z

Why can't shim install its security policy?

It can, but no signed build (and only those are usefull in a Secure Boot environment exists) Fedora ships with shim 0.8 and the Debian package is 0.9 (I think it's signed by Ubuntu/Canonical). To build with secure policy install one must set OVERRIDE_SECURITY_POLICY before calling make.

Why don't we require all binaries either to be signed by an installed key...

I think this is infeasible, usually this means every kernel image must be signed by Microsoft, also self-built kernels would be impossible.

... pre-loader to install its own security policy?

Linux Foundations PreLoader.efi does this, and it has its problems (e.g. [1]). This patch does not avoid having the need for a security policy

If a firmware cannot properly implement UEFI ...

It's not a bug in the firmware, its an optional UEFI feature. It's part of the Driver Execution Environment(DXE):
"The driver that produces the EFI_SECURITY_ARCH_PROTOCOL may also optionally install the EFI_SECURITY_POLICY_PROTOCOL_GUID onto a new handle with a NULL interface." [2, Platform Initialization Specification 1.5, Vol. 2 DXE Core Interface Specification, pg. 154]

Note, that this path does not provide authentication on UEFI implementation on platform that do not implement the security protocol, one must override LoadImage (like the patched version Fedora uses) to actually do that). It just allows the user to utilize shim like PreLoader.efi, which I personally find very annoying to use since it requires to hash the kernel after every update, before booting. With shim I can create a signature using a "systemd.path"-trigger for the kernel update, and use sbsign to create a signature, so booting can work without knowing that secure boot is even installed

[1] https://bbs.archlinux.org/viewtopic.php?id=169354
[2] http://www.uefi.org/specifications

msekletar · 2017-04-19T15:40:53Z

I'd really like if we keep this kind of code in one place. shim already installs security policy override if it is compiled with correct flags. Getting newer version of shim signed will take some time but this isn't really pressing issue. I'd consider merging this patch only when we know that getting Microsoft to sign the new version of shim is out of the question.

mxre · 2017-04-19T16:54:27Z

I agree that this should be a part of shim, I didn't know that a new signed version for shim is underway.
Considering this patch as a workaround for all those who can't wait (like me), makes sense.

msekletar · 2017-04-20T07:44:23Z

I agree that this should be a part of shim, I didn't know that a new signed version for shim is underway.

AFAICT, no such effort is underway right now. But at least Fedora should have prior experience with the whole process. Maybe it would make sense to start the discussion about this on fedora-devel.

poettering · 2017-04-24T15:23:17Z

So, after briefly talking about this with Kay: I think doing this is a bit besides the point of sd-boot, but I figure it also doesn't hurt, if this stuff is nicely isolated and non-invasive, hence I figure this can go in, if @msekletar is fine with it too.

poettering

a superficial review, mostly coding style issues.

(Note that I am no EFI guru, I only looked over this superficially. I kinda hope this is well tested. Also, I'd be thankful if @msekletar could have a look at this before we merge this, and give his OK too)

poettering · 2017-04-24T15:23:56Z

src/boot/efi/boot.c


+        if (shim_loaded()) {
+                Print(L"Shim:                   present\n");
+        }


Please follow coding style, i.e. no {} around single-line if blocks. For details see CODING_STYLE

poettering · 2017-04-24T15:24:31Z

src/boot/efi/shim.c

+
+BOOLEAN shim_loaded(void) {
+        SHIM_LOCK   *shim_lock;
+        EFI_GUID    ShimLockGuid = SHIM_LOCK_GUID;


please follow coding style: no spurious spaces, we do not align variable declarations like this...

poettering · 2017-04-24T15:24:54Z

src/boot/efi/shim.c

+        SHIM_LOCK   *shim_lock;
+        EFI_GUID    ShimLockGuid = SHIM_LOCK_GUID;
+
+        return (uefi_call_wrapper(BS->LocateProtocol, 3, &ShimLockGuid, NULL, (VOID**) &shim_lock) == EFI_SUCCESS);


no need for the outer (), as "return" isn't a function...

poettering · 2017-04-24T15:25:10Z

src/boot/efi/shim.c

+static BOOLEAN shim_validate(VOID *data, UINT32 size) {
+        SHIM_LOCK   *shim_lock;
+        EFI_GUID    ShimLockGuid = SHIM_LOCK_GUID;
+        EFI_STATUS  result;


no alignment of vars like this please, see above

poettering · 2017-04-24T15:26:49Z

src/boot/efi/shim.c

+        EFI_GUID    ShimLockGuid = SHIM_LOCK_GUID;
+        EFI_STATUS  result;
+
+        if ((data != NULL) && (uefi_call_wrapper(BS->LocateProtocol, 3, &ShimLockGuid, NULL, (VOID**) &shim_lock) == EFI_SUCCESS)) {


I think it would be nicer, to check the data != NULL in a separate if check and exit early, so that we don't have to indent the block in that case here...

i.e. just do:

if (!data) return FALSE

early.

also, we usually rely on C's downgrade-to-bool logic when checking whether ptrs are NULL or non NULL, and do not do explicit == NULL checks hence.

poettering · 2017-04-24T15:30:13Z

src/boot/efi/shim.c

+                return EFI_SUCCESS;
+        } else {
+                return Status;
+        }


no {} around single-line if blocks....

Also, given that you return in the "if" branch, you can drop the "else" branch, and just place the "return Status" in the normal control flow

poettering · 2017-04-24T15:30:27Z

src/boot/efi/shim.c

+        EFI_FILE          *root;
+        VOID              *FileBuffer = NULL;
+        UINTN             FileSize;
+        CHAR16            *DevPathStr;


weird alignment...

poettering · 2017-04-24T15:31:10Z

src/boot/efi/shim.c

+
+        if (DevicePathConst == NULL) {
+                return EFI_INVALID_PARAMETER;
+        }


no {} around single-line if blocks (here and everywhere else)

No comparing with == NULL, see above... (here and everywhere else)

poettering · 2017-04-24T15:32:15Z

src/boot/efi/shim.h

+ */
+
+/* well known shim lock guid */
+#define SHIM_LOCK_GUID { 0x605dab50, 0xe046, 0x4300, {0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23} }


header file is missing double inclusion guards with #ifndef ... #define..., see other header files

poettering · 2017-04-24T15:32:53Z

src/boot/efi/shim.c

+                status = uefi_call_wrapper(BS->LocateProtocol, 3, &SECURITY2_PROTOCOL_GUID, NULL, (VOID**) &security2_protocol);
+
+                if (status != EFI_SUCCESS)
+                return status;


strange indentation...

poettering · 2017-04-24T15:34:49Z

(also, needs a rebase, since github doesn't think this is mergable, see conflicts list)

poettering · 2017-04-24T15:36:33Z

Also, please meld the commits together: when we merge stuff we want commits for seperate logic steps if you so will, but only want to merge review history in exceptional cases. Hence: unless it makes sense to split up your work into separate logic steps, just merge the commits into one, thanks

mxre · 2017-04-24T19:31:19Z

I fixed all the coding style in shim.c (and the one in boot.c) rebased and squashed the commits.

mxre · 2017-04-24T19:38:56Z

(Note that I am no EFI guru, I only looked over this superficially. I kinda hope this is well tested. ...

I tested in on my laptop, uses Security Policy 1 and it works every day, I've tested Security Policy 2 on QEMU with OVMF which i also used during development, I just tested Policy 2 on real hardware for a short period, but not issues occurred.

On a system without SecureBoot enabled (or even available), this patch is invisible, the same is true for systems with SecureBoot enabled but where shim is not installed in the BootServices Table.
The first case I tested, the second not, but I do not think that it makes a difference.

Also all systems that I tested, are x86_64. I'm not even sure a i386 secure boot installation exists, or if shim is compatible with non x86_64 UEFI implementations.

poettering · 2017-04-25T07:45:15Z

looks OK to me now, but could you add an explanation of what this is and does to the commit msg? it appears to carry none right now. Please explain in detail what this does and what the usecase precisely is.

Also before this can go in @msekletar should say yay or nay! @msekletar?

mxre · 2017-04-25T09:49:40Z

I added an explanation to the commit message that hopefully summarizes the content of our discussion here in a meaningful way.

Adds support for booting in a SecureBoot environment with shim as a preloader. Install an appropriate UEFI security policy to check PE signature of a chained kernel or UEFI application (using LoadImage()) against the MOK database maintained by shim, using shim's installed BootServices. Implementation details for installing the security policy are based on code from the LinuxFoundation's SecureBoot PreLoader, part of efitools licensed under LGPL 2.1 Current signed (by Microsoft) versions of shim (Versions 0.8 & 0.9) so not install a security policy by themselves, future Versions of shim might (a compile time switch exists in rectent git versions), so in the future this PR might become unnecessary.

poettering · 2017-05-09T18:57:33Z

ok, will merge this now. @msekletar would still be good to get some review notes from you...

mirh · 2018-06-16T21:08:19Z

Also all systems that I tested, are x86_64. I'm not even sure a i386 secure boot installation exists, or if shim is compatible with non x86_64 UEFI implementations.

Just for the very records (hope I'm not cluttering), since shim 15.2, there's a fully fledged ia32 binary.
As described here by Intel, that's an actual (for as much certainly niche) use case.

poettering added needs-discussion 🤔 sd-boot/sd-stub/bootctl labels Apr 18, 2017

poettering requested changes Apr 24, 2017

View reviewed changes

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed needs-discussion 🤔 labels Apr 24, 2017

mxre force-pushed the master branch 3 times, most recently from a74f5cc to dd38b4c Compare April 24, 2017 19:26

mxre force-pushed the master branch from dd38b4c to a7bd4e9 Compare April 25, 2017 09:48

mxre force-pushed the master branch from a7bd4e9 to 40a2f06 Compare April 26, 2017 15:13

poettering approved these changes May 9, 2017

View reviewed changes

poettering merged commit b2bb40c into systemd:master May 9, 2017

Uh oh!

Conversation

mxre commented Apr 5, 2017

Uh oh!

poettering commented Apr 5, 2017

Uh oh!

mxre commented Apr 5, 2017

Uh oh!

dvdhrm commented Apr 18, 2017

Uh oh!

poettering commented Apr 18, 2017

Uh oh!

mxre commented Apr 18, 2017

Uh oh!

msekletar commented Apr 19, 2017

Uh oh!

mxre commented Apr 19, 2017

Uh oh!

msekletar commented Apr 20, 2017

Uh oh!

poettering commented Apr 24, 2017

Uh oh!

poettering left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poettering commented Apr 24, 2017

Uh oh!

poettering commented Apr 24, 2017

Uh oh!

mxre commented Apr 24, 2017

Uh oh!

mxre commented Apr 24, 2017

Uh oh!

poettering commented Apr 25, 2017

Uh oh!

mxre commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

poettering commented May 9, 2017

Uh oh!

mirh commented Jun 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

mxre commented Apr 25, 2017 •

edited

Loading