Closed Bug 622859 Opened 13 years ago Closed 10 years ago

Reject EV certificates with key sizes below RSA 2048

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: hanno, Assigned: Cykesiopka)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 13 obsolete files)

90.31 KB, patch
keeler
: review+
Details | Diff | Splinter Review
25.51 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
As already discussed in #622589 the eff ssl observatory found lots of certificates violating the EV certificate policy. The quoted bug handles revocation requests to the authorities.
But such certificates should be rejected automatically. The CAB forum policy states that certificates with a validity after 31. dec 2010 are not allowed to be less than 2048 bit RSA (or 256 bit ecc) [see Guidelines 1.3 on page 45]. Less than 1024 bit was never allowed.

So a reasonable pseudocode check would be something like
if isEV and not (ecc or rsa) then fail
if isEV and rsa and keysize < 2048 then fail
if isEV and ecc and keysize < 256 then fail

(checks to differentiate between 1024 and 2048 are not neccessary imho, as 31th dec 2010 has passed four days back and all 1024 bit ev certificates valid today are issued in error and should be rejected)
See also bug 470926. Maybe one conclusion of the EFF SSL observatory findings is that PSM should no longer simply rely on the CAs getting all the stuff in EV certs right.
OS: Linux → All
Hardware: x86_64 → All
When EV got implemented, people argued it's not necessary to check for certificate properties that CAs are required to follow by policy.

Apparently CAs do make mistakes.

I agree that we should enforce the rules.
Attached patch bug622859_WIPv1.patch (obsolete) — Splinter Review
Brian: Is this similar to what you had in mind?
Attachment #8491976 - Flags: feedback?(brian)
Attached patch bug622859_tests_WIPv1.patch (obsolete) — Splinter Review
This is the test related code so far. Still some TODOs to take care of, bunch of copy and paste etc... Attaching here as a backup.

I'm not sure what to do about opt builds not having the test EV root info - is there anything valid to test in this case?

Random try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=57df3e122ca0
Comment on attachment 8491976 [details] [diff] [review]
bug622859_WIPv1.patch

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

This is the right idea, but I'd rather change some of the details so that it is more flexible. See my comments below.

