Recently the nsPrefService, which is the gateway to reading and storing preference values, has been changed. It now enforces that all access to preferences from secondary threads is denied. This causes internal failures in PSM code that relies on reading preference values to derive behaviour, e.g. for SSL sockets, e.g. client authentication, warnings, etc. This means, several code pathes now fail to work. We already have learned that SSL client auth is broken. This bug asks to inspect all PSM code and find code locations that access preferences from secondary threads. For each such location, we must: - introduce a shared variable that copies the pref value - init the variable in PSM init - register the prefs for changes (pref::observe) - change the code to read our shared variables, rather than reading from pref service
Or remove the pref, if it isn't worth keeping.
Removing dependency on bug 624490 because I can't add correct dependencies of other bugs due to it.
Notes from Kai from IRC: * <these prefs> probably need to be dealt with: security.ssl.allow_unrestricted_renego_everywhere__temporarily_available_pref security.ssl.renego_unrestricted_hosts security.ssl.treat_unsafe_negotiation_as_broken security.ssl.require_safe_negotiation security.ssl.warn_missing_rfc5746 security.ssl.enable_false_start security.default_personal_cert security.remember_cert_checkbox_default_setting security.ask_for_password security.password_lifetime security.warn_entering_secure security.warn_entering_weak security.warn_leaving_secure security.warn_viewing_mixed security.warn_submit_insecure security.OCSP.enabled security.OCSP.require <kaie> instead of using a single variable inside nsNSSCertDB, it might make more sense to have a single shared class holding all prefs, and moving the mutex to that class <kaie> you could also have a look at class nsSSLIOLayerHelpers for inspiration <kaie> this might be a good place to keep our copies of the pref values, because it already has shared data and a mutex <kaie> also, nsNSSComponent::Observe is the code where we listen to other pref changes. that's where we catch pref changes and let NSS know <kaie> every pref that is used to immediately notify NSS and set a default setting, doesn't need to be remembered by PSM <bsmith> But, some of the related bugs <bsmith> https://bugzilla.mozilla.org/show_bug.cgi?id=624411 <kaie> filed today <kaie> so, people are noticing, and I agree that it must block <kaie> if beta 9 ships as is, we'll get a lot of complaints
I have backed out bug 619487 which is the reason behind this bug, so this doesn't need to hold off beta9.
The following two MXR queries should help in locating the affected code: http://mxr.mozilla.org/mozilla-central/search?string=NS_PREFSERVICE_CONTRACTID&find=security/manager http://mxr.mozilla.org/mozilla-central/search?string=%28Get|Set%29%28Bool|Char|Int%29Pref®exp=1&case=1&find=security%2Fmanager Many of these 55 hits (second search) are probably fine already - in particular those in nsNSSComponent relying on mPrefBranch. "security.OCSP.enabled" and "security.default_personal_cert" are the crucial ones right now, I think (didn't look at the CRL stuff in more detail, but currently that's code largely unused, I guess).
I am finishing up the patch to fix this now. I added the softblocker mark since this isn't a regression from 3.6. Still, it will get fixed before the next beta.
(In reply to comment #7) > *** Bug 624075 has been marked as a duplicate of this bug. *** Duped bug was a hard-blocker, just want to make sure this should remain a soft-blocker.
(In reply to comment #8) > (In reply to comment #7) > > *** Bug 624075 has been marked as a duplicate of this bug. *** > > Duped bug was a hard-blocker, just want to make sure this should remain a > soft-blocker. Yes, this is what I meant to do. Bug 624075 only occurs due to the patch from bug 619487, which was backed out.
What's the latest status on this? The last comment was on 01/13 indicating it should be fixed for the next beta. Should this be 2.0+? Johnny in bsmedberg's absence, can you triage this?
(In reply to comment #11) > What's the latest status on this? The last comment was on 01/13 indicating it > should be fixed for the next beta. Should this be 2.0+? Johnny in bsmedberg's > absence, can you triage this? Also CC'ing Mike and Juan as it sounds important.
IIRC, the patch in my queue is complete. I am working on another more critical bug right now and I will post the patch as soon as I'm done with that bug. The current behavior is no worse than 3.6 since the backout of bug 619487. I removed the regression keyword.
Softblocking on this one. Brian, if you disagree and think there are more important things or that this is too risky, please do bump back to nomination or otherwise.
I agree with the current assessment (final+ softblocker).
Updated summary by request. Originally, this bug was about the regression caused by bug 619487, but that regression was fixed by backing out that patch. Now the bug is about getting PSM fixed so that the patch in bug 619487 can be re-applied.
Not going to take bug 619487 for 4, so this doesn't block.
(In reply to comment #18) > Not going to take bug 619487 for 4, so this doesn't block. Just to be clear, we are OK with PSM doing unsafe things with the preference service for another release, right? Are we going to actually make this a priority after 4?
(In reply to comment #18) > Not going to take bug 619487 for 4, so this doesn't block. Renominating since I disagree with this reasoning. The main reason for fixing this bug isn't so that we can take bug 619487, but rather so that we can stop doing unsafe things which are potentially causing bad behavior and crashes. What we have here is us messing around in thread-unsafe hash tables on multiple threads. That seems like something we want to fix no matter what happens in bug 619487. I agree this isn't a hardblocker given that we don't have data showing this high in crash-stats, so there's no data to back up that this affects a lot of people. But softblocker seems appropriate to me still.
I don't see why this is more important than any of the things we know are causing crashes, not all of which are softblockers. We can "make it a priority" after 4 if we believe that it's more important than other work, as always.
Ok, marking blocking .x then as that seems to be how we flag "let's make it a priority after ff4 ships" right now.
I would really like to get bug 619487 in the tree. Can we get this in soonish?
I will write the new patch in the next day.
This work conflicts with the work to switch to libpkix. The patch for PSM that switches to libpkix will disable most of the code that accesses the prefs services off the main thread.
clearly flag. it isn't clear that this should block a release.
Created attachment 585570 [details] [diff] [review] Make PSM access the network.ntlm.send-lm-response pref only on the main thread Most of the PSM accesses of prefs off the main thread have been removed (e.g. through bug 675221 and bug 674147) by moving the code that accesses the pref onto the main thread. Bug 714477 is about the fact that IsExtendedValidation() should not be accessing the preference at all. Besides bug 714477, I believe the only remaining usage of the prefs service off the main thread in PSM is the NTLMv2 code. Patch attached.
Could we get some traction on the review here and in bug 714477? PSM is the only thing that is blocking us from making touching the pref service off the main thread fatal.
Comment on attachment 585570 [details] [diff] [review] Make PSM access the network.ntlm.send-lm-response pref only on the main thread please find a different reviewer
Comment on attachment 585570 [details] [diff] [review] Make PSM access the network.ntlm.send-lm-response pref only on the main thread You could also use Preferences::AddBoolVarCache.
Comment on attachment 585570 [details] [diff] [review] Make PSM access the network.ntlm.send-lm-response pref only on the main thread Review of attachment 585570 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/ssl/src/nsNTLMAuthModule.cpp @@ +120,5 @@ > #define NTLM_RESP_LEN 24 > > //----------------------------------------------------------------------------- > > +static bool sendLM = false; Call this gSendLM or gSendLMPref.
So, can we land this?
This also depends on bug 714477 to be fully fixed. I will check in both patches at the same time.