We should avoid using RSA 512 and probably deprecate usage of RSA 1024 in several places. This bug is about collecting the list of areas that needs to be addressed and getting it done.

FWIW we are looking in Chrome at blocking 512b certificates. 512b is no longer sufficiently strong to withstand attacks.

We have known for a while that 512-bit RSA certs are not sufficient for security. Recently this has been called out in bug 698753. We could create a patch for blocking 512-bit certs and have it ready tomorrow. We would need to decide the details today.

In Bug 698753 comment 9, Robert Relyea wrote: > NSS currently has no minimum requirements for key size usage. There are a > couple of ways we can kill 768 or smaller signatures Signatures: > > 1) We can start rejecting signatures in NSS: > Checks in PK11_VerifyRecover will affect all RSA signatures, except SSL > client auth/protocol usage (which uses PK11_Verify). > Checks in secvfy.c:vfy_CreateContext can affect any signature (including > DSA an ECC), except SSL clientauth/protocol usage. We can also change > DecryptSigBlock() and only affect RSA signatures (like changes to > PK11_VerifyRecover). These two seem too risky for breaking stuff and more general than we need, AFAICT. > Checks in certvfy.c:CERT_VerifySignedDataWithPublicKey() will affect > signatures on certificates, crls, and I think OCSP. This is where we > currently reject forbidden hash functions by policy. This would effect all > signature types (EC, DSA, RSA), so we probably would want a normalized > notion of key strength for these algorithms. This seems most reasonable to me, though I haven't looked into all the details yet. We only accept safe ECC signatures without MORE_THAN_SUITE_B by default anyway, right, so we could ignore them for now? We could make it work like the signature hash algorithm checks: have a new API call like CERT_SetRSAMinimumKeySize(unsigned int sizeInBits) and CERT_SetDSAMinimumKeySize(unsigned int sizeInBits). Long-term, for libpkix, see also bug 695268. > 2) Checks in Mozilla. We can check the chain returned from NSS and look at > the key sizes of the various certs in the chain. This gives the finest grain > checks, but is localized to mozilla. This is probably preferable to checks > in the NSS verify function. This would also work, if the NSS team doesn't want to make changes in NSS.

What is a reasonable cutoff? AFAICT, it would be higher than 512 bits. We may need to have a different cutoff for code signing signatures (e.g. for extensions) and for SSL. And for SSL, we might need different cutoff for untrusted issuer, user-added CA (not issued by a CA in our program), DV, and EV, And/or we many need to treat the error differently (warning vs soft fail vs hard fail, require fresh OCSP) depending on some other factors. It would be useful to know from our CAs what their current and recent policies are regarding key size. Let's get a list of all CAs in our program that have issued certificates with RSA keys less than 1024 bits that would still be valid (not expired) today. It seems too complicated to do anything for Firefox 8 but we might be able to do something for 9, and definitely for 10.

Numbers from the Observatory: <=512 4168 <1000 4208 <1023 4208 <1024 5185 It looks like a number of certs are 1023 bits, so banning < 1023 is the target that I suggest.

Thanks Adam. I also have the observatory data but it would be useful to know which, if any, CAs feel they would have customers negatively affected by this change.

> This seems most reasonable to me That would be my recommendation too. > We only accept safe ECC signatures without MORE_THAN_SUITE_B by default anyway, > right, so we could ignore them for now? To be clear, we only *sign* with ECC signatures if MORE_THAN_SUITE_B is on, We always validate ECC signatures of USE_ECC is on. Also the ECC tests have some applicability to DSA. There are 2 parameters in a DSA key, P and q. The P has about the same security strength as and equally sized RSA key, and q has about the same security strength as a equally sized ECDSA key. The overall strength is the weaker of the two. > CERT_SetRSAMinimumKeySize(unsigned int sizeInBits) and > CERT_SetDSAMinimumKeySize(unsigned int sizeInBits) I think I prefer a key strength that would be applicable to many different algorithms. I prefer not to create a whole bunch of algorithm specific functions here, where an application may miss one. (In general, applications don't actually know or care what kind of signature they are dealing with, I'd rather not break that for this functionality). Besides, DSA would need two key sizes, the size of Prime and the size of Q (subPrime). It probably makes the most sense to make key strength to symmetric key strength. Though I'm not against mapping everything to the equivalent RSA key strength. bob

BTW: Note the current proposal would have no affect on the key size of the *LEAF* cert. There is a chance that code signing may be affected. I believe the check for the code signing signature is done in PSM, so I'm not sure what function it uses. If it doesn't use CERT_VerifySignedData() and friends then it wouldn't be affected.

See also https://mxr.mozilla.org/security/ident?i=BAD_RSA_KEY_SIZE

