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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
sjg added inline comments.Feb 6 2019, 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 ↗(On Diff #53621)

Why is this necessary?

lib/libsecureboot/h/verify_file.h
28 ↗(On Diff #53621)

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

lib/libsecureboot/verify_file.c
200 ↗(On Diff #53621)

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

201 ↗(On Diff #53621)

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

lib/libsecureboot/vets.c
383 ↗(On Diff #53621)

space after for

share/mk/src.opts.mk
206 ↗(On Diff #53621)

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.Feb 7 2019, 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 ↗(On Diff #53621)

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 ↗(On Diff #53621)

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
201 ↗(On Diff #53621)

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 ↗(On Diff #53621)

Thanks.

share/mk/src.opts.mk
206 ↗(On Diff #53621)

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
206 ↗(On Diff #53621)

Actually I set it to depend on "LOADER_VERIEXEC"

sjg added inline comments.Feb 8 2019, 12:56 AM
lib/libsecureboot/verify_file.c
201 ↗(On Diff #53621)

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
206 ↗(On Diff #53621)

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)Feb 26 2019, 4:29 PM
mindal_semihalf.com updated this revision to Diff 54419.

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

sjg added inline comments.Feb 26 2019, 6:55 PM
lib/libsecureboot/Makefile.inc
107 ↗(On Diff #53621)

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
21 ↗(On Diff #54419)

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

25 ↗(On Diff #54419)

as above TAB indent pls

lib/libsecureboot/efi/efi_variables.c
192 ↗(On Diff #54419)

space after for

200 ↗(On Diff #54419)

space after if

244 ↗(On Diff #54419)

space after while

lib/libsecureboot/vets.c
365 ↗(On Diff #54419)

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

428 ↗(On Diff #54419)

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

emaste added inline comments.Feb 26 2019, 7:47 PM
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.

mw added inline comments.Feb 26 2019, 10:03 PM
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!

emaste added inline comments.Feb 26 2019, 10:20 PM
stand/efi/libefi/efienv.c
51 ↗(On Diff #53621)

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

mw added inline comments.Feb 27 2019, 7:19 AM
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 marked 5 inline comments as done.Feb 28 2019, 10:55 AM
mindal_semihalf.com added inline comments.
lib/libsecureboot/Makefile.inc
107 ↗(On Diff #53621)

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
365 ↗(On Diff #54419)

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

428 ↗(On Diff #54419)

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.

sjg added a comment.Feb 28 2019, 9:02 PM

Looking better....

lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54524)

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
43 ↗(On Diff #54524)

can you make sure this is aligned with other prototypes

lib/libsecureboot/libsecureboot-priv.h
58 ↗(On Diff #54524)

alignment?

lib/libsecureboot/vets.c
122 ↗(On Diff #54524)

alignment needs cleanup

362 ↗(On Diff #54524)

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.

428 ↗(On Diff #54419)

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 marked 2 inline comments as done.Mar 1 2019, 10:58 AM
mindal_semihalf.com added inline comments.
lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54524)

Thanks.

lib/libsecureboot/vets.c
122 ↗(On Diff #54524)

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

362 ↗(On Diff #54524)

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.

428 ↗(On Diff #54419)

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.

sjg accepted this revision.Mar 2 2019, 7:18 PM

One minor nit left

lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

quotes should encompass the :gmtime} reference

This revision is now accepted and ready to land.Mar 2 2019, 7:18 PM
mw added inline comments.Mar 3 2019, 9:46 AM
lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

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}
sjg added inline comments.Mar 3 2019, 5:02 PM
lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

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

emaste added a comment.Mar 4 2019, 2:59 PM

Oh! Is this

lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

Wait a moment, how does work with reproducible builds?

sjg added inline comments.Mar 4 2019, 8:47 PM
lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

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

emaste added inline comments.Mar 4 2019, 8:53 PM
lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

We need to make it reproducible in the default configuration

sjg added inline comments.Mar 4 2019, 9:09 PM
lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

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

sjg added inline comments.Mar 4 2019, 10:05 PM
lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

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

Rebase on r344784

This revision now requires review to proceed.Mar 5 2019, 3:28 PM
lib/libsecureboot/Makefile.inc
127 ↗(On Diff #54574)

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.