The patch for bug 360126 implements a hard coded minimum key size for non-ECC certificates. This should be user configurable with a pref, probably named security.ssl.minimum_non_ecc_bits

Implementing this should ignore any value less than 2048, possibly with a warning message to the user if such a value is specified.

Nobody can help us?

Also for legacy self-signed certificates that is used in routers etc. Eg, I read that Tomato used to use it as the default.

Also for legacy self-signed certificates that is used in routers etc. Eg, I read that Tomato used to use 512-bit RSA as the default.

I'm going to attach some WIPs I have for this, in case any one wants to work on this bug. (Although TBH I'm not very convinced this pref would add much security value at the moment over just fixing Bug 1049740).

Notes: - The approach taken here is to only allow bumping the size requirement higher. Anything lower than the default is silently ignored. - Bug 1084606 allowed overrides for ERROR_INADEQUATE_KEY_SIZE, so I feel allowing arbitrary values here would just be a foot-gun. - DSA support was removed from mozilla::pkix, so the variables names are changed to match that reality.

This patch adds some basic tests. Test cases should probably be added to test_keysize_ev.js as well.

Comment on attachment 8563352 [details] [diff] [review] bug1037407_min-key-size-pref_WIPv1.patch Review of attachment 8563352 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/certverifier/CertVerifier.cpp @@ +42,5 @@ > , mOCSPStrict(osc == ocspStrict) > , mOCSPGETEnabled(ogc == ocspGetEnabled) > , mPinningMode(pinningMode) > + , mMinimumRSABits(std::max(prefMinRSABits, MINIMUM_RSA_BITS_DV)) > + , mMinimumRSABitsEV(std::max(prefMinRSABits, MINIMUM_RSA_BITS_EV)) Let's use "Min" and "Minimum" consistently. I think "Min" is better. ::: security/certverifier/NSSCertDBTrustDomain.h @@ +20,5 @@ > > extern const char BUILTIN_ROOTS_MODULE_DEFAULT_NAME[]; > > +const unsigned int MINIMUM_RSA_BITS_DV = 1024; > +const unsigned int MINIMUM_RSA_BITS_EV = 2048; These should be moved to nsNSSComponent::setValidationOptions. Then, have nsNSSComponent.cpp do the std::max() calculations. This way, SharedCertVerifier and CertVerifier end up getting a consistent value, and also we make sure that these two constants are only used in that one place. ::: security/manager/ssl/src/nsNSSComponent.cpp @@ +1353,5 @@ > prefName.EqualsLiteral("security.OCSP.require") || > prefName.EqualsLiteral("security.OCSP.GET.enabled") || > prefName.EqualsLiteral("security.ssl.enable_ocsp_stapling") || > + prefName.EqualsLiteral("security.cert_pinning.enforcement_level") || > + prefName.EqualsLiteral("security.tls.preferred_min_rsa_key_size")) { Remove "preferred_" and replace "size" with "bits". In general, we should always label sizes as either "bytes" or "bits".

(In reply to Cykesiopka from comment #5) > I'm going to attach some WIPs I have for this, in case any one wants to work > on this bug. > (Although TBH I'm not very convinced this pref would add much security value > at the moment over just fixing Bug 1049740). This pref should enable/disable the behavior in bug 1049740.

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #9) > (In reply to Cykesiopka from comment #5) > > I'm going to attach some WIPs I have for this, in case any one wants to work > > on this bug. > > (Although TBH I'm not very convinced this pref would add much security value > > at the moment over just fixing Bug 1049740). > > This pref should enable/disable the behavior in bug 1049740. Hmm, I was assuming Bug 1049740 would be fixed by just bumping MINIMUM_NON_ECC_BITS to 2048, but I guess basing Bug 1049740 on top of this bug would be more flexible if the compat impact is too high...

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #8) > Remove "preferred_" I wanted to make it clear that the pref value can be ignored, but sure.

(In reply to Cykesiopka from comment #6) > Notes: > - The approach taken here is to only allow bumping the size requirement > higher. Anything lower than the default is silently ignored. Ignoring a lower value should not be silent. Users will not understand why their setting is ignored.

(In reply to David E. Ross from comment #12) > Ignoring a lower value should not be silent. Users will not understand why > their setting is ignored. I'm not sure this is really necessary (I envisioned this pref to be hidden), but I have another WIP just sitting around that does exactly this, so I'll attach it.

This is an updated version of a WIP I had lying around. It sends a warning to the browser console if the value of the pref is lower than the default.

Please flag the patches you intend to get landed as review? by :briansmith (do this for every patch that needs to be landed!). Mark previous versions of patches as obsolete (you can do that in the "details" page of an attachment, and/or when you upload a new patch). Once patches are complete and review is granted on them, please add "checkin-needed" to the keywords so the sheriffs can land the patch – if you do not have the necessary privileges, you can ask me (or anybody else) to do it, just needinfo? me!

(In reply to Florian Bender from comment #15) > Please flag the patches you intend to get landed as review? by :briansmith > (do this for every patch that needs to be landed!). Mark previous versions > of patches as obsolete (you can do that in the "details" page of an > attachment, and/or when you upload a new patch). > > Once patches are complete and review is granted on them, please add > "checkin-needed" to the keywords so the sheriffs can land the patch – if you > do not have the necessary privileges, you can ask me (or anybody else) to do > it, just needinfo? me! Hi Florian, Thank you for taking the time to write this, but I'm already familiar with the process, and I do have the necessary privileges. So there isn't any more confusion: I don't intend to work on this bug in the near future. I have attached the WIPs for the benefit of anyone who wants to work on it in the mean time (e.g. for the benefit of ESR38). If someone wants to work on this bug, please feel free.

OK! Sorry, it's not my field of expertise. If this should make it to the next ESR, please note that we are approaching the end of the cycle (uplift in the week of Feb 24) and thus the work needs to land as soon as possible! Brian, since you were so kind to give feedback on the patch, are you able to take over here? Or do you know anyone who can work on this (or knows someone who knows someone …) before this cycle ends?

(In reply to Florian Bender from comment #17) > OK! Sorry, it's not my field of expertise. If this should make it to the > next ESR, please note that we are approaching the end of the cycle (uplift > in the week of Feb 24) and thus the work needs to land as soon as possible! > > Brian, since you were so kind to give feedback on the patch, are you able to > take over here? Or do you know anyone who can work on this (or knows someone > who knows someone …) before this cycle ends? Just some extra clarification: I'm not aware of anyone wanting to rush this into the next ESR, I was just trying to point out a possibility. Also, I apologise if I came across as agressive in my previous message. It wasn't my intention.

I'm not planning to work on this.

Since it appears that no one is planning on working on this, I'm going to close this as WONTFIX for now. If someone does want to drive this to completion, they are welcome to reopen this bug to do so.