::: security/apps/AppTrustDomain.cpp
@@ +205,3 @@
>  {
>    return ::mozilla::pkix::VerifySignedData(signedData, subjectPublicKeyInfo,
> +                                           isEVAttempt, mPinArg);

For AppTrustDomain, you can always hard-code 2048 as the minimum size in bits.

::: security/pkix/include/pkix/pkixtypes.h
@@ +296,5 @@
>    //
>    // VerifySignedData() should do the same checks that this function does, but
>    // mainly for efficiency, some keys are not passed to VerifySignedData().
>    // This function is called instead for those keys.
> +  virtual Result CheckPublicKey(Input subjectPublicKeyInfo, bool isEVAttempt) = 0;

You do not need to pass isEVAttempt as a parameter here. Instead, when you construct the TrustDomain, you can have the TrustDomain remember whether it is an EV attempt or not.

@@ +307,5 @@
>    // In any case, the implementation must perform checks on the public key
>    // similar to how mozilla::pkix::CheckPublicKey() does.
>    virtual Result VerifySignedData(const SignedDataWithSignature& signedData,
> +                                  Input subjectPublicKeyInfo,
> +                                  bool isEVAttempt) = 0;

Ditto.

::: security/pkix/lib/pkixnss.cpp
@@ +38,5 @@
>  
>  typedef ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey> ScopedSECKeyPublicKey;
>  
>  Result
> +CheckPublicKeySize(Input subjectPublicKeyInfo, bool isEVAttempt,

Rather than pass in isEVAttempt here, I recommend that you just pass in the minimum RSA/DSA key size in bits (i.e. make minimumNonEccBits a parameter).
Attachment #8491976 - Flags: feedback?(brian) → feedback+
Attached patch bug622859_WIPv2.patch (obsolete) — Splinter Review
+ Makes the changes Brian suggested in Comment 5

(Side note: I will probably skip ECC checking for now, and do that in another bug).
Assignee: nobody → cykesiopka.bmo
Attachment #8491976 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8493265 - Flags: feedback?(brian)
Attached patch bug622859_tests_WIPv2.patch (obsolete) — Splinter Review
+ Addresses some TODOs
+ Regenerates all CA certs (DV and EV) with "keyUsage = keyCertSign, cRLSign" specified so that they don't run afoul of Bug 970196
Attachment #8491978 - Attachment is obsolete: true
Attached patch bug622859_WIPv3.patch (obsolete) — Splinter Review
+ Rebase on top of Bug 1071308
+ Rename (m)IsEVAttempt to (m)ForEV (seems slightly better, although the new name still feels a bit awkward)
Attachment #8493265 - Attachment is obsolete: true
Attachment #8493265 - Flags: feedback?(brian)
Attachment #8496397 - Flags: feedback?(brian)
Attached patch bug622859_tests_WIPv3.patch (obsolete) — Splinter Review
+ Run tests even for opt, but test that none of the chains get EV treatment instead
+ Switch xpcshell file code style to camel case
+ Address some TODOs
Attachment #8493268 - Attachment is obsolete: true
Comment on attachment 8496397 [details] [diff] [review]
bug622859_WIPv3.patch

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

Thanks for doing this.

Are you interested in writing more patches like this? I'd like to help you do the patches for checking that the ECC curve is correct, and for removing DSA, and other things, if you'd like to do them.

::: security/apps/AppTrustDomain.h
@@ +50,5 @@
>  private:
>    /*out*/ ScopedCERTCertList& mCertChain;
>    void* mPinArg; // non-owning!
>    ScopedCERTCertificate mTrustedRoot;
> +  static const unsigned int MINIMUM_NON_ECC_BITS = 2048;

Please move this to AppTrustDomain.cpp as a static variable.

In general, avoid putting stuff into header files that is only used by one .cpp files.

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +61,5 @@
>    , mOCSPCache(ocspCache)
>    , mPinArg(pinArg)
>    , mOCSPGetConfig(ocspGETConfig)
>    , mPinningMode(pinningMode)
> +  , mForEV(forEV)

You do not need to store mForEV.

::: security/certverifier/NSSCertDBTrustDomain.h
@@ +106,5 @@
>    OCSPCache& mOCSPCache; // non-owning!
>    void* mPinArg; // non-owning!
>    const CertVerifier::ocsp_get_config mOCSPGetConfig;
>    CertVerifier::PinningMode mPinningMode;
> +  const bool mForEV;

You do not need this member.

@@ +107,5 @@
>    void* mPinArg; // non-owning!
>    const CertVerifier::ocsp_get_config mOCSPGetConfig;
>    CertVerifier::PinningMode mPinningMode;
> +  const bool mForEV;
> +  unsigned int mMinimumNonEccBits;

make this const

Make sure you capitalize ECC everywhere in this patch as "ECC", not "Ecc".
Attachment #8496397 - Flags: feedback?(brian) → feedback+
Adjusting summary and dependencies as I'm moving the ECC curve stuff to Bug 1077790.
Blocks: 1077790
Summary: Key size for EV certificate below 2048 bit for RSA (below 256 bit for ECC) not allowed → Reject EV certificates with key sizes below RSA 2048
Attached patch bug622859_v1.patch (obsolete) — Splinter Review
+ Address issues from Comment 10
+ Update TODO to reference Bug 1077790
Attachment #8496397 - Attachment is obsolete: true
Attachment #8499978 - Flags: review?(brian)
Attached patch bug622859_tests_v1.patch (obsolete) — Splinter Review
Attachment #8496398 - Attachment is obsolete: true
Attachment #8499979 - Flags: feedback?(brian)
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #10)
> Thanks for doing this.

No problem.

> Are you interested in writing more patches like this? I'd like to help you
> do the patches for checking that the ECC curve is correct, and for removing
> DSA, and other things, if you'd like to do them.

Sure, thanks for the offer!
Comment on attachment 8499978 [details] [diff] [review]
bug622859_v1.patch

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

::: security/pkix/include/pkix/pkixnss.h
@@ +52,5 @@
>                   size_t digestBufLen);
>  
> +// Checks, for RSA keys and DSA keys, that the modulus is at least the given
> +// number of bits.
> +Result CheckPublicKey(Input subjectPublicKeyInfo, unsigned int minimumNonECCBits);

Nit: wrap this to 80 columns so that the "unsigned int" is left-algined with "Input".
Attachment #8499978 - Flags: review?(brian) → review+
Comment on attachment 8499979 [details] [diff] [review]
bug622859_tests_v1.patch

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

::: security/certverifier/ExtendedValidation.cpp
@@ +129,5 @@
> +      0x8B, 0x8E, 0xCB, 0xEF, 0x12, 0x51, 0xF4, 0xEA, 0x71, 0x7C },
> +    "MBYxFDASBgNVBAMMC2V2LXJzYS1jYU9L",
> +    "IxH9",
> +    nullptr
> +  },

