Re: [Differential] D16155: Add veriexec to loader

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: [Differential] D16155: Add veriexec to loader

Simon J. Gerraty
+freebsd-arch since I refuse to top-post via phab, and this all warrants
a discussion anyway...

Most style(9) comments will be dealt with - no discussion here.

>   - Why are we using PGP in any new cryptosystem?

Because lots of people in the real world want to be able to use it?
Like everything else, it is optional.

>   - Why are we using ASN.1 / x509 — a notoriously difficult scheme to parse correctly — in the kernel or loader?

Because it provides a high level of flexibility.
Ie. I can load a brand new version of Junos on a 10+ year old version,
and it will be able to verify all the signatures.
Unlike some other designs that would force me to install intermediate
versions in sequence.

>   - Why the plethora of duplicated functionality (e.g., above)?
>    (Also, the five different SHA hashes.  Pick one that's good enough

For the above requirement I need to ensure that the s/w I shipped 10+
years ago, knows how to handle hashes that were not required then.

Also there is no need to enable any hashes that you do not want.

>    and just use it everywhere.  SHA512 provides no meaningful benefit
>    over SHA256, but it also isn't clear that a basic hash is the
>    right construct anyway.  There is no reason to use SHA384 at all.)

Yes, there is - if/when SHA256 is deprecated.
Hopefully SHA512/256 will be approved before then.

>   - What alternatives to x509 or PGP were considered and dismissed?
>    Please make it clear that you've done your research and examined
>    other options.

I gave a talk about this at BSDCan in June.
At the end of the day, you can propose and implement an alternate design
and see if anyone wants it.

>   - Why RSA or ECDSA (both have easy ways to foot-shoot) in new
>     cryptosystems, vs something like Ed25519?

Because as a vendor who sells to US Govt I'm limited to algorithms
approved for that purpose.
You can of course add anything you like.

>   - SHA1 should not be used at all.

It is optional for that reason and not enabled by default.
However we are using it and will continue to until NIST ban it.

>   Meta issues:
>  
>   - Use of strcmp() for signature comparison.  You want timingsafe_memcmp().

In the loader?  It is a single threaded app.

>   - Have you thought about wiping key memory before releasing it?  I
>   don't see any invocations of explicit_bzero() (or bzero/memset,
>   for that matter).

There are no private keys involved here at all.
Public keys do not need any such treatment - they are not sensitive
data.

> > +#define VE_GUESS        -1           /* let verify_file work it out */
> > +#define VE_TRY          0            /* we don't mind if unverified */
> > +#define VE_WANT         1            /* we want this verified */
>
> Both of these concepts seem pretty dubious.
>
> The loader and kernel should not be guessing signing policy — period.

If you can propose a means whereby every bit of lua and .4th can
communicate to the loader the verification requirements...

> Files either have a signature or do not.  If they do, they must be
> verified.  If they don't, they cannot be verified.  Not sure what try
> or want has to do with that.

Please go watch my talk from BSDCan.
This is all covered.
Yes, anything which has a hash must verify correctly.

The above all only apply when no hash is found.
Whether that is acceptible depends on the caller - never acceptible for
modules, but may be ok for loader.conf and other files which may need to
be mutable.


