807890 - Add support for Microsoft Trust List Signing EKU

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Elio Maldonado]

NSS lacks support for Secure Boot in the libraries and tools. This problem was identified and brought to our attention by Peter Jones, one of the developers working on Secure Boot support for fedora. In his message Peter states that "One of the things needed for SB is certificates generated with a specific extended key usage" field that currently isn't supported by certutil."

The Fedora Secure Boot project page is https://fedoraproject.org/wiki/Secureboot
You also find the blog at http://mjg59.dreamwidth.org/12368.html worth reading.

Under guidance from Bob Relyea I have worked on modifications of Peter Jones original submission to adapt them for nss-3.14 and is done in a way where we don't break API. I will attach some patches as soon they get approval in fedora.

Comment 1

[Elio Maldonado]

Replace API with ABI in the previous statement.

Comment 2

[Elio Maldonado]

Attachment: secure boot support - ms extended key usage oid for code signing

Comment 3

[Robert Relyea]

Comment on attachment 681254 [details] [diff] [review]
secure boot support - ms extended key usage oid for code signing

r-

Get rid of the NO_SECURE_BOOT_SUPPORT_IN_NSS_UTIL define...
What was done in fedora is irrelevant for upstream here. 

szOID_KP_CTL_USAGE_SIGNING should be called SECOID_KP_CTL_USAGE_SIGNING

Don't add the oid as dynamic in moreoids.c

What was done in fedora is irrelevant for upstream here. When you update NSS you can simply drop the fedora patches.

Comment 4

[Elio Maldonado]

Attachment: Adds secure boot support: ms extended key usage oid for code signing

Comment 5

[Elio Maldonado]

Attachment: Adds secure boot support: ms extended key usage oid for code signing

Comment 6

[Robert Relyea]

Comment on attachment 683327 [details] [diff] [review]
Adds secure boot support: ms extended key usage oid for code signing

r+ rrelyea

Comment 7

[Wan-Teh Chang]

Comment on attachment 683327 [details] [diff] [review]
Adds secure boot support: ms extended key usage oid for code signing

>+    OD( msExtendedKeyUsageCodeSigning, 
>+        SEC_OID_KP_CTL_USAGE_SIGNING,
>+        "Microsoft Authenticode Code Signing",
>+	CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION )

Please come up with a better name than EC_OID_KP_CTL_USAGE_SIGNING.
The "KP_CTL" part makes no sense to me, and the "SIGNING" part
does not clarify it is code signing, as opposed to, for example,
cert signing, CRL signing, OCSP signing, etc.

Comment 8

[Ryan Sleevi]

Wan-Teh: This matches Microsoft's naming for the variable, which I think is compelling, given that the OID variable names tend to follow their vendor-assigned names.

See http://support.microsoft.com/kb/287547 for the list of Microsoft OIDs.

That said, I do think the OID should have a "Microsoft" name in it, much like other variables have NIST/PKIX/ns/ansi/secg/ev/etc

SEC_OID_MS_KP_CTL_USAGE_SIGNING perhaps.

Comment 9

[Elio Maldonado]

The original https://bugzilla.redhat.com/attachment.cgi?id=634839 had this
+    /* Microsoft Authenticode OIDs */
+    /* using the terrible microsoft name here... */    <-------------
+    szOID_KP_CTL_USAGE_SIGNING              = 314,

Perhaps I should have left Peter's editorial remarks :-)

Comment 10

[Kaspar]

(In reply to Wan-Teh Chang from comment #7)
> Please come up with a better name than EC_OID_KP_CTL_USAGE_SIGNING.
> The "KP_CTL" part makes no sense to me, and the "SIGNING" part

It's "Key Purpose Certificate Trust List", I think. AFAICT the patch mixes up two different OID arcs:

1.3.6.1.4.1.311.10   Crypto 2.0
1.3.6.1.4.1.311.2    Authenticode

The patch obviously wants to add a key purpose from the former arc, but the text description and the comment refer to "Authenticode" (which is obsolete stuff).

In the Windows GUI, the 1.3.6.1.4.1.311.10.3.1 EKU is shown as "Microsoft Trust List Signing".

Comment 11

[Wan-Teh Chang]

Thanks for all the comments. I also looked into this before reading your
comments. I think the new OID tag name must contain _MS_, as Ryan suggested.

If this OID is only used in the Microsoft Windows "ecosystem", I also think
it can be helpful for the OID tag name to be similar to szOID_KP_CTL_USAGE_SIGNING.
The "KP" part that I couldn't figure out can be explained with a comment like
"KP stands for Key Purpose".

NSS already has several OID tags named SEC_OID_EXT_KEY_USAGE_xxx:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/util/secoidt.h&rev=1.36&mark=204-208#204

so SEC_OID_MS_EXT_KEY_USAGE_CTL_SIGNING would also be a good OID tag.

Comment 12

[Kaspar]

Changing the summary to more accurately reflect what the bug is really about.

Comment 13

[Elio Maldonado]

Attachment: Support Microsoft Trust List Signing EKU

Changed the OID naming and added comments as follows:

$ interdiff ~/Downloads/secureboot.patch MicrosoftTrustListSigningEKU.patch
diff -u ./mozilla/security/nss/lib/util/secoidt.h ./mozilla/security/nss/lib/util/secoidt.h
--- ./mozilla/security/nss/lib/util/secoidt.h	19 Nov 2012 23:07:13 -0000
+++ ./mozilla/security/nss/lib/util/secoidt.h	18 Dec 2012 18:51:50 -0000
@@ -436,8 +436,11 @@
     SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA224_DIGEST     = 314,
     SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST     = 315,
 
-    /* Microsoft Authenticode oid */
-    SEC_OID_KP_CTL_USAGE_SIGNING    = 316,
+    /* Microsoft Trust List Signing
+     * szOID_KP_CTL_USAGE_SIGNING
+     * where KP stands for Key Purpose
+     */
+    SEC_OID_MS_EXT_KEY_USAGE_CTL_SIGNING    = 316,
 
     SEC_OID_TOTAL
 } SECOidTag;

