966820 - Add tests for that trust bits are interpreted correctly by CertVerifier

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P3)

Description

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

1. Test that a root cert must have the SSL CA trust bit in order to be a trust anchor for SSL certificates, and that Email or Object Signer trust bits do not work as a substitute.
2. Test that a root cert with multiple trust bits (including SSL) works.
3. Test that a root cert with no trust bits works.
4. Test the same for intermediates
5. Test the same for end-entities.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: test-trust-bits

Comment 2

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

Comment on attachment 8388832 [details] [diff] [review]
test-trust-bits

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

Overall, this looks good - everything is nicely organized. I think some more comments/documentation would go a long way. For example, explain a bit more what each trust string means, what each usage means, and why we're expecting the results we get. In particular, when insanity::pkix differs from classic, it's not clear to me if there's a bug we need to address.

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +9,5 @@
> +const certdb  = Cc["@mozilla.org/security/x509certdb;1"]
> +                  .getService(Ci.nsIX509CertDB);
> +
> +// This is the list of certificates needed for the test
> +// The certificates prefixed by 'int-' are intermediates

nit: this comment doesn't quite describe the list of certificates for this test

@@ +22,5 @@
> +  addCertFromFile(certdb, "test_cert_trust/" + cert_filename, trust_string);
> +}
> +
> +function setup_basic_trusts(ca_cert, int_cert) {
> +  const nsIX509Cert = Components.interfaces.nsIX509Cert;

This probably isn't necessary - you can just use Ci.nsIX509Cert, right?

@@ +32,5 @@
> +  certdb.setCertTrust(int_cert, nsIX509Cert.CA_CERT, 0);
> +}
> +
> +function check_cert_err_generic(cert, expected_error, usage) {
> +  const nsIX509Cert3 = Components.interfaces.nsIX509Cert3;

doesn't look like this is used

@@ +34,5 @@
> +
> +function check_cert_err_generic(cert, expected_error, usage) {
> +  const nsIX509Cert3 = Components.interfaces.nsIX509Cert3;
> +  do_print("cert cn=" + cert.commonName);
> +  do_print("cert issuer cn=" + cert.issuerCommonName);

nit: probably don't need these print statements, since any error should give you a backtrace that indicates which test failed

@@ +43,5 @@
> +  do_check_eq(error,  expected_error);
> +
> +};
> +
> +function test_ca_distrust(ee_cert, cert_to_distrust, isRootCA, useInsanity) {

maybe "cert_to_modify_trust"

@@ +44,5 @@
> +
> +};
> +
> +function test_ca_distrust(ee_cert, cert_to_distrust, isRootCA, useInsanity) {
> +  //check_cert_err_generic(cert, expected_error, certificateUsageSSLServer)

probably don't need this line

@@ +65,5 @@
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageObjectSigner);
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID
> +                                              : SEC_ERROR_INVALID_ARGS,

SEC_ERROR_INVALID_ARGS seems like a bug - what's going on here?

@@ +71,5 @@
> +  check_cert_err_generic(ee_cert, SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageStatusResponder);
> +
> +
> +  // Trust set to T  -  trusted CA to issue client certs

These tests don't make sense to me - why are we getting errors if the CA is trusted to issue client certs?

@@ +79,5 @@
> +                                           : 0,
> +                         certificateUsageSSLServer);
> +
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : 0

Why the classic/insanity::pkix discrepancy? This seems like a bug.

@@ +84,5 @@
> +                                           : 0,
> +                         certificateUsageSSLClient);
> +
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,

I would expect this to be INADEQUATE_CERT_TYPE in both cases... what's going on here?

@@ +98,5 @@
> +                                           : 0,
> +                         certificateUsageEmailRecipient);
> +
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : SEC_ERROR_INADEQUATE_CERT_TYPE 

nit: trailing whitespace

@@ +100,5 @@
> +
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : SEC_ERROR_INADEQUATE_CERT_TYPE 
> +                                           : useInsanity ? 0
> +                                                         : SEC_ERROR_INADEQUATE_CERT_TYPE,

classic/insanity::pkix are different - is this a bug?

@@ +104,5 @@
> +                                                         : SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageObjectSigner);
> +
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID
> +                                              : SEC_ERROR_INVALID_ARGS,

another discrepancy - bug?

