982292 - SEC_ERROR_INADEQUATE_CERT_TYPE due to intermediate CA certificates with SGC EKUs but without "TLS Web Server Authentication" EKU when mozilla::pkix is enabled

Status: VERIFIED FIXED

(Core :: Security: PSM, defect)

Description

[Matt Wobensmith [:mwobensmith][:matt:]]

Requires turning on insanity::pkix via pref security.use_insanity_verification = true

In Fx28 and Fx30 default - as well as Chrome 33 - site loads.

In Fx30 with insanity::pkix enabled, we receive SEC_ERROR_INADEQUATE_CERT_TYPE instead and site does not load.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In insanity::pkix, SEC_ERROR_INADEQUATE_CERT_TYPE means that there is a problem with the Extended Key Usage (EKU) field.

In this case, the problem is that the intermediate CA certificate has an EKU, but the EKU extension, but it doesn't list the "TLS Web Server Authentication (1.3.6.1.5.5.7.3.1)" EKU. Instead, the intermediate CA certificate lists only:
Microsoft Server Gated Crypto (1.3.6.1.4.1.311.10.3.3)
Netscape Server Gated Crypto (2.16.840.1.113730.4.1)

In insanity::pkix, we have implemented a version of Microsoft's EKU-in-intermediates enforcement. In NSS (classic and libpkix), EKU in intermediates is ignored. This is why we have a regression here.

There are multiple possible solutions:

1. COMODO can supply us with a new version of the intermediate that has the same subject name and the same public key, but with a different serial number, with the "TLS Web Server Authentication (1.3.6.1.5.5.7.3.1)" EKU. Then, we would embed this certificate into NSS without any trust records. This way, we would reject the intermediate sent by the web server and use this new good CA certificate.

2. We could (temporarily) turn off EKU-in-intermediates enforcement in insanity::pkix.

3. For the purposes of the EKU-in-intermediates enforcement, we could treat the SGC EKUs are being equivalent to "TLS Web Server Authentication." I think this is a bad option because SGC stuff is obsolete and also matching non-equal EKUs would is against the standard.

4. In CertVerifier::InsanityVerifyCert, in the case of certificateUsageSSLServer, if the non-EV validation fails with SEC_ERROR_INADEQUATE_CERT_TYPE then we could revalidate for the "Netscape Server Gated Crypto" EKU. I consider this to be the worst option; we should avoid implementing obsolete SGC stuff if we can at all avoid it. This would make certificate verification slower than the other options for the affected sites.

Note that we've had a goal for a while (see bug 725351) of enabling the EKU-in-intermediates enforcement as part of our goal to help CAs use technical constraints as a way of reducing risks of externally-operated sub-CAs. For example, if we enforce EKU-in-intermediates then an externally-operated sub-CA certificate that has an EKU but which omits "TLS Web Server Authentication" could be used for the issuance of client certificates without ever being able to be mis-used (in Mozilla products) for the issuance of web server certificates.

Accordingly, I'd like to try to avoid turning off the enforcement of EKU-in-intermediates. On the other hand, it would reduce risk the most if we turned it off *temporarily* with a follow-up bug to re-enable it later. So, I think we should turn of EKU-in-intermediates enforcement now, and file a follow-up bug to re-enable it. In parallel with that, we should work with the CAs (Comodo in this case) to phase out, distrust, and supply replacements for the intermediates that contain EKUs with SGC EKUs that do not have the "TLS Web Server Authentication" EKU.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This affects:
https://webmail.opec.org (Comodo) and
https://www.ipros.jp (GlobalSign)

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Also https://www.josefstadt.org/ (Comodo EssentialSSL)

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

These have the same issue, all either GlobalSign or Comodo:
https://community.cimaglobal.com 
https://higheredexperts.com 
https://tickets.fanat.ru 
https://www.tischtennis.de 
https://www.xtivate.de

Comment 8

[Rob Stradling]

libpkix faced this same issue a couple of years ago.  See bug 737802.  That bug was resolved by restoring the code that treated the Netscape Step-Up EKU OID as equivalent to the TLS Server Authentication EKU OID.

Brian,
Please, please, please resolve this bug the same way.  i.e. Option 3 in comment 1.

You wrote: "3...I think this is a bad option because SGC stuff is obsolete and also matching non-equal EKUs would is against the standard."

Yes, SGC is long obsolete, but we're a long way from standards-land here.  Using EKU OIDs in Intermediates to constrain which EKUs are acceptable lower down the chain is also against the standard (RFC5280)!

Comment 9

[Gervase Markham [:gerv]]