(In reply to Adam Langley from comment #5) > It looks like a number of certs are 1023 bits, so banning < 1023 is > the target that I suggest. That's mainly an artifact of OpenSSL's way of reporting the RSA modulus size. Assuming that the size checks in NSS will be based on SECKEY_PublicKeyStrengthInBits(), rejecting < 1024 is fine. (For those wondering: OpenSSL uses BN_num_bits() when printing the details of an RSA key - see the NOTES section in BN_num_bytes(3) for further background information.)

(In reply to Robert Relyea from comment #7) > > We only accept safe ECC signatures without MORE_THAN_SUITE_B by default anyway, > > right, so we could ignore them for now? > > To be clear, we only *sign* with ECC signatures if MORE_THAN_SUITE_B is on, > We always validate ECC signatures of USE_ECC is on. But, don't we only work with Suite B curves (P-256, P-384, and P-521)? > > CERT_SetRSAMinimumKeySize(unsigned int sizeInBits) and > > CERT_SetDSAMinimumKeySize(unsigned int sizeInBits) > > I think I prefer a key strength that would be applicable to many different > algorithms. I prefer not to create a whole bunch of algorithm specific > functions here, where an application may miss one. (In general, applications > don't actually know or care what kind of signature they are dealing with, > I'd rather not break that for this functionality). Besides, DSA would need > two key sizes, the size of Prime and the size of Q (subPrime). > > It probably makes the most sense to make key strength to symmetric key > strength. Though I'm not against mapping everything to the equivalent RSA > key strength. I think we should avoid mappings like this. I also think it is likely that we will need different restrictions in different contexts. For example, we might end up with different requirements for the signing of extensions than for SSL. So, for products like Firefox, long-term an approach like bug 695268 is best, I think. Until then, we should take your other suggestion and do whatever checks we decide to do by walking up the cert chain after verification.

> But, don't we only work with Suite B curves (P-256, P-384, and P-521)? As shipped by mozilla. If you are built on Linux, and you've installed 3rd party module, you can use the full set of known ECC curves. > I think we should avoid mappings like this. I believe we can't avoid it. > > I also think it is likely that we will need different restrictions in different > contexts. It's a really bad idea to require applications to *Know* about the various internals of the different algorithms. The issue is when we add new algorithms, the application would just start using them. There's nothing specific today in Firefox that need to know what kind of signature is used to sign a cert, for instance. This means if we add a new algorithm, and Firefox has chosen some minimum level of security, then that new algorithm will end up bypassing that security level that Firefox wanted to set. We can set those security levels based on RSA usage if we want, or we can set them at the equivalent symmetric key usage, but either way we will have to do some sort of mapping. bob

(In reply to Robert Relyea from comment #8) > BTW: Note the current proposal would have no affect on the key size of the > *LEAF* cert. Is there a way to modify the current proposal to also block weak key sizes at the end-entity cert level?

(In reply to Robert Relyea from comment #12) > This means if we add a new algorithm, and > Firefox has chosen some minimum level of security, then that new algorithm > will end up bypassing that security level that Firefox wanted to set. In Firefox, we could/should just whitelist which algorithms we support, and cause use of algorithms we don't know about to fail. > We can set those security levels based on RSA usage if we want, or we can > set them at the equivalent symmetric key usage, but either way we will have > to do some sort of mapping. I forsee difficulty in us all agreeing to the same mappings. I think NSS should choose reasonable defaults but allow the application to implement its own rules. Also, restrictions need to be per-validation and even per-certificate-per-validation. For example, at some point we might require RSA 2048+ for extension code signatures but require only RSA 1024+ for SSL certificate signatures. Or, we might require RSA 2048+ for signatures of certificate but allow RSA 1024 to be used for data signed with those certificates. And, I imagine there will be a point where we might allow SHA-1 in some cert signatures but not others. Further, when we implement DOMCrypt, then the policies that we choose for certificate verification will have to be independent of the restrictions we place on DOMCrypt apps' signature and verification options.

We are supposed to be requiring RSA 2048+ for EV certs (bug 622859), but we won't be able to require that for non-EV certs. Since EV status is determined by the application and not by NSS, this is more evidence for the need for the application to be able to dynamically set the key strength requirements on a per-validation basis.

For years, NSS has implemented a function to enable applications (primarily Mozilla apps) to know about and display information about key strengths, including cert key strengths. But to this day, AFAIK, no Mozilla application uses SSL_GetChannelInfo. :( I'm sure that, with that function, PSM could achieve all the goals that Brian has described above.

Apple has now implemented this as a change in OS X 10.7.4 (also included in Security Update 2012-002 for 10.6.8): > Description: Certificates signed using RSA keys with insecure key lengths were accepted by > libsecurity. This issue is addressed by rejecting certificates containing RSA keys less than > 1024 bits. This affects apps using the system crypto library, including Safari, Chrome and Mail. Gerv

(In reply to Gervase Markham [:gerv] from comment #17) > Apple has now implemented this as a change in OS X 10.7.4 The check was added to tp_policyVerify() in libsecurity_apple_x509_tp's tpPolicies.cpp, FWIW: http://opensource.apple.com/source/libsecurity_apple_x509_tp/libsecurity_apple_x509_tp-55009/lib/tpPolicies.cpp [lines 2150-2158, or search for "rdar://6892837"] Maybe someone should point out the advantage of using macros in C/C++ to Apple (in http://opensource.apple.com/source/libsecurity_apple_csp/libsecurity_apple_csp-55003/lib/RSA_DSA_utils.cpp at least they were smart enough to realize this, cf. RSA_MAX_KEY_SIZE).

Add Microsoft to the list too, as of August 2012: http://blogs.technet.com/b/pki/archive/2012/06/12/rsa-keys-under-1024-bits-are-blocked.aspx Gerv

This is proof-of-concept code. Of course the value 1024 should be a macro, or preferably a preference. I am stuck at the moment picking the right return code. Since I do not want to touch nss, I am bound by the errors defined in security/nss/lib/util/SECerrs.h First I picked SEC_ERROR_DECRYPTION_DISALLOWED which gave a semi-appropriate error message of "Cannot decrypt: encrypted using a disallowed algorithm or key size." However, the user was not given the option to add an exception and continue anyway. So for the POC I now used SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED which works, but gives the wrong "technical details". Any suggestions? Other comments on the patch?

It seems I was a bit overzealous and also blocked DSS and DH keysizes under 1024 bits. It this inappropriate? security/nss/lib/ssl/ssl3con.c contains a check for DH keys smaller than 512 bits and returns SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY when found (no user exceptions allowed). For DSA FIPS 186 and SP 800-57 suggest 1024 is the minimum keysize anyway. Btw, I am using https://prosurf.sbb.ch/ as a test case for this code.

(In reply to Richard van den Berg from comment #20) > This is proof-of-concept code. Of course the value 1024 should be a macro, > or preferably a preference. Richard, thanks for the contribution. Definitely you have the idea right. One thing that is not-so-obvious is that there can be multiple certificate chains for a given cert: EE-1024 <- Intermediate-512 <- Root EE-1024 <- Intermediate-1024 <- Root If we (rightly) reject the first option, we have to try the second option. Unfortunately, the current SSLServerCertVerification code only tries one chain. Camilo is working on bug 764973 which adds a callback from libpkix that allows us (once we switch to libpkix) to use the logic you provided for each possible certificate chain. You can see how he is using that callback function in SSLServerCertVerification in bug 744204. > I am stuck at the moment picking the right return code. Since I do not want > to touch nss, I am bound by the errors defined in > security/nss/lib/util/SECerrs.h First I picked > SEC_ERROR_DECRYPTION_DISALLOWED which gave a semi-appropriate error message > of "Cannot decrypt: encrypted using a disallowed algorithm or key size." > However, the user was not given the option to add an exception and continue > anyway. So for the POC I now used > SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED which works, but gives the wrong > "technical details". Any suggestions? Other comments on the patch? This is a problem that Camilo also has. I will ask the other NSS developers what they recommend we do at our next NSS meeting.

(In reply to Richard van den Berg from comment #21) > It seems I was a bit overzealous and also blocked DSS and DH keysizes under > 1024 bits. It this inappropriate? Thanks again for working on this. I talked about your patch with Camillo. He is going to refactor his patch to help provide a base for you to use for your patch. That work will be done in bug 790809. I suggest that you modify the logic to work on a whitelist basis instead of a blacklist basis. This way, we can ensure that we've done explicitly done a security analysis of any new key types before Firefox starts supporting them: unsigned int minimumBits = 1024; switch (publickey->keyType) { case ecKey: // TODO: we should check which curve return SECSuccess; case dsaKey: // fall through case rsaKey: // TODO: the limit should be 2048 for EV certificates, // accoridng to the policy minimumBits = 1024; break; default: PR_SetError(SEC_ERROR_UNSUPPORTED_KEYALG, 0); return SECFailure; } > security/nss/lib/ssl/ssl3con.c contains a check for DH keys smaller than 512 > bits and returns SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY when found (no user > exceptions allowed). We do not support cipher suites that use static DH keys anyway. See http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSComponent.cpp#1004 I will find out about the error code issue soon. Let me know what I can do to further help you with this.

Thanks Brian. I'll wait until bug 764973 and bug 790809 have landed. Since I don't know how long that will be I've attached the current state of my code. It includes your switch suggestion and adds 2 new prefs: security.ssl.rsa_minimum_key_size and security.ssl.dsa_minimum_key_size.

I'm glad to see this bug being worked on! I'd like to ask for one clarification... Some CAs have valid certs with 1023-bit keys. (https://wiki.mozilla.org/CA:Communications#Responses) Can we change minimumBits to 1023? (instead of 1024)

(In reply to Kathleen Wilson from comment #25) > I'm glad to see this bug being worked on! > > I'd like to ask for one clarification... > > Some CAs have valid certs with 1023-bit keys. > (https://wiki.mozilla.org/CA:Communications#Responses) > > Can we change minimumBits to 1023? (instead of 1024) I don't see a problem with that. I'd prefer to be consistent with Google and Microsoft here.

Although I actually support the idea of testing < 1023, Chrome does require 1024 bits.

Your concern about a key length of 1023 bits has already been addressed in comment 10.

(In reply to Richard van den Berg from comment #20) > > I am stuck at the moment picking the right return code. Since I do not want > to touch nss, I am bound by the errors defined in > security/nss/lib/util/SECerrs.h You can use SEC_ERROR_INVALID_KEY or SEC_ERROR_BAD_KEY. Note: although the error message for SEC_ERROR_BAD_KEY seems like a closer match, I found that NSS is using SEC_ERROR_INVALID_KEY in this situation, for example, see http://mxr.mozilla.org/security/ident?i=BAD_RSA_KEY_SIZE http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/freebl/rsa.c&rev=1.44&mark=831-832#831 and http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11err.c&rev=1.14&mark=68#68 NSS may not be using these two error codes consistently. Also, it is fine to add a new SEC_ERROR_BAD_KEY_SIZE error code to NSS.

Richard, are you still working on this? Are you only blocked on Bug 790809?

I can my patch so it will work with all the changes from last year. If bug 790809 is going to land soon, it would be best to wait for it though.

Does not look like it will land soon: (In reply to Camilo Viecco (:cviecco) from Bug 790809 comment #13) > Hello Florian, the goal now is to stop depending on libpkix and use > insanity::pkix. Depending on the timing of insanity this will be done or > will be marked not to do. Will insanity::pkix help you? I can mark Bug 878932 and/or Bug 915930 (or any other bug you depend on) as dependencies if you want me to, otherwise if you want this to land without insanity::pkix or the libpkix, I will drop the depencies. Let me know.

/depencies/dependancies/

insanity::pkix is now mozilla::pkix and is (as of now) enabled for v31. Richard, are you still interested in this bug? Otherwise, I'll reset the assignee. With mozilla::pkix doing the certificate checking, is this bug even valid anymore, and if yes, is it in the right component?

I am still interested, but lack the spare cycles to continue working on it at the moment. Feel free to reset the assignee so someone else can pick it up.

Richard, thanks for contributing the original patch here! I've reset the assignee based on your request. Florian, this bug is still relevant and this is the right component.

Something like this? This patch seems to work when I raise the rsaKey limit to 2048 and do the following manual tests: - Visit a page from Bug 986014 Comment 1 and check it is blocked - Visit https://pinningtest.appspot.com and check pinning still works - Visit https://www.google.ca and check the 1024 bit Equifax CA chain is rejected, but the alternate chain is accepted I haven't written any automated tests yet, or integrated/tested with anything other than mozpkix though.

Comment on attachment 8444202 [details] [diff] [review] bug360126_WIPv1.patch; Original patch by Richard van den Berg Review of attachment 8444202 [details] [diff] [review]: ----------------------------------------------------------------- Looks initially good. You still needs some tests for this. See the test_cert_* files in security/manager/ssl/test/unit for inspiration ::: security/certverifier/CertVerifier.cpp @@ +189,5 @@ > + !CERT_LIST_END(node, certList); > + node = CERT_LIST_NEXT(node)) { > + SECKEYPublicKey* publicKey = CERT_ExtractPublicKey(node->cert); > + > + if (publicKey) { To make the code more readable, just return failure early when !puclicKey. @@ +190,5 @@ > + node = CERT_LIST_NEXT(node)) { > + SECKEYPublicKey* publicKey = CERT_ExtractPublicKey(node->cert); > + > + if (publicKey) { > + unsigned int minimumBits = 1024; You always use minimumBits = 1024; So lets make it a const, and remove all the assingments in the switch. @@ +192,5 @@ > + > + if (publicKey) { > + unsigned int minimumBits = 1024; > + unsigned int actualBits = SECKEY_PublicKeyStrengthInBits(publicKey); > + switch (publicKey->keyType) { Maybe someting like: const KeyType keyType = publicKey->keyType; if (keyType != rsa || keyTtype != dsa || keyTtype != ecKey) { PR_SetError .... return SECFailure; } if (keyTtype == ecKEY) { continue; } Is more readable than the switch?

Comment on attachment 8444202 [details] [diff] [review] bug360126_WIPv1.patch; Original patch by Richard van den Berg Review of attachment 8444202 [details] [diff] [review]: ----------------------------------------------------------------- Hi. First, thanks for writing the patch. IMO, The chain verification callback is not the best place to put this check. I suggest that you instead put the check in pkixkey.cpp's VerifySignedData function. This way, you will also catch the case where an OCSP responder certificate is using a key that is too small; your current patch doesn't catch this case. Also, the code would be simpler because you won't have to loop through the chain. And then the check would also be enforced for AppSignatureVerifiation/AppTrustDomain, which don't use CertVerifier. However, my suggestion won't cover the case of a too-small end-entity certificate key. I suggest you handle that by having pkixbuild.cpp's BuildCertChain call a new function "SECStatus TrustDomain::CheckEndEntityPublicKey(const SECItem& endEntitySubjectPublicKeyInfo)" and then add a function to pkixkey.cpp that does the key checking that VerifySignedData does, except without the actual signature verification. Another advantage of my suggested approach is that it will stop traversing chains of 1024-bit certificates earlier. Consider: int-2048-a <-- int-1024-a <- int-1024-b / \ ee-2048 root-2048 \ / int-2048-a <-- int-2048-b <------------ If you wait until the chain verification callback is called, you may end up wastefully building the top chain completely before rejecting it. With my suggested approach, you reject that chain halfway through and switch to the other chain. ::: security/certverifier/CertVerifier.cpp @@ +192,5 @@ > + > + if (publicKey) { > + unsigned int minimumBits = 1024; > + unsigned int actualBits = SECKEY_PublicKeyStrengthInBits(publicKey); > + switch (publicKey->keyType) { I think the switch is much more readable. Also, using switch is what we do everywhere else.

(In reply to Cykesiopka from comment #37) > I haven't [...] integrated/tested with anything other than mozpkix though. As far as Gecko/Firefox/PSM are concerned, mozilla::pkix is now the only thing you need to worry about. If you want to contribute similar checks to the NSS certificate verification code (which I just removed the option of using in Gecko), then please do that in a separate bug.

(In reply to Camilo Viecco (:cviecco) from comment #38) > Looks initially good. You still needs some tests for this. See the > test_cert_* files in security/manager/ssl/test/unit for inspiration Thanks for the pointer! > @@ +190,5 @@ > > + node = CERT_LIST_NEXT(node)) { > > + SECKEYPublicKey* publicKey = CERT_ExtractPublicKey(node->cert); > > + > > + if (publicKey) { > > + unsigned int minimumBits = 1024; > > You always use minimumBits = 1024; So lets make it a const, and remove all > the assingments in the switch. Sure.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #39) > IMO, The chain verification callback is not the best place to put this > check. I suggest that you instead put the check in pkixkey.cpp's > VerifySignedData function. OK, I'll try what you suggested instead. (In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #40) > As far as Gecko/Firefox/PSM are concerned, mozilla::pkix is now the only > thing you need to worry about. If you want to contribute similar checks to > the NSS certificate verification code (which I just removed the option of > using in Gecko), then please do that in a separate bug. My main interest is Gecko/Firefox/PSM anyways, so less complexity sounds good...

Hopefully this is close to what you suggested in Comment 39...

Adds some basic tests. I still need to figure out how I can get certutil to create 1023 bit certs (if it's even possible), hence why the bad certs are 1008 bit instead. Hopefully this patch is going in the right direction though.

Comment on attachment 8446945 [details] [diff] [review] bug360126_tests_WIPv1.patch Review of attachment 8446945 [details] [diff] [review]: ----------------------------------------------------------------- This is very good. But you are not testing what you think Lets use the scheme used in test_cert_version and test_name_constraints (so that we use the certs directly and we dont have the overhead or timing issues associated with a tls server. (there is also the issue of ocsp which on windows can be problematic) (I am sorry I give you incorrect advise on what tests to look) so 1. make a new dir: security/manager/ssl/tests/unit/test_keysize/ 2. modify generate_cert_generic in security/manager/ssl/tests/unit/psm_common_py/CertUtils.py so that it has an additional keysize parameter (defaulted to 2048). 3. copy security/manager/ssl/tests/unit/test_name_contraints/generate.py into security/manager/ssl/tests/unit/test_keysize/ and modify 4. use test_cert_version.js (or test_name_contraints.js) as your template. (you need to add 5 certs to the db (2 roots 3 intermediates) for your 4 tests)

Comment on attachment 8446942 [details] [diff] [review] bug360126_WIPv2.patch; Original patch by Richard van den Berg Review of attachment 8446942 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good to me. ::: security/apps/AppTrustDomain.cpp @@ +179,5 @@ > return SECSuccess; > } > > +SECStatus > +AppTrustDomain::CheckEndEntityPubKey(const CERTSubjectPublicKeyInfo& endEntitySpki) NIT: Please spell out "Public". Note that it is spelled out everywhere else. @@ +181,5 @@ > > +SECStatus > +AppTrustDomain::CheckEndEntityPubKey(const CERTSubjectPublicKeyInfo& endEntitySpki) > +{ > + return ::mozilla::pkix::CheckEndEntityPubKey(endEntitySpki); Ditto. ::: security/apps/AppTrustDomain.h @@ +35,5 @@ > /*optional*/ const SECItem* stapledOCSPresponse, > /*optional*/ const SECItem* aiaExtension); > SECStatus IsChainValid(const CERTCertList* certChain) { return SECSuccess; } > > + SECStatus CheckEndEntityPubKey(const CERTSubjectPublicKeyInfo& endEntitySpki); NIT: It is OK to abbreviate SPKI but please use all caps unless it is by itself: "spki", "endEntitySPKI". ::: security/certverifier/NSSCertDBTrustDomain.cpp @@ +577,5 @@ > return SECFailure; > } > > +SECStatus > +NSSCertDBTrustDomain::CheckEndEntityPubKey(const CERTSubjectPublicKeyInfo& endEntitySpki) Ditto. @@ +579,5 @@ > > +SECStatus > +NSSCertDBTrustDomain::CheckEndEntityPubKey(const CERTSubjectPublicKeyInfo& endEntitySpki) > +{ > + return ::mozilla::pkix::CheckEndEntityPubKey(endEntitySpki); Ditto. ::: security/certverifier/NSSCertDBTrustDomain.h @@ +74,5 @@ > > virtual SECStatus IsChainValid(const CERTCertList* certChain); > > + virtual SECStatus > + CheckEndEntityPubKey(const CERTSubjectPublicKeyInfo& endEntitySpki); Please indent this similarly to how FindPotentialIssuers is indented: return type on same line as name. ::: security/pkix/include/pkix/pkix.h @@ +103,5 @@ > SECStatus VerifySignedData(const CERTSignedData* sd, > const SECItem& subjectPublicKeyInfo, > void* pkcs11PinArg); > > +// Check that the key size of an end entity cert meets requirements End the sentence with a period. ::: security/pkix/include/pkix/pkixtypes.h @@ +172,5 @@ > // checks are done. Called to compute additional chain level checks, by the > // TrustDomain. > virtual SECStatus IsChainValid(const CERTCertList* certChain) = 0; > > + // Check that the key size of an end entity cert meets requirements End the sentence with a period. @@ +174,5 @@ > virtual SECStatus IsChainValid(const CERTCertList* certChain) = 0; > > + // Check that the key size of an end entity cert meets requirements > + virtual SECStatus > + CheckEndEntityPubKey(const CERTSubjectPublicKeyInfo& endEntitySpki) = 0; Fix all the nits as mentioned above. ::: security/pkix/lib/pkixbuild.cpp @@ +392,5 @@ > > + if (trustDomain.CheckEndEntityPubKey(nssCert->subjectPublicKeyInfo) > + != SECSuccess) { > + return SECFailure; > + } Oops! I suggested that you name this "CheckEndEntityPublicKey" but actually it may be used to check a non-end-entity public key when BuildCertChain is called with EndEntityOrCA==MustBeCA. Just drop "EndEntity" from all the names. ::: security/pkix/lib/pkixkey.cpp @@ +42,5 @@ > + PR_SetError(SEC_ERROR_INVALID_ARGS, 0); > + return SECFailure; > + } > + > + static const unsigned int minimumBits = 1024; i suggest the name MINIMUM_NON_ECC_BITS. We usually name constants with ALL CAPS unless the name comes from a specification. @@ +48,5 @@ > + switch (publicKey->keyType) { > + case ecKey: > + // TODO: we should check which curve > + return SECSuccess; > + case dsaKey: // Fallthrough NIT: "fall through" @@ +51,5 @@ > + return SECSuccess; > + case dsaKey: // Fallthrough > + case rsaKey: > + // TODO: the limit should be 2048 for EV certificates, > + // according to the policy 1. Actually, I think this comment is out of date, since the 2048 minimum now applies to all certs. 2. Ping me in the bug about EV certificates being limited to 2048 bits if you want hints for fixing that bug. It looks pretty easy, given the code you've written. @@ +59,5 @@ > + return SECFailure; > + } > + > + if (SECKEY_PublicKeyStrengthInBits(publicKey) < minimumBits) { > + PR_SetError(SEC_ERROR_INVALID_KEY, 0); //TODO Create new error code? Nit: "// TODO(bug <bug number for a new bug>): Create a new error code." (Note whitespace and punctuation.) @@ +61,5 @@ > + > + if (SECKEY_PublicKeyStrengthInBits(publicKey) < minimumBits) { > + PR_SetError(SEC_ERROR_INVALID_KEY, 0); //TODO Create new error code? > + return SECFailure; > + } Please put this under "case rsaKey". @@ +71,5 @@ > +CheckEndEntityPubKey(const CERTSubjectPublicKeyInfo& endEntitySpki) > +{ > + ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey> > + pubKey(SECKEY_ExtractPublicKey(&endEntitySpki)); > + Nit: remove this blank line.

+ Address points from Comment 46

+ Switch to technique described in Comment 45 + Add DSA tests as well Thanks for the pointers Camilo!

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #46) > 2. Ping me in the bug about EV certificates being limited to 2048 bits if > you want hints for fixing that bug. It looks pretty easy, given the code > you've written. Thanks for the offer! Also I don't think it's possible to block 1017-1023 bit certs at the moment due to how SECKEY_PublicKeyStrengthInBits() is currently coded (but please correct me if I'm wrong): https://hg.mozilla.org/projects/nss/file/81e8b725bf7a/lib/cryptohi/seckey.c#l983 983 SECKEY_PublicKeyStrengthInBits(const SECKEYPublicKey *pubk) 984 { 985 unsigned size; 986 switch (pubk->keyType) { 987 case rsaKey: 988 case dsaKey: 989 case dhKey: 990 return SECKEY_PublicKeyStrength(pubk) * 8; /* 1 byte = 8 bits */ Maybe the summary of this bug should be changed?

Comment on attachment 8447770 [details] [diff] [review] bug360126_tests_v1.patch Review of attachment 8447770 [details] [diff] [review]: ----------------------------------------------------------------- Great. r+ with nits addressed ::: security/manager/ssl/tests/unit/head_psm.js @@ +34,5 @@ > const SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE = SEC_ERROR_BASE + 30; // -8162 > const SEC_ERROR_EXTENSION_VALUE_INVALID = SEC_ERROR_BASE + 34; // -8158 > const SEC_ERROR_EXTENSION_NOT_FOUND = SEC_ERROR_BASE + 35; // -8157 > const SEC_ERROR_CA_CERT_INVALID = SEC_ERROR_BASE + 36; > +const SEC_ERROR_INVALID_KEY = SEC_ERROR_BASE + 40; nit: add a comment with the decimal number value (helps debug) ::: security/manager/ssl/tests/unit/xpcshell.ini @@ +90,5 @@ > [test_ocsp_no_hsts_upgrade.js] > run-sequentially = hardcoded ports > # Bug 1009158: this test times out on Android > skip-if = os == "android" > + nit remove this space.

+ Deal with bitrot + Correct CheckPublicKey() param name from |endEntitySPKI| to |subjectPublicKeyInfo| I get the feeling that this will be bitrotted again soon (or already has been), but I told Harsh over IRC I would post a new patch tonight, so here it is.

(In reply to Camilo Viecco (:cviecco) from comment #50) > nit: add a comment with the decimal number value (helps debug) Done. > nit remove this space. Done. Thanks for the review! I'll post an updated test patch when the main patch is ready to be committed.

Comment on attachment 8452798 [details] [diff] [review] bug360126_v2.patch; Original patch by Richard van den Berg Review of attachment 8452798 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good. Mostly the documentation needs to be improved. ::: security/apps/AppTrustDomain.h @@ +34,5 @@ > /*optional*/ const SECItem* stapledOCSPresponse, > /*optional*/ const SECItem* aiaExtension); > SECStatus IsChainValid(const CERTCertList* certChain) { return SECSuccess; } > > + SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo); 1. No blank line above this, to be consistent with the rest. 2. Add MOZ_OVERRIDE to be consistent with (most of) the rest. ::: security/pkix/include/pkix/pkix.h @@ +102,5 @@ > const SECItem& subjectPublicKeyInfo, > void* pkcs11PinArg); > > +// Check that the key size of a cert meets requirements. > +SECStatus CheckPublicKey(const SECItem& subjectPublicKeyInfo); Please elaborate on what this function checks in more detail. For example, I would say "Checks, for RSA keys and DSA keys, that the modulus is at least 1024 bits." ::: security/pkix/include/pkix/pkixtypes.h @@ +206,5 @@ > // checks are done. Called to compute additional chain level checks, by the > // TrustDomain. > virtual SECStatus IsChainValid(const CERTCertList* certChain) = 0; > > + // Check that the key size of a cert meets requirements. 1. I suggest saying something along the lines of "Check that the key size, algorithm, and parameters of the given public key are acceptable." 2. Please document that VerifySignedData should do the same checks that CheckPublicKey does, and that VerifySignedData will only be called for keys that are not passed to VerifySignedData. 3. Update the documentation for VerifySignedData too, to say that the implementation must check the given key similarly to how it checks the given public key in CheckPublickey. 4. Provide some explanation for why some keys are passed to CheckPublicKey and others aren't. (The main reason is for efficiency.) ::: security/pkix/lib/pkixbuild.cpp @@ +331,5 @@ > if (rv != Success) { > return SECFailure; > } > > + if (trustDomain.CheckPublicKey(cert.GetSubjectPublicKeyInfo()) != SECSuccess) { Add a comment that we require the TrustDomain to check key sizes as part of their VerifySignedData implementation, but we need to check the key passed to BuildCertChain separately because it is never passed to the TrustDomain's VerifySignedData function. Or, if you put this documentation into pkixtypes.h, then add a comment here referring to that documentation. ::: security/pkix/lib/pkixkey.cpp @@ +34,5 @@ > > namespace mozilla { namespace pkix { > > SECStatus > +CheckPublicKeySize(SECKEYPublicKey* publicKey) I suggest you change this signature to: const SECItem& subjectPublicKeyInfo, /*out*/ ScopedSECKeyPublicKey& publicKey @@ +40,5 @@ > + if (!publicKey) { > + PR_NOT_REACHED("Invalid args to CheckPublicKeySize"); > + PR_SetError(SEC_ERROR_INVALID_ARGS, 0); > + return SECFailure; > + } Instead of doing a dynamic check that a pointer isn't null, just change the parameter type to a reference type so that null isn't even possible. (mozilla::pkix didn't do this consistently but I'm trying to fix it to be consistent as I go along). @@ +77,5 @@ > + ScopedPtr<SECKEYPublicKey, SECKEY_DestroyPublicKey> > + pubKey(SECKEY_ExtractPublicKey(spki.get())); > + if (!pubKey) { > + return SECFailure; > + } If you put the logic in the above 10 lines into CheckPublicKeySize like I suggest above, then you can avoid duplicating this logic into two functions.

Comment on attachment 8447770 [details] [diff] [review] bug360126_tests_v1.patch Review of attachment 8447770 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/tests/unit/test_keysize.js @@ +68,5 @@ > + check_fail_ca(load_cert(key_type + "-intBad-caOK", ",,")); > + check_fail(certFromFile(key_type + "-eeOK-intBad-caOK.der")); > + > + // OK CA -> OK INT -> Bad EE > + check_fail(certFromFile(key_type + "-eeBad-intOK-caOK.der")); Please also add a test case for where all the certificate keys are good, but the key of a delegated OCSP responder certificate is too small. It may be easier to add this test in test_ocsp_stapling.js or another OCSP test file instead of in this file.

Sorry to join the v2 party so late. The current patch has a const MINIMUM_NON_ECC_BITS = 1024. I would like to see this as a pref, possibly security.ssl.minimum_non_ecc_bits. If we do not want users to lower the 1024 minimum, the pref could be optional to increase the minimum only. In my current environment I would set this to 2048, and soon perhaps even 4096.

(In reply to Richard van den Berg from comment #55) > Sorry to join the v2 party so late. The current patch has a const > MINIMUM_NON_ECC_BITS = 1024. I would like to see this as a pref, possibly > security.ssl.minimum_non_ecc_bits. If we do not want users to lower the 1024 > minimum, the pref could be optional to increase the minimum only. In my > current environment I would set this to 2048, and soon perhaps even 4096. Richard, let's either do that in a separate bug, or just go ahead with the blocking of <2048-bit certificates by default. We shouldn't block this bug on adding a pref for that. Cykesiopka, my patch for bug 1036107 will probably land before you can update your patch. I will help you rebase your patch on top of it, if you need it. It looks worse than it is (just use "hg rebase" and a good three-way merge tool).

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #56) > (In reply to Richard van den Berg from comment #55) > > The current patch has a const > > MINIMUM_NON_ECC_BITS = 1024. I would like to see this as a pref, possibly > > security.ssl.minimum_non_ecc_bits. > > Richard, let's either do that in a separate bug, Agreed, I've opened bug 1037407 for this.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #53) > 2. Please document that VerifySignedData should do the same checks that > CheckPublicKey does, and that VerifySignedData will only be called for keys > that are not passed to VerifySignedData. I assume you meant "[...] and that CheckPublicKey will only". > 4. Provide some explanation for why some keys are passed to CheckPublicKey > and others aren't. (The main reason is for efficiency.) Sure. I skimmed through the mozilla:pkix code comments and didn't see an explanation for why this is, so I'm only mentioning efficiency for now. > Instead of doing a dynamic check that a pointer isn't null, just change the > parameter type to a reference type so that null isn't even possible. > (mozilla::pkix didn't do this consistently but I'm trying to fix it to be > consistent as I go along). Noted, I'll watch out for this in the future. > If you put the logic in the above 10 lines into CheckPublicKeySize like I > suggest above, then you can avoid duplicating this logic into two functions. Good suggestion, thanks!

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #56) > Cykesiopka, my patch for bug 1036107 will probably land before you can > update your patch. I will help you rebase your patch on top of it, if you > need it. It looks worse than it is (just use "hg rebase" and a good > three-way merge tool). Sure. I still need some time to solve and confirm fixes for some issues anyways. The rebase went fine, but thanks for your offer! FWIW, Here's a try run with updated versions of the two patches here plus the new OCSP test on top of attachment 8454282 [details] [diff] [review] from Bug 1036107: https://tbpl.mozilla.org/?tree=Try&rev=db79ebd89ce3 - The Windows build failure I assume I can fix with MOZ_NON_EXHAUSTIVE_SWITCH - The Android 4.0 test failure is baffling - I regenerated the relevant certs just a few hours ago - B2G ICS build failure I'm guessing is because of a missing #include "pkix/nullptr.h" in attachment 8454282 [details] [diff] [review] - I have no idea what the Mulet OSX build failure is about I will attach the OCSP test once I confirm it tests what I think it does...

+ Address review comments from Comment 55 + Add code to get the patch compiling across all platforms (hopefully) Additional try link to complement the one I posted previously: https://tbpl.mozilla.org/?tree=Try&rev=652465d13269 Win 8 tests are still pending, but the rest are all green.

Comment on attachment 8455899 [details] [diff] [review] bug360126_v3.patch; Original patch by Richard van den Berg Review of attachment 8455899 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixkey.cpp @@ +53,5 @@ > + } > + > + static const unsigned int MINIMUM_NON_ECC_BITS = 1024; > + > +#ifdef _MSC_VER I couldn't get the pragma stuff from MOZ_NON_EXHAUSTIVE_SWITCH to work on this switch for some reason, so I've gone with the push-disable-pop pattern.

I think this tests the case from Comment 54, but apologies if it doesn't...

Comment on attachment 8455899 [details] [diff] [review] bug360126_v3.patch; Original patch by Richard van den Berg Review of attachment 8455899 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, besides the issue with the switch statement. I will re-review this right away when you post the new version. ::: security/pkix/lib/pkixkey.cpp @@ +53,5 @@ > + } > + > + static const unsigned int MINIMUM_NON_ECC_BITS = 1024; > + > +#ifdef _MSC_VER I suggest that, instead of doing this, you just make the switch exhaustive by also explicitly handling these cases: nullKey = 0, fortezzaKey = 3, /* deprecated */ dhKey = 4, keaKey = 5, /* deprecated */ rsaPssKey = 7, rsaOaepKey = 8 Just unconditionally return SEC_ERROR_INVALID_KEY for those types. @@ +86,5 @@ > + > +SECStatus > +CheckPublicKey(const SECItem& subjectPublicKeyInfo) > +{ > + ScopedSECKeyPublicKey publicKey; NIT: I suggest renaming this to "unused".

Comment on attachment 8455900 [details] [diff] [review] bug360126_ocsp_test_v1.patch Review of attachment 8455900 [details] [diff] [review]: ----------------------------------------------------------------- Looks like you are on the right track. r- mostly because I don't understand some parts of the patch, and because I think it would be easier to maintain this if it were put in test_ocsp_stapling.js. ::: security/manager/boot/src/StaticHPKPins.h @@ +88,5 @@ > "WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; > > /* End Entity Test Cert */ > static const char kEnd_Entity_Test_CertFingerprint[] = > + "CVZnLwnduVt4+YewjZk8KmxWixCVp3ercTfBGExJqxI="; Is this change part of your patch? @@ +1066,5 @@ > // Pinning Preload List Length = 325; > > static const int32_t kUnknownId = -1; > > +static const PRTime kPreloadPKPinsExpirationTime = INT64_C(1413863051320000); Is this change part of your patch? ::: security/manager/ssl/tests/unit/test_keysize_ocsp.js @@ +32,5 @@ > + > + add_tls_server_setup("OCSPStaplingServer"); > + > + add_ocsp_test("keysize-ocsp-delegated.example.com", > + getXPCOMStatusFromNSS(SEC_ERROR_INVALID_KEY)); Does putting just this add_ocsp_test() line into test_ocsp_stapling.js work? If so, let's do that.

(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #64) > ::: security/manager/boot/src/StaticHPKPins.h > @@ +88,5 @@ > > "WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; > > > > /* End Entity Test Cert */ > > static const char kEnd_Entity_Test_CertFingerprint[] = > > + "CVZnLwnduVt4+YewjZk8KmxWixCVp3ercTfBGExJqxI="; > > Is this change part of your patch? Yes. I did this because of the comment near the top of generate_certs.sh: # NB: You must run genHPKPStaticPins.js after running this file, since its # output (StaticHPKPins.h) depends on default-ee.der And indeed not doing this causes test_pinning.js to fail. > @@ +1066,5 @@ > > // Pinning Preload List Length = 325; > > > > static const int32_t kUnknownId = -1; > > > > +static const PRTime kPreloadPKPinsExpirationTime = INT64_C(1413863051320000); > > Is this change part of your patch? Same as above. > ::: security/manager/ssl/tests/unit/test_keysize_ocsp.js > @@ +32,5 @@ > > + > > + add_tls_server_setup("OCSPStaplingServer"); > > + > > + add_ocsp_test("keysize-ocsp-delegated.example.com", > > + getXPCOMStatusFromNSS(SEC_ERROR_INVALID_KEY)); > > Does putting just this add_ocsp_test() line into test_ocsp_stapling.js work? > If so, let's do that. Done.

+ Address review comments from Comment 63 (but using SEC_ERROR_UNSUPPORTED_KEYALG for the nullKey etc cases as agreed over IRC)

+ Add a comment with the decimal number value for SEC_ERROR_INVALID_KEY in head_psm.js to help debug + Update test_keysize.js top level comment to mention DSA as well + Use equal() instead of deprecated do_check_eq()

+ Put relevant add_ocsp_test() line into test_ocsp_stapling.js instead

Thanks for the reviews! https://tbpl.mozilla.org/?tree=Try&rev=673f522225d1

https://hg.mozilla.org/integration/mozilla-inbound/rev/83b81059b2a2 https://hg.mozilla.org/integration/mozilla-inbound/rev/7c53c5e9bd3a https://hg.mozilla.org/integration/mozilla-inbound/rev/c712bbb43cad

https://hg.mozilla.org/mozilla-central/rev/83b81059b2a2 https://hg.mozilla.org/mozilla-central/rev/7c53c5e9bd3a https://hg.mozilla.org/mozilla-central/rev/c712bbb43cad