An error code specific to inadequate key sizes should be added and used, instead of abusing SEC_ERROR_INVALID_KEY.

win32 Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e4a6d9c641b0 Asking for feedback for now because the wording probably needs to be improved/refined, and because of this: - Raise MINIMUM_NON_ECC_BITS in ::mozilla:pkix::CheckPublicKeySize() to 2048 - Go to https://ssltest29.bbtest.net/ (1024 bit certs + SIGNATURE_ALGORITHM_DISABLED) - Add temporary exception - Can continue on to site, instead of getting error page I'm not sure if this is because of something I haven't done in this patch/Bug 360126, if it's another issue, or if this is intended behaviour...

Comment on attachment 8471572 [details] [diff] [review] bug1052257_v0.patch Review of attachment 8471572 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! ::: security/manager/locales/en-US/chrome/pipnss/nsserrors.properties @@ +298,5 @@ > SEC_ERROR_LOCKED_PASSWORD=The password is locked. > SEC_ERROR_UNKNOWN_PKCS11_ERROR=Unknown PKCS #11 error. > SEC_ERROR_BAD_CRL_DP_URL=Invalid or unsupported URL in CRL distribution point name. > SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED=The certificate was signed using a signature algorithm that is disabled because it is not secure. > +MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE=The server or the OCSP Delegated Responder presented a certificate with an inadequately small public key size. (don't forget to update this when updating the other string) ::: security/pkix/lib/pkixnss.cpp @@ +303,5 @@ > "The server uses a certificate with a basic constraints extension " > "identifying it as a certificate authority. For a properly-issued " > + "certificate, this should not be the case." }, > + { "MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE", > + "The server or the OCSP Delegated Responder presented a certificate with " I would leave out "or the OCSP Delegated Responder". I might also say something like "a certificate with a key size that is too small to establish a secure connection". If we want to be specific about OCSP, we can add another error code and translate from MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE to MOZILLA_PKIX_ERROR_OCSP_INADEQUATE_KEY_SIZE like we do with SEC_ERROR_BAD_DER to SEC_ERROR_OCSP_MALFORMED_RESPONSE, for example.

(In reply to Cykesiopka from comment #1) > Asking for feedback for now because the wording probably needs to be > improved/refined, and because of this: > - Raise MINIMUM_NON_ECC_BITS in ::mozilla:pkix::CheckPublicKeySize() to 2048 > - Go to https://ssltest29.bbtest.net/ (1024 bit certs + > SIGNATURE_ALGORITHM_DISABLED) > - Add temporary exception > - Can continue on to site, instead of getting error page > > I'm not sure if this is because of something I haven't done in this > patch/Bug 360126, if it's another issue, or if this is intended behaviour... My guess is that the error that results in SIGNATURE_ALGORITHM_DISABLED is encountered before checking the key size, so that's the error reported, which happens to be overridable. We might have to do some refactoring to get this to work how we want, but that can be in a follow-up bug.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #2) > If we want to be specific about OCSP, we can add > another error code and translate from MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE > to MOZILLA_PKIX_ERROR_OCSP_INADEQUATE_KEY_SIZE like we do with > SEC_ERROR_BAD_DER to SEC_ERROR_OCSP_MALFORMED_RESPONSE, for example. Noted. (In reply to David Keeler (:keeler) [use needinfo?] from comment #3) > My guess is that the error that results in SIGNATURE_ALGORITHM_DISABLED is > encountered before checking the key size, so that's the error reported, > which happens to be overridable. We might have to do some refactoring to get > this to work how we want, but that can be in a follow-up bug. I see, thanks!

- Remove OCSP Delegated Responder bits from error messages + Add "that is too small to establish a secure connection" to error message

Comment on attachment 8472097 [details] [diff] [review] bug1052257_v1.patch Review of attachment 8472097 [details] [diff] [review]: ----------------------------------------------------------------- Great!

Thanks for the review! https://tbpl.mozilla.org/?tree=Try&rev=0f847feaa547 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4893c4497dff