Page MenuHomeFreeBSD

Extend libsecureboot(old libve) to obtain trusted certificates from UEFI and implement revocation
Needs ReviewPublic

Authored by mindal_semihalf.com on Wed, Feb 6, 1:00 PM.

Details

Summary

UEFI related headers were copied from edk2.

A new build option "MK_LOADER_EFI_SECUREBOOT" was added to allow the loading of trusted anchors from UEFI.

Certificate revocation support is also introduced.
The forbidden certificates are loaded from dbx variable.
Verification fails in two cases:

  1. There is a direct match between cert in dbx and the one in the chain.
  2. The CA used to sign the chain is found in dbx.

One can also insert a hash of TBS section of a certificate into dbx.
In this case verifications fails only if a direct match with a certificate in chain is found.

It depends on @sjg patches:

https://reviews.freebsd.org/D16334
https://reviews.freebsd.org/D16335
https://reviews.freebsd.org/D16336
https://reviews.freebsd.org/D16337
https://reviews.freebsd.org/D16575

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sjg added inline comments.Wed, Feb 6, 8:34 PM
lib/libbearssl/Makefile
13 ↗(On Diff #53621)

Why is this necessary?
This will defeat the reason it was set in the first place and will not restore the default behavior.

lib/libsecureboot/Makefile.inc
107

Why is this necessary?

lib/libsecureboot/h/verify_file.h
28

FWIW I just added struct stat; but haven't been able to update the review

lib/libsecureboot/verify_file.c
205

skip is const char *, not supposed to touch it.

206

This would defeat the point of skip.
Perhaps you want a MANIFEST_SKIP_NEVER ?

lib/libsecureboot/vets.c
383

space after for

share/mk/src.opts.mk
205

should this depend on BEARSSL?

stand/common/interp_forth.c
383 ↗(On Diff #53621)

This could be buried inside verify_file or ve_trust_init

stand/ficl/fileaccess.c
71 ↗(On Diff #53621)

yes, burying this within libsecureboot would be a win

imp added inline comments.Thu, Feb 7, 4:45 AM
stand/common/interp_forth.c
384 ↗(On Diff #53621)

I'd prefer if we didn't create more references than necessary to EFI symbols. Ideally we'd have none in interp_*.o. While they are currently compiled multiple times, there's a desire to move it into a single built library.
Here and below.

stand/efi/libefi/efienv.c
51 ↗(On Diff #53621)

this is unrelated and should be made as a simple commit.

stand/loader.mk
81 ↗(On Diff #53621)

This won't work. It will ALWAYS define EFI_SECURE_BOOT, which is undesirable, and why you had to jump through the EFI && EFI_SECURE_BOOT hoops above.

lib/libbearssl/Makefile
13 ↗(On Diff #53621)

Sorry, it is a leftover from when I was playing with build options. Will remove it.

lib/libsecureboot/Makefile.inc
107

I changed the cert paths to be absolute. Without it the "BUILD_UTC_FILE" looks for the certificate in the object dir. Also IMO using absolute paths when possible is a good idea in general.

lib/libsecureboot/h/verify_file.h
28

Ok, I'll remove it. I supposed that in the end I'll have to rebase on your path after it is merged into the tree.

lib/libsecureboot/verify_file.c
206

As for the skip, I'm not sure if I understand its purpose and if its working as intended. For example lets say that the manifest is in /boot/manifest and we want to verify /boot/kernel/kernel. This will look for and load /boot/kernel/../manifest. Without these changes the manifest will load with skip="kernel". This will result with only the files in /boot/kernel being found correctly.
I can't find a reference to MANIFEST_SKIP_NEVER anywhere in code.

lib/libsecureboot/vets.c
383

Thanks.

share/mk/src.opts.mk
205

You're right, thanks.

stand/common/interp_forth.c
383 ↗(On Diff #53621)

The reason it is done like this is because the efi related routines in libsecureboot can't be linked against a loader that doesn't support UEFI.
The libsecureboot is a part of libsa which is only compile once(or twice if you count the i368 build), where as the interp objects are compiled separately for UEFI and other targets.

384 ↗(On Diff #53621)

I suppose that these could be moved to stand/efi/loader/main.c.

stand/efi/libefi/efienv.c
51 ↗(On Diff #53621)

You're right, will move it. By the way this is needed to get the size of variable we are about to load, so that we know how much memory we need to allocate.

stand/loader.mk
81 ↗(On Diff #53621)

The LOADER_EFI_SECUREBOOT is a build option that is turned off by default. My intention behind this is to load the trusted certificates from UEFI only if user explicitly turned it on during compilation

Updated diff based on suggestions, also added imp to reviewers since he seems to be interested.

share/mk/src.opts.mk
205

Actually I set it to depend on "LOADER_VERIEXEC"

sjg added inline comments.Fri, Feb 8, 12:56 AM
lib/libsecureboot/verify_file.c
206

If the manifest is /boot/manifest and the path loaded is /boot/kernel/kernel
the manifest entry should generally be kernel/kernel
the manifest would be found via ../manifest and skip=kernel is likely correct.

skip is badly named - though I've not come up with anything better, because prefix is already used - to identify where the manifest was found.
without skip=kernel, the loader is probably going to look for just kernel in the manifest and not find it - skip=kernel causes it to look for kernel/kernel which should be correct given the layout you described.

MANIFEST_SKIP_NEVER does not exist (yet) I was positing that you might be wanting something that simply disables the setting of skip

regardless skip=NULL is better than *skip = '\0' which should produce a compile error.

share/mk/src.opts.mk
205

I actually mean LOADER_EFI_SECUREBOOT/BEARSSL
if you wanted to do LOADER_EFI_SECUREBOOT/LOADER_VERIEXEC
you'd need to test that actually works - it would certainly need to be listed after LOADER_VERIEXEC/BEARSSL

Updated after email discussion with @sjg.