969188 - Test and verify behaviour of v1 and v2 x509 certificates

Status: RESOLVED FIXED

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

Description

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

See David's review comment in bug 921892. We should test that we treat v1 roots as isCA==true and other v1 certificates as isCA==false.

Comment 1

[Camilo Viecco (:cviecco)]

But only if they are a trusted CA?

Comment 2

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

(In reply to Camilo Viecco (:cviecco) from comment #1)
> But only if they are a trusted CA?

We should test multiple cases:
1. Is a trust anchor without basic constraints.
2. Is an intermediate without basic constraints that isn't a trust anchor.
3. Has basic constraints (v1 + basic constraints -> faliure, since it doesn't make sense, AFAICT).
4. v1 certificate w/o basic constraints being validated as an end-entity -> failure.
5. Any v2 certificate -> failure.

Comment 3

[Camilo Viecco (:cviecco)]

Attachment: add-certversion-to-certutil

Comment 4

[Camilo Viecco (:cviecco)]

Attachment: test-v1-certs

There a a lot of typos but I wanted you to have some say on the coverage. (probably the generation of certs will not work on windows by default as I use pexpect)

Comment 5

[Camilo Viecco (:cviecco)]

The good news: The current code does enforce base constraints no matter what version
bad news:
1. The current code does not ensure that the certifcates are formatted correctly: v1 and v2 intermediates with base constraints (a v3 extension) are considered OK.
2. We do no enforce that the an issued cert must be of verion equal or higher than the signer version. This a v3 intermediate can issue a v1 cert and we are OK with that.

Comment 6

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

Comment on attachment 8380016 [details] [diff] [review]
test-v1-certs

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

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +11,5 @@
> +
> +function constructCertFromFile(filename) {
> +  let der = readFile(do_get_file("test_cert_version/" + filename, false));
> +  return certdb.constructX509(der, der.length);
> +}

This function should be added to head_psm.js. I have written a patch that does that already.

@@ +67,5 @@
> +  check_fail(constructCertFromFile('v3_bc_ee-v1_int-v1_ca.der'));
> +
> +  //Next block camilo is so sad: a v1 intermediate with basic constriants
> +  // Which makes no sense at all is OK.  Further there is no checks 
> +  // on the  children

Please clarify what you mean by "Further theire is no checks on the children."

@@ +68,5 @@
> +
> +  //Next block camilo is so sad: a v1 intermediate with basic constriants
> +  // Which makes no sense at all is OK.  Further there is no checks 
> +  // on the  children
> +  check_ok_ca(constructCertFromFile('v1_int_bc-v1_ca.der'));  //XXX wtf a v1 cert doest not have bc

Let's not put "WTF" in the test suite.

Please file bugs for the issues that you think need to be fixed and reference those bugs with "XXX(bug ######)" or "TODO(bug ######)."

@@ +105,5 @@
> +  check_fail(constructCertFromFile('v2_bc_ee-v3_int_missing_bc-v1_ca.der'));
> +  check_fail(constructCertFromFile('v3_missing_bc-v3_int_missing_bc-v1_ca.der'));
> +  check_fail(constructCertFromFile('v3_bc_ee-v3_int_missing_bc-v1_ca.der'));
> +
> +  // This is kind of OK (issue with the v3 ee with no bc)

See note [1] belw.

@@ +163,5 @@
> +  check_fail(constructCertFromFile('v2_bc_ee-v3_int_missing_bc-v1_ca_bc.der'));
> +  check_fail(constructCertFromFile('v3_missing_bc-v3_int_missing_bc-v1_ca_bc.der'));
> +  check_fail(constructCertFromFile('v3_bc_ee-v3_int_missing_bc-v1_ca_bc.der'));
> +
> +  // Mostly OK (v3 missing bc is ok as EE)

[1] What is the difference between this section and the section for v3 with missing bc above?

::: security/nss/cmd/certutil/certutil.c
@@ +1736,5 @@
>  static SECStatus
>  SignCert(CERTCertDBHandle *handle, CERTCertificate *cert, PRBool selfsign, 
>           SECOidTag hashAlgTag,
> +         SECKEYPrivateKey *privKey, char *issuerNickName,
> +         int certVersion, void *pwarg)

Please move the patch to certutil.c to a new bug in the NSS product and ask me to review it.

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: test-version-patches

Comment 8

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

Comment on attachment 8383345 [details] [diff] [review]
test-version-patches

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

I didn't review the entire patch yet because I think the SEC_ERROR_BAD_DER stuff is indicative of a bug of insanity::pkix that we should fix here.

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +61,5 @@
> +
> +function enableDERError(useInsanity) {
> +  if (useInsanity) {
> +    ca_fail_value = SEC_ERROR_BAD_DER;
> +    ee_fail_value = SEC_ERROR_BAD_DER;

Please file a follow-up bug for improving the error code here. We should have a SEC_ERROR_BAD_CERT_VERSION error, but NSS doesn't have such an error code right now. SEC_ERROR_BAD_DER is a bad error to return.

@@ +80,5 @@
> +    ca_fail_value = SEC_ERROR_INADEQUATE_CERT_TYPE;
> +    ee_fail_value = SEC_ERROR_UNKNOWN_ISSUER;
> +  }
> +}
> +

nit: extra blank line.

@@ +82,5 @@
> +  }
> +}
> +
> +
> +function tests_in_mode(useInsanity)