@@ +114,5 @@
> +  // Now tests on the SSL trust bit
> +  setCertTrust(cert_to_distrust, 'p,C,C');
> +  check_cert_err_generic(ee_cert, SEC_ERROR_UNTRUSTED_ISSUER,
> +                         certificateUsageSSLServer);
> +  check_cert_err_generic(ee_cert, useInsanity ? 0

this seems like a bug?

@@ +120,5 @@
> +                         certificateUsageSSLClient);
> +
> +  check_cert_err_generic(ee_cert, 0, certificateUsageEmailSigner);
> +  check_cert_err_generic(ee_cert, 0, certificateUsageEmailRecipient);
> +  check_cert_err_generic(ee_cert, useInsanity ? 0

bug?

@@ +127,5 @@
> +
> +
> +  // Inherited trust SSL
> +  setCertTrust(cert_to_distrust, ',C,C');
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER 

nit: trailing whitespace

@@ +131,5 @@
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER 
> +                                                         : SEC_ERROR_UNTRUSTED_ISSUER
> +                                           : 0,
> +                         certificateUsageSSLServer);
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? 0  // XXX Insanity Bug

please file a new bug, have it block the insanity::pkix tracking bug, and then reference it here

@@ +145,5 @@
> +
> +  // Now tests on the EMAIL trust bit
> +  setCertTrust(cert_to_distrust, 'C,p,C');
> +  check_cert_err_generic(ee_cert, 0, certificateUsageSSLServer);
> +  check_cert_err_generic(ee_cert, isRootCA ? SEC_ERROR_UNTRUSTED_ISSUER 

nit: trailing space

@@ +170,5 @@
> +                         certificateUsageSSLClient);
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : SEC_ERROR_UNTRUSTED_ISSUER
> +                                           : 0,
> +                         certificateUsageEmailSigner); /// Bug on both NSS and pkix?

file a bug, please

@@ +176,5 @@
> +
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : SEC_ERROR_UNTRUSTED_ISSUER
> +                                           : 0,
> +                         certificateUsageEmailRecipient); /// Bug on both NSS and pkix?

bug please

@@ +178,5 @@
> +                                                         : SEC_ERROR_UNTRUSTED_ISSUER
> +                                           : 0,
> +                         certificateUsageEmailRecipient); /// Bug on both NSS and pkix?
> +  check_cert_err_generic(ee_cert, useInsanity ? 0
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,

is this a bug?

@@ +180,5 @@
> +                         certificateUsageEmailRecipient); /// Bug on both NSS and pkix?
> +  check_cert_err_generic(ee_cert, useInsanity ? 0
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageObjectSigner);
> +

What about object signing trust?

@@ +190,5 @@
> +  clearOCSPCache();
> +  clearSessionCache();
> +
> +  let ca_cert = certdb.findCertByNickname(null, 'ca');
> +  do_check_false(!ca_cert)

do_check_true(ca_cert) should work

::: security/manager/ssl/tests/unit/test_cert_trust/generate.py
@@ +1,1 @@
> +#!/usr/bin/python

nit: python mode line before this, no empty line after

@@ +31,5 @@
> +authority_key_ident = "authorityKeyIdentifier = keyid, issuer\n"
> +subject_key_ident = "subjectKeyIdentifier = hash\n"
> +
> +
> +def self_sign_csr(db_dir, dst_dir, csr_name, key_file, serial_num, ext_text,

is this still necessary? (the call to it is commented out, below)

@@ +57,5 @@
> +                                                        key_type,
> +                                                        'ca',
> +                                                        ca_ext)
> +
> +    name = 'int'

I'm not sure this is necessary - just pass in 'int' like you do with the ca and ee.

@@ +80,5 @@
> +
> +
> +    #shutil.copy(ca_cert, srcdir + "/" + "ca-1.der")
> +    #self_sign_csr(db, srcdir, db + "/ca.csr", ca_key, 2, ca_ext, "ca-2")
> +    #os.remove(ca_cert);

nit: remove unused lines

Comment 3

[Camilo Viecco (:cviecco)]

Comment on attachment 8388832 [details] [diff] [review]
test-trust-bits

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

Thanks for the fast review. Agreed on all the nits, and will file bugs for the misbehaving issues.

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +65,5 @@
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageObjectSigner);
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID
> +                                              : SEC_ERROR_INVALID_ARGS,

It is from https://mxr.mozilla.org/mozilla-central/source/security/certverifier/CertVerifier.cpp#463 on classic we do not allow UsageVerifyCA (which looks like a bug now)

@@ +71,5 @@
> +  check_cert_err_generic(ee_cert, SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageStatusResponder);
> +
> +
> +  // Trust set to T  -  trusted CA to issue client certs

Client cert/server cert (is clear on the SSLServer/SSL client case) but obscure in the emailsigner/emailrecipient case.

@@ +79,5 @@
> +                                           : 0,
> +                         certificateUsageSSLServer);
> +
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : 0

Yes it is

@@ +84,5 @@
> +                                           : 0,
> +                         certificateUsageSSLClient);
> +
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,

Need to look.

@@ +100,5 @@
> +
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : SEC_ERROR_INADEQUATE_CERT_TYPE 
> +                                           : useInsanity ? 0
> +                                                         : SEC_ERROR_INADEQUATE_CERT_TYPE,

