Closed Bug 877376 Opened 7 years ago Closed 6 years ago

add tests for correct handling of intermediate certificates (basic constraints)

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: cviecco, Assigned: cviecco)

References

Details

Attachments

(1 file, 5 obsolete files)

There are no PSM layer tests of the correct handling of intermediate certificates. As first stage we shall cover basic constrants and maybe key usage and extended key usages.
Attached patch intermediates-tests (v1) (obsolete) — Splinter Review
assumes psm_head.js has landed.
Attachment #756743 - Flags: feedback?(dkeeler)
Comment on attachment 756743 [details] [diff] [review]
intermediates-tests (v1)

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

Looks good - just a few comments.

::: security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints.js
@@ +1,1 @@
> +"use strict";

I think we need a license header here

@@ +1,2 @@
> +"use strict";
> +/* To regenerate the certificates for this test:

commonly these block comments have an asterisk as the second character of each line to visually indicate that it's one big long comment

@@ +22,5 @@
> +
> +var ca_usage1= 'Client,Server,Sign,Encrypt,SSL CA,Status Responder';
> +var ee_usage1= 'Client,Server,Sign,Encrypt';
> +//new:
> +//var ca_usage1= 'Client,Server,Sign,Encrypt,Object Signer,SSL CA,Status Responder';

are these commented-out lines necessary?

@@ +38,5 @@
> + dict['int-limited-depth-invalid']= ca_usage1;
> + dict['ee-int-limited-depth-invalid']= "";
> +
> + dict['ee-int-valid-ku-no-eku'] = ee_usage1;
> + dict['int-valid-ku-no-eku'] = 'SSL CA';

You can initialize dict inline:
var dict = {
  ee-int-no-extensions: "",
  int-no-extensions: ee_usage1,
  ... etc...
};

@@ +48,5 @@
> +  var ca_filename = ca_name + ".der";
> +  var root_cert_der =
> +      do_get_file("test_intermediate_basic_usage_constraints/" + ca_filename, false);
> +  var der = readFile(root_cert_der);
> +  certdb.addCert(der, "CTu,CTu,CTu", ca_name);

now that head_psm.js has landed, we have the utility function to do this

@@ +52,5 @@
> +  certdb.addCert(der, "CTu,CTu,CTu", ca_name);
> +
> +  do_print("ca_name=" + ca_name);
> +  var cert;
> +  cert = certdb.findCertByNickname(null, ca_name);

I believe addCert completely disregards ca_name, so this is unlikely to work properly

@@ +59,5 @@
> +  var usages = {};
> +  cert.getUsagesString(true, verified, usages);
> +  do_print("usages.value=" + usages.value);
> +  do_check_eq(ca_usage1, usages.value)
> +  

whitespace

@@ +60,5 @@
> +  cert.getUsagesString(true, verified, usages);
> +  do_print("usages.value=" + usages.value);
> +  do_check_eq(ca_usage1, usages.value)
> +  
> +  ////load certs first

inline comments are generally just two slashes followed by a space followed by the comment

@@ +71,5 @@
> +    var der = readFile(cert_der);
> +    certdb.addCert(der, ',,', cert_name);
> +  }
> +
> +  //now do the checks 

whitespace

@@ +86,5 @@
> +    do_print("usages.value=" + usages.value);
> +    var value = dict[key];
> +
> +    do_check_eq(value, usages.value); 
> +  }  

whitespace here and the line above

::: security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints/generate.pl
@@ +1,1 @@
> +#!/usr/bin/perl

Is there any way I could convince you to not use perl? It's my impression that not many contributors know it that well (this includes me), and there are other languages that might be more widely known. You could even just write a shell script, since you're essentially only issuing a series of openssl commands.

@@ +8,5 @@
> +use strict;
> +
> +my $srcdir=getcwd();
> +my $db = tempdir( CLEANUP => 1 );
> +#my $db="/tmp/foo";

remove commented-out line?

@@ +36,5 @@
> +  #gen key
> +  my $key_name="$db/$name.key";
> +  dsystem("openssl genpkey -algorithm RSA -out $key_name -pkeyopt rsa_keygen_bits:2048");
> +  my $csr_name="$db/$name.csr";
> +  dsystem("openssl req -new -key $key_name -days 3650 -extensions v3_ca -batch -out $csr_name -utf8 -subj '/CN=$name'");

Only 10 years? Maybe go for 30 or 40...

@@ +37,5 @@
> +  my $key_name="$db/$name.key";
> +  dsystem("openssl genpkey -algorithm RSA -out $key_name -pkeyopt rsa_keygen_bits:2048");
> +  my $csr_name="$db/$name.csr";
> +  dsystem("openssl req -new -key $key_name -days 3650 -extensions v3_ca -batch -out $csr_name -utf8 -subj '/CN=$name'");
> +  

whitespace

@@ +52,5 @@
> +  }
> +  else {
> +      dsystem("openssl x509 -req -days 3650 -in $csr_name -CAkey $signer_key_filename -CA $signer_cert_filename -CAform DER  -set_serial $serial -out $cert_name -outform DER  -extfile $extensions_filename ")
> +  }
> +  $serial=$serial+1;  

whitespace

@@ +68,5 @@
> +
> +sub generate_certs(){
> +  my $ca_name = "ca";
> +  my ($ca_key, $ca_cert) = generate_cert($ca_name,$CA_basic_constraints);
> + 

whitespace

@@ +74,5 @@
> +
> +  #now the intermediates
> +  generate_int_and_ee($ca_key, $ca_cert,"int-no-extensions","",$ee_ext_text);
> +  generate_int_and_ee($ca_key, $ca_cert,"int-not-a-ca",$EE_basic_constraints,$ee_ext_text);
> +  

whitespace

@@ +79,5 @@
> +  my ($int_key1,$int_cert1) = generate_int_and_ee($ca_key, $ca_cert,"int-limited-depth",$CA_limited_basic_constraints,$ee_ext_text);
> +  #and a child using this one
> +  generate_int_and_ee($int_key1, $int_cert1,"int-limited-depth-invalid",$CA_basic_constraints,$ee_ext_text);
> +
> +  #now we do it again for valid basic constraints but strange eku/ku at the intermediate layer 

whitespace
Attachment #756743 - Flags: feedback?(dkeeler) → feedback+
Attached patch intermediates-tests (v2) (obsolete) — Splinter Review
Attachment #756743 - Attachment is obsolete: true
Attached patch intermediates-tests (v2.1) (obsolete) — Splinter Review
Attachment #758628 - Attachment is obsolete: true
Attached patch intermediates-tests (v2.2) (obsolete) — Splinter Review
Attachment #758630 - Attachment is obsolete: true
Attachment #758631 - Flags: review?(bsmith)
Assignee: nobody → cviecco
Comment on attachment 758631 [details] [diff] [review]
intermediates-tests (v2.2)

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

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

Not being up on certificate constraints, it would be nice to have some human-readable comments here :)

@@ +73,5 @@
> +    addCertFromFile(certdb,"test_intermediate_basic_usage_constraints/" + cert_filename,',,');
> +  }
> +
> +  // Now do the checks
> +  for (var key in dict) {

I believe this var key has the same scope as the one above, thanks to JS non-intuitive scoping rules. In this case it doesn't matter, but for (let key in dict) will be block scoped.

@@ +77,5 @@
> +  for (var key in dict) {
> +    var cert_name = key;
> +    do_print("cert_name=" + cert_name);
> +
> +    var cert;

same here. You probably want to replace most of these vars with let, and optionally move the var declarations up top to avoid confusion.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let
Comment on attachment 758631 [details] [diff] [review]
intermediates-tests (v2.2)

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

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

this is unnecessary since you redeclare it below

@@ +82,5 @@
> +    cert = certdb.findCertByNickname(null, cert_name);
> +
> +    var verified = {};
> +    var usages = {};
> +    cert.getUsagesString(true, verified, usages);

It looks like verified is never used. Is it meaningful to check this against the verification result?

https://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/public/nsIX509Cert.idl#207
Depends on: 895601
Attached patch intermediate-tests2 (obsolete) — Splinter Review
Attachment #758631 - Attachment is obsolete: true
Attachment #758631 - Flags: review?(brian)
Attachment #791496 - Flags: review?(brian)
Comment on attachment 791496 [details] [diff] [review]
intermediate-tests2

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

::: security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints/generate.py
@@ +16,5 @@
> +EE_basic_constraints = "basicConstraints=CA:FALSE\n"
> +
> +CA_min_ku = "keyUsage=critical, keyCertSign\n"
> +CA_bad_ku = "keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, cRLSign\n"
> +EE_full_ku ="keyUsage=digitalSignature, nonRepudiation, keyEncipherment, dataEncipherment, keyAgreement, keyCertSign, cRLSign\n"

keyCertSign isn't a valid end-entity key usage and this gets rejected in insanity::pkix. I suggest that we remove keyCertSign from this, and I also suggest we add a test for this to test_certificate_usages

@@ +30,5 @@
> +                                      ca_cert,
> +                                      name,
> +                                      int_ext_text,
> +                                      ee_ext_text,
> +                                      key_type) 

whitespace.
Comment on attachment 791496 [details] [diff] [review]
intermediate-tests2

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

::: security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints.js
@@ +27,5 @@
> +
> +  'int-limited-depth' : ca_usage1,
> +  'ee-int-limited-depth' : ee_usage1,
> +
> +  'int-limited-depth-invalid' : ca_usage1,

NSS classic cert verification gives the wrong result for this. 'SSL CA' is wrong because the cert will never be trusted as an SSL CA certificate due to the path length constraint violation when it is actually used as an intermediate CA cert. Please add a comment to this effect.

insanity::pkix gets this right.
Comment on attachment 791496 [details] [diff] [review]
intermediate-tests2

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

See older review comments.

Please make sure the script to generate the certs runs on Windows with the MozillaBuild toolchain.
Attachment #791496 - Flags: review?(brian) → review-
(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #12)
> Please make sure the script to generate the certs runs on Windows with the
> MozillaBuild toolchain.

Note: it is OK if I (or anybody else) has to download and install a newer OpenSSL for the scripts to work on Windows. Just specify which version of OpenSSL.
Attachment #791496 - Attachment is obsolete: true
Attachment #8344881 - Flags: review?(brian)
Comment on attachment 8344881 [details] [diff] [review]
intermediate-tests2 (v2)

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

Note that I refactored this and addressed some nits in my patch in bug 971178.
Attachment #8344881 - Flags: review?(brian) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ccbdd0430f2
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla30
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/1ccbdd0430f2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.