If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

PSM accesses the pref service off the main thread

RESOLVED FIXED in mozilla21

Status

()

Core
Security: PSM
--
critical
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: kaie, Assigned: briansmith)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 .x+, blocking-fennec1.0 -, fennec-)

Details

Attachments

(1 attachment)

(Reporter)

Description

7 years ago
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

7 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
(Reporter)

Updated

7 years ago
Blocks: 624075
(Reporter)

Updated

7 years ago
Depends on: 624490
Depends on: 624411, 624315, 585706

Comment 1

7 years ago
Or remove the pref, if it isn't worth keeping.
No longer depends on: 624411
No longer depends on: 585706, 624315
Removing dependency on bug 624490 because I can't add correct dependencies of other bugs due to it.
No longer depends on: 624490
Blocks: 585706
Blocks: 624411
Assignee: nobody → bsmith
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
Blocks: 624315
I have backed out bug 619487 which is the reason behind this bug, so this doesn't need to hold off beta9.

Comment 5

7 years ago
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&regexp=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.
Whiteboard: [softblocker]
Duplicate of this bug: 624075
(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.
Blocks: 619487
(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.
Duplicate of this bug: 624411

Comment 11

7 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

7 years ago
Whiteboard: [softblocker]
(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.
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]
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.
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
Duplicate of this bug: 624315
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]
(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
tracking-fennec: ? → 2.0next+
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.
Depends on: 479393
clearly flag.  it isn't clear that this should block a release.
tracking-fennec: 2.0next+ → ---
Depends on: 655340
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.
Attachment #585570 - Flags: review?(kaie)
Depends on: 714477
No longer blocks: 585706
Depends on: 585706
No longer depends on: 714477
Duplicate of this bug: 753656
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.
tracking-fennec: --- → +
tracking-fennec: + → 14+
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → .N+
blocking-fennec1.0: .N+ → -
tracking-fennec: 14+ → -
(Reporter)

Comment 30

5 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?
Attachment #585570 - Flags: review? → review?(bsmith)
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 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)
This also depends on bug 714477 to be fully fixed. I will check in both patches at the same time.
Flags: needinfo?(bsmith)
Depends on: 714477
No longer depends on: 585706
https://hg.mozilla.org/integration/mozilla-inbound/rev/46726c3ab4e1
Target Milestone: --- → mozilla21
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/46726c3ab4e1
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.