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

[kd]

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.

[sjg]

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]

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.

[kd]

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

[kd]

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]

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

[kd]

Updated after email discussion with @sjg.

[kd]

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

[sjg]

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]

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]

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]

stand/efi/libefi/efienv.c

51 ↗

(On Diff #53621)

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

[mw]

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 :)

[kd]

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

[kd]

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]

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.

[kd]

Update style issues and add some comments.

[kd]

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]

One minor nit left

lib/libsecureboot/Makefile.inc

127 ↗

(On Diff #54574)

quotes should encompass the :gmtime} reference

[mw]

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]

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]

Oh! Is this

lib/libsecureboot/Makefile.inc

127 ↗

(On Diff #54574)

Wait a moment, how does work with reproducible builds?

[sjg]

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]

lib/libsecureboot/Makefile.inc

127 ↗

(On Diff #54574)

We need to make it reproducible in the default configuration

[sjg]

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]

lib/libsecureboot/Makefile.inc

127 ↗

(On Diff #54574)

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

[kd]

Rebase on r344784

[kd]

lib/libsecureboot/Makefile.inc

127 ↗

(On Diff #54574)

Thanks, the changes discussed above were removed.