Solution 1. could cause these certs to stop working if people used NSS with their own trust store, which contained the relevant Comodo roots but not the special intermediate (as they didn't know they needed it).

I think it is more important to have a clear algorithm for deciding what is an SSL server cert, that doesn't include certs we don't want to include and does include certs that we do, than it is to have a position that EKU Server Auth is absolutely required. If Comodo were asking for certs without an EKU to be treated as SSL server, or certs with Any EKU, that would be a different matter - both of those categories include piles of certs which are not intended for SSL server use. But it seems to me to be reasonable to think that certs with one or more of these obsolete OIDs were intended, and known to be intended, for SSL server use.

We also want our definition of what an SSL server cert is to match the CAB Forum's scope statement, by adjusting one, the other, or both.

So I think we should do 3). If we can enumerate the certs concerned, we should consider adding this hack just for those certs.

Gerv

Comment 10

[Kathleen Wilson]

I vote for: 3. For the purposes of the EKU-in-intermediates enforcement, we could treat the SGC EKUs are being equivalent to "TLS Web Server Authentication." 

But I would want to have a bug for tracking the impacted sites and eventually removing this -- after giving impacted CAs time to transition.

Kathleen

Comment 11

[Camilo Viecco (:cviecco)]

Since I am doing the EKU testing I might as well take this one using option 3.

Comment 12

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Rob Stradling from comment #8)
> You wrote: "3...I think this is a bad option because SGC stuff is obsolete
> and also matching non-equal EKUs would is against the standard."
> 
> Yes, SGC is long obsolete, but we're a long way from standards-land here. 
> Using EKU OIDs in Intermediates to constrain which EKUs are acceptable lower
> down the chain is also against the standard (RFC5280)!

Thanks for the pointer to bug 737802. I am fine with doing option 3 for now.

Camilo, please add a comment like the following in your patch:

  // Treat certs with step-up OID as also having SSL server type.
  // COMODO have issued certificates that require this behavior
  // that don't expire until June 2020!

From looking at the certs that are having problems, all of the *end-entity* certs do have the "TLS Web Server Authentication" EKU. So, it seems like we should limit the equality of SGC="TLS Web Server Authentication" to *only* CA certificates. I.e. we shouldn't consider an end-entity certificate with a SGC EKU but without the "TLS Web Server Authentication" EKU as being valid for "TLS Web Server Authentication".

Comment 13

[Rob Stradling]

(In reply to Kathleen Wilson from comment #10)
<snip>
> But I would want to have a bug for tracking the impacted sites and
> eventually removing this -- after giving impacted CAs time to transition.

Kathleen, what sort of timeframe did you have in mind?

In bug 737802 comment 6 I wrote:
"These cross-certs are so widely deployed across the Internet and in browser CA certificate caches that there is no way we can stop large numbers of clients from using them in chain building."

In short, realistically, the only available transition mechanism is to wait for them to expire.

Hence the "COMODO needs this behaviour until June 2020" comment in the fix for bug 737802:
https://bugzilla.mozilla.org/attachment.cgi?id=608587&action=diff

Comment 14

[Kathleen Wilson]

I like Brian's suggestion in Comment #12 to only allow the SGC EKU for issuing certs (not end-entity certs).

Could we take it one step further, and only allow that if the intermediate cert was issued before a certain date? That way CAs would get an error if they mistakenly create a new intermediate cert using the obsolete EKU.

Rob, I don't have a good answer for that. Maybe, if we have reasonable checks in place, it would be OK to let those intermediate certs expire. Still to be determined.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Rob Stradling from comment #13)
> In bug 737802 comment 6 I wrote:
> "These cross-certs are so widely deployed across the Internet and in browser
> CA certificate caches that there is no way we can stop large numbers of
> clients from using them in chain building."
> 
> In short, realistically, the only available transition mechanism is to wait
> for them to expire.

No, there is another option, which I explained in my first comment above:

Issue new certificates with the same issuer name and the same public key, with the EKU fixed, off of your latest/best roots. Give us a copy of the intermediates. We can embed those intermediates into Gecko and/or NSS so that we end up building paths through those updated intermediate certs even when the server gives us the old certs with the unfortunate EKU.

At a minimum, we should ask CAs to stop issuing new certificates from any of these intermediates, if they haven't done so already.

Kathleen, your issuance check seems like a reasonable solution, but I suggest that we consider doing the "embed better intermediates" thing before that. And, I suggest we do what Camilo is planning to do here first.

Comment 16

[Kathleen Wilson]

Here's what I think the plan is:

1) Camilo add SGC="TLS Web Server Authentication" to CA certificates. That fix will close this bug.

2) I will create a separate bug to add a check to make sure that new CA certs with the SGC EKU get an error. We'll want this done soon, but it is not a blocker for insanity::pkix.

3) I will create a bug for me to communicate to CAs the need to migrate off of the SGC EKU, and for CAs to send us the replacement intermediates, so we can embed the new intermediate certs (until there are no more valid certs signed by the old intermediates with the obsolete EKU). We will want to do this soon, but this is not a blocker for insanity::pkix.

OK?

Comment 17