> > tvo.c:36
> > +{
> > +    int n;
> > +    int fd;
>
> indent style in this file seems wrong

Will go check - might have missed during recent style9 update.
 
> > vectx.c:124
> > + if (strncmp(cp, "sha256=", 7) == 0) {
> > + ctx->vec_md = &br_sha256_vtable;
> > + hashsz = br_sha256_SIZE;
>
> OCF (or openssl — this appears to be userspace) supports all of these
> hashes.  Why are we using bearssl for this?

No, this is not userland, this api is specificially for the loader and
specific to its loading of modules and kernel.
It is not currently used - that will require a significant rototill of
load_elf.c

OpenSSL is at least an order of magnitude too big to be used in the loader.
BearSSL allows all this to be done in ~100K

All covered in my BSDCan talk

> > veopen.c:117
> > + }
> > + LIST_FOREACH(fip, &fi_list, entries) {
> > + if (nfip->fi_prefix_len >= fip->fi_prefix_len) {
>
> This is not going to be pretty with any significant number of verified files.

The loader does not deal with significant numbers of files.
Further, it only deals with each file once.
 
> > veopen.c:136-137
> > +{
> > + char pbuf[MAXPATHLEN+1];
> > + char nbuf[MAXPATHLEN+1];
> > + struct stat st;
>
> This is a lot of stack — is this file userspace-only?

Yes it is (quite a lot), and no it is loader only.
It has not proven to be an issue, even when using old boot2 to boot
stable/6

> > veopen.c:302
> > + n = 2*hlen;
> > + if ((rc = strncmp(hex, want, n))) {
> > + ve_error_set("%s: %.*s != %.*s", path, n, hex, n, want);
>
> Why are we comparing a printed hash at all?

Because that's what's captured in the manifest.

> > veopen.c:404-412
> > + * @brief
> > + * open a file if it can be verified
> > + *
> > + * @param[in] path
> > + * pathname to open
> > + *
> > + * @param[in] flags
>
> None of the previous portion of this comment adds anything of value
> for the reader beyond the information already present in the function
> name and parameter types and names below.

The comment is for extraction to api documentation (doxygen)

> > verify.c:270
> > + strcmp(cp, ".hints") == 0)
> > + return VE_TRY;
> > + }
>
> What does "try" mean in this context?

It means we don't really expect this file to have a hash.
So even in strict mode (eg for FIPS 140) we won't get upset if it
doesn't have one.
If it does have one of course, then it must match.

> > verify.c:380
> > + cp++;
> > + if (strncmp(cp, "loader.ve.", 10) == 0) {
> > + cp += 10;
>
> kludge?

Yes and no.
We are taking advantage of the fact that the pathname is verified as
well as its content, so we can use the pathname to communicate tuning
info to loader eg "strict" mode for FIPS-140, even "off" for folk that
don't care and want to speed up boot time.

Again; covered in my BSDCan talk.

> > vets.c:208
> > + /* This is deprecated! do not enable unless you absoultely have to */
> > + br_x509_minimal_set_hash(&mc, br_sha1_ID, &br_sha1_vtable);
> > +#endif
>
> Just remove it.  There's no reason to be using SHA1 in novel
> cryptographic designs in 2018.  If you use it in JunOS, keep the SHA1
> stuff in JunOS, but I'd suggest moving away from SHA1 there, too.

This is not a novel design - it's 15+ years old,
but in this case perhaps, was useful for testing.

> > vets.c:294
> > +
> > + return hex;
> > +}
>
> Ew.  Is this loader-only code?

Mostly, but not necessarily.

>
> > vets.c:381
> > + if (!vrfy(sdata, slen, hash_oid, mlen, pkey, vhbuf) ||
> > +    memcmp(vhbuf, mdata, mlen) != 0) {
> > + return 0; /* fail */
>
> should this be timingsafe_memcmp?

I cannot think of a reason why...

> > vets.c:497-500
> > + cn_oid[0] = 3;
> > + cn_oid[1] = 0x55;
> > + cn_oid[2] = 4;
> > + cn_oid[3] = 3;
>
> This is pretty magical

Yes.  BearSSL is a very low level library.

The comment above it, indicates that this is the DER encoded OID for the
commonName field.

--sjg
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: [Differential] D16155: Add veriexec to loader

Conrad Meyer-2
Hi Simon,

On Thu, Jul 5, 2018 at 9:16 PM, Simon J. Gerraty <[hidden email]> wrote:
> +freebsd-arch since I refuse to top-post via phab, and this all warrants
> a discussion anyway...

Please follow-up in Phabricator, or there is little point in using it.
(I don't know where the "top-post" characterization comes from —
phabricator presents conversations top-to-bottom, in the same fashion
as bottom posting.)

Without getting into point-by-point specifics, I'll address a couple
(meta-)issues of that come up multiple times in the conversation:

1. It's unclear in what context files are used (loader, userspace,
and/or kernel).  Some files in directories are built in multiple
contexts, but not others, and the contexts aren't clear from the
pathnames.  That lead(s) to some confusion.  For crypto review you
really want clarity.  It is almost certainly better to break this into
several pieces.  I.e., the mechanical build system changes to import
bearssl can be separated out; you could maybe add loader-only
verification code next, then bring in the kernel pieces, then
userspace (as separate reviews).  You know this work better than I do;
how you choose to split it is up to you.  But I would encourage
smaller pieces.

2. A lot of the responses to my questions or comments are "JunOS does
(or has done) it this way."  Those are great rationales for Juniper
continuing to use the existing design in its commercial product!  But
this isn't JunOS, and booting JunOS is useless to FreeBSD.  If all you
want to do with the changes is boot JunOS, I don't see any reason to
include it in FreeBSD.  If your concern is that the implementations
will diverge slightly, well, they will.  That's sort of the nature of
being a downstream commercial product of FreeBSD.  For anything
removed in FreeBSD (i.e., obsolete SHA1 support, or even RSA/ECDSA
signatures) that you need to retain in JunOS, you can still include
that as a small local patch in JunOS.  We do not want crufty 2003
crypto in FreeBSD.

3. It is an unreasonable response to question or critique to refer
reviewers to a 60 minute video of a talk.  If you addressed that
specific question or concern in your talk, and want to provide *a
specific timestamp and duration* in the video stream, great.  I'm
happy to watch a short, specific clip, if that is your preferred media
for representing a few sentences.  But I'm not going to sit down and
watch a 60 minute talk just to dig for the response to a specific
concern, which may or may not even be addressed.

Thanks,
Conrad
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: [Differential] D16155: Add veriexec to loader

Simon J. Gerraty
> 1. It's unclear in what context files are used (loader, userspace,
> and/or kernel).  Some files in directories are built in multiple
> contexts, but not others, and the contexts aren't clear from the
> pathnames.  That lead(s) to some confusion.  For crypto review you

Originally all this was only for the loader.
But then the need for a veriexec userland tool that would verify
manifests before feeding the kernel was brought up.
A subset of libve is needed for that.

The Makefile.libsa.inc in both libbearssl and libve show what get's used
by libsa - for loader.

Of libve only vets.c (trust store) and the openpgp/ code (optionally)
is needed for userland.

> really want clarity.  It is almost certainly better to break this into
> several pieces.  I.e., the mechanical build system changes to import
> bearssl can be separated out; you could maybe add loader-only
> verification code next, then bring in the kernel pieces, then
> userspace (as separate reviews).  You know this work better than I do;
> how you choose to split it is up to you.  But I would encourage
> smaller pieces.

Yes, the initial review was bigger than I'd expected - beyond the point
at which a gui is helpful.

I'm open to alternate arrangements - the current diff is a minimal
re-org to fit into the new stand/ environment and present the work so
others can provide feedback.

> 2. A lot of the responses to my questions or comments are "JunOS does
> (or has done) it this way."  Those are great rationales for Juniper
> continuing to use the existing design in its commercial product!  But
> this isn't JunOS, and booting JunOS is useless to FreeBSD.  If all you

Perhaps I've not made myself clear.

Junos is a FreeBSD based OS, it's booting requirements are in some
respects more complicated than a typical FreeBSD install - so it serves
as a useful example.

I shoud also point out that we always provide the kernel with an
md_image for its initial rootfs - and that md_image is verified by the
loader - obviating the need for any of this stuff in the kernel itself.
Everything needed to get mac_veriexec initialized and enforced is in
that md_image.

If that's not done, then someone would need to consider adding code to
kernel to verify init, and the rc scripts etc etc.

> want to do with the changes is boot JunOS, I don't see any reason to
> include it in FreeBSD.  If your concern is that the implementations

No, we could skip upstreaming this completely - but other vendors who
also use FreeBSD have expressed interest.

> will diverge slightly, well, they will.  That's sort of the nature of

That doesn't concern me at all.

> being a downstream commercial product of FreeBSD.  For anything
> removed in FreeBSD (i.e., obsolete SHA1 support, or even RSA/ECDSA

Sorry, if you want to support signature other methods you are welcome to
add them.  Many of those vendors interested in this work face the same
limitations we do - needing to use US Govt approved algorithms.

Perhaps you could enumerate some of the alternatives you'd support.
You've veto'd pretty much everything here, so what do you think the
modern world needs?

Eg. X.509 is horrible - everyone agrees, but what is the alternative
that offers the same flexibility?
RSA and ECDSA are old fasioned?
What are the proposed alternatives? and what libraries implement them
that are small enough to incorporate into the loader?

This project has been on my todo list for a decade, but was not viable
until BearSSL showed up last year.
OpenSSL was simply too big - the loader stops working somewhere around 500K
(based on my experiments yesterday) and the OpenSSL code required is 3M+

--sjg
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-arch
To unsubscribe, send any mail to "[hidden email]"