NIt: Please name it run_tests_in_mode to be consistent with the other tests.

@@ +104,5 @@
> +  check_fail(constructCertFromFile('v3_missing_bc-v1_int-v1_ca.der'));
> +  check_fail(constructCertFromFile('v3_bc_ee-v1_int-v1_ca.der'));
> +  check_fail(constructCertFromFile('v4_bc_ee-v1_int-v1_ca.der'));
> +
> +  // Blocks with  enableDERError are handled correctly in insanity

nit extra space.

@@ +116,5 @@
> +  check_fail(constructCertFromFile('v2-v1_int_bc-v1_ca.der'));
> +  check_fail(constructCertFromFile('v2_bc_ee-v1_int_bc-v1_ca.der'));
> +  check_fail(constructCertFromFile('v3_missing_bc-v1_int_bc-v1_ca.der'));
> +  check_fail(constructCertFromFile('v3_bc_ee-v1_int_bc-v1_ca.der'));
> +  check_fail(constructCertFromFile('v4_bc_ee-v1_int_bc-v1_ca.der'));

Why should these fail with SEC_ERROR_BAD_DER? It seems like this is a bug in insanity::pkix, even if we ignore the issue that we don't have a SEC_ERROR_BAD_CERT_VERSION error code to return.

Maybe the problem is that we need to change the code in CheckBasicConstraints to something like this:

    if (endEntityOrCA == MustBeCA && isTrustAnchor) {
      const CERTCertificate* nssCert = cert.GetNSSCert();
      // We only allow trust anchor CA certs to omit the
      // basicConstraints extension if they are v1. v1 is encoded
      // implicitly. 
      if (!nssCert->version.data && !nssCert->version.len) {
        basicConstraints.isCA = true;
        basicConstraints.pathLenConstraint = CERT_UNLIMITED_PATH_CONSTRAINT;
      }
    }

Note in particular that the comparison in the existing code (version == 1) is wrong. It should be (version == der::v1) since der::v1 is 0, not 1.

Comment 9

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

http://osvdb.org/show/osvdb/103309

Comment 10

[Camilo Viecco (:cviecco)]

Attachment: instanity-only-decode-extensions-for-v3certs

Comment 11

[Camilo Viecco (:cviecco)]

Attachment: fix-insanity-basic-constraints-check-v1-certs

Comment 12

[Camilo Viecco (:cviecco)]

Attachment: test-version-patches (v2)

Comment 13

[Camilo Viecco (:cviecco)]

Attachment: test-version-patches (v2.1)

Comment 14

[Camilo Viecco (:cviecco)]

Attachment: test-version-patches (v2.2)

Comment 15

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

Comment on attachment 8386193 [details] [diff] [review]
instanity-only-decode-extensions-for-v3certs

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

::: security/insanity/lib/pkixbuild.cpp
@@ +36,5 @@
> +  // We only decode v3 extensions for v3 certificates
> +  if (nssCert->version.len != 1 ||
> +      nssCert->version.data[0] != SEC_CERTIFICATE_VERSION_3) {
> +    return Success;
> +  }

1. pkixder.h defines insanity::der::v3 which would be better than SEC_CERTIFICATE_VERSION_3.

2. The check is misguided. If the extensions exist, they generally attempt restrict the validity of the certificate. So, if we skip processing the extensions then we're at risk of accepting certificates that are explicitly trying to restrict themselves. That seems backwards.

A better check would be to fail if any extensions are encountered for a version LESS THAN v3.

Comment 16

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

Comment on attachment 8386246 [details] [diff] [review]
test-version-patches (v2.2)

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

I didn't review the specifics of the test. I think it is something that would benefit from having a live discussion to you can better explain what each case (or set of cases) is testing. Please work with David on that.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +21,5 @@
>  const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;
>  const SSL_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SSL_ERROR_BASE;
>  
>  // Sort in numerical order
> +const SEC_ERROR_BAD_DER                                 = SEC_ERROR_BASE +   9; // -8183

You can remove this now.

@@ +26,4 @@
>  const SEC_ERROR_EXPIRED_CERTIFICATE                     = SEC_ERROR_BASE +  11;
>  const SEC_ERROR_REVOKED_CERTIFICATE                     = SEC_ERROR_BASE +  12; // -8180
>  const SEC_ERROR_UNKNOWN_ISSUER                          = SEC_ERROR_BASE +  13;
> +

