Closed
Bug 624514
Opened 14 years ago
Closed 12 years ago
PSM accesses the pref service off the main thread
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: KaiE, Assigned: briansmith)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mayhemer
:
review+
sicking
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•14 years ago
|
Severity: normal → critical
blocking2.0: --- → ?
tracking-fennec: --- → ?
Summary: nsPrefService fallout: Ensure PSM will continue to work in FF 4 → nsPrefService fallout: Ensure PSM (e.g. SSL) will continue to work in FF 4 / mozilla2.0
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Updated•14 years ago
|
Assignee | ||
Comment 2•14 years ago
|
||
Removing dependency on bug 624490 because I can't add correct dependencies of other bugs due to it.
No longer depends on: 624490
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → bsmith
Assignee | ||
Comment 3•14 years ago
|
||
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
Keywords: regression
Comment 4•14 years ago
|
||
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).
Assignee | ||
Comment 6•14 years ago
|
||
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.
Whiteboard: [softblocker]
(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.
Assignee | ||
Comment 9•14 years ago
|
||
(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.
Comment 11•14 years ago
|
||
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?
Updated•14 years ago
|
Whiteboard: [softblocker]
Comment 12•14 years ago
|
||
(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.
Assignee | ||
Comment 13•14 years ago
|
||
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.
Keywords: regression
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.
blocking2.0: ? → final+
Whiteboard: [softblocker]
Assignee | ||
Comment 16•14 years ago
|
||
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.
Summary: nsPrefService fallout: Ensure PSM (e.g. SSL) will continue to work in FF 4 / mozilla2.0 → PSM accesses the pref service from the main thread
Summary: PSM accesses the pref service from the main thread → PSM accesses the pref service off the main thread
Updated•14 years ago
|
tracking-fennec: ? → 2.0+
Not going to take bug 619487 for 4, so this doesn't block.
blocking2.0: final+ → -
tracking-fennec: 2.0+ → ?
Whiteboard: [softblocker]
Comment 19•14 years ago
|
||
(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.
blocking2.0: - → ?
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.
blocking2.0: ? → -
Ok, marking blocking .x then as that seems to be how we flag "let's make it a priority after ff4 ships" right now.
blocking2.0: - → .x
Updated•14 years ago
|
tracking-fennec: ? → 2.0next+
Comment 23•14 years ago
|
||
I would really like to get bug 619487 in the tree. Can we get this in soonish?
Assignee | ||
Comment 25•14 years ago
|
||
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.
Depends on: psm-pkix
Comment 26•14 years ago
|
||
clearly flag. it isn't clear that this should block a release.
tracking-fennec: 2.0next+ → ---
Assignee | ||
Comment 27•13 years ago
|
||
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.
Attachment #585570 -
Flags: review?(kaie)
Assignee | ||
Updated•13 years ago
|
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.
Updated•13 years ago
|
tracking-fennec: --- → +
Updated•12 years ago
|
tracking-fennec: + → 14+
blocking-fennec1.0: --- → ?
Updated•12 years ago
|
blocking-fennec1.0: ? → .N+
Updated•12 years ago
|
blocking-fennec1.0: .N+ → -
Updated•12 years ago
|
tracking-fennec: 14+ → -
Reporter | ||
Comment 30•12 years ago
|
||
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
Attachment #585570 -
Flags: review?(kaie) → review?
Updated•12 years ago
|
Attachment #585570 -
Flags: review? → review?(bsmith)
Assignee | ||
Updated•12 years ago
|
Attachment #585570 -
Flags: review?(bsmith) → review?(honzab.moz)
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.
Attachment #585570 -
Flags: review+
![]() |
||
Comment 32•12 years ago
|
||
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.
Attachment #585570 -
Flags: review?(honzab.moz) → review+
So, can we land this?
Flags: needinfo?(bsmith)
Assignee | ||
Comment 34•12 years ago
|
||
This also depends on bug 714477 to be fully fixed. I will check in both patches at the same time.
Flags: needinfo?(bsmith)
Updated•12 years ago
|
Assignee | ||
Comment 35•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/46726c3ab4e1
Target Milestone: --- → mozilla21
Updated•12 years ago
|
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
Comment 36•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/46726c3ab4e1
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•