[Rob Stradling]

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #15)
> (In reply to Rob Stradling from comment #13)
<snip>
> > In short, realistically, the only available transition mechanism is to wait
> > for them to expire.
> 
> No, there is another option, which I explained in my first comment above:
> 
> Issue new certificates with the same issuer name and the same public key,
> with the EKU fixed

Where by "fixed" you really mean "used in a different non-standard way"!!

> , off of your latest/best roots. Give us a copy of the
> intermediates. We can embed those intermediates into Gecko and/or NSS

Can you answer Gerv's concern from comment 9 paragraph 1?

Given the number of NSS bugs we've faced over the years re: our use of cross-certification, the thought of having to issue yet more cross-certificates and risk making the situation worse makes me sad.  :-(

> so that we end up building paths through those updated intermediate certs even
> when the server gives us the old certs with the unfortunate EKU.

Can you 100% guarantee that it would work and that there would be no side effects?

Why would this be a better solution than treating Step-Up/SGC and Server Authentication as equivalent (as, IINM, libpkix, the classic NSS code, and MS CryptoAPI all do) ?

> At a minimum, we should ask CAs to stop issuing new certificates from any of
> these intermediates, if they haven't done so already.

Stop issuing certificates directly from these intermediates?
Or stop issuing certificates that chain up to these intermediates?

Neither of these are possible for us.  We're talking about cross-certificates between our currently active root certificates, which have also cross-certified our next generation of root certificates.

> Kathleen, your issuance check seems like a reasonable solution, but I
> suggest that we consider doing the "embed better intermediates" thing before
> that. And, I suggest we do what Camilo is planning to do here first.

The issuance check idea assumes that the notBefore date won't be backdated.  Backdating intermediates is not uncommon.  Backdating cross-certificates is very common.

Comment 18

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Hi, let's discuss Gerv's and Rob's concerns in one of the new bugs that Kathleen is planning to file. Camilo has a plan for resolving this bug that everybody seems to agree with, so let's not distract from that.

Comment 19

[Kathleen Wilson]

(In reply to Kathleen Wilson from comment #16)

> 2) I will create a separate bug to add a check to make sure that new CA
> certs with the SGC EKU get an error. We'll want this done soon, but it is
> not a blocker for insanity::pkix.

Bug #982932


> 3) I will create a bug for me to communicate to CAs the need to migrate off
> of the SGC EKU, and for CAs to send us the replacement intermediates, so we
> can embed the new intermediate certs (until there are no more valid certs
> signed by the old intermediates with the obsolete EKU). We will want to do
> this soon, but this is not a blocker for insanity::pkix.

Bug #982936

Comment 20

[Camilo Viecco (:cviecco)]

Attachment: eku-oid-int-fix-insanity

Comment 21

[Camilo Viecco (:cviecco)]

Comment on attachment 8390172 [details] [diff] [review]
eku-oid-int-fix-insanity

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

Tests will come tomorrow. First stab at patch

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8390172 [details] [diff] [review]
eku-oid-int-fix-insanity

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

Please add a test for this.

::: security/insanity/lib/pkixcheck.cpp
@@ +320,5 @@
>        SECOidTag oidTag = SECOID_FindOIDTag(*oids);
>        if (requiredEKU != SEC_OID_UNKNOWN && oidTag == requiredEKU) {
>          found = true;
>        }
> +      // Bug 982292

We don't need to reference the bug number, so please remove this.

However, we could add a "TODO(bug 982932): Limit this exception to old certificates."

@@ +321,5 @@
>        if (requiredEKU != SEC_OID_UNKNOWN && oidTag == requiredEKU) {
>          found = true;
>        }
> +      // Bug 982292
> +      // Treat certs with step-up OID as also having SSL server type.

s/certs/CA certs/