No, is is expected given the different properties of the objectsigner in classic vs insanity

@@ +146,5 @@
> +  // Now tests on the EMAIL trust bit
> +  setCertTrust(cert_to_distrust, 'C,p,C');
> +  check_cert_err_generic(ee_cert, 0, certificateUsageSSLServer);
> +  check_cert_err_generic(ee_cert, isRootCA ? SEC_ERROR_UNTRUSTED_ISSUER 
> +                                           : useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER

This is the actual bug! (insanity claims is not valid, when is clear it is).

@@ +170,5 @@
> +                         certificateUsageSSLClient);
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : SEC_ERROR_UNTRUSTED_ISSUER
> +                                           : 0,
> +                         certificateUsageEmailSigner); /// Bug on both NSS and pkix?

actually this is correct.

@@ +180,5 @@
> +                         certificateUsageEmailRecipient); /// Bug on both NSS and pkix?
> +  check_cert_err_generic(ee_cert, useInsanity ? 0
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageObjectSigner);
> +

I tought about it, but since the behaviours are so different, I decided not to. (new certs would be needed with deprecated NSS object signer OIDS in them).

@@ +190,5 @@
> +  clearOCSPCache();
> +  clearSessionCache();
> +
> +  let ca_cert = certdb.findCertByNickname(null, 'ca');
> +  do_check_false(!ca_cert)

It did not :o

Comment 4

[Camilo Viecco (:cviecco)]

Comment on attachment 8388832 [details] [diff] [review]
test-trust-bits

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

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +146,5 @@
> +  // Now tests on the EMAIL trust bit
> +  setCertTrust(cert_to_distrust, 'C,p,C');
> +  check_cert_err_generic(ee_cert, 0, certificateUsageSSLServer);
> +  check_cert_err_generic(ee_cert, isRootCA ? SEC_ERROR_UNTRUSTED_ISSUER 
> +                                           : useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER

arrg. NSS has the bug, not insanity. The itnermediate is expliciyly distrusted here,

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: test-trust-bits (v 2)

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: test-trust-bits (v 2.1)

Comment 7

[Camilo Viecco (:cviecco)]

Comment on attachment 8389491 [details] [diff] [review]
test-trust-bits  (v 2.1)

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

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +71,5 @@
> +                         certificateUsageStatusResponder);
> +
> +
> +  // Trust set to T  -  trusted CA to issue client certs, where client cert is
> +  // usageSSLClient. 

nit: missed this one.

Comment 8

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

Comment on attachment 8389491 [details] [diff] [review]
test-trust-bits  (v 2.1)

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

This is looking really good. My two main concerns are:
1. It doesn't look like my comments on generate.py from the previous review were addressed
2. There are 8 usages we handle in CertVerifier::InsanityVerifyCert. I think we should test that each usage returns the results we're expecting every time we change the trust on these certificates (so, for example, certificateUsageSSLCA should always fail).

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +5,5 @@
> +
> +"use strict";
> +
> +do_get_profile(); // must be called before getting nsIX509CertDB
> +const certdb  = Cc["@mozilla.org/security/x509certdb;1"]

nit: maybe 'gCertDB' or something?

@@ +15,5 @@
> +  'ca',
> +]
> +
> +function load_cert(cert_name, trust_string) {
> +  var cert_filename = cert_name + ".der";

nit: let, not var

@@ +40,5 @@
> +
> +};
> +
> +function test_ca_distrust(ee_cert, cert_to_modify_trust, isRootCA, useInsanity) {
> +  // On reset everyting is successful

Well, but there are some usages we're always expecting to fail, right? Let's include them here.

@@ +64,5 @@
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageObjectSigner);
> +  check_cert_err_generic(ee_cert, useInsanity ? SEC_ERROR_CA_CERT_INVALID
> +                                               : SEC_ERROR_INVALID_ARGS,

nit: alignment

@@ +79,5 @@
> +                                           : 0,
> +                         certificateUsageSSLServer);
> +
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : 0

Did you file a bug on this? (In any case, include the bug number here, please.)