Remove this blank line.

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +74,5 @@
>  # Bug 676972: test hangs consistently on Android
>  skip-if = os == "android"
> +[test_cert_version.js]
> +# Bug 676972: test hangs consistently on Android
> +skip-if = os == "android"

Are you sure you need to skip this test on Android? We've got almost all the tests working and the ones that aren't working are the ones that are testing OCSP. AFAICT, this one should work, so we shouldn't skip it on Android. Also, usually if we need to skip a test on Android we'd need to skip it on B2G too.

@@ +79,3 @@
>  
>  
> +

Remove this extra blank line.

Comment 17

[Camilo Viecco (:cviecco)]

Comment on attachment 8386193 [details] [diff] [review]
instanity-only-decode-extensions-for-v3certs

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

::: security/insanity/lib/pkixbuild.cpp
@@ +36,5 @@
> +  // We only decode v3 extensions for v3 certificates
> +  if (nssCert->version.len != 1 ||
> +      nssCert->version.data[0] != SEC_CERTIFICATE_VERSION_3) {
> +    return Success;
> +  }

I changed it to fail if extensions are found for non v3 certs.

Comment 18

[Camilo Viecco (:cviecco)]

Attachment: only-decode-for-v3-certs (v2)

Comment 19

[Camilo Viecco (:cviecco)]

Comment on attachment 8389331 [details] [diff] [review]
only-decode-for-v3-certs (v2)

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

I should combine move the isv3cert.

Comment 20

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

Comment on attachment 8389331 [details] [diff] [review]
only-decode-for-v3-certs (v2)

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