@@ +324,5 @@
> +      // Bug 982292
> +      // Treat certs with step-up OID as also having SSL server type.
> +      // COMODO have issued certificates that require this behavior
> +      // that don't expire until June 2020!
> +      if (endEntityOrCA ==  MustBeCA &&

} else if (

@@ +326,5 @@
> +      // COMODO have issued certificates that require this behavior
> +      // that don't expire until June 2020!
> +      if (endEntityOrCA ==  MustBeCA &&
> +          requiredEKU == SEC_OID_EXT_KEY_USAGE_SERVER_AUTH &&
> +          oidTag ==  SEC_OID_NS_KEY_USAGE_GOVT_APPROVED ) {

Remove the space before the closing parenthesis.

Comment 23

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8390172 [details] [diff] [review]
eku-oid-int-fix-insanity

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

::: security/insanity/lib/pkixcheck.cpp
@@ +326,5 @@
> +      // COMODO have issued certificates that require this behavior
> +      // that don't expire until June 2020!
> +      if (endEntityOrCA ==  MustBeCA &&
> +          requiredEKU == SEC_OID_EXT_KEY_USAGE_SERVER_AUTH &&
> +          oidTag ==  SEC_OID_NS_KEY_USAGE_GOVT_APPROVED ) {

nit: also, there's one extra space after the '==' here and on the first line of the if statement.

Comment 24

[Camilo Viecco (:cviecco)]

Attachment: extend-eku-testing (wip)

generator generates tests AND test-script

Comment 25

[Camilo Viecco (:cviecco)]

Attachment: extend-eku-testing

Dont look at the test file, look at the generator (there are almost 3000 tests).

Comment 26

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8393828 [details] [diff] [review]
extend-eku-testing

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

I see what you're doing here, and I think the plan is good. However, I think you need to add a lot more documentation and a bit of cleanup (be careful of spaces around operators and so on).
One thing is it would be nice to not have so many files. In theory, instead of having each EE type for each INT type (i.e. multiplicative), you could just re-use common names and keys and just have one of each type you need. The only question is if we can convince the NSS cert db to forget certs as it goes, so each time we load up a new one, we know that one gets used (and not one from a previous testcase).

::: security/manager/ssl/tests/unit/test_cert_eku/generate.py
@@ +11,5 @@
> +
> +sys.path.append(libpath)
> +
> +import CertUtils
> +

nit: I think some of these blank lines are unnecessary.

@@ +17,5 @@
> +srcdir = os.getcwd()
> +db = tempfile.mkdtemp()
> +
> +CA_basic_constraints = "basicConstraints = critical, CA:TRUE\n"
> +CA_limited_basic_constraints = "basicConstraints = critical,CA:TRUE, pathlen:0\n"

Do we have any tests for a pathlen > 0 (but not unbounded)?

@@ +39,5 @@
> +               'NS': "nsSGC",
> +               'OS': "1.3.6.1.5.5.7.3.9"
> +             };
> +
> +certUsages = [

nit: mixing naming styles - just stick to using_underscores or mixedCase

@@ +45,5 @@
> +               "certificateUsageSSLServer",
> +               "certificateUsageSSLCA",
> +               "certificateUsageEmailSigner",
> +               "certificateUsageEmailRecipient",
> +               #"certificateUsageObjectSigner",

nit: let's be consistent about where the # goes to comment out the code signing/object signing cases we're not testing

@@ +49,5 @@
> +               #"certificateUsageObjectSigner",
> +               'certificateUsageStatusResponder',
> +               ];
> +
> +js_file_header = """//// AUTOGENERATED FILE, DO NOT EDIT

maybe say "autogenerated by test_cert_eku/generate.py"?

@@ +66,5 @@
> +  let der = readFile(do_get_file("test_cert_eku/" + filename, false));
> +  return certdb.constructX509(der, der.length);
> +}
> +
> +function constructCertFromFile(filename) {

doesn't this function do the same thing as certFromFile?

@@ +84,5 @@
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let error = certdb.verifyCertNow(cert, usage,
> +                                   NO_FLAGS, verifiedChain, hasEVPolicy);
> +  if(expected_error != -1 ) {

I'm inferring that an expected_error of -1 indicates that we are expecting some error, but we don't know which one? This seems like not a great testing situation to be in.

@@ +85,5 @@
> +  let verifiedChain = {};
> +  let error = certdb.verifyCertNow(cert, usage,
> +                                   NO_FLAGS, verifiedChain, hasEVPolicy);
> +  if(expected_error != -1 ) {
> +    do_check_eq(error,  expected_error);

nit: extra space before expected_error

@@ +87,5 @@
> +                                   NO_FLAGS, verifiedChain, hasEVPolicy);
> +  if(expected_error != -1 ) {
> +    do_check_eq(error,  expected_error);
> +  } else {
> +    do_check_neq (error, 0);

nit: space after do_check_neq

@@ +89,5 @@
> +    do_check_eq(error,  expected_error);
> +  } else {
> +    do_check_neq (error, 0);
> +  }
> +  

nit: unnecessary whitespace

@@ +92,5 @@
> +  }
> +  
> +}
> +
> +function check_ok_ca (x) {

nit: cert, not x

@@ +96,5 @@
> +function check_ok_ca (x) {
> +  return check_cert_err_generic(x, 0, certificateUsageSSLCA);
> +}
> +
> +function check_fail_ca (cert, errorvalue){

nit: space after function name here and check_ok_ca

@@ +107,5 @@
> +
> +function check_ee_err(cert, expected_error) {
> +  check_cert_err_generic(cert, expected_error, certificateUsageSSLServer)
> +}
> +

nit: unnecessary blank line

@@ +111,5 @@
> +
> +
> +function run_test_in_mode(useInsanity) {
> +  Services.prefs.setBoolPref("security.use_insanity_verification", useInsanity)
> +  do_check_eq (1, 1); 

nit: trailing space. Also, what's this check for?

@@ +119,5 @@
> +
> +}
> +
> +function run_test() {
> +  load_cert("ca", "CT,CT,CT")

not "CTu,CTu,CTu"? (my understanding is the "u" makes the cert a terminal record - is that what we want?)

@@ +166,5 @@
> +            return 0;
> +        else:
> +            return -1;
> +
> +

nit: unnecessary blank lines

@@ +169,5 @@
> +
> +
> +    else:
> +        return -1
> +    return -5 #impossible value

maybe raise an exception?

@@ +171,5 @@
> +    else:
> +        return -1
> +    return -5 #impossible value
> +
> +

nit: unnecessary blank line

@@ +173,5 @@
> +    return -5 #impossible value
> +
> +
> +def gen_os_js_output(int_string, ee_string, certUsage, ee_name):
> +    intHasOSEKU = 0

use boolean types

@@ +180,5 @@
> +    eeHasOSEKU = 0
> +    if ( "OS" in ee_string):
> +        eeHasOSEKU = 1
> +    intHasSAEKU = 0
> +    if ( "OS" in int_string):

typo? Is this supposed to be "SA"?

@@ +189,5 @@
> +    classicEEError =  getError(ee_string, certUsage)
> +    insanityEEError = getInsanityEEError(ee_string, certUsage);
> +    if (eeHasOSEKU == 1):
> +        if (certUsage == "certificateUsageStatusResponder"):
> +            if (intHasOSEKU == 0): 

nit: trailing whitespace

@@ +195,5 @@
> +                if (intHasSAEKU == 1) or ("NONE" in int_string):
> +                    return ("  check_ok_ee(certFromFile('" + ee_name + ".der'), " + certUsage + ");\n")
> +                else:
> +                    return ("  check_cert_err_generic(certFromFile('" +
> +                         ee_name + ".der'), useInsanity? SEC_ERROR_INADEQUATE_CERT_TYPE : 0, "+certUsage+");\n");

nit: space after ?

@@ +220,5 @@
> +            if (classicEEError != 0) :
> +                 return ("  check_cert_err_generic(certFromFile('" +
> +                          ee_name + ".der'), SEC_ERROR_INADEQUATE_CERT_TYPE, "+certUsage+");\n");
> +            ##classic is really messy, sometimes OK, sometimes INVALID_CA, so limiting testing to insanity
> +            return (  " if (useInsanity) {\n    check_cert_err_generic(certFromFile('" +

nit: the indentation in the generated code doesn't come out correctly here

@@ +253,5 @@
> +
> +
> +def gen_js_output(int_string, ee_string, certUsage, ee_name):
> +    intError = getError(int_string, certUsage)
> +    classicEEError =  getError(ee_string, certUsage)

nit: extra space before getError

@@ +284,5 @@
> +        return ("  check_cert_err_generic(certFromFile('" +
> +                 ee_name + ".der'), SEC_ERROR_INADEQUATE_CERT_TYPE, "+certUsage+");\n");
> +
> +
> +    return ""

is it an error if we get here? Raise an exception?

@@ +398,5 @@
> +                                                        ee_ext_text,
> +                                                        int_key,
> +                                                        int_cert);
> +            for certUsage in (certUsages):
> +                js_outfile.write(gen_js_output(int_name, ee_base_name, certUsage, ee_name)) 

nit: trailing space

Comment 27

[Camilo Viecco (:cviecco)]

Attachment: extend-eku-testing (v2)

Comment 28

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=701463b7a288

Comment 29

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8396710 [details] [diff] [review]
extend-eku-testing (v2)

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

This still needs some cleanup. This includes:
* removing everything that isn't used
* fixing indentation issues in the generated test file
* fixing whitespace issues, including appropriate spaces (or lack thereof) around operators, unnecessary vertical space, trailing spaces, etc.
* comments: '# Contents of the comment.'
I have a difficult time convincing myself the code is correct when I get distracted by things like this. Please make sure all instances of these issues are addressed before asking for review again.

::: security/manager/ssl/tests/unit/test_cert_eku.js
@@ +43,5 @@
> +function check_ok_ca (x) {
> +  return check_cert_err_generic(x, 0, certificateUsageSSLCA);
> +}
> +
> +function check_fail_ca (cert, errorvalue){

doesn't look like this ever gets called

@@ +51,5 @@
> +function check_ok_ee(cert, usage) {
> +  return check_cert_err_generic(cert, 0, usage);
> +}
> +
> +function check_ee_err(cert, expected_error) {

doesn't look like this ever gets called

@@ +60,5 @@
> +function run_test_in_mode(useMozillaPKIX) {
> +  Services.prefs.setBoolPref("security.use_mozillapkix_verification", useMozillaPKIX)
> +
> +
> +   check_cert_err_generic(load_cert('int-EKU-CA', ',,'), useMozillaPKIX? -1 : 0 ,certificateUsageSSLCA) ;

nit: fix the indentation on however this gets generated.
Also, I think the formatting for the latter part should be "useMozillaPKIX ? -1 : 0, certificateUsageSSLCA"

@@ +74,5 @@
> +  check_cert_err_generic(certFromFile('ee-EKU-CA_EP-int-EKU-CA.der'), useMozillaPKIX ? SEC_ERROR_INADEQUATE_CERT_TYPE : -1, certificateUsageEmailSigner);
> +  check_cert_err_generic(certFromFile('ee-EKU-CA_EP-int-EKU-CA.der'), useMozillaPKIX ? SEC_ERROR_INADEQUATE_CERT_TYPE : -1, certificateUsageEmailRecipient);
> +  check_cert_err_generic(certFromFile('ee-EKU-CA_EP-int-EKU-CA.der'), SEC_ERROR_INADEQUATE_CERT_TYPE, certificateUsageStatusResponder);
> +  check_cert_err_generic(certFromFile('ee-EKU-CA_EP_NS_OS_SA_TS-int-EKU-CA.der'), useMozillaPKIX? SEC_ERROR_INADEQUATE_CERT_TYPE : 0, certificateUsageSSLClient);
> + if (useMozillaPKIX) {

nit: fix how this gets indented, and all other instances of the same issue

@@ +86,5 @@
> +    check_cert_err_generic(certFromFile('ee-EKU-CA_EP_NS_OS_SA_TS-int-EKU-CA.der'), SEC_ERROR_INADEQUATE_CERT_TYPE, certificateUsageEmailRecipient);
> +  }
> +  check_cert_err_generic(certFromFile('ee-EKU-CA_EP_NS_OS_SA_TS-int-EKU-CA.der'), useMozillaPKIX ? SEC_ERROR_INADEQUATE_CERT_TYPE : 0, certificateUsageStatusResponder);
> +  check_ok_ee(certFromFile('ee-EKU-CA_NS-int-EKU-CA.der'), certificateUsageSSLClient);
> +  check_cert_err_generic(certFromFile('ee-EKU-CA_NS-int-EKU-CA.der'), useMozillaPKIX ? SEC_ERROR_INADEQUATE_CERT_TYPE : 0 , certificateUsageSSLServer);

nit: no space after 0 (fix how this gets generated, here and elsewhere)

@@ +3519,5 @@
> +  check_cert_err_generic(certFromFile('ee-EKU-TS-int-EKU-TS.der'), -1, certificateUsageSSLCA);
> +  check_cert_err_generic(certFromFile('ee-EKU-TS-int-EKU-TS.der'), SEC_ERROR_INADEQUATE_CERT_TYPE, certificateUsageEmailSigner);
> +  check_cert_err_generic(certFromFile('ee-EKU-TS-int-EKU-TS.der'), SEC_ERROR_INADEQUATE_CERT_TYPE, certificateUsageEmailRecipient);
> +  check_cert_err_generic(certFromFile('ee-EKU-TS-int-EKU-TS.der'), SEC_ERROR_INADEQUATE_CERT_TYPE, certificateUsageStatusResponder);
> +

nit: fix unnecessary vertical space generated

@@ +3529,5 @@
> +
> +  run_test_in_mode(true);
> +  run_test_in_mode(false);
> +}
> +

nit: fix unnecessary vertical space generated

::: security/manager/ssl/tests/unit/test_cert_eku/generate.py
@@ +12,5 @@
> +sys.path.append(libpath)
> +
> +import CertUtils
> +
> +

nit: unnecessary vertical space

@@ +17,5 @@
> +srcdir = os.getcwd()
> +db = tempfile.mkdtemp()
> +
> +CA_basic_constraints = "basicConstraints = critical, CA:TRUE\n"
> +CA_limited_basic_constraints = "basicConstraints = critical,CA:TRUE, pathlen:0\n"

this isn't used

@@ +20,5 @@
> +CA_basic_constraints = "basicConstraints = critical, CA:TRUE\n"
> +CA_limited_basic_constraints = "basicConstraints = critical,CA:TRUE, pathlen:0\n"
> +EE_basic_constraints = "basicConstraints = CA:FALSE\n"
> +
> +CA_min_ku = "keyUsage = critical, keyCertSign\n"

this isn't used

@@ +24,5 @@
> +CA_min_ku = "keyUsage = critical, keyCertSign\n"
> +CA_full_ku = "keyUsage = keyCertSign, digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, cRLSign\n"
> +EE_full_ku ="keyUsage = digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement\n"
> +
> +Server_eku = "extendedKeyUsage = critical, serverAuth, clientAuth\n "

this isn't used

@@ +35,5 @@
> +               'CA': "clientAuth",
> +               #'CS': "codeSigning",
> +               'EP': "emailProtection",
> +               'TS': "timeStamping",
> +               'NS': "nsSGC",

What is nsSGC?

@@ +46,5 @@
> +               "certificateUsageSSLCA",
> +               "certificateUsageEmailSigner",
> +               "certificateUsageEmailRecipient",
> +               #"certificateUsageObjectSigner",
> +               'certificateUsageStatusResponder',

nit: inconsistent use of ' vs "

@@ +47,5 @@
> +               "certificateUsageEmailSigner",
> +               "certificateUsageEmailRecipient",
> +               #"certificateUsageObjectSigner",
> +               'certificateUsageStatusResponder',
> +               ];

nit: indentation

@@ +61,5 @@
> +do_get_profile(); // must be called before getting nsIX509CertDB
> +const certdb = Cc["@mozilla.org/security/x509certdb;1"]
> +                 .getService(Ci.nsIX509CertDB);
> +
> +function certFromFile(filename) {

nit: mixed naming style (underscores vs intercaps)

@@ +66,5 @@
> +  let der = readFile(do_get_file("test_cert_eku/" + filename, false));
> +  return certdb.constructX509(der, der.length);
> +}
> +
> +function constructCertFromFile(filename) {

nit: isn't this the same as certFromFile? (delete this function if we don't need it)

@@ +77,5 @@
> +  addCertFromFile(certdb, "test_cert_eku/" + cert_filename, trust_string);
> +  return certFromFile(cert_filename);
> +}
> +
> +function check_cert_err_generic(cert, expected_error, usage) {

maybe file a follow-up on this: a lot of these functions are the same as in the basic constraints test - we should refactor them up to head_psm.js

@@ +84,5 @@
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let error = certdb.verifyCertNow(cert, usage,
> +                                   NO_FLAGS, verifiedChain, hasEVPolicy);
> +  if (expected_error != -1 ) {

again, this needs to be documented: what does it mean if expected_error is -1?

@@ +91,5 @@
> +    do_check_neq (error, 0);
> +  }
> +}
> +
> +function check_ok_ca (x) {

nits: "cert", not "x", space after check_ok_ca

@@ +95,5 @@
> +function check_ok_ca (x) {
> +  return check_cert_err_generic(x, 0, certificateUsageSSLCA);
> +}
> +
> +function check_fail_ca (cert, errorvalue){

nit: space after check_fail_ca

@@ +96,5 @@
> +  return check_cert_err_generic(x, 0, certificateUsageSSLCA);
> +}
> +
> +function check_fail_ca (cert, errorvalue){
> +  return check_cert_err_generic(cert, errorvalue, certificateUsageSSLCA)

nit: missing ;

@@ +104,5 @@
> +  return check_cert_err_generic(cert, 0, usage);
> +}
> +
> +function check_ee_err(cert, expected_error) {
> +  check_cert_err_generic(cert, expected_error, certificateUsageSSLServer)

nit: missing ;

@@ +107,5 @@
> +function check_ee_err(cert, expected_error) {
> +  check_cert_err_generic(cert, expected_error, certificateUsageSSLServer)
> +}
> +
> +

nit: unnecessary vertical space

@@ +109,5 @@
> +}
> +
> +
> +function run_test_in_mode(useMozillaPKIX) {
> +  Services.prefs.setBoolPref("security.use_mozillapkix_verification", useMozillaPKIX)

nit: missing ;

@@ +117,5 @@
> +
> +}
> +
> +function run_test() {
> +  load_cert("ca", "CT,CT,CT")

nit: missing ;

@@ +165,5 @@
> +        else:
> +            return -1;
> +    else:
> +        return -1
> +    return -5 #impossible value

raise an exception

@@ +177,5 @@
> +        return ("  check_cert_err_generic(certFromFile('" +
> +                     ee_name + ".der'), SEC_ERROR_INADEQUATE_CERT_TYPE, " +
> +                     cert_usage + ");\n");
> +
> +    ##classic mode considers some values of EKU equivalent to SSL server 

nit: for comments, I think just "# the comment" will do

@@ +180,5 @@
> +
> +    ##classic mode considers some values of EKU equivalent to SSL server 
> +    if  (((cert_usage == "certificateUsageSSLServer") and ("CA" in int_string)) or
> +         ((cert_usage == "certificateUsageSSLClient") and ("NS" in int_string)) or 
> +         ((cert_usage == "certificateUsageSSLClient") and ("SA" in int_string)) or 

nit: trailing space on a few of these lines

@@ +186,5 @@
> +         ((cert_usage == "certificateUsageSSLServer") and ("int-EKU-TS" == int_string)) or
> +         ((cert_usage == "certificateUsageEmailSigner") and ("int-EKU-TS" == int_string)) or
> +         ((cert_usage == "certificateUsageEmailRecipient") and ("int-EKU-TS" == int_string))
> +
> +   ):

nit: formatting - I don't think this closing ): belongs here

@@ +197,5 @@
> +               "SEC_ERROR_INADEQUATE_CERT_TYPE : -1, " + cert_usage + ");\n");
> +
> +
> +
> +

nit: lots of unnecessary vertical space

@@ +238,5 @@
> +                         ee_name + ".der'), useMozillaPKIX? SEC_ERROR_INADEQUATE_CERT_TYPE : 0, "+cert_usage+");\n");
> +            if (classic_ee_error != 0) :
> +                 return ("  check_cert_err_generic(certFromFile('" +
> +                          ee_name + ".der'), SEC_ERROR_INADEQUATE_CERT_TYPE, "+cert_usage+");\n");
> +            ## classic is really messy, sometimes OK, sometimes INVALID_CA, 

nit: trailing space

@@ +243,5 @@
> +            ## so limiting testing to mozpkix
> +            return (  " if (useMozillaPKIX) {\n    check_cert_err_generic(certFromFile('" +
> +                         ee_name + ".der'), SEC_ERROR_INADEQUATE_CERT_TYPE, " + cert_usage + ");\n  }\n")
> +
> +        #return ""

raise an exception

@@ +262,5 @@
> +
> +    return generate_simple_overlay_output(int_string, ee_name,  int_error,
> +                                          classic_ee_error, cert_usage);
> +
> +

nit: vertical space

@@ +281,5 @@
> +        # usageStatusResponder the calculations are not trivial and thus they
> +        # go into their own routine
> +        return gen_os_js_output(int_string, ee_string, cert_usage, ee_name)
> +
> +

nit: vertical space

@@ +325,5 @@
> +                                                        ca_name,
> +                                                        CA_basic_constraints)
> +    ee_ext_text = EE_basic_constraints + EE_full_ku
> +
> +

nit: vertical space

@@ +392,5 @@
> +    js_outfile.close();
> +
> +generate_certs()
> +
> +

nit: trailing vertical space

Comment 30

[Camilo Viecco (:cviecco)]

Attachment: extend-eku-testing (v3)

Comment 31

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8397398 [details] [diff] [review]
extend-eku-testing (v3)

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

AFAICT, this looks good. There are still some style nits, so I would appreciate it if you had another look at generate.py for things like spaces being in the right place, etc.

::: security/manager/ssl/tests/unit/test_cert_eku/generate.py
@@ +87,5 @@
> +function check_ok_ca(cert) {
> +  return check_cert_err_generic(cert, 0, certificateUsageSSLCA);
> +}
> +
> +function check_fail_ca(cert, errorvalue){

nit: this isn't used

@@ +95,5 @@
> +function check_ok_ee(cert, usage) {
> +  return check_cert_err_generic(cert, 0, usage);
> +}
> +
> +function check_ee_err(cert, expected_error) {

nit: this isn't used

@@ +155,5 @@
> +            return -1;
> +    else:
> +        return -1
> +    print "impossible state"
> +    raise

nit: raise Exception("impossible state")

@@ +275,5 @@
> +            return  ("  check_cert_err_generic(cert_from_file('" +
> +                     ee_name + ".der'), useMozillaPKIX ? SEC_ERROR_INADEQUATE_CERT_TYPE : 0, "
> +                     + cert_usage +");\n")
> +
> +    return generate_simple_overlay_output(int_string, ee_name,  int_error,

nit: two spaces before int_error

@@ +278,5 @@
> +
> +    return generate_simple_overlay_output(int_string, ee_name,  int_error,
> +                                          classic_ee_error, cert_usage);
> +
> +def generate_test_eku ():

nit: space after generate_test_eku

@@ +299,5 @@
> +    outmap[all_names] = all_values
> +    return outmap
> +
> +def generate_certs():
> +    js_outfile = open ("../test_cert_eku.js", 'w');

nit: space after open

@@ +371,5 @@
> +                js_outfile.write(gen_js_output(int_name, ee_base_name,
> +                                 cert_usage, ee_name))
> +
> +    js_outfile.write(js_file_footer);
> +    js_outfile.close();

nit: no trailing ';' in python

Comment 32

[Camilo Viecco (:cviecco)]

https://tbpl.mozilla.org/?tree=Try&rev=7a5e0ed2063f See if now we can resolve timeouts on b2g

Comment 33

[Camilo Viecco (:cviecco)]

Attachment: eku-oid-int-fix-insanity (v2)

Keeping r+ from bsmith

Comment 34

[Camilo Viecco (:cviecco)]

removing evaluation on b2g arm due to timeouts.
https://tbpl.mozilla.org/?tree=Try&rev=dee10e7f1f65

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/49ee8ceaf9fe
https://hg.mozilla.org/mozilla-central/rev/13011996555f

Comment 38

[u279076]

Not sure if this needs further testing or not. I'll let Matt Wobensmith decide.

Comment 39

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed fixed in Fx31, 2014-04-03, mozilla::pkix enabled.

All URLs open and resolve without error. 

Some of the URLs in comment 7 redirect to HTTP instead, but all sites are now reachable.