Simon J. Gerraty
2018-07-06 04:16:30 UTC
+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.
Like everything else, it is optional.
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.
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.
Hopefully SHA512/256 will be approved before then.
At the end of the day, you can propose and implement an alternate design
and see if anyone wants it.
approved for that purpose.
You can of course add anything you like.
However we are using it and will continue to until NIST ban it.
Public keys do not need any such treatment - they are not sensitive
data.
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...
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.
Will go check - might have missed during recent style9 update.
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
The loader does not deal with significant numbers of files.
Further, it only deals with each file once.
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
Because that's what's captured in the manifest.
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)
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.
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.
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.
Mostly, but not necessarily.
I cannot think of a reason why...
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
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+(Also, the five different SHA hashes. Pick one that's good enough
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.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.)
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.Please make it clear that you've done your research and examined
other options.
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 algorithmscryptosystems, vs something like Ed25519?
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.
- 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.don't see any invocations of explicit_bzero() (or bzero/memset,
for that matter).
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.+#define VE_TRY 0 /* we don't mind if unverified */
+#define VE_WANT 1 /* we want this verified */
The loader and kernel should not be guessing signing policy — period.
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.verified. If they don't, they cannot be verified. Not sure what try
or want has to do with that.
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+{
+ int n;
+ int fd;
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+ if (strncmp(cp, "sha256=", 7) == 0) {
+ ctx->vec_md = &br_sha256_vtable;
+ hashsz = br_sha256_SIZE;
hashes. Why are we using bearssl for this?
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.+ }
+ LIST_FOREACH(fip, &fi_list, entries) {
+ if (nfip->fi_prefix_len >= fip->fi_prefix_len) {
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?+{
+ char pbuf[MAXPATHLEN+1];
+ char nbuf[MAXPATHLEN+1];
+ struct stat st;
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?+ n = 2*hlen;
+ if ((rc = strncmp(hex, want, n))) {
+ ve_error_set("%s: %.*s != %.*s", path, n, hex, n, want);
veopen.c:404-412
+ * open a file if it can be verified
+ *
+ * pathname to open
+ *
None of the previous portion of this comment adds anything of value+ * open a file if it can be verified
+ *
+ * pathname to open
+ *
for the reader beyond the information already present in the function
name and parameter types and names below.
verify.c:270
+ strcmp(cp, ".hints") == 0)
+ return VE_TRY;
+ }
What does "try" mean in this context?+ strcmp(cp, ".hints") == 0)
+ return VE_TRY;
+ }
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?+ cp++;
+ if (strncmp(cp, "loader.ve.", 10) == 0) {
+ cp += 10;
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+ /* This is deprecated! do not enable unless you absoultely have to */
+ br_x509_minimal_set_hash(&mc, br_sha1_ID, &br_sha1_vtable);
+#endif
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.
but in this case perhaps, was useful for testing.
vets.c:294
+
+ return hex;
+}
Ew. Is this loader-only code?+
+ return hex;
+}
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?+ if (!vrfy(sdata, slen, hash_oid, mlen, pkey, vhbuf) ||
+ memcmp(vhbuf, mdata, mlen) != 0) {
+ return 0; /* fail */
vets.c:497-500
+ cn_oid[0] = 3;
+ cn_oid[1] = 0x55;
+ cn_oid[2] = 4;
+ cn_oid[3] = 3;
This is pretty magical+ cn_oid[0] = 3;
+ cn_oid[1] = 0x55;
+ cn_oid[2] = 4;
+ cn_oid[3] = 3;
The comment above it, indicates that this is the DER encoded OID for the
commonName field.
--sjg