938046 - nsNSSCertificate::GetChain and nsNSSCertificate::GetIssuer use inconsistent/wrong usage when building chain

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

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

+++ This bug was initially created as a clone of Bug #813418 +++

NS_IMETHODIMP 
> nsNSSCertificate::GetIssuer(nsIX509Cert * *aIssuer)
> {

[...]

>   issuer = CERT_FindCertIssuer(mCert, PR_Now(), certUsageSSLClient);

1. This should be certUsageSSLClient, at a minimum.
2. The result should be made consistent the second element of the result of calling GetChain.
3. The only user of GetIssuer is checkCert in CertUtils.jsm. The way it uses GetIssuer is bad and can be replaced by use of GetChain.

Perhaps #2 and #3 are separate bugs. Please file new bugs if you don't fix them when fixing this bug.

> NS_IMETHODIMP
> nsNSSCertificate::GetChain(nsIArray **_rvChain)

[...]

>   for (int usage = certificateUsageSSLClient;
>        usage < certificateUsageAnyCA && !pkixNssChain;
>        usage = usage << 1) {
>     if (usage == certificateUsageSSLServer) {
>       continue;
>     }
>     PR_LOG(gPIPNSSLog, PR_LOG_DEBUG, ("pipnss: PKIX attempting chain(%d) for '%s'\n",usage, mCert->nickname));
>     srv = certVerifier->VerifyCert(mCert,
>                                    certificateUsageSSLClient, PR_Now(),
>                                    nullptr, /*XXX fixme*/
>                                    CertVerifier::FLAG_LOCAL_ONLY,
>                                    &pkixNssChain);

The hard-coded "certificateUsageSSLClient" should be "usage" instead.

See also bug 651994.

Comment 1

[Camilo Viecco (:cviecco)]

Attachment: fix-938046 part1

Comment 2

[Camilo Viecco (:cviecco)]

Attachment: fix-938046-p2

Can you double check my memory management here?

Comment 3

[Camilo Viecco (:cviecco)]

Attachment: fix-938046-p2 (v1.1)

Can you double check addrefs.

Comment 4

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

Comment on attachment 831637 [details] [diff] [review]
fix-938046 part1

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

LGTM.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +856,5 @@
>                                     &pkixNssChain);
>    }
>  
>    if (!pkixNssChain) {
>      // There is not verified path for the chain, howeever we still want to 

Can we fix this line too? s/not/no/ s/, howeever/. However,/ and no trailing space

Comment 5

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

Comment on attachment 831641 [details] [diff] [review]
fix-938046-p2 (v1.1)

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

Looks good.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +784,5 @@
> +
> +  nsCOMPtr<nsIArray> chain;
> +  nsresult rv;
> +  rv = GetChain(getter_AddRefs(chain));
> +  if (NS_FAILED(rv)) {

NS_ENSURE_SUCCESS(rv, rv); works here

@@ +789,5 @@
> +    return rv;
> +  }
> +  uint32_t length;
> +  if(!chain || NS_FAILED(chain->GetLength(&length)) || length == 0) {
> +    return NS_ERROR_UNEXPECTED;

NS_ERROR_FAILURE might be better, but I don't think it's a big deal

@@ +796,5 @@
> +    return NS_OK;
> +  }
> +  nsCOMPtr<nsIX509Cert> cert;
> +  chain->QueryElementAt(1, NS_GET_IID(nsIX509Cert), getter_AddRefs(cert));
> +  if (cert) {

if (!cert), we probably want to return an error, right?

@@ +798,5 @@
> +  nsCOMPtr<nsIX509Cert> cert;
> +  chain->QueryElementAt(1, NS_GET_IID(nsIX509Cert), getter_AddRefs(cert));
> +  if (cert) {
> +    *aIssuer = cert;
> +    NS_ADDREF(*aIssuer);

This looks correct to me. NS_ADDREF(*aIssuer = cert); seems to be common as well.

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: fix-938046-p2 (v2)

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: fix-938046-p3

Comment 8

[Camilo Viecco (:cviecco)]

Attachment: tests-938046

Comment 9

[Camilo Viecco (:cviecco)]

Comment on attachment 8342651 [details] [diff] [review]
fix-938046-p3

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

Do not try to verify unverifiable usages or usages done

Comment 10

[Camilo Viecco (:cviecco)]

Comment on attachment 8342656 [details] [diff] [review]
tests-938046

Or do you prefer to put them in add_tests at the end?

Comment 11

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

Comment on attachment 8342650 [details] [diff] [review]
fix-938046-p2 (v2)

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

Looks great.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +800,5 @@
> +  nsCOMPtr<nsIX509Cert> cert;
> +  chain->QueryElementAt(1, NS_GET_IID(nsIX509Cert), getter_AddRefs(cert));
> +  if (cert) {
> +    *aIssuer = cert;
> +    NS_ADDREF(*aIssuer);

I think you can do "*aIssuer = cert.forget();" and then we don't have the manual NS_ADDREF line, but I think this is fine as well.

Comment 12

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

Comment on attachment 8342650 [details] [diff] [review]
fix-938046-p2 (v2)

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +798,5 @@
> +    return NS_OK;
> +  }
> +  nsCOMPtr<nsIX509Cert> cert;
> +  chain->QueryElementAt(1, NS_GET_IID(nsIX509Cert), getter_AddRefs(cert));
> +  if (cert) {

(In reply to David Keeler (:keeler) from comment #5)
> @@ +796,5 @@
> > +    return NS_OK;
> > +  }
> > +  nsCOMPtr<nsIX509Cert> cert;
> > +  chain->QueryElementAt(1, NS_GET_IID(nsIX509Cert), getter_AddRefs(cert));
> > +  if (cert) {
> 
> if (!cert), we probably want to return an error, right?
> 

I take that back - I still think we want to return an error here.

Comment 13

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

Comment on attachment 8342651 [details] [diff] [review]
fix-938046-p3

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +860,5 @@
>         usage = usage << 1) {
> +    if (usage == certificateUsageSSLServer ||
> +        usage == certificateUsageUserCertImport ||
> +        usage == certificateUsageVerifyCA ||
> +        usage == certificateUsageProtectedObjectSigner) {

How about instead of listing the usages we don't want to check for, just iterate through a list of usages we do want to check for? Or am I misunderstanding the purpose of this?

Comment 14

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

Comment on attachment 8342656 [details] [diff] [review]
tests-938046

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

Looks good, except for a bit of cleanup. My main request is for much more documentation - what is going on in the test, what is expected to happen, and why? (E.g. explain all of the cert trust settings changes that go on.)

::: security/manager/ssl/tests/unit/test_getchain/generate.py
@@ +21,5 @@
> +CA_min_ku = "keyUsage = critical, keyCertSign\n"
> +CA_bad_ku = ("keyUsage = digitalSignature, nonRepudiation, keyEncipherment, " +
> +             " dataEncipherment, keyAgreement, cRLSign\n")
> +CA_full_ku = ("keyUsage = digitalSignature, nonRepudiation, keyEncipherment, " +
> +              " dataEncipherment, keyAgreement, keyCertSign, cRLSign\n")

Super tiny nit: since there's a trailing space in the string on the line above, there's no need for a leading space on this line, right? This also applies to CA_bad_ku.

Comment 15

[Camilo Viecco (:cviecco)]

Comment on attachment 8342650 [details] [diff] [review]
fix-938046-p2 (v2)

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +798,5 @@
> +    return NS_OK;
> +  }
> +  nsCOMPtr<nsIX509Cert> cert;
> +  chain->QueryElementAt(1, NS_GET_IID(nsIX509Cert), getter_AddRefs(cert));
> +  if (cert) {

I dont want to return an error. There could be certificates with no issuer, and besides this would change the semantics of the current function. Before my changes no issuer is considered ok.

Comment 16

[Camilo Viecco (:cviecco)]

Comment on attachment 8342651 [details] [diff] [review]
fix-938046-p3

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +860,5 @@
>         usage = usage << 1) {
> +    if (usage == certificateUsageSSLServer ||
> +        usage == certificateUsageUserCertImport ||
> +        usage == certificateUsageVerifyCA ||
> +        usage == certificateUsageProtectedObjectSigner) {

We could but the list or wanted is bigger than the list of not wanted. (it would be more code to do).

Another choice for the code is to do only one computation here (in each iteration) by the use of some bitwise magic:
const int usagesToAvoid = certificateUsageSSLServer | certificateUsageUserCertImport..
and in the loop:
if (usages & usagesToAvoid !=0) continue

Comment 17

[Camilo Viecco (:cviecco)]

Attachment: fix-938046-p2 (v3)

Comment 18

[Camilo Viecco (:cviecco)]

Attachment: fix-938046-p3 (v2)

Comment 19

[Camilo Viecco (:cviecco)]

Attachment: tests-938046 (v2)

Comment 20

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

I'll review these first thing tomorrow morning.

Comment 21

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

Comment on attachment 8343266 [details] [diff] [review]
fix-938046-p2 (v3)

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

Great!

Comment 22

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

Comment on attachment 8343267 [details] [diff] [review]
fix-938046-p3 (v2)

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

Looks good. r=me with a comment on why we avoid the other usages.

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +855,5 @@
>                                   certificateUsageSSLServer, PR_Now(),
>                                   nullptr, /*XXX fixme*/
>                                   CertVerifier::FLAG_LOCAL_ONLY,
>                                   &pkixNssChain);
> +  const int otherUsagesToTest = certificateUsageSSLClient |

This should have a comment about why we avoid the other usages (like what we talked about on irc).

Comment 23

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

Comment on attachment 8343269 [details] [diff] [review]
tests-938046 (v2)

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

Looking good, but still needs some work. In particular, are there any tests on CA certificates with different key usages?

::: security/manager/ssl/tests/unit/test_getchain.js
@@ +45,5 @@
> +  do_check_eq(expected_issuer_serial, cert.issuer.serialNumber);
> +  let chain = cert.getChain();
> +  let issuer_via_getchain = chain.queryElementAt(1, nsIX509Cert);
> +  // The issuer returned by cert.issuer or cert.getchain should be consistent.
> +  // Assuming a static certificate store

This comment line isn't necessary, I think.

@@ +73,5 @@
> +    load_cert(certList[i], ',,');
> +  }
> +  load_cert("ca-1", ",,");
> +  load_cert("ca-2", ",,");
> +  load_cert("ca-3", ",,");

When is ca-3 used?

@@ +77,5 @@
> +  load_cert("ca-3", ",,");
> +
> +  let ee_cert = certdb.findCertByNickname(null, 'ee');
> +  if (!ee_cert) {
> +    do_check_eq(1, 0);

"do_check_true(ee_cert);" instead of these two lines

@@ +82,5 @@
> +  }
> +  let ca = get_ca_array();
> +
> +  check_getchain(ee_cert, ca[1], ca[2]);
> +  // next: swap ca certs to deal with ensure is due to our db changes

I think I understand what you're saying here, but this comment isn't very clear. Maybe "swap CA certs to ensure trust changes are reflected in the db" or something?

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

We should probably have a license block here - do the other cert generation files have licenses? We should file a bug on that if not.

@@ +18,5 @@
> +CA_basic_constraints = "basicConstraints = critical, CA:TRUE\n"
> +EE_basic_constraints = "basicConstraints = CA:FALSE\n"
> +
> +CA_min_ku = "keyUsage = critical, keyCertSign\n"
> +CA_bad_ku = ("keyUsage = digitalSignature, nonRepudiation, keyEncipherment, " +

These two aren't used - are they supposed to be?

@@ +39,5 @@
> +    f = open(extensions_filename, 'w')
> +    f.write(ext_text)
> +    f.close()
> +    cert_name = dst_dir + "/" + out_prefix + ".der"
> +    os.system ("openssl x509 -req -sha256 -days 3650 -in " + csr_name +

Nit: unnecessary space after "system"

@@ +43,5 @@
> +    os.system ("openssl x509 -req -sha256 -days 3650 -in " + csr_name +
> +               " -signkey " + key_file +
> +               " -set_serial " + str(serial_num) +
> +               " -extfile " + extensions_filename +
> +               " -outform DER -out "+ cert_name)

Tiny nit: space before the "+" here.

@@ +57,5 @@
> +                                      1,
> +                                      key_type,
> +                                      'ca',
> +                                      ca_ext)
> +    CertUtils.generate_cert_generic( db,

Nit: no space needed after the "(" (also 6 lines above)

@@ +68,5 @@
> +                                      ca_cert)
> +
> +    shutil.copy(ca_cert, srcdir + "/" + "ca-1.der")
> +    self_sign_csr(db, srcdir, db + "/ca.csr" , ca_key, 2, ca_ext, "ca-2")
> +    self_sign_csr(db, srcdir, db + "/ca.csr" , ca_key, 3, ca_ext, "ca-3")

Nit: there's an unnecessary space before a "," here and on the line above.
Also, as far as I can tell, this generates 3 identical (except for the serial number) CA certs. Do we want to check for different key usages or something?

@@ +72,5 @@
> +    self_sign_csr(db, srcdir, db + "/ca.csr" , ca_key, 3, ca_ext, "ca-3")
> +    os.remove(ca_cert);
> +
> +    
> +'''    

What's going on here? There's some unnecessary whitespace and it looks like this whole block is wrapped in a string literal or something?

@@ +78,5 @@
> +    ca_key  = 'evroot.key'
> +    prefix =  "ev-valid"
> +    key_type = 'rsa'
> +    ee_ext_text = (EE_basic_constraints + EE_full_ku + Server_eku +
> +                   authority_key_ident + aia_prefix + prefix  +aia_suffix +

Nit: "prefix + aia_suffix"

Comment 24

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

Comment on attachment 8343267 [details] [diff] [review]
fix-938046-p3 (v2)

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

::: security/manager/ssl/src/nsNSSCertificate.cpp
@@ +856,5 @@
>                                   nullptr, /*XXX fixme*/
>                                   CertVerifier::FLAG_LOCAL_ONLY,
>                                   &pkixNssChain);
> +  const int otherUsagesToTest = certificateUsageSSLClient |
> +                                certificateUsageSSLServerWithStepUp |

We should avoid the step-up usage. That is obsolete.

Comment 25

[Camilo Viecco (:cviecco)]

Attachment: tests-938046 (v3)

Comment 26

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

Comment on attachment 8344765 [details] [diff] [review]
tests-938046 (v3)

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

Just a few more nits and questions about the test.

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

this isn't used

@@ +51,5 @@
> +
> +function check_getchain(ee_cert, ssl_ca, email_ca){
> +  // A certificate should first build a chain/issuer to
> +  // a SSL trustdomain, then an EMAIL trust domain and then
> +  // and object signer trustdomain

I don't see the object signer trust domain test...?
Also, s/and/an/, s/trustdomain/trust domain/g

@@ +62,5 @@
> +  check_matching_issuer_and_getchain(ssl_ca.serialNumber, ee_cert);
> +  certdb.setCertTrust(ssl_ca, nsIX509Cert.CA_CERT, 0);
> +  check_matching_issuer_and_getchain(email_ca.serialNumber, ee_cert);
> +  certdb.setCertTrust(email_ca, nsIX509Cert.CA_CERT, 0);
> +  check_matching_issuer_and_getchain(ee_cert.issuer.serialNumber, ee_cert);

What exactly is this last one checking? Is this the object signer test? A comment would be helpful as to why that would be the case, if so.

@@ +67,5 @@
> +}
> +
> +function run_test() {
> +  for (let i = 0 ; i < certList.length; i++) {
> +    let cert_filename = certList[i] + ".der";

this isn't used

@@ +71,5 @@
> +    let cert_filename = certList[i] + ".der";
> +    load_cert(certList[i], ',,');
> +  }
> +  load_cert("ca-1", ",,");
> +  load_cert("ca-2", ",,");

Either put these in the certList or just load all of the certs with load_cert, since there are only three total (but be consistent either way)

@@ +76,5 @@
> +
> +  let ee_cert = certdb.findCertByNickname(null, 'ee');
> +  do_check_false(!ee_cert);
> +
> +  let ca = get_ca_array();

maybe "ca_certs" or "ca_array" or something that indicates it's more than one thing

@@ +82,5 @@
> +  check_getchain(ee_cert, ca[1], ca[2]);
> +  // Swap ca certs to deal alternate trust settings.
> +  check_getchain(ee_cert, ca[2], ca[1]);
> +
> +  run_next_test();

Since everything is synchronous, you don't need to call run_next_test.

::: security/manager/ssl/tests/unit/test_getchain/generate.py
@@ +26,5 @@
> +              "dataEncipherment, keyAgreement, keyCertSign, cRLSign\n")
> +
> +CA_eku = ("extendedKeyUsage = critical, serverAuth, clientAuth, " +
> +          "emailProtection, codeSigning, 1.3.6.1.5.5.7.3.9\n")
> +EE_full_eku = ("extendedKeyUsage = critical, serverAuth, clientAuth, " +

This still isn't used

@@ +52,5 @@
> +def generate_certs():
> +    key_type = 'rsa'
> +    ca_ext = CA_basic_constraints + CA_full_ku + subject_key_ident + CA_eku;
> +    ee_ext_text = (EE_basic_constraints + authority_key_ident)
> +    [ca_key, ca_cert] = CertUtils.generate_cert_generic( db,

nit: unnecessary space before db

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

nit: unnecessary space before ","

Comment 27

[Camilo Viecco (:cviecco)]

Comment on attachment 8344765 [details] [diff] [review]
tests-938046 (v3)

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

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

This actually used by getCerts, changing it there to reflect this.

@@ +51,5 @@
> +
> +function check_getchain(ee_cert, ssl_ca, email_ca){
> +  // A certificate should first build a chain/issuer to
> +  // a SSL trustdomain, then an EMAIL trust domain and then
> +  // and object signer trustdomain

Was removed, we dont actually need it. (ca-3) But this comment should reflect this reality

@@ +62,5 @@
> +  check_matching_issuer_and_getchain(ssl_ca.serialNumber, ee_cert);
> +  certdb.setCertTrust(ssl_ca, nsIX509Cert.CA_CERT, 0);
> +  check_matching_issuer_and_getchain(email_ca.serialNumber, ee_cert);
> +  certdb.setCertTrust(email_ca, nsIX509Cert.CA_CERT, 0);
> +  check_matching_issuer_and_getchain(ee_cert.issuer.serialNumber, ee_cert);

In the case of no trust, the system will build a chain up to the highest part of the chain. What chain is built is not defined but results between issuer and getchain should be consistent.

Comment 28

[Camilo Viecco (:cviecco)]

Attachment: tests-938046 (v4)

Comment 29

[Camilo Viecco (:cviecco)]

Attachment: tests-938046 (v4.1)

Comment 30

[Camilo Viecco (:cviecco)]

Attachment: tests-938046 (v4.2)

Comment 31

[Camilo Viecco (:cviecco)]

Attachment: tests-938046 (v4.3)

Comment 32

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

Comment on attachment 8344908 [details] [diff] [review]
tests-938046 (v4.3)

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

Looks good. r=me with the comment fixed (see below).

::: security/manager/ssl/tests/unit/test_getchain.js
@@ +53,5 @@
> +
> +function check_getchain(ee_cert, ssl_ca, email_ca){
> +  // A certificate should first build a chain/issuer to
> +  // a SSL trustdomain, then an EMAIL trust domain and then
> +  // and object signer trustdomain

nit: we should be consistent about whether the term is "trustdomain" or "trust domain"
Also, there's still the reference to the object signer here.

@@ +66,5 @@
> +  check_matching_issuer_and_getchain(email_ca.serialNumber, ee_cert);
> +  certdb.setCertTrust(email_ca, nsIX509Cert.CA_CERT, 0);
> +  // Do a final test on the case of no trust. The results must
> +  // be cosistent (the actual value is non-deterministic).
> +  check_matching_issuer_and_getchain(ee_cert.issuer.serialNumber, ee_cert);

My one concern is that if there is no trusted issuer, ee_cert.issuer/issuer_via_getchain could be null, right? (It looks like this isn't the case currently, but the implementation could change when our verification library changes, I imagine.) Anyway, we'll know if that happens, so I don't think it's a problem right now.

Comment 33

[Camilo Viecco (:cviecco)]

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

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d91fa2402ecd
https://hg.mozilla.org/mozilla-central/rev/7a9882dd62ac
https://hg.mozilla.org/mozilla-central/rev/080cd15c7c90
https://hg.mozilla.org/mozilla-central/rev/cde32f9a08c8