Closed Bug 938046 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: briansmith, Assigned: cviecco)

References

Details

Attachments

(4 files, 10 obsolete files)

1.14 KB, patch
keeler
: review+
Details | Diff | Splinter Review
1.60 KB, patch
keeler
: review+
Details | Diff | Splinter Review
1.77 KB, patch
keeler
: review+
Details | Diff | Splinter Review
11.05 KB, patch
keeler
: review+
Details | Diff | Splinter Review
+++ 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.
Attached patch fix-938046 part1Splinter Review
Attachment #831637 - Flags: review?(dkeeler)
Attached patch fix-938046-p2 (obsolete) — Splinter Review
Can you double check my memory management here?
Attachment #831640 - Flags: feedback?(dkeeler)
Attached patch fix-938046-p2 (v1.1) (obsolete) — Splinter Review
Can you double check addrefs.
Attachment #831640 - Attachment is obsolete: true
Attachment #831640 - Flags: feedback?(dkeeler)
Attachment #831641 - Flags: feedback?(dkeeler)
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
Attachment #831637 - Flags: review?(dkeeler) → review+
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.
Attachment #831641 - Flags: feedback?(dkeeler) → feedback+
Attached patch fix-938046-p2 (v2) (obsolete) — Splinter Review
Attachment #831641 - Attachment is obsolete: true
Attached patch fix-938046-p3 (obsolete) — Splinter Review
Attached patch tests-938046 (obsolete) — Splinter Review
Attachment #8342650 - Flags: review?(dkeeler)
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
Attachment #8342651 - Flags: review?(dkeeler)
Comment on attachment 8342656 [details] [diff] [review]
tests-938046

Or do you prefer to put them in add_tests at the end?
Attachment #8342656 - Flags: feedback?(dkeeler)
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.
Attachment #8342650 - Flags: review?(dkeeler) → review+
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.
Attachment #8342650 - Flags: review+ → review-
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?
Attachment #8342651 - Flags: review?(dkeeler) → review-
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.
Attachment #8342656 - Flags: feedback?(dkeeler) → feedback+
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 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
Attachment #8342650 - Attachment is obsolete: true
Attachment #8343266 - Flags: review?(dkeeler)
Attachment #8342651 - Attachment is obsolete: true
Attachment #8343267 - Flags: review?(dkeeler)
Attached patch tests-938046 (v2) (obsolete) — Splinter Review
Attachment #8342656 - Attachment is obsolete: true
Attachment #8343269 - Flags: review?(dkeeler)
I'll review these first thing tomorrow morning.
Comment on attachment 8343266 [details] [diff] [review]
fix-938046-p2 (v3)

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

Great!
Attachment #8343266 - Flags: review?(dkeeler) → review+
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).
Attachment #8343267 - Flags: review?(dkeeler) → review+
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"
Attachment #8343269 - Flags: review?(dkeeler) → review-
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.
Attached patch tests-938046 (v3) (obsolete) — Splinter Review
Attachment #8343269 - Attachment is obsolete: true
Attachment #8344765 - Flags: review?(dkeeler)
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 ","
Attachment #8344765 - Flags: review?(dkeeler) → review-
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.
Attached patch tests-938046 (v4) (obsolete) — Splinter Review
Attachment #8344765 - Attachment is obsolete: true
Attached patch tests-938046 (v4.1) (obsolete) — Splinter Review
Attachment #8344898 - Attachment is obsolete: true
Attached patch tests-938046 (v4.2) (obsolete) — Splinter Review
Attachment #8344899 - Attachment is obsolete: true
Attachment #8344901 - Attachment is obsolete: true
Attachment #8344908 - Flags: review?(dkeeler)
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.
Attachment #8344908 - Flags: review?(dkeeler) → review+
You need to log in before you can comment on or make changes to this bug.