For the "good" case, you can just use the existing root that is used for EV testing. You don't need a new one. If the existing root has the wrong size, then we should replace it with one that is the right size.

@@ +1109,5 @@
>      if (!entry.cert) {
>  #ifdef DEBUG
> +      // The debug CA structs are at positions 0 to 2 inclusive, and are NOT in
> +      // the NSS root db
> +      if (iEV <= 2) {

Rather than hard-code 2 here, I suggest you define a named constant for that value. That way, in the future, we can easily find and review all the places where that constant is used when we need to update the number of test EV roots.

::: security/manager/ssl/tests/unit/test_keysize/generate.py
@@ +5,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  import tempfile, os, sys
>  import random
> +import pexpect

I didn't review this file. It is a bit of a bummer that pexpect doesn't work on Windows, though, as this means that I and others will always have to run this script in a VM, if we need to update the tests. If it isn't too inconvenient, it would be good to find an alternative to pexpect that can accomplish the same thing. But, also, this is an existing problem with other scripts, so maybe it is a lost cause.

::: security/manager/ssl/tests/unit/test_keysize_ev.js
@@ +1,4 @@
> +// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
> +// This Source Code Form is subject to the terms of the Mozilla Public
> +// License, v. 2.0. If a copy of the MPL was not distributed with this
> +// file, You can obtain one at http://mozilla.org/MPL/2.0/.

Recently, Mozilla's licensing policy for tests changed. See https://www.mozilla.org/MPL/license-policy.html and https://www.mail-archive.com/dev-platform@lists.mozilla.org/msg10569.html

Accordingly (unless you object), please use this for test code, instead:
Any copyright is dedicated to the Public Domain.
http://creativecommons.org/publicdomain/zero/1.0/

@@ +63,5 @@
> +      isDebugBuild
> +      ? [ certNamePrefix + "-intOK-caOK",
> +          certNamePrefix + "-eeOK-intOK-caOK" ]
> +      : [ certNamePrefix + "-eeOK-intOK-caOK" ];
> +    let ocspResponder = getOCSPResponder(expectedNames);

Please add a comment explaining why the OCSP fetch is skipped for intOK-caOK.

@@ +71,5 @@
> +    checkEVStatus(certFromFile(certNamePrefix + "-eeOK-intOK-caOK.der"),
> +                  certificateUsageSSLServer, isDebugBuild);
> +
> +    ocspResponder.stop(run_next_test);
> +  });

I suggest that you try to write a function that encapsulates all the common logic, parameterized on the case-specific data:

function testKeySizeForEV(expectedNames, endEntityCertFileName, subCACertFileNames, rootCACertFileName, expectedResult)
{
   clearOCSPCache();
   let ocspResponder = getOCSPResponder(expectedNames);
   // is it necessary to call checkCA? not sure
   checkCA(loadCert(certNamePrefix + rootCACertFileName, "CTU,CTu,CTU"));
   for (caCertFileName in subCACertFileNames) {
     checkCA(loadCert(certNamePrefix + caCertFileName, ",,"));
   }
   checkEVStatus(certFromFile(certNamePrefix + endEntityCertFileName), certificateUsageSSLServer, expectedResult);
   ocspResponder.stop(run_next_test);
}