Comment 14

[Wan-Teh Chang]

Comment on attachment 693511 [details] [diff] [review]
Support Microsoft Trust List Signing EKU

Review of attachment 693511 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. I only reviewed lib/util/{secoidt.h,secoid.c}. I assume
Bob already reviewed the rest of the patch.

::: ./mozilla/security/nss/lib/util/secoid.c
@@ +1644,5 @@
>  	"DSA with SHA-256 Signature",
> +	CKM_INVALID_MECHANISM /* not yet defined */, INVALID_CERT_EXTENSION),
> +    OD( msExtendedKeyUsageCodeSigning, 
> +        SEC_OID_KP_CTL_USAGE_SIGNING,
> +        "Microsoft Authenticode Code Signing",

Should we also change this string to "Microsoft Trust List Signing"?

Comment 15

[Elio Maldonado]

(In reply to Wan-Teh Chang from comment #14)
> 
> r=wtc. I only reviewed lib/util/{secoidt.h,secoid.c}. I assume
> Bob already reviewed the rest of the patch.

That's correct, Bob did and he gave it an r+.
> 
> ::: ./mozilla/security/nss/lib/util/secoid.c
> @@ +1644,5 @@
> >  	"DSA with SHA-256 Signature",
> > +	CKM_INVALID_MECHANISM /* not yet defined */, INVALID_CERT_EXTENSION),
> > +    OD( msExtendedKeyUsageCodeSigning, 
> > +        SEC_OID_KP_CTL_USAGE_SIGNING,
> > +        "Microsoft Authenticode Code Signing",
> 
> Should we also change this string to "Microsoft Trust List Signing"?

Yes, we should. I'll make this change and check it in.

Comment 16

[Elio Maldonado]

Find two other files that needed changes.

$ interdiff ~/Downloads/secureboot.patch MicrosoftTrustListSigningEKU.patch 
diff -u ./mozilla/security/nss/cmd/certutil/certext.c ./mozilla/security/nss/cmd/certutil/certext.c
--- ./mozilla/security/nss/cmd/certutil/certext.c	19 Nov 2012 23:07:12 -0000
+++ ./mozilla/security/nss/cmd/certutil/certext.c	19 Dec 2012 02:00:17 -0000
@@ -556,7 +556,7 @@
             rv = AddOidToSequence(os, SEC_OID_NS_KEY_USAGE_GOVT_APPROVED);
             break;
         case 7:
-            rv = AddOidToSequence(os, SEC_OID_KP_CTL_USAGE_SIGNING);
+            rv = AddOidToSequence(os, SEC_OID_MS_EXT_KEY_USAGE_CTL_SIGNING);
             break;
         default:
             goto endloop;
diff -u ./mozilla/security/nss/lib/util/secoid.c ./mozilla/security/nss/lib/util/secoid.c
--- ./mozilla/security/nss/lib/util/secoid.c	19 Nov 2012 23:07:13 -0000
+++ ./mozilla/security/nss/lib/util/secoid.c	19 Dec 2012 02:00:17 -0000
@@ -1644,8 +1644,8 @@
 	"DSA with SHA-256 Signature",
 	CKM_INVALID_MECHANISM /* not yet defined */, INVALID_CERT_EXTENSION),
     OD( msExtendedKeyUsageCodeSigning, 
-        SEC_OID_KP_CTL_USAGE_SIGNING,
-        "Microsoft Authenticode Code Signing",
+        SEC_OID_MS_EXT_KEY_USAGE_CTL_SIGNING,
+        "Microsoft Trust List Signing",
 	CKM_INVALID_MECHANISM, INVALID_CERT_EXTENSION )
 };
 
