Build libve for loader and sbin/veriexec

[sjg]

Summary

Most of the code here is for the loader only.
It is basically a simple standalone implementation
of Verified Exec for the loader.

The srcs listed in Makefile.libsa.inc are only
used by the loader (or the tests/ application).

local.trust.mk is provided as an example
it needs to be tweaked for local environment.
Its job is to populate the trust anchors that get embedded into vets.c
and opgp_key.c (if OPENPGP support is enabled).
Right now it enables sha1 simply to facilitate testing with a Junos VM
which has sha1 fingerprints in its manifests.
That will be removed before commit.

All the hashes and signature algorithms to be supported are
configurable via local.trust.mk

[sjg]

and agc

[sjg]

Run self tests from test app

[sjg]

Set MANIFEST_SKIP if configured

[sjg]

Use file2c to embed certs/keys

[sjg]

Adding xrefs to related reviews

D16337 for build options etc
D16336 for changes to stand/
D16334 for libbearssl

[bdrewery]

lib/libve/Makefile

16–18 ↗

(On Diff #47708)

This kind of thing isn't standard and I think it could be done cleaner like ports ala something like MK_REQUIRES= BEARSSL

[cem]

I realize this is a very bikesheddy ask but would it be at all possible to make the library name longer? libveriexec or something (I don't care in particular). The vast majority of libraries have descriptive names and I don't think scrimping on a few letters is making anyone's life better.

[cem]

lib/libve/openpgp/decode.c

34 ↗

(On Diff #47708)

Seems like n should just be size_t instead of int

40 ↗

(On Diff #47708)

This cast has no effect

41 ↗

(On Diff #47708)

style(9) nit: pointers should not be used as boolean values

lib/libve/openpgp/opgp_key.c

108 ↗

(On Diff #47708)

DigestFinal's 3rd argument is unsigned int* — this cast is a NOP.

(If it were not a nop, casting pointer out parameters to different sizes is totally bogus in general.)

Style(9) nit: spaces after commas in arguments list

110 ↗

(On Diff #47708)

The truncated hash key->id is subsequently used for lookup in trusted keys. This is dubious (I don't know whether it does or does not affect the overall security of the system as specifically constructed, but it's not a good practice regardless). Similar truncated truncated hash attacks have been used to attack the PGP/GPG ecosystem in the past, e.g., this from 2014: https://evil32.com/ . The truncated hash is effectively 32 bits, which is far weaker than even SHA1's 160 bits.

(Also octets2hex is returning a pointer to its internal static buffer here so in general the key lifetime is questionable.)

169 ↗

(On Diff #47708)

I'm not really pleased to see arbitrary octal constants throughout the source

277–278 ↗

(On Diff #47708)

TOCTOU

[sjg]

cem (Conrad Meyer) <[e-mail]> wrote:

I realize this is a very bikesheddy ask but would it be at all
possible to make the library name longer?  libveriexec or something

I have no objection to and indeed welcome better names.
Sadly libveriexec is already taken.

(I don't care in particular).  The vast majority of libraries have
descriptive names and I don't think scrimping on a few letters is
making anyone's life better.

Agreed - naming stuff is hard.

[sjg]

lib/libve/Makefile

16–18 ↗

(On Diff #47708)

Isn't that MK_REQUIRES an abuse of the option system?

lib/libve/openpgp/decode.c

34 ↗

(On Diff #47708)

probably, yes

40 ↗

(On Diff #47708)

recollection is vague but I expect gcc or clang complained, but making n size_t would render the question moot.

41 ↗

(On Diff #47708)

ok

lib/libve/openpgp/opgp_key.c

108 ↗

(On Diff #47708)

pretty sure the cast is/was there because one of the toolchains complained

110 ↗

(On Diff #47708)

The key id usage is from the RFC.
OpenPGP is obviously not perfect - but it is still in wide use.

And yes the the id life time is transitory.

169 ↗

(On Diff #47708)

Suggestions welcome - the goal is to test if 8th bit is set - would a comment help?

277–278 ↗

(On Diff #47708)

Sorry what does that mean?

[sjg]

lib/libve/openpgp/opgp_key.c

108 ↗

(On Diff #47708)

Ah, the cast is only a no-op if size_t is unsigned int which is certainly not the case for all the architectures that code is compiled for.

Note: This is currently dead code for FreeBSD - this being in the not-BEARSSL case,
but for sbin/veriexec D16575 it may be desirable to add back the OpenSSL support.

[sjg]

size_t for 2nd arg to octets2hex

[sjg]

remove check for MK_BEARSSL from Makefile

[cem]

In D16335#363841, @sjg wrote:

I have no objection to and indeed welcome better names.
Sadly libveriexec is already taken.

It doesn't seem to be taken. The only library with that name is brand new, tiny, and also associated with Juniper's "verified exec" feature. It seems plausible to suggest moving it out of the way if the name would fit better on this one.

On the other hand, if this one is predominantly for loader, maybe "libveriexec_standalone" (or some other combination of "stand" or "loader" and "veriexec") would be suitable. You could even call it libveriexec2 — almost anything is better than libve.

lib/libve/openpgp/opgp_key.c

108 ↗

(On Diff #47708)

Ah, I thought mlen was unsigned int for some reason. I think the dead code should be fixed or removed. If the idea is it may be used in the future, fixing it is the way to go. Casting size_t* to unsigned* is totally bogus.

110 ↗

(On Diff #47708)

RFCs aren't intended to be a substitute for thought.

What's wrong with identifying keys using the full fingerprint instead of just the last 32 bits?

169 ↗

(On Diff #47708)

Usually significant bits can be named with macros. Like #define ARMORED 0x80 or whatever makes the most sense here. (And in general hex constants are more common and easier (for me) to understand, but naming is best.)

277–278 ↗

(On Diff #47708)

There appears to be a race between Time Of Check and Time Of Use.

https://en.wikipedia.org/wiki/Time_of_check_to_time_of_use

This is in the section #ifndef standalone so I guess it may be used in ordinary userspace (not just the constrained loader environment).

[sjg]

lib/libve/openpgp/opgp_key.c

108 ↗

(On Diff #47708)

So what is your suggestion for passing a size_t * to an API that wants unsigned int * ?

110 ↗

(On Diff #47708)

Have a look at the RFC - or opgp_sig.c - the signature contains the truncated SHA1 hash as key-id - which is how we need to find the key to verify the signature. Using full hash thus would serve no purpose.

RFC's are about interoperability as much as anything else.

277–278 ↗

(On Diff #47708)

Since the check is not a verification, I'm not sure what the concern is.
If someone plays games with replacing/removing the key during that window, signature verification will fail so a denial of service at worst?
If someone is playing such games you are already in trouble.

[cem]

lib/libve/openpgp/opgp_key.c

108 ↗

(On Diff #47708)

It cannot be done. Use an unsigned stack variable and pass its address instead.

110 ↗

(On Diff #47708)

How does such a signature scheme protect against forgery? Can an attacker just generate a key that collides the 32 bit key-id and sign anything as if they are the first key?

(I don't think interoperability with a broken scheme is useful.)

277–278 ↗

(On Diff #47708)

If the check serves no purpose, why have it?

[sjg]

lib/libve/openpgp/opgp_key.c

108 ↗

(On Diff #47708)

ok will do

110 ↗

(On Diff #47708)

No, of course you cannot create a key that collides the id, and thus cheat the system.
The key-id is for looking up a trusted key - if the wrong key is found (because you have a collision) then verification will fail.

169 ↗

(On Diff #47708)

ARMORED is not really correct - will see if I can think of something suitable.
Octal masks btw are much easier to translate to binary.

277–278 ↗

(On Diff #47708)

It serves the purpose of avoiding an attempt to open a file that does not exist.

[sjg]

define OPENPGP_TAG_*

[sjg]

Back to the question of lib name...

It doesn't seem to be taken. The only library with that name is brand new, tiny, and also associated with Juniper's "verified exec" feature. It seems plausible to suggest moving it out of the way > if the name would fit better on this one.

The other libveriexec is a userland lib for talking to the in-kernel mac_veriexec - which is a completely different beast to what is in libve.

As noted below, working 'stand' into the name might work though
in a followup commit I'll likely want to rework libve so that it can utilize OpenSSL api's when built for userland (for sbin/veriexec needed for mac_veriexec)
so 'stand' might seem odd in that context. This would allow use of mac_veriexec without the need to BearSSL.

I'll be posting an update to this review shortly - but still punting on lib name for now.

On the other hand, if this one is predominantly for loader, maybe "libveriexec_standalone" (or some other combination of "stand" or "loader" and "veriexec") would be suitable. You could even call it libveriexec2 — almost anything is better than libve.

[sjg]

Fix error message in vectx_open for VE_FINGERPRINT_UNKNOWN
verify - only panic if severity is above accepted threshold
(eg we are in strict mode)
Add comment to vepcr.c to explain why we use SHA256 when TPM
only supports SHA1

[kd]

Currently at Semihalf we work on a similar solution to make FreeBSD work with UEFI Secure Boot. The main difference is that instead of creating a manifest with files and their hashes a signature is appended to each file that is supposed to be verified. We also use BearSSL as the cryptographic backend.

There are two main advantages of this approach. Since all files have their signatures attached to themselves there is no need to update a manifest each time a module/kernel has changed, as long as the file is properly signed we are good to go. Also it is subjectively easier and faster to simply use a tool to sign a new file instead of updating and signing a manifest.

The current status on this project is as follows:
A tool to sign binaries was created. It simply appends an RSA PKCS#1 v2 signature together with a X509 certificate to the end of the file.
Loader was changed to allow only to load signed kernel and modules when Secure Boot mode is detected to have been enabled in UEFI. The trusted root certificates are obtained from UEFI DB authenticated variable. Also the DBX variable is searched for blacklisted certificates.
Currently we are missing a verification routine in kernel to allow only the loading of signed modules.

We expect to be ready to show it on Phabricator in a week or two.

[sjg]

In D16335#400537, @mindal_semihalf.com wrote:

Currently at Semihalf we work on a similar solution to make FreeBSD work with UEFI Secure Boot. The main difference is that instead of creating a manifest with files and their hashes a signature is appended to each file that is supposed to be verified. We also use BearSSL as the cryptographic backend.

Signing files individually works, but is more expensive (the cost to compare hash is the same, but you also have the signature to verify).
If files are installed via packages and you have a manifest per package (as we do in junos) then there little disadvantage.

Of course the loader generally works with a small number of files compared to the kernel.

There are two main advantages of this approach. Since all files have their signatures attached to themselves there is no need to update a manifest each time a module/kernel has changed, as long as the file is properly signed we are good to go. Also it is subjectively easier and faster to simply use a tool to sign a new file instead of updating and signing a manifest.

This also increases the cost of generating packages.
We run a bunch of signing servers in multiple geographies, and they can easily handle the load from 1000's of builds per day, but if you were to increase the number of signatures per build by a few orders of magnitude it becomes more challenging.

The current status on this project is as follows:
A tool to sign binaries was created. It simply appends an RSA PKCS#1 v2 signature together with a X509 certificate to the end of the file.

I hope you mean certificate chain?

Loader was changed to allow only to load signed kernel and modules when Secure Boot mode is detected to have been enabled in UEFI. The trusted root certificates are obtained from UEFI DB authenticated variable. Also the DBX variable is searched for blacklisted certificates.
Currently we are missing a verification routine in kernel to allow only the loading of signed modules.

That comes with mac_veriexec - a key advantage of this loader impl is that it leverages all the infra for supporting mac_veriexec

We expect to be ready to show it on Phabricator in a week or two.

[sjg]

A tool to sign binaries was created. It simply appends an RSA PKCS#1 v2 signature together with a X509 certificate to the end of the file.
Loader was changed to allow only to load signed kernel and modules when Secure Boot mode is detected to have been enabled in UEFI. The trusted root certificates are obtained from UEFI DB authenticated variable. Also the DBX variable is searched for blacklisted certificates.

I forgot to mention, that my plan was for DBX etc keystore to be used to verify the loader itself.
But all other verification is done via signed manifests and associated certificate chains - which has proved extremely flexible over past 15 years

[sjg]

Rename libve to libsecureboot

[kd]

lib/libsecureboot/brssl.sed

6 ↗

(On Diff #52850)

Hiding blocks of code like results with empty select cases, which breaks compilation for me.

Using the following instead seems to work:

/fprintf/i\
#ifndef _STANDALONE

/EXIT_FAILURE/a\
#else \
; \
#endif

lib/libsecureboot/h/libsecureboot.h

28 ↗

(On Diff #52850)

Header guard is missing.

lib/libsecureboot/h/verify_file.h

25 ↗

(On Diff #52850)

Header guard is missing.

lib/libsecureboot/libsecureboot-priv.h

31 ↗

(On Diff #52850)

Header guard is missing.

[kd]

lib/libsecureboot/vets.c

426 ↗

(On Diff #52850)

Looks like unused variable.

[kd]

lib/libsecureboot/local.trust.mk

96 ↗

(On Diff #52850)

Rather than for *.pem we should search for t*.pem since later only the certificates found in t*.pem are parsed.

[sjg]

lib/libsecureboot/local.trust.mk

96 ↗

(On Diff #52850)

Actually we want [tv]*.pem and [tv]*.asc
The t* are for trust anchors and the v* for verification (should be one per t*)

lib/libsecureboot/vets.c

426 ↗

(On Diff #52850)

Thanks

[sjg]

Rename libve to libsecureboot

[sjg]

Trim trailing ../ from prefix

[sjg]

Only pass prefix to load_manifest if skip!=NULL