This way, it is clearer for the reviewer to see what is different between the various tests and which test cases are missing. Also, it should make adding additional test cases easier.
Attachment #8499979 - Flags: feedback?(brian) → feedback+
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #16)
> ::: security/certverifier/ExtendedValidation.cpp
> @@ +129,5 @@
> > +      0x8B, 0x8E, 0xCB, 0xEF, 0x12, 0x51, 0xF4, 0xEA, 0x71, 0x7C },
> > +    "MBYxFDASBgNVBAMMC2V2LXJzYS1jYU9L",
> > +    "IxH9",
> > +    nullptr
> > +  },
> 
> For the "good" case, you can just use the existing root that is used for EV
> testing. You don't need a new one. If the existing root has the wrong size,
> then we should replace it with one that is the right size.

OK. Hopefully this won't add too much complexity.

> @@ +1109,5 @@
> >      if (!entry.cert) {
> >  #ifdef DEBUG
> > +      // The debug CA structs are at positions 0 to 2 inclusive, and are NOT in
> > +      // the NSS root db
> > +      if (iEV <= 2) {
> 
> Rather than hard-code 2 here, I suggest you define a named constant for that
> value. That way, in the future, we can easily find and review all the places
> where that constant is used when we need to update the number of test EV
> roots.

Done.

> ::: security/manager/ssl/tests/unit/test_keysize/generate.py
> @@ +5,5 @@
> >  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
> >  
> >  import tempfile, os, sys
> >  import random
> > +import pexpect
> 
> I didn't review this file. It is a bit of a bummer that pexpect doesn't work
> on Windows, though, as this means that I and others will always have to run
> this script in a VM, if we need to update the tests. If it isn't too
> inconvenient, it would be good to find an alternative to pexpect that can
> accomplish the same thing. But, also, this is an existing problem with other
> scripts, so maybe it is a lost cause.

I've removed the only use of pexpect in this file. Pretty simple - just changed the call to pk12util to pass in the pwfile.

> ::: security/manager/ssl/tests/unit/test_keysize_ev.js
> @@ +1,4 @@
> > +// -*- indent-tabs-mode: nil; js-indent-level: 2 -*-
> > +// This Source Code Form is subject to the terms of the Mozilla Public
> > +// License, v. 2.0. If a copy of the MPL was not distributed with this
> > +// file, You can obtain one at http://mozilla.org/MPL/2.0/.
> 
> Recently, Mozilla's licensing policy for tests changed. See
> https://www.mozilla.org/MPL/license-policy.html and
> https://www.mail-archive.com/dev-platform@lists.mozilla.org/msg10569.html
> 
> Accordingly (unless you object), please use this for test code, instead:
> Any copyright is dedicated to the Public Domain.
> http://creativecommons.org/publicdomain/zero/1.0/

No objections.

> @@ +63,5 @@
> > +      isDebugBuild
> > +      ? [ certNamePrefix + "-intOK-caOK",
> > +          certNamePrefix + "-eeOK-intOK-caOK" ]
> > +      : [ certNamePrefix + "-eeOK-intOK-caOK" ];
> > +    let ocspResponder = getOCSPResponder(expectedNames);
> 
> Please add a comment explaining why the OCSP fetch is skipped for intOK-caOK.

OK.

> @@ +71,5 @@
> > +    checkEVStatus(certFromFile(certNamePrefix + "-eeOK-intOK-caOK.der"),
> > +                  certificateUsageSSLServer, isDebugBuild);
> > +
> > +    ocspResponder.stop(run_next_test);
> > +  });
> 
> I suggest that you try to write a function that encapsulates all the common
> logic, parameterized on the case-specific data:
> 
> function testKeySizeForEV(expectedNames, endEntityCertFileName,
> subCACertFileNames, rootCACertFileName, expectedResult)
> {
>    clearOCSPCache();
>    let ocspResponder = getOCSPResponder(expectedNames);
>    // is it necessary to call checkCA? not sure
>    checkCA(loadCert(certNamePrefix + rootCACertFileName, "CTU,CTu,CTU"));
>    for (caCertFileName in subCACertFileNames) {
>      checkCA(loadCert(certNamePrefix + caCertFileName, ",,"));
>    }
>    checkEVStatus(certFromFile(certNamePrefix + endEntityCertFileName),
> certificateUsageSSLServer, expectedResult);
>    ocspResponder.stop(run_next_test);
> }
> 
> This way, it is clearer for the reviewer to see what is different between
> the various tests and which test cases are missing. Also, it should make
> adding additional test cases easier.

