Page MenuHomeFreeBSD

Build libve for loader and sbin/veriexec
ClosedPublic

Authored by sjg on Jul 19 2018, 12:30 AM.

Details

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

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 updated this revision to Diff 46360.Aug 6 2018, 10:56 PM

Run self tests from test app

sjg updated this revision to Diff 46491.Aug 10 2018, 12:54 AM

Set MANIFEST_SKIP if configured

sjg updated this revision to Diff 47708.Sep 5 2018, 8:22 PM

Use file2c to embed certs/keys

sjg added a comment.Sep 5 2018, 10:02 PM

Adding xrefs to related reviews

D16337 for build options etc
D16336 for changes to stand/
D16334 for libbearssl
bdrewery added inline comments.
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

This revision now requires changes to proceed.Sep 6 2018, 12:07 AM
cem added a comment.Sep 6 2018, 4:35 PM

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 added inline comments.Sep 6 2018, 7:44 PM
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 added a comment.Sep 6 2018, 8:38 PM

cem (Conrad Meyer) <phabric-noreply@freebsd.org> 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 added inline comments.Sep 6 2018, 8:49 PM
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 added inline comments.Sep 6 2018, 9:01 PM
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 updated this revision to Diff 47779.Sep 6 2018, 9:14 PM

size_t for 2nd arg to octets2hex

sjg marked 3 inline comments as done.Sep 6 2018, 9:15 PM
sjg updated this revision to Diff 47781.Sep 6 2018, 11:02 PM

remove check for MK_BEARSSL from Makefile

sjg marked an inline comment as done.Sep 6 2018, 11:04 PM
cem added a comment.Sep 8 2018, 4:52 AM
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 added inline comments.Sep 10 2018, 8:20 PM
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 added inline comments.Sep 11 2018, 12:27 AM
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 added inline comments.Sep 11 2018, 7:56 PM
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 updated this revision to Diff 47928.Sep 11 2018, 8:48 PM

define OPENPGP_TAG_*

sjg added a comment.Dec 25 2018, 9:47 PM

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 updated this revision to Diff 52320.Dec 25 2018, 9:52 PM

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

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 added a comment.Jan 8 2019, 11:18 PM

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 added a comment.Jan 8 2019, 11:55 PM

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 updated this revision to Diff 52850.Jan 15 2019, 6:16 AM

Rename libve to libsecureboot

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.

lib/libsecureboot/vets.c
426 ↗(On Diff #52850)

Looks like unused variable.

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 added inline comments.Jan 16 2019, 11:30 PM
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 updated this revision to Diff 52904.Jan 16 2019, 11:49 PM

Rename libve to libsecureboot

sjg updated this revision to Diff 53809.Feb 11 2019, 10:47 PM

Trim trailing ../ from prefix

sjg updated this revision to Diff 53852.Feb 12 2019, 9:23 PM

Only pass prefix to load_manifest if skip!=NULL

This revision was not accepted when it landed; it landed in state Needs Review.Feb 26 2019, 6:09 AM
Closed by commit rS344565: Add libsecureboot (authored by sjg). · Explain Why
This revision was automatically updated to reflect the committed changes.
sjg mentioned this in rS344565: Add libsecureboot.