Page MenuHomeFreeBSD

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

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

Details

Summary

UEFI related headers were copied from edk2.

A new build option "MK_LOADER_EFI_SECUREBOOT" was added to allow 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.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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
130

Why is this necessary?

lib/libsecureboot/h/verify_file.h
29

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

lib/libsecureboot/verify_file.c
211

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

212

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

lib/libsecureboot/vets.c
377

space after for

share/mk/src.opts.mk
204

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

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
130

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
29

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
212

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
377

Thanks.

share/mk/src.opts.mk
204

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
204

Actually I set it to depend on "LOADER_VERIEXEC"

lib/libsecureboot/verify_file.c
212

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
204

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.

mindal_semihalf.com edited the summary of this revision. (Show Details)

Updated and rebased on HEAD, since all related @sjg patches were upstreamed.

lib/libsecureboot/Makefile.inc
130

I think it would be preferable to specify BUILD_UTC_FILE as an absolute path if need be.
Leave these rules as flexible as possible

lib/libsecureboot/Makefile.libsa.inc
20

My preference is for list like this to be indented by single TAB
makes for more consistent look. To that end SRCS+= \
helps too

24

as above TAB indent pls

lib/libsecureboot/efi/efi_variables.c
192

space after for

200

space after if

244

space after while

lib/libsecureboot/vets.c
362

this seems wasteful. Why not compute the hash after you know which type it needs to be compared to?

448

This call really depends on use of EFI for trust store.
Since as currently implemented it adds a lot of overhead, avoiding when pointless would be good

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

These changes can certainly go in as independent commits, and just go ahead when there's a positive comment in here, no need to create another phab review for them.

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

Actually I commited that change some time ago:
https://svnweb.freebsd.org/base?view=revision&revision=343911
I think it was mentioned during patchset update.

Thanks!

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

Ok, thanks. Perhaps upload a new diff to Phabricator?

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

We are discussing under old diff - the current version of https://reviews.freebsd.org/D19093 has not been including this chunk of code for a while :)

Remove changes to how files with trusted certs are found in makefiles. Also calculate cert digests for revocation only when necessary.

mindal_semihalf.com added inline comments.
lib/libsecureboot/Makefile.inc
130

I removed these changes altogether. The BUILD_UTC can now be defined by calling "date +%s" when no BUILD_UTC_FILE is present instead.

lib/libsecureboot/vets.c
362

Changed it so that now digests are calculated when necessary. Thanks.

448

I made checking for forbidden digests to be the last call. IMO in the most popular case - successful verification it won't matter since we have to perform all these checks.

Looking better....

lib/libsecureboot/Makefile.inc
137

You do not need to use date, ${%s:L:gmtime} will produce the same output
btw bonus points for the use of .OODATE !

lib/libsecureboot/h/verify_file.h
42

can you make sure this is aligned with other prototypes

lib/libsecureboot/libsecureboot-priv.h
57

alignment?

lib/libsecureboot/vets.c
121

alignment needs cleanup

359

I think a comment is needed to explain what you are trying to achieve,
since I'm still not clear why you might need to compare more than one hash.

448

Unless you are using EFI though the forbidden checks make no sense because there are not going to be any. I'd like to avoid any unnecessary overhead.

Update style issues and add some comments.

mindal_semihalf.com added inline comments.
lib/libsecureboot/Makefile.inc
137

Thanks.

lib/libsecureboot/vets.c
121

Sorry, I went through the entire patch and cleaned up everything I noticed, should be good now.

359

Added some comments to clarify this.
Basically UEFI standard allows to store three types of digests - sha256, sha384 and sha512.
In here firstly the tbs section from each certificate in chain is extracted.
Then its digest is compared against entries extracted from UEFI.

448

I suppose that they can be put inside an ifdef to call them only when library is compiled with EFI support.
On the other hand when EFI is not used these calls add an overhead of only checking the sizes of two vectors.

One minor nit left

lib/libsecureboot/Makefile.inc
137

quotes should encompass the :gmtime} reference

This revision is now accepted and ready to land.Mar 2 2019, 7:18 PM
lib/libsecureboot/Makefile.inc
137

Hi @sjg,

I will commit this change, so to be sure, - is below correct?

echo '#define BUILD_UTC ' ${%s:L:"gmtime"} >> ${.TARGET} ${.OODATE:MNOMETA_CMP}
lib/libsecureboot/Makefile.inc
137

No. Sorry for being too lazy to type ;-)
I was referring to the single quote:
echo '#define BUILD_UTC ${%s:L:gmtime}' >> ${.TARGET}
ie the closing single quote moved past the variable reference

Oh! Is this

lib/libsecureboot/Makefile.inc
137

Wait a moment, how does work with reproducible builds?

lib/libsecureboot/Makefile.inc
137

If you want reproducible you'd need to specify BUILD_UTC_FILE
and its mtime will be used

lib/libsecureboot/Makefile.inc
137

We need to make it reproducible in the default configuration

lib/libsecureboot/Makefile.inc
137

All depends on what you call the default configuration.
First off; none of this is enabled by default.
In our build there is a BUILD_UTC_FILE so that's reproducible,
but semihalf are trying to allow for the case where there is no suitable file.
I guess you could allow for forcing BUILD_UTC=0 or any other value you like.
The result will be reproducible - whether it works is a different matter.

BUILD_UTC provides a starting point for the loader's notion of time (it lacks access to RTC), it will get updated with mtime of files it reads (if > current value), so BUILD_UTC=0 could work in most cases, but is not ideal since you cannot handle expired certs

lib/libsecureboot/Makefile.inc
137

Ok this part of the change is now mooted by commit from D19464

This revision now requires review to proceed.Mar 5 2019, 3:28 PM
lib/libsecureboot/Makefile.inc
137

Thanks, the changes discussed above were removed.

This revision was not accepted when it landed; it landed in state Needs Review.Mar 6 2019, 6:40 AM
This revision was automatically updated to reflect the committed changes.