::: security/insanity/lib/pkixbuild.cpp
@@ +32,5 @@
>  // that the decoding doesn't have to happen more than once per cert.
>  Result
>  BackCert::Init()
>  {
> +  // We only decode v3 extensions for v3 certificates

Please add an explanation of *why* we're doing this. Is it purely because it doesn't make sense for a v1 cert to have v3 extensions, or is the correctness of some part of the rest of the code dependent on this? If the latter, please explain that.

@@ +35,5 @@
>  {
> +  // We only decode v3 extensions for v3 certificates
> +  bool isv3Cert = false;
> +  if (nssCert->version.len == 1 &&
> +      nssCert->version.data[0] == SEC_CERTIFICATE_VERSION_3) {

Use insanity::der:v3 (or insanity::der::Version::v3).

@@ +44,5 @@
>    if (!exts) {
>      return Success;
>    }
> +  if (!isv3Cert) {
> +    return Fail(RecoverableError, SEC_ERROR_CERT_NOT_VALID);

Use SEC_ERROR_EXTENSION_VALUE_INVALID. SEC_ERROR_EXTENSION_VALUE_INVALID is already used in another place in this function. All the other functions (e.g. in pkixcheck) generally use one error code for the whole function. This makes it easier to see which function the problem comes from.

Comment 21

[Camilo Viecco (:cviecco)]

Attachment: only-decode-for-v3-certs (v3)

Keeping briansmith's r+

Comment 22

[Camilo Viecco (:cviecco)]

Attachment: test-version-patches (v3)

Comment 23

[Camilo Viecco (:cviecco)]

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

Comment 24

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

Comment on attachment 8390069 [details] [diff] [review]
only-decode-for-v3-certs (v3)

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

::: security/insanity/lib/pkixbuild.cpp
@@ +41,5 @@
> +  // 1. They make no sense in non-v3 certs
> +  // 2. An invalid cert can embed a basic constraints extension and the
> +  //    check basic constrains will asume that this is valid. Making it
> +  //    posible to create chains with v1 and v2 intermediates with is
> +  //    not desirable.

I don't understand #2.

I understand that part about us not wanting to allow v1 and v2 intermediates but I don't understand what you mean by the first sentence. I suggest just removing the first sentence.

Also, remove "with" in "with is not desirable".

Maybe the whole comment should just be replaced by "x509 before v3 did not have extensions, so a v1 cert with extensions does not make sense. In particular, we want to prohibit v1 and v2 certs from having basic constraints with isCA=true."

Also, please add a comment to CheckBasicConstraints pointing out that v1 and v2 certificates are prohibited from having the basic constraints extension during extension parsing.

@@ +43,5 @@
> +  //    check basic constrains will asume that this is valid. Making it
> +  //    posible to create chains with v1 and v2 intermediates with is
> +  //    not desirable.
> +  if (! (nssCert->version.len == 1 &&
> +      nssCert->version.data[0] == insanity::der::Version::v3)) {

nit: no space after "!"
nit: indention of second line.

nit: After receiving Stefan's comments on the DER decoder logic, I suggest writing this in terms of der::OptionalVersion. He made some good points about how we're doing this stuff and we might as well do it right now.

Comment 25

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

Comment on attachment 8390074 [details] [diff] [review]
test-version-patches (v3)

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

This is good. However, I'm concerned about the way the expected error is indicated - see my comments in test_cert_version.js on that. Also, generate.py needs a little cleanup. Also also, watch out for having multiple blank lines where one will do.

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +36,5 @@
> +function check_ca_err(cert, expected_error) {
> +  check_cert_err_generic(cert, expected_error, certificateUsageSSLCA)
> +}
> +
> +

nit: unnecessary blank line

@@ +37,5 @@
> +  check_cert_err_generic(cert, expected_error, certificateUsageSSLCA)
> +}
> +
> +
> +function check_ok(x) {

nit: cert instead of x?

@@ +41,5 @@
> +function check_ok(x) {
> +  return check_cert_err(x, 0);
> +}
> +
> +function check_ok_ca (x) {

nit: space after check_ok_ca

@@ +45,5 @@
> +function check_ok_ca (x) {
> +  return check_cert_err_generic(x, 0, certificateUsageSSLCA);
> +}
> +
> +let ee_fail_value = SEC_ERROR_UNKNOWN_ISSUER;

Let's find another way to do this.

@@ +51,5 @@
> +function check_fail(x) {
> +  return check_cert_err(x, ee_fail_value);
> +}
> +
> +let ca_fail_value = SEC_ERROR_INADEQUATE_CERT_TYPE;

Again, I don't think this is the best way to do this.

@@ +57,5 @@
> +function check_fail_ca(x) {
> +  return check_cert_err_generic(x, ca_fail_value, certificateUsageSSLCA);
> +}
> +
> +

nit: unnecessary blank line

@@ +64,5 @@
> +    ca_fail_value = SEC_ERROR_CA_CERT_INVALID;
> +    ee_fail_value = SEC_ERROR_CA_CERT_INVALID;
> +  }
> +  else{
> +    // Classic NSS incorrectly asserts this section as OK

Please file a bug and reference it here (it shouldn't block this landing, but it would be good to be at least tracking it).

@@ +76,5 @@
> +    ca_fail_value = SEC_ERROR_EXTENSION_VALUE_INVALID;
> +    ee_fail_value = SEC_ERROR_EXTENSION_VALUE_INVALID;
> +  }
> +  else{
> +    // Classic NSS incorrectly asserts this section as OK

Another bug, please.

@@ +82,5 @@
> +    ee_fail_value = 0;
> +  }
> +}
> +
> +

nit: blank line

@@ +94,5 @@
> +    ee_fail_value = SEC_ERROR_UNKNOWN_ISSUER;
> +  }
> +}
> +
> +function enableInvalidCertError(useInsanity) {

Please stick to either mixedCase or using_underscores (preferably the former), but don't mix them.

@@ +106,5 @@
> +  }
> +}
> +
> +
> +

nit: two unnecessary blank lines

@@ +115,5 @@
> +
> +  // All blocks that start with enableInvalidError have the same
> +  // effect in practice. However insanity provides a more appropiate
> +  // error value
> +  // Blocks with  disableErrorInClassic are handled more strictly in insanity

nit: extra space before disableErrorInClassic

@@ +124,5 @@
> +  // encodings), v2 certs are not considered CAs in any position.
> +  // Note that currently there are no change of behavior based on the
> +  // version of the end entity.
> +
> +

nit: blank line

@@ +131,5 @@
> +  //////////////////
> +
> +  // v1 intermediate with v1 trust anchor
> +  enableInvalidError(useInsanity);
> +  check_fail_ca(constructCertFromFile('v1_int-v1_ca.der'));

let's do something like this:

let expected_ca_error = get_expected_ca_error(useInsanity);
let expected_ee_error = get_expected_ee_error(useInsanity);
check_fail_ca(..., expected_ca_error);
check_fail(..., expected_ee_error);
check_fail_ca(..., expected_ca_error);
check_fail(..., expected_ee_error);
...

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

nit: modeline

@@ +9,5 @@
> +libpath = os.path.abspath('../psm_common_py')
> +
> +sys.path.append(libpath)
> +
> +import CertUtils

this doesn't appear to be used - why not? Let's remove the import if we don't need it, or make use of it (since I'm guessing it could make this code simpler?)

@@ +17,5 @@
> +
> +def init_nss_db(db_dir):
> +  #create noise file
> +  noise_file = db_dir + "/noise";
> +  os.system("dd if=/dev/urandom bs=512 count=1 of=" + noise_file)

dd isn't available on windows, iirc. I think the noise file just needs to be unique, so you can use the current time.

@@ +22,5 @@
> +  #create pwd file
> +  pwd_file = db_dir + "/pwfile";
> +  os.system("echo > "+ pwd_file)
> +  #create nss db
> +  os.system("certutil -d "+ db_dir + " -N -f "+ pwd_file);

nit: spaces around +

@@ +28,5 @@
> +  return [noise_file, pwd_file]
> +
> +def generate_ca_cert(db_dir, dest_dir, noise_file,  name, version, do_bc):
> +  out_name = dest_dir + "/" + name + ".der"
> +  if (not do_bc):

I think 'if do_bc: ... else: ...' is more clear

@@ +31,5 @@
> +  out_name = dest_dir + "/" + name + ".der"
> +  if (not do_bc):
> +    os.system("certutil -S -z " + noise_file + " -g 2048 -d "+ db_dir +"/ -n " +
> +              name +" -v 120 -s 'CN=" + name +
> +              ",O=BOGUS PSM,L=Mountain View,ST=California,C=US'" +

maybe 'O=PSM Testing' or something?

@@ +32,5 @@
> +  if (not do_bc):
> +    os.system("certutil -S -z " + noise_file + " -g 2048 -d "+ db_dir +"/ -n " +
> +              name +" -v 120 -s 'CN=" + name +
> +              ",O=BOGUS PSM,L=Mountain View,ST=California,C=US'" +
> +              " -t C,C,C -x --certVersion="+ str(int(version)))

will string concatenation not coerce version to a string here (and elsewhere)?

@@ +33,5 @@
> +    os.system("certutil -S -z " + noise_file + " -g 2048 -d "+ db_dir +"/ -n " +
> +              name +" -v 120 -s 'CN=" + name +
> +              ",O=BOGUS PSM,L=Mountain View,ST=California,C=US'" +
> +              " -t C,C,C -x --certVersion="+ str(int(version)))
> +    #os.system("certutil -d "+ db_dir + "/ -L -n "+ name +" -r > " + out_name)

nit: remove commented-out lines

@@ +52,5 @@
> +  os.system("certutil -d "+ db_dir + "/ -L -n "+ name +" -r > " + out_name)
> +
> +def generate_child_cert(db_dir, dest_dir, noise_file, name, ca_nick, cert_version, do_bc, is_ee):
> +  out_name = dest_dir + "/" + name + ".der"
> +  if (not do_bc):

again, 'if do_bc: ... else: ...'

@@ +63,5 @@
> +              name +" -v 120 -m "+ str(random.randint(100,40000000)) +" -s 'CN=" + name +
> +              ",O=BOGUS PSM,L=Mountain View,ST=California,C=US'" +
> +              " -2 -t C,C,C -c "+ ca_nick +" --certVersion="+ str(int(cert_version)))
> +    child.logfile = sys.stdout
> +    child.expect('Is this a CA certificate \[y/N\]?')

maybe factor out these strings?

@@ +74,5 @@
> +    child.expect('Is this a critical extension \[y/N\]?')
> +    child.sendline('')
> +    child.expect(pexpect.EOF)
> +
> +

nit: unnecessary blank line

@@ +77,5 @@
> +
> +
> +  os.system("certutil -d "+ db_dir + "/ -L -n "+ name +" -r > " + out_name)
> +
> +def generate_intermediate(db_dir, dest_dir, noise_file, name, ca_nick, cert_version, do_bc):

doesn't look like this is used - let's just remove it

@@ +81,5 @@
> +def generate_intermediate(db_dir, dest_dir, noise_file, name, ca_nick, cert_version, do_bc):
> +  generate_child_cert(db_dir, dest_dir, noise_file, name, ca_nick, cert_version, do_bc, False)
> +
> +def generate_ee_family(db_dir, dest_dir, noise_file, ca_name):
> +  name = "v1-"+ ca_name;

how about v1_ee?

@@ +86,5 @@
> +  generate_child_cert(db_dir, dest_dir, noise_file, name, ca_name, 1, False, False)
> +  name = "v1_bc_ee-"+ ca_name;
> +  generate_child_cert(db_dir, dest_dir, noise_file, name, ca_name, 1, True, True)
> +
> +  name = "v2-"+ ca_name;

v2_ee?

@@ +92,5 @@
> +  name = "v2_bc_ee-"+ ca_name;
> +  generate_child_cert(db_dir, dest_dir, noise_file, name, ca_name, 2, True, True)
> +
> +
> +  name = "v3_missing_bc-"+ ca_name;

v3_missing_bc_ee?

@@ +127,5 @@
> +  generate_ca_cert(db_dir, dest_dir, noise_file,  name, version, do_bc)
> +  generate_intermediates_and_ee_set(db_dir, dest_dir, noise_file, name)
> +
> +def generate_certs():
> +  [noise_file, pwd_file] = init_nss_db(db); #should be db

what is meant by this comment?

@@ +131,5 @@
> +  [noise_file, pwd_file] = init_nss_db(db); #should be db
> +
> +  name = "v1_ca"
> +  generate_ca(db, srcdir, noise_file, name, 1, False )
> +  #generate_intermediates_and_ee_set(db, srcdir, noise_file, name)

Hmm - looks like this gets called in generate_ca. I think it would be more clear to either call generate_ca something like generate_cert_family and remove this line or remove the call to generate_intermediates_and_ee_set and un-comment out this line.

@@ +137,5 @@
> +  name = "v1_ca_bc"
> +  generate_ca(db, srcdir, noise_file, name, 1, True)
> +  #generate_intermediates_and_ee_set(db, srcdir, noise_file, name)
> +
> +

nit: unnecessary blank line

@@ +144,5 @@
> +
> +  generate_ca(db, srcdir, noise_file, "v3_ca", 3, True )
> +  generate_ca(db, srcdir, noise_file, "v3_ca_missing_bc", 3, False)
> +
> +

nit: unnecessary blank line

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ +56,5 @@
>  fail-if = os == "android" || buildapp == "b2g"
>  [test_intermediate_basic_usage_constraints.js]
>  [test_name_constraints.js]
> +[test_cert_version.js]
> +

nit: unnecessary blank line

Comment 26

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

Comment on attachment 8390074 [details] [diff] [review]
test-version-patches (v3)

It seems like a lot of changes are needed to address David's review comments. I'm happy to defer to David on this.

Comment 27

[Camilo Viecco (:cviecco)]

Comment on attachment 8390074 [details] [diff] [review]
test-version-patches (v3)

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

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +131,5 @@
> +  //////////////////
> +
> +  // v1 intermediate with v1 trust anchor
> +  enableInvalidError(useInsanity);
> +  check_fail_ca(constructCertFromFile('v1_int-v1_ca.der'));

That was how I originally wrote this, when insanity misbehaved with v1/v2 certs, but after the fix this is no
longer possible as two types of errors, depending on where is the invalid part. And the invalid part is almost always
the same certs. I could rewrite this to be:

//Same CA block
let expected_ca_error = get_expected_ca_error1(useInsanity);
let expected_ee_error = get_expected_ee_error1(useInsanity);
check_fail_ca(..., expected_ca_error);
check_fail(..., expected_ee_error); 
....
let expected_ca_error = get_expected_ca_error2(useInsanity);
let expected_ee_error = get_expected_ee_error2(useInsanity);
check_fail(..., expected_ee_error);

But since there are changes in the error, ensuring that all
cases are present becomes tricky. What do you think?

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +127,5 @@
> +  generate_ca_cert(db_dir, dest_dir, noise_file,  name, version, do_bc)
> +  generate_intermediates_and_ee_set(db_dir, dest_dir, noise_file, name)
> +
> +def generate_certs():
> +  [noise_file, pwd_file] = init_nss_db(db); #should be db

leftover cruft

Comment 28

[Camilo Viecco (:cviecco)]

Attachment: test-version-patches (v4)

Comment 29

[Camilo Viecco (:cviecco)]

Comment on attachment 8394941 [details] [diff] [review]
test-version-patches (v4)

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

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +127,5 @@
> +  generate_intermediates_and_ee_set(db_dir, dest_dir, noise_file, name)
> +
> +def generate_certs():
> +  [noise_file, pwd_file] = init_nss_db(db); 
> +

nit, space

Comment 30

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

Comment on attachment 8394941 [details] [diff] [review]
test-version-patches (v4)

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

Looking good. Basically 99% of the way there, but for a few nits:
1. Replace all references to "insanity" with "mozilla::pkix"/"mozillapkix" as appropriate
2. It doesn't look like the generated certificates were included in this patch
3. Misc comments (see below)

::: security/manager/ssl/tests/unit/head_psm.js
@@ +29,5 @@
>  const SEC_ERROR_UNKNOWN_ISSUER                          = SEC_ERROR_BASE +  13;
>  const SEC_ERROR_BAD_DATABASE                            = SEC_ERROR_BASE +  18;
>  const SEC_ERROR_UNTRUSTED_ISSUER                        = SEC_ERROR_BASE +  20; // -8172
>  const SEC_ERROR_UNTRUSTED_CERT                          = SEC_ERROR_BASE +  21; // -8171
> +const SEC_ERROR_CERT_NOT_VALID                          = SEC_ERROR_BASE +  28; // -8164

I'm not seeing this in test_cert_version.js - is it still necessary?

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

cert_from_file

@@ +24,5 @@
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let error = certdb.verifyCertNow(cert, usage,
> +                                   NO_FLAGS, verifiedChain, hasEVPolicy);
> +  do_check_eq(error,  expected_error);

nit: extra space before expected_error

@@ +43,5 @@
> +function check_ok_ca(x) {
> +  return check_cert_err_generic(x, 0, certificateUsageSSLCA);
> +}
> +
> +

nit: unnecessary vertical space

@@ +47,5 @@
> +
> +function run_tests_in_mode(useInsanity)
> +{
> +  Services.prefs.setBoolPref("security.use_insanity_verification",
> +                             useInsanity);

remember - it's mozillapkix, now

@@ +49,5 @@
> +{
> +  Services.prefs.setBoolPref("security.use_insanity_verification",
> +                             useInsanity);
> +
> +  // All blocks that start with enableInvalidError have the same

Maybe "sections" instead of "blocks"? Also, I think this documentation is a little out of date.

@@ +52,5 @@
> +
> +  // All blocks that start with enableInvalidError have the same
> +  // effect in practice. However insanity provides a more appropiate
> +  // error value
> +  // Blocks with  disableErrorInClassic are handled more strictly in insanity

nit: two spaces before disableErrorInClassic

@@ +65,5 @@
> +  let ee_error = 0;
> +  let ca_error = 0;
> +
> +  //////////////
> +  // V1 CA supersection

let's be consistent on capitalization here - in most other cases, the v in "v1" is not capitalized

@@ +88,5 @@
> +  check_cert_err(certFromFile('v1_bc_ee-v1_int-v1_ca.der'), ee_error);
> +  check_cert_err(certFromFile('v2_bc_ee-v1_int-v1_ca.der'), ee_error);
> +  check_cert_err(certFromFile('v4_bc_ee-v1_int-v1_ca.der'), ee_error);
> +
> +  // V1 intermediate with v3 extensions. CA is invalid.

v1

@@ +142,5 @@
> +  check_cert_err(certFromFile('v3_missing_bc_ee-v2_int_bc-v1_ca.der'), ee_error);
> +  check_cert_err(certFromFile('v3_bc_ee-v2_int_bc-v1_ca.der'), ee_error);
> +  check_cert_err(certFromFile('v4_bc_ee-v2_int_bc-v1_ca.der'), ee_error);
> +
> +  // Section is OK. A x509 v3 CA MUST have bc

s/Section is OK. //

@@ +181,5 @@
> +  check_cert_err(certFromFile('v2_bc_ee-v3_int-v1_ca.der'), ee_error);
> +  check_cert_err(certFromFile('v4_bc_ee-v3_int-v1_ca.der'), ee_error);
> +
> +  // The next groups change the v1 ca for a v1 ca with base constraints
> +  // (invalid trust anchor). The error patter is the same as the groups

"pattern"

@@ +774,5 @@
> +  load_cert("v3_ca_missing_bc", "CTu,CTu,CTu");
> +
> +  check_ok_ca(certFromFile('v1_ca.der'));
> +  check_ok_ca(certFromFile('v2_ca.der'));
> +  check_ok_ca(certFromFile('v3_ca.der'));

Shouldn't we do check_ok_ca for each of the roots in both classic and mozilla::pkix mode?

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +45,5 @@
> +    child.expect('Is this a critical extension \[y/N\]?')
> +    child.sendline('')
> +    child.expect(pexpect.EOF)
> +  else:
> +    os.system("certutil -S -z " + noise_file + " -g 2048 -d "+ db_dir +"/ -n " +

How about factoring out the common parts of this string? (I think the only different part is whether or not it contains -2, right?)

@@ +50,5 @@
> +              name +" -v 120 -s 'CN=" + name +
> +              ",O=PSM Testing,L=Mountain View,ST=California,C=US'" +
> +              " -t C,C,C -x --certVersion="+ str(int(version)))
> +
> +

nit: unnecessary vertical space

@@ +72,5 @@
> +    child.expect('Is this a critical extension \[y/N\]?')
> +    child.sendline('')
> +    child.expect(pexpect.EOF)
> +  else:
> +    os.system("certutil -S -z " + noise_file + " -g 2048 -d "+ db_dir +"/ -n " +

again, this string can be factored out

@@ +81,5 @@
> +  os.system("certutil -d "+ db_dir + "/ -L -n "+ name +" -r > " + out_name)
> +
> +def generate_ee_family(db_dir, dest_dir, noise_file, ca_name):
> +  name = "v1_ee-"+ ca_name;
> +  generate_child_cert(db_dir, dest_dir, noise_file, name, ca_name, 1, False, False)

shouldn't this last argument be True? In fact, shouldn't the last arguments for all calls to generate_child_cert in generate_ee_family be True (for is_ee)?

@@ +133,5 @@
> +  generate_ca(db, srcdir, noise_file, "v1_ca_bc", 1, True)
> +  generate_ca(db, srcdir, noise_file, "v2_ca", 2, False )
> +  generate_ca(db, srcdir, noise_file, "v2_ca_bc", 2, True)
> +  generate_ca(db, srcdir, noise_file, "v3_ca", 3, True )
> +  generate_ca(db, srcdir, noise_file, "v3_ca_missing_bc", 3, False)

I think the naming convention here is a little strange, because "v1_ca" doesn't have a basic constraints extension, but "v3_ca" does. How about calling these last two "v3_ca" and "v3_ca_bc"?

@@ +135,5 @@
> +  generate_ca(db, srcdir, noise_file, "v2_ca_bc", 2, True)
> +  generate_ca(db, srcdir, noise_file, "v3_ca", 3, True )
> +  generate_ca(db, srcdir, noise_file, "v3_ca_missing_bc", 3, False)
> +
> +

nit: unnecessary vertical space

Comment 31

[Camilo Viecco (:cviecco)]

Comment on attachment 8394941 [details] [diff] [review]
test-version-patches (v4)

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

All the rest of comments will be addressed in next revision.

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +81,5 @@
> +  os.system("certutil -d "+ db_dir + "/ -L -n "+ name +" -r > " + out_name)
> +
> +def generate_ee_family(db_dir, dest_dir, noise_file, ca_name):
> +  name = "v1_ee-"+ ca_name;
> +  generate_child_cert(db_dir, dest_dir, noise_file, name, ca_name, 1, False, False)

The last argument only matters if the next-to last is true, but for clarity I will change this,

@@ +133,5 @@
> +  generate_ca(db, srcdir, noise_file, "v1_ca_bc", 1, True)
> +  generate_ca(db, srcdir, noise_file, "v2_ca", 2, False )
> +  generate_ca(db, srcdir, noise_file, "v2_ca_bc", 2, True)
> +  generate_ca(db, srcdir, noise_file, "v3_ca", 3, True )
> +  generate_ca(db, srcdir, noise_file, "v3_ca_missing_bc", 3, False)

Thats because a v3 is supposed to have bc while v1 and v2 are not supposed to have them. vX_ca means it is a as speced ca.

Comment 32

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

Hmmm - maybe we could do: v1_ca, v1_invalid_has_bc, ..., v3_ca, v3_invalid_no_bc? I realize that's a bit more verbose. Well, go with whatever you think is best, but at least add a comment explaining the naming convention.

Comment 33

[Camilo Viecco (:cviecco)]

Attachment: test-version-patches (v5)

Comment 34

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

Comment on attachment 8395885 [details] [diff] [review]
test-version-patches (v5)

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

Awesome - just a few nits. Also, I'm not seeing the generated certificate files in this patch - don't forget to include them when you check this in :)

::: security/manager/ssl/tests/unit/head_psm.js
@@ +29,5 @@
>  const SEC_ERROR_UNKNOWN_ISSUER                          = SEC_ERROR_BASE +  13;
>  const SEC_ERROR_BAD_DATABASE                            = SEC_ERROR_BASE +  18;
>  const SEC_ERROR_UNTRUSTED_ISSUER                        = SEC_ERROR_BASE +  20; // -8172
>  const SEC_ERROR_UNTRUSTED_CERT                          = SEC_ERROR_BASE +  21; // -8171
> +const SEC_ERROR_CERT_NOT_VALID                          = SEC_ERROR_BASE +  28; // -8164

I'm still not seeing this being used in test_cert_version.js - let's not include it here if we don't need it yet.

::: security/manager/ssl/tests/unit/test_cert_version.js
@@ +59,5 @@
> +  check_ok_ca(cert_from_file('v3_ca.der'));
> +  check_ca_err(cert_from_file('v3_ca_missing_bc.der'),
> +               useMozillaPKIX ? SEC_ERROR_CA_CERT_INVALID : 0);
> +
> +  // Classic allows v1 and v2 certs to be CA certsin trust anchor positions and

nit "certs in"

@@ +781,5 @@
> +
> +  run_tests_in_mode(false);
> +  run_tests_in_mode(true);
> +}
> +

nit: unnecessary blank line

::: security/manager/ssl/tests/unit/test_cert_version/generate.py
@@ +32,5 @@
> +  out_name = dest_dir + "/" + name + ".der"
> +  base_exec_string = ("certutil -S -z " + noise_file + " -g 2048 -d "+ db_dir +"/ -n " +
> +              name +" -v 120 -s 'CN=" + name +
> +              ",O=PSM Testing,L=Mountain View,ST=California,C=US'" +
> +              " -t C,C,C -x --certVersion="+ str(int(version)))

nit: indentation

@@ +34,5 @@
> +              name +" -v 120 -s 'CN=" + name +
> +              ",O=PSM Testing,L=Mountain View,ST=California,C=US'" +
> +              " -t C,C,C -x --certVersion="+ str(int(version)))
> +  if (do_bc):
> +    child = pexpect.spawn( base_exec_string + " -2")

nit: unnecessary space before base_exec_string

Comment 35

[Camilo Viecco (:cviecco)]

Attachment: test-version-patches (v6)

Keeping keeler's r+

Comment 36

[Camilo Viecco (:cviecco)]

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

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e7b142b3e801
https://hg.mozilla.org/mozilla-central/rev/aed9a773eb63
https://hg.mozilla.org/mozilla-central/rev/fbf4eadfbd69

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

mwobensmith, is it better to ask/need-info you to verify bugs like this or should I just assign you as QA contact if I come across them in triage?   Thanks!

Comment 40

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

Liz, any of the above would be fine - QA owner, needinfo, both, etc. 

For this bug, it's tests written by the developers, so I don't think they need my input. Also, I don't think this needs QA verification.