diff -u ./mozilla/security/nss/lib/util/secoidt.h ./mozilla/security/nss/lib/util/secoidt.h
--- ./mozilla/security/nss/lib/util/secoidt.h	19 Nov 2012 23:07:13 -0000
+++ ./mozilla/security/nss/lib/util/secoidt.h	19 Dec 2012 02:00:17 -0000
@@ -436,8 +436,11 @@
     SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA224_DIGEST     = 314,
     SEC_OID_NIST_DSA_SIGNATURE_WITH_SHA256_DIGEST     = 315,
 
-    /* Microsoft Authenticode oid */
-    SEC_OID_KP_CTL_USAGE_SIGNING    = 316,
+    /* Microsoft Trust List Signing
+     * szOID_KP_CTL_USAGE_SIGNING 
+     * where KP stands for Key Purpose
+     */
+    SEC_OID_MS_EXT_KEY_USAGE_CTL_SIGNING    = 316,
 
     SEC_OID_TOTAL
 } SECOidTag;

Comment 17

[Elio Maldonado]

Actually one file, certext.c, and an extra line on secoid.c.

Comment 18

[Elio Maldonado]

Changes committed to the trunk for 3.14.2:
Checking in mozilla/security/nss/cmd/certcgi/ca_form.html;
/cvsroot/mozilla/security/nss/cmd/certcgi/ca_form.html,v  <--  ca_form.html
new revision: 1.5; previous revision: 1.4
done
Checking in mozilla/security/nss/cmd/certcgi/certcgi.c;
/cvsroot/mozilla/security/nss/cmd/certcgi/certcgi.c,v  <--  certcgi.c
new revision: 1.23; previous revision: 1.22
done
Checking in mozilla/security/nss/cmd/certcgi/stnd_ext_form.html;
/cvsroot/mozilla/security/nss/cmd/certcgi/stnd_ext_form.html,v  <--  stnd_ext_form.html
new revision: 1.5; previous revision: 1.4
done
Checking in mozilla/security/nss/cmd/certutil/certext.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certext.c,v  <--  certext.c
new revision: 1.13; previous revision: 1.12
done
Checking in mozilla/security/nss/cmd/certutil/certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.163; previous revision: 1.162
done
Checking in mozilla/security/nss/cmd/lib/moreoids.c;
/cvsroot/mozilla/security/nss/cmd/lib/moreoids.c,v  <--  moreoids.c
new revision: 1.4; previous revision: 1.3
done
Checking in mozilla/security/nss/cmd/lib/secutil.h;
/cvsroot/mozilla/security/nss/cmd/lib/secutil.h,v  <--  secutil.h
new revision: 1.51; previous revision: 1.50
done
Checking in mozilla/security/nss/lib/util/secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.68; previous revision: 1.67
done
Checking in mozilla/security/nss/lib/util/secoidt.h;
/cvsroot/mozilla/security/nss/lib/util/secoidt.h,v  <--  secoidt.h
new revision: 1.37; previous revision: 1.36

Comment 19

[Elio Maldonado]

Attachment: The patch as it was applied

Comment 20

[Kaspar]

Attachment: Use consistent terms, and fix certutil usage etc.

The patch from attachment 693708 [details] [diff] [review] is now using confusing terms, IMO. Code signing and trust list signing are different things (and a CTL isn't really code in the classic sense, cf. http://msdn.microsoft.com/en-us/library/windows/desktop/aa376545%28v=vs.85%29.aspx).

I suggest to apply this additional patch - note that in addition to the naming fixes, it also addresses a bug in certcgi.c and shortcomings in certext.c and certutil.c.

Comment 21

[Robert Relyea]

Comment on attachment 693907 [details] [diff] [review]
Use consistent terms, and fix certutil usage etc.

r+ rrelyea

Comment 22

[Elio Maldonado]

Additional changes committed to the truck for 3.12.4:
Checking in ca_form.html;
/cvsroot/mozilla/security/nss/cmd/certcgi/ca_form.html,v  <--  ca_form.html
new revision: 1.6; previous revision: 1.5
done
Checking in certcgi.c;
/cvsroot/mozilla/security/nss/cmd/certcgi/certcgi.c,v  <--  certcgi.c
new revision: 1.24; previous revision: 1.23
done
Checking in stnd_ext_form.html;
/cvsroot/mozilla/security/nss/cmd/certcgi/stnd_ext_form.html,v  <--  stnd_ext_form.html
new revision: 1.6; previous revision: 1.5
done
Checking in certext.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certext.c,v  <--  certext.c
new revision: 1.14; previous revision: 1.13
done
Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.164; previous revision: 1.163
done
Checking in secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.69; previous revision: 1.68
done

Comment 23

[Wan-Teh Chang]

Attachment: Don't need to include both secoid.h and secoidt.h

Many NSS headers follow the convention that xxx.h includes xxxt.h
and xxxt.h defines the types while xxx.h declares the functions.
So it is not necessary to include both secoid.h and secoidt.h.

Patch checked in on the NSS trunk (NSS 3.14.2).

Checking in secoid.c;
/cvsroot/mozilla/security/nss/lib/util/secoid.c,v  <--  secoid.c
new revision: 1.70; previous revision: 1.69
done