Closed
Bug 471539
Opened 16 years ago
Closed 15 years ago
Stop honoring digital signatures in certificates and CRLs based on weak hashes
Categories
(NSS :: Libraries, defect, P1)
NSS
Libraries
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.3
People
(Reporter: nelson, Assigned: nelson)
References
()
Details
(Keywords: fixed1.9.1, verified1.9.0.13, verified1.9.0.14, Whiteboard: [sg:want P1])
Attachments
(1 file, 2 obsolete files)
12.19 KB,
patch
|
rrelyea
:
review+
julien.pierre
:
superreview+
|
Details | Diff | Splinter Review |
In light of the paper released today http://www.win.tue.nl/hashclash/rogue-ca/ NSS should at least have an option to stop honoring digital signatures based on MD5 hashes
Assignee | ||
Comment 1•16 years ago
|
||
I believe we only want to disable MD5 in certificate signatures, and not (yet) more broadly than that. I believe this patch does that. More testing is needed. Bob, please review. Of course, we will also want to stop creating certificates with MD5 based signatures in certutil, but that can come later.
Attachment #354834 -
Flags: review?(rrelyea)
Assignee | ||
Updated•16 years ago
|
Attachment #354834 -
Attachment description: pat → patch v1 for NSS trunk
Comment 2•16 years ago
|
||
I found a sample site that currently uses MD5 https://ssl247.com/legal.php Nelson asked me to apply the patch and test how Firefox behaves. The result: Secure Connection Failed An error occurred during a connection to ssl247.com. Peer's certificate has an invalid signature. (Error code: sec_error_bad_signature)
Comment 3•16 years ago
|
||
Please create a meaningful error code.
Comment 4•16 years ago
|
||
What about MD2? Yes, that's still used, surprisingly.
Assignee | ||
Comment 5•16 years ago
|
||
A number of additional suggestions have been made for how to address this problem, including: 1) Add a new error code explaining that the cert uses an insecure algorithm (requires changes to PSM also) 2) also disallow MD2 (easy enough) 3) Since the vulnerability only applies to certs whose serial numbers are predictable, only apply the limitation to certs whose serial numbers are "short" (e.g. less than 65 bits), on the grounds that larger serial numbers are very likely to be random 4) make this rejection of old hash algorithms configurable (e.g. allow it to be turned on or off) 5) invent a callback API by which applications can supply a function that decides whether to accept or reject the signature based on the hash algorithm and any other info it can glean from the certificate.
Comment 6•16 years ago
|
||
I was going to suggest 4) in particular. * we can't realistically turn off md5 support today in a shipping browser, but will want to as the current certs expire or are replaced. * Some concerned users will appreciate the ability to be more cautious in the meanwhile. Assuming they know how to set this option they know how to unset it should they encounter a site they really have to visit. This will be a hard failure; users can't add an exception for certs invalid for this reason, right? * SHA1 may someday fall to a similar attack. The NIST has started the process to define a SHA-3 so they must expect SHA-2 to fail eventually as well. If we disallow MD2 (which IMO we should) what happens to the roots that are self-signed using MD2? A couple of them expire soon anyway but this might clean out the rest.
Comment 7•16 years ago
|
||
It seems that eliminating MD5 is not so onerous, if the result is a jump to the "add security exception" flow. Maybe have a streamlined version of this flow?
Assignee | ||
Comment 9•16 years ago
|
||
The patch attached to this bug will affect signatures on certs and CRLs.
Summary: Stop honoring digital signatures based on MD5 hashes → Stop honoring digital signatures based on MD5 hashes in certificates and CRLs
Comment 10•16 years ago
|
||
Comment on attachment 354834 [details] [diff] [review] patch v1 for NSS trunk r+
Attachment #354834 -
Flags: review?(rrelyea) → review+
Comment 11•16 years ago
|
||
After reading the paper, I realized I misremembered. It was Lentra's 2005 collision paper I was remembering, which evidently used MD5. What the chaos paper did was show a practical example of this attack using real CAs. SHA-1 is fine for now, though we should continue to push the use of SHA-2 when possible. EV already requires SHA-1 and requires SHA-2 by 2010. Anyway, it seems this should be a reasonable patch. bob
Comment 12•16 years ago
|
||
Bob, > EV already requires SHA-1 That's correct, for Intermediate CA Certificates and end-entity EV Certificates. For self-signed Root CA Certificates, MD5 is allowed until 2010. (IINM, NSS always ignores the signatures on the self-signed Root CA Certificates in its trust list). > and requires SHA-2 by 2010. Incorrect. SHA-1 is currently allowed post-2010, but the EV Guidelines also say... "* SHA-1 SHOULD be used only until SHA-256 is supported widely by browsers used by a substantial portion of relying parties worldwide."
Comment 13•16 years ago
|
||
(In reply to comment #7) > It seems that eliminating MD5 is not so onerous, if the result is a jump to > the "add security exception" flow. This patch creates a fatal error that does not allow an exception. Allowing exceptions would not be an improvement, it would only further train users to bypass SSL errors and let sites get away with continuing to use weak certs.
Comment 14•16 years ago
|
||
Somewhat tangential, but will NSS ever decide to stop recognizing a particular hash algorithm, preventing even the possibility of configurable behavior? I for one would want MD5 configurable in the short term (maybe a couple years) and off permanently after that. (It's probably a good thing my somewhat hardline position isn't the consensus opinion here. :-) )
Comment 15•16 years ago
|
||
afaict, most NSS things are guarded by SSL_CipherPolicySet/SSL_SetPolicy. You can disable/enable SSL2, SSL3, I believe even TLS, although I haven't actually tried to do that. I was looking around to see how hard it would be to use SSL_CipherPolicySet to implement this such that people could turn off MD2, MD5, and SHA1 for testing purposes. I need to ask someone to explain why we have code which still calls SSL_SetPolicy even though the api says it's deprecated.
Comment 16•16 years ago
|
||
US CERT writes: <http://www.kb.cert.org/vuls/id/836068> > Do not use the MD5 algorithm > Software developers, Certification Authorities, website owners, > and users should avoid using the MD5 algorithm in any capacity. > As previous research has demonstrated, it should be considered > cryptographically broken and unsuitable for further use. I propose to * Announce now that we will drop MD5 in 1 or 3 months * Apply the patch in a future security release I think that waiting one more 1-2 years more is too long. If somebody finds a way that does not require two random parts (signed cert determined by attacker and attackers matching faked cert), but only one (take one of the already signed ones and create a matching fake cert), it's entirely over.
Comment 17•16 years ago
|
||
There is no sign anyone is close to coming up with a preimage attack, as opposed to this refinement of the collision attacks known for several years. In addition to the hash weakness this attack also took advantage of weaknesses in a particular CA's process which have now been fixed. We do not need to rush into invalidating up to 30% of known SSL sites (by the researcher's numbers). A year gives current certs time to expire. I'd say drop them in "3.2" nightlies and betas (now? six months?) but don't flip the switch on shipping Firefox before a year. Meanwhile if we had this user-configurable (prefs?) we'd be prepared should someone come up with a better attack, or an attack on SHA1 which seems eventually likely (especially if there's some kind of a breakthrough which leads to a preimage attack). Is this code in the part of NSS that is FIPS certified? IIRC there's a current push to get 3.12 certified and it'd be nice if this functionality either got certified or was in code that wouldn't invalidate the certification.
Comment 18•16 years ago
|
||
Plea from a user. Just give me a warning if the cert chain contains a weak (e.g. md5) cert and let me decide what to do, just as you let me decide what to do when the url doesn't match the certificate. I'm smart enough to tell the difference between some site I'm just browsing and my bank, and treat the exceptions differently. The warning could be either a pop-up or a new color on the security bar.
Comment 19•16 years ago
|
||
My 2 cents: Please don't use more colours to code states than strictly necessary. They are anyway quite unobstrusive and difficult to note and a significant percentage of people are in some way or another colour-blind. Asking the user _before_ transmitting any data to the site is definitely preferable.
Assignee | ||
Comment 20•16 years ago
|
||
In reply to comment 17, This patch affects code that is not inside of the FIPS boundary. I think that if One browser unilaterally disables MD5, and other browsers do not, in the short term, it will merely drive users to other browsers. So, it seems best (IMO) for the browsers to act together in concert on this. I think that's feasible. The different browsers have been remarkably cooperative in this area, especially within the CA-Browser forum. But this begs some questions about older browsers. Will we update FF2? Will we expect MS to update IE6?
Comment 21•16 years ago
|
||
Kai would have to comment on the idea of doing warnings. I don't think it's a good idea. Either we consider MD5 safe enough or we don't. As a rule we should not be asking users questions that the user can't reasonably answer. RE disabling: I agree with Nelson. I think Browsers need to act in concert. The critical browsers are all part of the CAB, making a suggestion to Sunset MD5 (after a reasonable period... like several months and a very public announcement) in that forum is probably a good idea. The only major browser not in the CAB is Safari. They should be contacted separately. I also think we should decide when we want to reject all MD5 only signatures. MD5 is now at the point (security wise) MD4 was when NSS decided not to implement it at all (MD4 is now completely broken). bob
Comment 22•16 years ago
|
||
Currently, having a certificate that is not signed by an authority known to the browser is a warning. The user can decide to "add a security exception" and still establish communication. If MD5 is used in the cert chain, and MD5 is considered insecure, then I believe the same situation obtains - i.e. the target certificate should be considered unsigned. So I believe the user should still be able to "add a security exception". In sum, having an insecure signature and having an unrecognized signer should be handled in the same way.
Comment 23•16 years ago
|
||
(In reply to comment #22) > Currently, having a certificate that is not signed by an authority known to the > browser is a warning. The user can decide to "add a security exception" and > still establish communication. > > If MD5 is used in the cert chain, and MD5 is considered insecure, then I > believe the same situation obtains - i.e. the target certificate should be > considered unsigned. So I believe the user should still be able to "add a > security exception". Even with a self-signed or otherwise unverified certificate, the promise is that if you decide add the exception, you continue to gain the benefits of TLS in terms of integrity and confidentiality. That isn't true if the cert is using technology which can be compromised by an attacker (though this is not the scenario that was presented in the 25c3 talk, I think it's clear as Bob R says, that md5 is in decline.) I do agree strongly with Dan though - if we had per-algorithm prefs to toggle support, it would allow us to respond more fluidly to similar incidents in the future, while at the same time giving people in strange legacy environments the ability to re-enable support while migrating. Nelson, is that much pain, or otherwise ill-considered?
Comment 24•16 years ago
|
||
NSS should implement a function that allows an application to toggle the behavior with or without Nelson's patch. void NSS_HashPrefsMD5InSigsSetDefault(PRBool enabled); {{ A more general API can be done later (which could allow to control the use of various hashes in various contexts). I believe such an API would require more thinking, and this additional thinking shouldn't delay us at this point. Something similar to existing SSL_CipherPrefSetDefault(). Maybe a function enum HashAndContext { hac_MD5_in_CertAndCRLSignatures, hac_MD5_everywhere, hac_SHA1_in_CertAndCRLSignatures, ... }; void NSS_HashPrefSetDefault(enum HashAndContext, PRBool enabled); }} NSS should add this function in NSS 3.12.3 PSM should add a preference, in both Firefox 3.0.x and Firefox 3.1.x, (about:config only) which can be used to disable MD5 / activate the patch. I propose to continue to have MD5 allowed for now, but this gives us the opportunity to change the default to disallowed at any time (with security updates), and allows security conscious users to disable it right away. I propose a new error code gets added to NSS, like SEC_ERROR_UNSAFE_ALGO (where the meaning UNSAFE is based on runtime defaults). Now let's say the runtime configuration is "forbid MD5" and the user gets an error page. Should the user be able to override the error, just like other errors? I think it should, but it would require much more logic to be added to both NSS and PSM. Similar to today's HandshakeCallback/AuthCertificateCallback functions, the NSS would need to call back into PSM and asks whether an override is active... More APIs to define...
Comment 25•16 years ago
|
||
I support adding a new NSS error code for weak/broken crypto algorithms. SEC_ERROR_WEAK_CRYPTO?
Assignee | ||
Comment 26•16 years ago
|
||
Responding to a lot of things in this bug... 1) Is Mozilla willing to disable MD5 cert signatures and "break the web" unilaterally, even if it drives users to IE? Johnathan, as Mozilla's representative to CABForum, have you broached this subject with the other browser representatives there? I have so many things on my plate now that I must prioritize my work carefully. Until I see that either a) the browsers are willing to act in concert on this, and collectively announce a date on which they will desupport it, or b) someone speaking for Mozilla says here that Mozilla is willing to unilaterally "break the web", I don't think this bug is really urgent. 2) Error code Whatever error code is set by CERT_VerifySignedDataWithPublicKey is likely to be overridden by the caller, (see examples below) so I wouldn't fret too much about which error code it is. http://mxr.mozilla.org/security/source/security/nss/lib/certdb/crl.c#1565 http://mxr.mozilla.org/security/source/security/nss/lib/certhigh/certvfy.c#180 http://mxr.mozilla.org/security/source/security/nss/lib/certhigh/certvfy.c#635 http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11nobj.c#793 3) hash configuration API design If we're going to make this configurable, then clearly we don't want to have MD5 be a special case. We want it to be possible to configure all the hashes. The configuration API needs to allow the caller to specify which hash it wants to enable/disable. Then there's the matter of the level of granularity of functional uses that will be controlled by this API. Presently, we're proposing to be able to control the acceptability of hashes in the context of verifying signatures on certificates and CRLs (and probably OCSP responses). Are those the only operations for which we will ever want to exert such control? What about: - SMIME signatures? - signatures on JAR and XPI files? - disabling MD5 alltogether? Here's a first draft proposal for an API. Two functions, a set and a get. The hash algorithm is specified using a SECOidTag (effectively an enum, whose value maps to/from OIDs). The data associated with each SecOidTag is a 32-bit value treated as a bit array. Initially, only 1 bit is defined, bit 0 (value 0x00000001) which specifies the use in signatures of certs and CRLs. The API is extensible in that we have 31 more bits to define as we think of uses for them. #define NSS_ALG_USABLE_IN_CERT_SIGNATURE 0x00000001 extern SECStatus NSS_SetAlgorithmPolicy(SECOidTag tag, PRUint32 mask, PRUint32 value); extern SECStatus NSS_GetAlgorithmPolicy(SECOidTag tag, PRUint32 *pValue); Either function may return SECSuccess or SECFailure with the error code set to SEC_ERROR_UNKNOWN_OBJECT_TYPE if the SECOidTag is out of range. The Get function outputs the 32-bit value associated with the SECOidTag. The Set function modifies the stored value according to the following algorithm: policy[tag] = (policy[tag] & mask) | value; Default value for any policy would be 0xffffffff (enabled for all purposes). How's that sound?
Assignee | ||
Comment 27•16 years ago
|
||
Here are a few more details about that API. The able to policy bits would be initialized when NSS is initialized. It would have no persistence. Each time NSS is initialized, the application must then make any changes to the policy table that it wishes to make. To disable MD5 for use in certs, the application might use this code: SECStatus rv; rv = NSS_SetAlgorithmPolicy(SEC_OID_MD5, ~NSS_ALG_USABLE_IN_CERT_SIGNATURE, 0);
Assignee | ||
Comment 28•16 years ago
|
||
Make that "The table of policy bits would be initialized ...".
Comment 29•16 years ago
|
||
(In reply to comment #26) > 1) Is Mozilla willing to disable MD5 cert signatures and "break the web" > unilaterally, even if it drives users to IE? > > Johnathan, as Mozilla's representative to CABForum, have you broached this > subject with the other browser representatives there? > > I have so many things on my plate now that I must prioritize my work > carefully. Until I see that either > a) the browsers are willing to act in concert on this, and collectively > announce a date on which they will desupport it, or > b) someone speaking for Mozilla says here that Mozilla is willing to > unilaterally "break the web", > I don't think this bug is really urgent. I have spoken with the CABForum about this issue. The message I gave the CAs there (not all CAs by a long shot, but representing the vast majority of issued certs on the public web) is that while we still need to have our own community discussions about the specifics, we consider MD5 to be on the way out. I told them we were unlikely to kill it tomorrow, but that no CA should be counting on support for MD5 to last. I suggested a general ballpark for retirement at 6-18 months, very much dependent on the state of deployed certificates on the web. I asked the CAs if that was a surprising answer, and a few confirmed that this was as they expected. The rest may need to consult counsel before they can answer a question like that. :) I have spoken to MS and Opera (Apple is not a member of the CABForum and George Staikos from Konqueror/Torch Mobile was not on the call) and while I am most decidedly not speaking for them, I got the impression that they were considering similar approaches and timelines. If they weren't though, and we were otherwise satisfied that the state of the deployed web (and the state of MD5) was such that we felt comfortable making the change, I don't feel that we would wait for other browsers to do so first. Making my policy thoughts here into concrete policy action is not really for this bug, of course. I hope it clarifies the state of my thinking, anyhow. How does that impact your estimation of priority? > How's that sound? From a distance, the API description sounds good.
Comment 30•16 years ago
|
||
A small plea (because I think we made a mistake in filing this bug) to everyone else: this bug is filed against NSS, and I think the NSS team has made it clear that they're talking about enabling library functionality (which will be the majority of my comment). Anyone interested in Firefox policy should please ignore this bug and campaign in a newsgroup or a Core/Firefox component. This bug is about NSS which is focussed on the underlying API. It's unfortunate that the bug wasn't originally written to limit its scope to something like "enable applications to ask NSS to stop honoring certain classes of digital certificates", but that's hindsight. --- the api looks technically supports the requirements. but my first read of it totally missed the way you were using the MASK. It's clever and does what's necessary but I really don't like the method name because i was totally mislead. Below please find what I ended up writing because I couldn't properly read your API: I'm slightly worried. if the NSS policy is later: ~FOO_BIT and we hard code that does SetPolicy( ... ~BAR_BIT ...) then we've stepped on ~FOO_BIT. This is likely to happen. So either all example code needs to be of this form: ... #define NSS_ALG_DEFAULT_POLICY 0xffffffff SECStatus rv; PRUint32 policy = NSS_ALG_DEFAULT_POLICY; rv = NSS_GetAlgorithmPolicy(SEC_OID_MD5, &policy); if (...FAILED(rv)) ... policy ^= NSS_ALG_USABLE_IN_CERT_SIGNATURE; rv = NSS_SetAlgorithmPolicy(SEC_OID_MD5, policy, 0); if (...FAILED(rv)) ... or you should seriously consider a different API. There are two possibilities. One: #define NSS_ENABLE_ALGORITHM PRBIT(31) #define NSS_DISABLE_ALGORITHM 0 * NSS_ENABLE_ALGORITHM #define NSS_ALG_USABLE_IN_CERT_SIGNATURE PRBIT(0) extern SECStatus NSS_SetAlgorithmPolicy(SECOidTag tag, PRUint32 policyChanges); extern SECStatus NSS_GetAlgorithmPolicy(SECOidTag tag, PRUint32 *pValue); to disable an algorithm: rv = NSS_SetAlgorithmPolicy(SEC_OID_MD5, NSS_DISABLE_ALGORITHM | NSS_ALG_USABLE_IN_CERT_SIGNATURE); to enable an algorithm: rv = NSS_SetAlgorithmPolicy(SEC_OID_MD5, NSS_ENABLE_ALGORITHM | NSS_ALG_USABLE_IN_CERT_SIGNATURE); Yes, I'm aware my api wastes one bit (it's also obviously possible to have distinct enable/disable methods). --- yes I eventually understood what the mask was doing. But it's complicated, with your API, to enable something one has to do: rv = NSS_SetAlgorithmPolicy(SEC_OID_MD5, NSS_ALG_USABLE_IN_CERT_SIGNATURE, NSS_ALG_USABLE_IN_CERT_SIGNATURE); or rv = NSS_SetAlgorithmPolicy(SEC_OID_MD5, NSS_ALG_USABLE_IN_CERT_SIGNATURE, 0xffffffff); neither of these make me happy. -- While I'm commenting, this may sound stupid, but are we really going to be miserly and try to use 1 bit to represent both CRLs and Certs? iiuc CRLs merely contain lists of serial numbers. While I'd hope that the CAs would be using SHA1 (or better [and iiuc better isn't supported]), and while I can understand that a similar risk of collision for an MD5 signature over a random sequence of serial numbers exists, as I haven't heard much information about the current state of CRLs, I think at least tentatively it'd be nice from an API perspective for it to be its own distinct bit, even if all consumers use NSS_ALG_USABLE_IN_CERT_SIGNATURE | NSS_ALG_USABLE_IN_CRL_SIGNATURE.
Assignee | ||
Comment 31•16 years ago
|
||
Comment on attachment 354834 [details] [diff] [review] patch v1 for NSS trunk This patch unconditionally excludes MD5 from certificate and CLR signatures. We're going to make it conditional.
Attachment #354834 -
Attachment is obsolete: true
Assignee | ||
Comment 32•15 years ago
|
||
Timeless's comment 30 tells me that something about the API I proposed in comment 26 is confusing. It it confused Timeless, then I expect it will confuse other less-capable developers too. Trouble is: I don't know what's confusing about it, so I don't know how to improve it. It seemed dirt simple to me. Here's an alternative definition of the interface. It's very slightly different from the previous one, but maybe enough to eliminate the confusion? #define NSS_USE_ALG_IN_CERT_SIGNATURE 0x00000001 /* CRLs and OCSP, too */ #define NSS_USE_ALG_IN_CMS_SIGNATURE 0x00000002 /* used in S/MIME */ extern SECStatus NSS_SetAlgorithmPolicy(SECOidTag tag, PRUint32 setBits, PRUint32 clearBits); extern SECStatus NSS_GetAlgorithmPolicy(SECOidTag tag, PRUint32 *pValue); Either function may return SECSuccess or SECFailure with the error code set to SEC_ERROR_UNKNOWN_OBJECT_TYPE if the SECOidTag is out of range. The Get function outputs the 32-bit value associated with the SECOidTag. Default value for any algorithm would be 0xffffffff (enabled for all purposes). The Set function modifies the stored value according to the following algorithm: policy[tag] = (policy[tag] & ~clearBits) | setBits; Examples: 1) enable MD5 in cert/CRL/OCSP signatures NSS_SetAlgorithmPolicy(SEC_OID_MD5, NSS_USE_ALG_IN_CERT_SIGNATURE, 0); 2) disable MD2 in cert/CRL/OCSP signatures NSS_SetAlgorithmPolicy(SEC_OID_MD2, 0, NSS_USE_ALG_IN_CERT_SIGNATURE); Timeless, Do you find this API definition to be clearer? If not, please write (here or in email) explaining what was/is unclear. Thanks.
Comment 33•15 years ago
|
||
You could separate set and clear into two functions. Also, the terms were confusing to me. Maybe call the param "algo" instead of "tag", and "usage" instead of "setBits", and I'd call the functions differently: NSS_GetAlgorithmUsage(SECOidTag algo, PRUint32 *usageFlags); NSS_EnableAlgorithmUsage(SECOidTag algo, PRUint32 usageFlags); NSS_DisableAlgorithmUsage(SECOidTag algo, PRUint32 usageFlags); (The more abstract something is, the harder it is to comprehend what you mean, here "tag" and "policy", because they could mean anything.)
Assignee | ||
Comment 34•15 years ago
|
||
Some questions for reviewers: 1) is PRUint32 enough? Should we make it PRUint64? 2) Should the GET/SET API allow only one bit to bet set/cleared/tested in a single call? 3) Should we forcibly disallow setting reserved (undefined) policy flags?
Attachment #365856 -
Flags: superreview?(wtc)
Attachment #365856 -
Flags: review?(rrelyea)
Assignee | ||
Updated•15 years ago
|
Priority: -- → P1
Summary: Stop honoring digital signatures based on MD5 hashes in certificates and CRLs → Stop honoring digital signatures based on MD2 & MD5 hashes in certificates and CRLs
Target Milestone: --- → 3.12.3
Comment 36•15 years ago
|
||
Why don't we implement the "secure by default" principle and just ban MD2 and MD4? CERT_VerifyCert would fail with the SEC_ERROR_WEAK_CRYPTO error if any cert in the cert chain (except the root CA cert) is signed with RSA-MD2 or RSA-MD4? Then apps, when upgrading to the new NSS, will automatically get verification failures on those certs. The apps that still want to support RSA-MD2 and RSA-MD4 can choose to stick with NSS 3.12.2 or add code to handle the SEC_ERROR_WEAK_CRYPTO error. If all apps need to both upgrade to the new NSS and add calls to NSS_SetAlgorithmPolicy to get the secure behavior, that'll be a big hassle and that's even more boilerplate code one needs to write to use NSS.
Assignee | ||
Comment 37•15 years ago
|
||
Wan-Teh, I gather you would prefer the patch to do this instead? /* initialize any policy flags that are disabled by default */ xOids[SEC_OID_MD2 ].notPolicyFlags = ~0; xOids[SEC_OID_MD4 ].notPolicyFlags = ~0; xOids[SEC_OID_PKCS1_MD2_WITH_RSA_ENCRYPTION ].notPolicyFlags = ~0; xOids[SEC_OID_PKCS1_MD4_WITH_RSA_ENCRYPTION ].notPolicyFlags = ~0; xOids[SEC_OID_PKCS5_PBE_WITH_MD2_AND_DES_CBC].notPolicyFlags = ~0; Any thoughts (objections) to using yet-another environment variable to override that?
Comment 38•15 years ago
|
||
I haven't read the patch. I only read the prototypes of the new public functions. What I proposed in comment 36 would not require adding new functions. I proposed a new error code, SEC_ERROR_WEAK_CRYPTO, for our signature and certificate verification functions. If any signature involved (except the signature in root CA certs) uses a weak hash algorithm such as MD2 and MD4, the signature or certificate verification function would fail with SEC_ERROR_WEAK_CRYPTO. I think my proposal is difficult to implement right, so you should feel free to continue work on your proposal. But it's important that your proposal also be "secure by default". I suspect that's what your code in comment 37 does. Only the apps that still want to support RSA-MD2 and RSA-MD4 signatures have to do extra work.
Updated•15 years ago
|
Flags: wanted1.9.0.x+
Flags: blocking1.9.1?
Whiteboard: [sg:want P1]
Assignee | ||
Comment 40•15 years ago
|
||
In reply to Wan-Teh's comment 38, I have no objection to adding a new error code. But a new API function is definitely going to be needed. If all the calls to the relevant function came from the application, and not from elsewhere within NSS libraries, then I would agree that new functions are not required. But there are numerous places inside NSS libraries where NSS makes decisions based on the outcome of a signature verification, and if we make all MD2 signatures fail unilaterally, without any way for the application to revert that, then those places will all fail with no recourse for the application. Finally, we come to the question of what to do by default. There are clearly two schools of thought here, and two groups with diametrically opposing values. One school says "be secure by default, even if that breaks compatibility and breaks existing deployments of NSS-based applications". The other school says "Don't ever break existing deployments of NSS-based applications in a way that requires modification (or even recompilation) of application code to return the application to a working state." If we change the default behavior, we break the users in the latter camp. Presently, the sponsors of NSS development fall into both camps. If we change the default behavior, the only solution to the problem for the users who cannot (or refuse to) rebuild their programs (the only one that immediately occurs to me) is to continue to use environment variables to select the old or new modes of operation. I'm beginning to think that NSS needs one environment variable that says "In all cases where a new behavior might break compatibility, choose the old behavior", rather than (or in addition to) continuing to have separate environment variables for every such case, as documented in https://developer.mozilla.org/En/NSS_reference:NSS_environment_variables
Comment 41•15 years ago
|
||
> NSS needs one environment variable that says
> "In all cases where a new behavior might break compatibility,
> choose the old behavior"
I think that's a bit too broad. Who wouldn't want compat, after all?
I'd rather make one "NSS_I_WANT_TO_BE_INSECURE". If set to "YES", you'd allow MD2 for verification. I can't see why anyone would want that, though.
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Assignee | ||
Comment 42•15 years ago
|
||
This patch incorporates most of the comments previously discussed in this bug. Rather than invent another new NSS error code this late, I found one that is already used to mean "Hash algorithm not allowed for this purpose" and reused it. It is SEC_ERROR_INVALID_ALGORITHM. In this patch, I use NSS_ALLOW_WEAK_SIGNATURE_ALG as the overriding environment variable. There was strong opposition to a single envar that means "override all compatibility changes since 3.11", so I won't do that. Review invited from any NSS peer.
Attachment #365856 -
Attachment is obsolete: true
Attachment #366681 -
Flags: superreview?(julien.pierre.boogz)
Attachment #366681 -
Flags: review?(rrelyea)
Attachment #365856 -
Flags: superreview?(wtc)
Attachment #365856 -
Flags: review?(rrelyea)
Comment 43•15 years ago
|
||
Comment on attachment 366681 [details] [diff] [review] Patch v3 - for review I am not opposed to the idea of this patch, but I don't think we should keep this environment variable forever. This should be only a stopgap measure to give time to people to migrate their infrastructure away from using MD2. I haven't had time to get up to speed on just how broken MD2 is. But I would prefer a stronger wording for the environment variable chosen. Something like : NSS_MAKE_VULNERABLE_TO_MD2_ATTACK_FIX_YOUR_CERTS_BEFORE_OCT_01_2009_OR_YOU_WILL_BE_SORRY . I know it's quite a mouthful, but it drives the point home. Other strong wordings are also welcome. The idea would be that we remove support for this environment variable in any NSS release made after that date - there would be no way to turn MD2 back in any later release of NSS . Another suggestion also - depending on how bad the attacks on MD2 are, we may also want to consider removing the MD2 implementation from softoken at a set date in the future.
Comment 44•15 years ago
|
||
Comment on attachment 366681 [details] [diff] [review] Patch v3 - for review r+ I do have some caveats: 1) we define a policy to turn off SMIME signatures, but there is not code for that. 2) we probably should have a policy for *all* signatures (including those the application validates using the VFY_ directly). 3) ssl3 uses PK11_Verify directly. Currently it either uses or includes a SHA1 hash, but if SHA1 is turned off by policy, SSL would continue to accept client auth certs even though they use sha1 or sha1+md5 signatures. I supposed we can live with that until TLS 1.2 where we have hash agility, but we should make the conscious decision to do so rather than do so by default. Anyway this patch provides important step forward, and is self contained, so an r+ bob bob
Attachment #366681 -
Flags: review?(rrelyea) → review+
Comment 45•15 years ago
|
||
Comment on attachment 366681 [details] [diff] [review] Patch v3 - for review I spoke to Nelson for a while about this since I was lacking background. I have several concerns. A) This patch only allows MD5 to be turned off programmatically. MD5 is on by default, and cannot be turned off by environment variable. I think this may be an issue for some end customers at Sun who want to be secure, as many Sun applications will not make use of the programmatic method to turn off MD5. One way to improve this would be to have finer-grain environment variables, ie. a way to turn off each algorithm separately, rather than having one environment variable controlling a set of 4 algorithms. B) This may not be the best place to deal with MD2 and MD4. My understanding is that MD2 and MD4 are far more broken than MD5, which is why the patch turns them off by default for signature verification. My concern is that this may not go far enough. Depending on how much computational power is needed to break MD2 and MD4, even short-term hashes may be at risk. As Bob pointed out in his review, there are other uses of these algorithms that this patch doesn't address, and didn't intend to address. Plugging these other holes would require changes to several more places. Because of the additional security concerns on MD2 and MD4, I think it would be preferable to throw the switch for them in a different place. Since we have a layered architecture in NSS, we should take advantage of it to turn off all uses of MD2 and MD4. The 2 places I suggest we add a switch (with MD2 and MD4 off by default) are : 1) in pk11wrap This should take care of all uses by higher levels, including for certificate signatures, S/MIME, VFY APIs, and of course pk11wrap APIs. The algorithms should be off by default. It should be possible to turn them back on either by environment variable or programmatically, as in this patch. For the environment variable method, we may want fine-grain control for each algorithm, not one variable controlling a bunch of algorithms together. 2) in softoken or freebl This is needed to prevent use of MD2 and MD4 by programs such as Java apps that only use the PKCS#11 but not the higher-level NSS libraries. softoken already has a method of passing initialization parameters in to C_Initialize. This may not be acceptable to many applications, however. So we may want to also use the same environment variables as I propose to use in pk11wrap to turn off these algorithms in softoken/freebl. If we add those switches for MD2 and MD4 at those 2 other layers I propose, then there is no longer a need for controlling MD2 and MD4 in the certificate verification layer as in this patch, as it would be redundant. At the time being, we probably would only require an environment variable at for MD5 in that layer.
Attachment #366681 -
Flags: superreview?(julien.pierre.boogz) → superreview-
Assignee | ||
Comment 46•15 years ago
|
||
Julien, there is no agreement that MD5 should be universally disabled, or even disabled for all cert signatures. By some estimates, turning off support for MD5 now would break 10-20% of https web sites. Mozilla should work with the other browser vendors to have them all agree to publish a drop-dead date for MD5, and we can drop it at that point. The scope of this bug is specifically about the use of certain hashes in signatures processed by NSS's higher layers. The scope excludes other non-signature uses of these hashes, and excludes use of Softoken without using libNSS3. It's true that the patch here does nothing to prevent programs that use Softoken by itself from using Softoken's MD2 and MD4/5, but that is outside the scope of this bug. If you want to disable all uses of MD2, 4 & 5 in Softoken, please file a bug for that. If you want to disable all uses of MD2, 4 & 5 for things other than signatures, please file a bug for that.
Comment 47•15 years ago
|
||
Nelson, Re: comment 46, 1) I am well aware that there is no current agreement on disabling MD5 universally. I don't have a problem with MD5 remaining turned on by default in the library for the time being. But the MD5 attacks have already been published, and I think there needs to be a way for end-users to turn it off earlier if they wish to. Individual users don't necessarily have to wait for all application vendors to decide to change their default. Your patch allows MD5 to be turned off only programmatically, and so Sun customers who cannot use the programmatic method (or users of any other products that haven't had a code change yet) will not be able to if they wish. There needs to be an environment variable support for MD5 in signatures in NSS somehow, and it's missing from your patch. 2) Regarding MD2 and MD4, I'm OK with filing a separate bug to turn MD2 and MD4 off in 2 other places. The point I was trying to make, and why I made it as part of my review comments to your patch, is that we should turn MD2 and MD4 off in these 2 places *instead* of turning them off in the signature verification code. If we choose to make these MD2/MD4 changes in a separate bug, then the scope of this bug would be reduced to be about MD5 in signatures only, as it used to be.
Comment 48•15 years ago
|
||
I filed bug 482882 about disabling all uses of MD2 and MD4.
Comment 49•15 years ago
|
||
Comment on attachment 366681 [details] [diff] [review] Patch v3 - for review You should explain why you store notPolicyFlags instead of policyFlags in privXOid. The reason (for static initialization to 0 to be the desired default value of "enabled for all purposes") is not obvious.
Comment 50•15 years ago
|
||
Comment on attachment 366681 [details] [diff] [review] Patch v3 - for review Another future optimization: the xOids array, which has 303 (the current value of SEC_OID_TOTAL) elements, is very sparse. By default we only use 5 elements. Some of the elements will never be used because those OIDs are not algorithms (e.g, the OIDs for certificate and CRL extensions). We should come up with a more space-saving way to store the policy flags for algorithms.
Comment 51•15 years ago
|
||
Bob discovered that not only is there no MD4 implementation in NSS, there is also no PKCS#11 mechanism defined for it. I think that means we shouldn't waste our time trying to "disable" MD4 - it's already effectively disabled today, and cannot be enabled. I think that invites further comments on attachment 366681 [details] [diff] [review] : a) Any line in that patch relevant to MD4 is probably not needed b) The environment variable "NSS_ALLOW_WEAK_SIGNATURE_ALG" defined in attachment 366681 [details] [diff] [review] really only does one thing - it enables MD2 support . We may want to rename it to "NSS_ALLOW_WEAK_MD2_SIGNATURE_ALG" or something along those lines that contains the word "MD2". In today's meeting, we discussed the need for fine-grain hash algorithm control with environment variables. This is needed not just to deal with today's attacks on MD2 and MD5, but may also be needed in the future for attacks against SHA-1. At the same time, we have a short-term need (about one week) for a 3.12.3 release to address the MD2/MD5 issue. If we had a bit more time, having fine grain support would probably be the right thing to do. I think short-term, we can use a slightly modified version of attachment 366681 [details] [diff] [review]. I would be OK with it going in with the changes I outlined above in this comment (a and b), plus the addition of another PR_GetEnv call in SECOID_Init to disable MD5. That's probably the quickest path to getting things done. An alternative is that we define the generic syntax today, but perhaps only implement the subcomponents that we need now (ie. just MD2 and MD5). But it may be hard to get agreement, a patch, review, and checkin by end of next week. Nevertheless, it shouldn't be brain surgery, so I will take a crack at it. The environment variable could be defined as follows : name : NSS_HASH_ALG_SUPPORT value : comma-separated list of [+/-]ALGNAME This environment variable would not need to define every algorithm, it would only make changes from the default. Eg. NSS_HASH_ALG_SUPPORT=+MD2,-MD5 would enable MD2, which we decided to disable by default, and disable MD5, which we decided to leave on by default. I think that's a simple enough syntax that can hopefully meet our current and future needs. We can add more algorithms to the supported list in the future.
Assignee | ||
Comment 52•15 years ago
|
||
Julien, please file a new separate RFE for your fine grained environment variable suggestion in comment 51.
Summary: Stop honoring digital signatures based on MD2 & MD5 hashes in certificates and CRLs → Stop honoring digital signatures in certificates and CRLs based on weak hashes
Assignee | ||
Comment 53•15 years ago
|
||
In today's weekly NSS meeting, I think we agreed to go forward with the following steps (in no particular order): 1. Commit the patch attached to this bug, as it establishes a base API for future controls, and solves an immediate vulnerability. 2. File an RFE to "completely disable certain hashes, not only for use in signatures, but for all higher level security uses within NSS. Bug 482882. Note that Bob and I oppose disabling hashes for non-cryptographic purposes. 3. File an RFE that requests that users/admins be able to manipulate this table using an environment variable with a syntax that allows various hashes to be selectively enabled/disabled. That is now bug 483113.
Assignee | ||
Comment 54•15 years ago
|
||
Comment on attachment 366681 [details] [diff] [review] Patch v3 - for review Committed on trunk. Checking in util/nssutil.def; new revision: 1.9; previous revision: 1.8 Checking in util/secoid.c; new revision: 1.47; previous revision: 1.46 Checking in util/secoid.h; new revision: 1.12; previous revision: 1.11 Checking in util/secoidt.h; new revision: 1.29; previous revision: 1.28 Checking in certhigh/certvfy.c; new revision: 1.69; previous revision: 1.68
Comment 55•15 years ago
|
||
Comment on attachment 366681 [details] [diff] [review] Patch v3 - for review OK now that bug 483113 has been resolved.
Attachment #366681 -
Flags: superreview- → superreview+
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 56•15 years ago
|
||
(In reply to comment #53) > Note that Bob and I oppose disabling hashes for non-cryptographic purposes. Me three. Lots of old things clients may want to deal with still use MD5 hashes, and it's convenient to be able to get all our hashing done by NSS. We could of course reimplement MD5 in the client if we had to, but that seems like a shame when we've got a big crypto library sitting right there next to us.
Comment 57•15 years ago
|
||
fwiw, the final NSS_Set API seems clear enough for me, thanks. I'm sorry I didn't comment earlier.
Comment 58•15 years ago
|
||
This bug is marked blocking1.9.1. As I understand it, this change was included in the NSS_3_12_3_RC0 tag, which Kai pushed to the 1.9.1 branch in bug 481968 (changeset http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f160224c5221 ). Therefore, I'm marking this bug fixed1.9.1. Please let me know if I've misunderstood. Also, just to make sure I'm clear: the effect of this change for Firefox 3.5 is: a) MD2 (and, trivially given comment 51, MD4) is immediately disabled as a supported hash by virtue of its inclusion here: http://mxr.mozilla.org/mozilla-central/source/security/nss/lib/util/secoid.c#1864 b) We gain the ability to disable other algorithms (e.g. MD5) through the same mechanism Is that accurate?
Keywords: fixed1.9.1
Assignee | ||
Comment 59•15 years ago
|
||
> Is that accurate? Yes. Plus, we have the ability to disable algorithms through the use of an environment variable. See Bug 483113.
Updated•15 years ago
|
Flags: blocking1.9.0.13+
Comment 60•15 years ago
|
||
500495 is security sensitive?
Comment 61•15 years ago
|
||
(In reply to comment #60) > 500495 is security sensitive? Yes.
Comment 62•15 years ago
|
||
This was fixed on the 1.9.0 branch by landing the new NSS
Keywords: fixed1.9.0.13,
fixed1.9.0.14
Comment 63•15 years ago
|
||
Verified for 1.9.0.14 and 1.9.0.13 since the new NSS has been verified to be there. I'd verify it for 1.9.1 except status1.9.1 wasn't set but a keyword incorrectly used.
Comment 64•15 years ago
|
||
(In reply to comment #63) > I'd verify it for 1.9.1 except status1.9.1 wasn't set but a keyword incorrectly > used. This was fixed and verified on 1.9.1 prior to Firefox 3.5 releasing. The keyword was not use incorrectly.
Comment 65•15 years ago
|
||
Why is not it not marked "verified1.9.1" if it was verified then?
See Also: → https://launchpad.net/bugs/312536
You need to log in
before you can comment on or make changes to this bug.
Description
•