Sounds good.
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #16)
> function testKeySizeForEV(expectedNames, endEntityCertFileName,
> subCACertFileNames, rootCACertFileName, expectedResult)
> {
>    clearOCSPCache();
>    let ocspResponder = getOCSPResponder(expectedNames);
>    // is it necessary to call checkCA? not sure

No, but it can help in debugging the tests. I've removed it in the testing patch I'm about to post.
Attached patch bug622859_v2.patch (obsolete) — Splinter Review
+ Fix CheckPublicKey() alignment in pkixnss.h
+ Fix Windows build failure by explicitly marking 2048 and 1024 as unsigned for assignment to mMinimumNonECCBits
Attachment #8499978 - Attachment is obsolete: true
Attachment #8503561 - Flags: review+
Attached patch bug622859_tests_v2.patch (obsolete) — Splinter Review
+ Addresses feedback from Comment 16
+ Adds some more documentation

Previous mostly good try push: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ee9327b534ff
(The Windows build issues should be fixed by the main v2 patch - I have both sets of key size tests passing locally.)
Attachment #8499979 - Attachment is obsolete: true
Attachment #8503562 - Flags: review?(brian)
Comment on attachment 8503562 [details] [diff] [review]
bug622859_tests_v2.patch

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

Looks good to me. I'm going to ask David to review the certificate generation stuff, because it's been a long time since I looked at it, and it's a little bit too foreign to me. In particular, I'm not sure why we're using PKCS#12 files. Is it because the test certs are generated with openssl instead of certutil?

::: security/manager/ssl/tests/unit/test_keysize_ev.js
@@ +100,5 @@
> +
> +  // Reuse the existing test RSA EV root
> +  let rootCAOKCertFileName = keyType == "rsa"
> +                             ? "../test_ev_certs/evroot"
> +                             : "-caOK";

NIT: Line up the "="/"?"/":" when you need three lines, or do this when you need two lines:

  let rootCAOKCertFileName = keyType == "rsa" ? "../test_ev_certs/evroot"
	                                      : "-caOK";

@@ +102,5 @@
> +  let rootCAOKCertFileName = keyType == "rsa"
> +                             ? "../test_ev_certs/evroot"
> +                             : "-caOK";
> +
> +  // OK CA -> OK INT -> OK EE

Please document what "OK" means (2048 bit?) and what "Bad" means (1024-bit). It might be better to just use the terms "2048-bit" and "1024-bit" instead of "Good" and "Bad". In particular, it is confusing that a "Bad" root is still trusted for DV. Normally, we'd consider a root "bad" if we wouldn't trust it at all.
Attachment #8503562 - Flags: review?(dkeeler)
Attachment #8503562 - Flags: review?(brian)
Attachment #8503562 - Flags: review+
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #21)
> Please document what "OK" means (2048 bit?) and what "Bad" means (1024-bit).
> It might be better to just use the terms "2048-bit" and "1024-bit" instead
> of "Good" and "Bad". In particular, it is confusing that a "Bad" root is
> still trusted for DV. Normally, we'd consider a root "bad" if we wouldn't
> trust it at all.

Noted. Maybe something like "acceptable size for (DV|EV)" and "inadequate size for (DV|EV)" would be better? I was intentionally vague about the key sizes so that we wouldn't need to change comments, cert file names and so on whenever we change the key size requirements.

But, if that's still confusing, I'm fine with stating the explicit key sizes instead.
Comment on attachment 8503562 [details] [diff] [review]
bug622859_tests_v2.patch

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

This looks good. I just have a few comments.

Is it necessary to keep around the .p12 files? If I understand correctly, we need the private keys of the CAs to sign OCSP responses in the EV cases, but those end up in keys4.db, so we shouldn't need to keep both, right?

::: security/certverifier/ExtendedValidation.cpp
@@ +119,5 @@
>      nullptr
>    },
> +  {
> +    // The bad RSA root used for EV key size checking
> +    // CN=ev-rsa-caBad

Maybe giving this a more descriptive subject would be good (like the other testing root).

::: security/manager/ssl/tests/unit/test_keysize/generate.py
@@ +95,5 @@
> +    if generate_ev:
> +        pkcs12_filename = CertUtils.generate_pkcs12(db_dir, srcdir,
> +                                                    cert_filename, key_filename,
> +                                                    cert_name)
> +        import_cert_and_pkcs12(cert_filename, pkcs12_filename, cert_name, ',,')

The pkcs12 file includes the cert already, right? (So, I think you just need to import the pkcs12 file, but maybe I'm wrong.)

@@ +228,1 @@
>  # tested at the moment is 1016

I don't understand how bug 636807 results in this as a consequence.

@@ +231,4 @@
>  
> +generate_certs('dsa', '960', '1024', False)
> +
> +print ('You now MUST update the compiled test EV root information to match ' +

It would be nice to output the exact information that needs to be pasted into myTrustedEVInfos.

::: security/manager/ssl/tests/unit/test_keysize_ev.js
@@ +34,5 @@
> +  do_print("cert issuer cn=" + cert.issuerCommonName);
> +  let hasEVPolicy = {};
> +  let verifiedChain = {};
> +  let error = certDB.verifyCertNow(cert, usage,
> +                                   NO_FLAGS, verifiedChain, hasEVPolicy);

nit: rearrange these two lines so the first one gets as close as possible to 80 chars

@@ +43,5 @@
> +/**
> + * Adds a single EV key size test.
> + *
> + * @param {Array} expectedNamesForOCSP
> + *        An array of nicknames of the certs to be responsed to. The cert name

"responded to"

@@ +69,5 @@
> +    // Don't prepend the cert name prefix if rootCACertFileName starts with ".."
> +    // to support reusing certs in other directories.
> +    let rootCertNamePrefix = rootCACertFileName.startsWith("..")
> +                             ? ""
> +                             : certNamePrefix;

Same thing here with the "="/"?"/":"

@@ +106,5 @@
> +  // OK CA -> OK INT -> OK EE
> +  // In opt builds, this chain is only validated for DV. Hence, an OCSP fetch
> +  // will not be done for the "-intOK-caOK" intermediate in such a build.
> +  let expectedNamesForOCSP =
> +    isDebugBuild

nit: I think I would reorganize this like so:

let expectedNamesForOCSP = isDebugBuild
                         ? [ certNamePrefix + "-intOK-caOK",
                             certNamePrefix + "-eeOK-intOK-caOK" ]
                         : [ certNamePrefix + "-eeOK-intOK-caOK" ];

@@ +128,5 @@
> +                      false);
> +
> +  // OK CA -> Bad INT -> OK EE
> +  expectedNamesForOCSP =
> +    isDebugBuild

same here

@@ +139,5 @@
> +                      false);
> +
> +  // OK CA -> OK INT -> Bad EE
> +  expectedNamesForOCSP = [
> +    certNamePrefix + "-eeBad-intOK-caOK"

this all can probably go on one line, right?

@@ +152,5 @@
> +function run_test() {
> +  // Setup OCSP responder
> +  Services.prefs.setCharPref("network.dns.localDomains", "www.example.com");
> +
> +  checkForKeyType("rsa");

Are we expecting to add dsa and ec in the near future?
Attachment #8503562 - Flags: review?(dkeeler) → review+
Comment on attachment 8503561 [details] [diff] [review]
bug622859_v2.patch

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

::: security/certverifier/NSSCertDBTrustDomain.cpp
@@ +61,5 @@
>    , mOCSPCache(ocspCache)
>    , mPinArg(pinArg)
>    , mOCSPGetConfig(ocspGETConfig)
>    , mPinningMode(pinningMode)
> +  , mMinimumNonECCBits(forEV ? 2048u : 1024u)

These values should be defined as constants (in just this file) much like MINIMUM_NON_ECC_BITS in AppTrustDomain.cpp (even though they're just used once, currently).

::: security/pkix/test/lib/pkixtestnss.cpp
@@ +258,5 @@
>  Result
>  TestCheckPublicKey(Input subjectPublicKeyInfo)
>  {
>    InitNSSIfNeeded();
> +  return CheckPublicKey(subjectPublicKeyInfo, 1024);

Rather than hard-coding this in multiple places, it would be nice to have a constant that can be referenced. In the case of tests, it could be MINIMUM_TEST_KEY_BITS = 1024 or whatever.
(In reply to David Keeler (:keeler) [use needinfo?] from comment #23)
> Is it necessary to keep around the .p12 files? If I understand correctly, we
> need the private keys of the CAs to sign OCSP responses in the EV cases, but
> those end up in keys4.db, so we shouldn't need to keep both, right?

Doesn't look like it's necessary. I'll remove the .p12 files from the patch, and change the script to redirect the .p12 files to the temporary directory.

> ::: security/certverifier/ExtendedValidation.cpp
> @@ +119,5 @@
> >      nullptr
> >    },
> > +  {
> > +    // The bad RSA root used for EV key size checking
> > +    // CN=ev-rsa-caBad
> 
> Maybe giving this a more descriptive subject would be good (like the other
> testing root).

OK. I'll give the other certs more descriptive subjects as well.

> The pkcs12 file includes the cert already, right? (So, I think you just need
> to import the pkcs12 file, but maybe I'm wrong.)

Not sure, but I tried various combinations of importing and not importing the cert and .p12 files, and only importing both seemed to work.

> @@ +228,1 @@
> >  # tested at the moment is 1016
> 
> I don't understand how bug 636807 results in this as a consequence.

SECKEY_PublicKeyStrengthInBits() rounds up the number of bits to the next multiple of 8, which is what Bug 636807 is about. So technically for DV we only block <= 1016-bit certs, and not < 1024. I'll keep the original text and just add the bug number so this is less confusing.

> @@ +231,4 @@
> >  
> > +generate_certs('dsa', '960', '1024', False)
> > +
> > +print ('You now MUST update the compiled test EV root information to match ' +
> 
> It would be nice to output the exact information that needs to be pasted
> into myTrustedEVInfos.

Not sure what you mean... Do you want me to mention "SHA-256 fingerprint + Issuer DER Base64 + Serial DER Base64"? I'm inclined to just let the HOWTO section in ExtendedValidation.cpp explain the necessary steps.

> Are we expecting to add dsa and ec in the near future?

DSA no, EC yes.
(ni? from Brian on Comment 22).
Flags: needinfo?(brian)
(ni? from David on the myTrustedEVInfos bit in Comment 25).
Flags: needinfo?(dkeeler)
(In reply to Cykesiopka from comment #25)
> (In reply to David Keeler (:keeler) [use needinfo?] from comment #23)
> > @@ +228,1 @@
> > >  # tested at the moment is 1016
> > 
> > I don't understand how bug 636807 results in this as a consequence.
> 
> SECKEY_PublicKeyStrengthInBits() rounds up the number of bits to the next
> multiple of 8, which is what Bug 636807 is about. So technically for DV we
> only block <= 1016-bit certs, and not < 1024. I'll keep the original text
> and just add the bug number so this is less confusing.

Oh, I get it. I think what you just wrote there would be a good comment to put there.

> > @@ +231,4 @@
> > >  
> > > +generate_certs('dsa', '960', '1024', False)
> > > +
> > > +print ('You now MUST update the compiled test EV root information to match ' +
> > 
> > It would be nice to output the exact information that needs to be pasted
> > into myTrustedEVInfos.
> 
> Not sure what you mean... Do you want me to mention "SHA-256 fingerprint +
> Issuer DER Base64 + Serial DER Base64"? I'm inclined to just let the HOWTO
> section in ExtendedValidation.cpp explain the necessary steps.

Well, would it be possible to automatically output text that can be copied into ExtendedValidation.cpp for the EV root just generated?
Flags: needinfo?(dkeeler)
Attached patch bug622859_v3.patch (obsolete) — Splinter Review
+ Use MINIMUM_NON_ECC_BITS constants for NSSCertDBTrustDomain.cpp as well
+ Add MINIMUM_TEST_KEY_BITS constant for tests
Attachment #8503561 - Attachment is obsolete: true
Attachment #8506087 - Flags: review+
- Remove .p12 files
- Revert SECKEY_PublicKeyStrengthInBits() comment change, and instead just add the appropriate bug number

+ Give certs more descriptive subjects
+ Automatically print out info for generated EV roots
+ Fix remaining review comments from Comment 21 and Comment 23 (although I'm going to defer changing "OK" and "Bad" to another bug, since I have other patches that depend on this bug being resolved)
+ Move some functions to CertUtils for reuse by test_ev_certs/ scripts
+ Update some documentation

win32 try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=44795568b005
Attachment #8503562 - Attachment is obsolete: true
Comment on attachment 8506091 [details] [diff] [review]
bug622859_tests_v3.patch

This new version has non-trivial changes, so asking for re-review just in case.
(Bugzilla Interdiff works correctly between tests_v2.patch and tests_v3.patch. I'll post a proper interdiff next time!)
Attachment #8506091 - Flags: review?(dkeeler)
Comment on attachment 8506091 [details] [diff] [review]
bug622859_tests_v3.patch

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

Great!
Attachment #8506091 - Flags: review?(dkeeler) → review+
Thanks for the reviews!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=44795568b005
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=dcab2ab61925
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=289970c40c84

- This should mostly cover the full set of systems.
- The test_TelemetryPing.js failures seen in the second link are from something below these patches, as I've annotated for a few of them.
- OSX 10.8 xpcshell runs are still pending 11 and 6 hours after I pushed to try, so I'm not going to bother waiting further.
- I'm going to assume the B2G ICS Emulator debug X1 issue is also caused by whatever infra slowness is going on.
Keywords: checkin-needed
(In reply to Carsten Book [:Tomcat] from comment #35)
> sorry had to backout this changes since i guess it caused
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3083123&repo=mozilla-inbound

Thanks, I'll look into it.
(In reply to Carsten Book [:Tomcat] from comment #35)
> sorry had to backout this changes since i guess it caused
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3083123&repo=mozilla-inbound

I'm fairly certain this test fails because it uses the 1024-bit marketplace-stage.crt cert. In particular, commenting out the relevant Marketplace-stage test functions lets the test pass again even with the patch applied.

It doesn't look like I have the ability to regenerate marketplace-stage.crt, so I will post a new patch that reverts the minimum size for apps to 1024 bits.
Attached patch bug622859_v4.patch (obsolete) — Splinter Review
- Reverts minimum key size for AppTrustDomain to 1024

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c0f326a92348
Attachment #8506087 - Attachment is obsolete: true
Attachment #8507339 - Flags: review+
Keywords: checkin-needed
Comment on attachment 8507339 [details] [diff] [review]
bug622859_v4.patch

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

::: security/apps/AppTrustDomain.cpp
@@ +29,5 @@
>  #ifdef PR_LOGGING
>  extern PRLogModuleInfo* gPIPNSSLog;
>  #endif
>  
> +static const unsigned int MINIMUM_NON_ECC_BITS = 1024;

We shouldn't revert this limit to 1024 bits for production use too just because the staging server has a weak key. Instead, in AppTrustDomain::SetTrustedRoot, you can remember what the minimum non-ECC root size should be on a per-root basis (default 2048 bits).

Also, it is surprising (concerning) that the staging certificate got generated using different security strength parameters from the production certificates.
(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #39)
> Instead, in
> AppTrustDomain::SetTrustedRoot, you can remember what the minimum non-ECC
> root size should be on a per-root basis (default 2048 bits).

Ah, yes. This is a better approach.
Keywords: checkin-needed
+ Special case the staging cert instead of lowering the minimum for all the certs

I will attach a v3 -> v5 interdiff next for easier review.
Attachment #8507339 - Attachment is obsolete: true
Attachment #8507433 - Flags: review?(brian)
Attached patch bug622859_v3-v5_interdiff.patch (obsolete) — Splinter Review
Attachment #8507433 - Flags: review?(brian) → review+
Attachment #8507434 - Attachment is obsolete: true
Flags: needinfo?(brian)
Blocks: 1085074
https://hg.mozilla.org/mozilla-central/rev/92f19cf15b2d
https://hg.mozilla.org/mozilla-central/rev/f564fff0642c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
My bzpost extension published comment 46 because those 2 commits were never referenced in this bug. My guess is https://hg.mozilla.org/mozilla-central/pushloghtml?changeset=a7e637d5287d never went through mcMerge properly or something. Sorry about the spam.
You need to log in before you can comment on or make changes to this bug.