@@ +99,5 @@
> +                         certificateUsageEmailRecipient);
> +
> +  check_cert_err_generic(ee_cert, isRootCA ? useInsanity ? SEC_ERROR_UNKNOWN_ISSUER
> +                                                         : SEC_ERROR_INADEQUATE_CERT_TYPE
> +                                           : useInsanity ? 0

A comment explaining why this is the case would be good.

@@ +114,5 @@
> +  // Now tests on the SSL trust bit
> +  setCertTrust(cert_to_modify_trust, 'p,C,C');
> +  check_cert_err_generic(ee_cert, SEC_ERROR_UNTRUSTED_ISSUER,
> +                         certificateUsageSSLServer);
> +  check_cert_err_generic(ee_cert, useInsanity ? 0  //XXX Bug Bug 982340

nit: probably only need one "bug" :)

@@ +147,5 @@
> +  setCertTrust(cert_to_modify_trust, 'C,p,C');
> +  check_cert_err_generic(ee_cert, 0, certificateUsageSSLServer);
> +  check_cert_err_generic(ee_cert, isRootCA ? SEC_ERROR_UNTRUSTED_ISSUER
> +                                           : useInsanity ? SEC_ERROR_UNTRUSTED_ISSUER
> +                                                         : 0, // Insanity is OK, NSS bug

well, we should probably at least file a bug...

@@ +179,5 @@
> +                         certificateUsageEmailRecipient);
> +  check_cert_err_generic(ee_cert, useInsanity ? 0
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageObjectSigner);
> +

nit: unnecessary blank line

@@ +181,5 @@
> +                                              : SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageObjectSigner);
> +
> +}
> +

nit: unnecessary blank line

@@ +186,5 @@
> +
> +function run_test_in_mode(useInsanity) {
> +  Services.prefs.setBoolPref("security.use_insanity_verification", useInsanity);
> +  clearOCSPCache();
> +  clearSessionCache();

I don't think either of these are necessary - we aren't doing OCSP fetching, and we're not connecting to any servers, right?

@@ +189,5 @@
> +  clearOCSPCache();
> +  clearSessionCache();
> +
> +  let ca_cert = certdb.findCertByNickname(null, 'ca');
> +  do_check_false(!ca_cert)

nit: missing ';'

@@ +191,5 @@
> +
> +  let ca_cert = certdb.findCertByNickname(null, 'ca');
> +  do_check_false(!ca_cert)
> +  let int_cert = certdb.findCertByNickname(null, 'int');
> +  do_check_false(!int_cert)

nit: missing ';'

::: security/manager/ssl/tests/unit/test_cert_trust/generate.py
@@ +80,5 @@
> +
> +
> +    #shutil.copy(ca_cert, srcdir + "/" + "ca-1.der")
> +    #self_sign_csr(db, srcdir, db + "/ca.csr", ca_key, 2, ca_ext, "ca-2")
> +    #os.remove(ca_cert);

Doesn't look like my comments were addressed from the previous review of this file.

Comment 9

[Camilo Viecco (:cviecco)]

Comment on attachment 8389491 [details] [diff] [review]
test-trust-bits  (v 2.1)

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

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +5,5 @@
> +
> +"use strict";
> +
> +do_get_profile(); // must be called before getting nsIX509CertDB
> +const certdb  = Cc["@mozilla.org/security/x509certdb;1"]

all the xpcshell tests that I have written use this convetion. Lets keep it or mass migrate on another bug.

@@ +186,5 @@
> +
> +function run_test_in_mode(useInsanity) {
> +  Services.prefs.setBoolPref("security.use_insanity_verification", useInsanity);
> +  clearOCSPCache();
> +  clearSessionCache();

agreed

::: security/manager/ssl/tests/unit/test_cert_trust/generate.py
@@ +80,5 @@
> +
> +
> +    #shutil.copy(ca_cert, srcdir + "/" + "ca-1.der")
> +    #self_sign_csr(db, srcdir, db + "/ca.csr", ca_key, 2, ca_ext, "ca-2")
> +    #os.remove(ca_cert);

mea culpa

Comment 10

[Camilo Viecco (:cviecco)]

Attachment: test-trust-bits (v3)

Comment 11

[Camilo Viecco (:cviecco)]

Attachment: test-trust-bits (v3.1)

Comment 12

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

Comment on attachment 8390648 [details] [diff] [review]
test-trust-bits (v3.1)

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

Great!

::: security/manager/ssl/tests/unit/test_cert_trust.js
@@ +212,5 @@
> +                         certificateUsageVerifyCA);
> +  check_cert_err_generic(ee_cert, SEC_ERROR_INADEQUATE_CERT_TYPE,
> +                         certificateUsageStatusResponder);
> +}
> +

nit: unnecessary blank line

::: security/manager/ssl/tests/unit/test_cert_trust/generate.py
@@ +32,5 @@
> +
> +authority_key_ident = "authorityKeyIdentifier = keyid, issuer\n"
> +subject_key_ident = "subjectKeyIdentifier = hash\n"
> +
> +

nit: not sure we need all this vertical space here

@@ +65,5 @@
> +                                    int_key,
> +                                    int_cert)
> +
> +
> +

nit: again, we probably don't need so much vertical space - one line should do it.

Comment 13

[Camilo Viecco (:cviecco)]

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

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8c23b2efbe3c