Date: Thu, 5 Jul 2018 21:16:30 -0700 From: "Simon J. Gerraty" <sjg@juniper.net> To: <D16155+284+e8dc059c9d8797dc@web1.nyi.freebsd.org> Cc: <sjg@juniper.net>, <freebsd-arch@freebsd.org> Subject: Re: [Differential] D16155: Add veriexec to loader Message-ID: <93705.1530850590@kaos.jnpr.net> In-Reply-To: <84d9b7dd268a8cb64b51e4c49753bed8@localhost.localdomain> References: <differential-rev-PHID-DREV-jfitweed3urwpaigoztb-req@FreeBSD.org> <84d9b7dd268a8cb64b51e4c49753bed8@localhost.localdomain>
next in thread | previous in thread | raw e-mail | index | archive | help
+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 =E2=80=94 a notoriously difficult schem=
e to parse correctly =E2=80=94 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.=20
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?=20
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:
>=20=20=20
> - 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 */
>=20
> Both of these concepts seem pretty dubious.
>=20
> The loader and kernel should not be guessing signing policy =E2=80=94 per=
iod.
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;
>=20
> indent style in this file seems wrong
Will go check - might have missed during recent style9 update.
=20
> > vectx.c:124
> > + if (strncmp(cp, "sha256=3D", 7) =3D=3D 0) {
> > + ctx->vec_md =3D &br_sha256_vtable;
> > + hashsz =3D br_sha256_SIZE;
>=20
> OCF (or openssl =E2=80=94 this appears to be userspace) supports all of t=
hese
> 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 >=3D fip->fi_prefix_len) {
>=20
> This is not going to be pretty with any significant number of verified fi=
les.
The loader does not deal with significant numbers of files.
Further, it only deals with each file once.
=20
> > veopen.c:136-137
> > +{
> > + char pbuf[MAXPATHLEN+1];
> > + char nbuf[MAXPATHLEN+1];
> > + struct stat st;
>=20
> This is a lot of stack =E2=80=94 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 =3D 2*hlen;
> > + if ((rc =3D strncmp(hex, want, n))) {
> > + ve_error_set("%s: %.*s !=3D %.*s", path, n, hex, n, want);
>=20
> 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
>=20
> 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") =3D=3D 0)
> > + return VE_TRY;
> > + }
>=20
> 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) =3D=3D 0) {
> > + cp +=3D 10;
>=20
> 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
>=20
> 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;
> > +}
>=20
> Ew. Is this loader-only code?
Mostly, but not necessarily.
>=20
> > vets.c:381
> > + if (!vrfy(sdata, slen, hash_oid, mlen, pkey, vhbuf) ||
> > + memcmp(vhbuf, mdata, mlen) !=3D 0) {
> > + return 0; /* fail */
>=20
> should this be timingsafe_memcmp?
I cannot think of a reason why...
> > vets.c:497-500
> > + cn_oid[0] =3D 3;
> > + cn_oid[1] =3D 0x55;
> > + cn_oid[2] =3D 4;
> > + cn_oid[3] =3D 3;
>=20
> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?93705.1530850590>