624514 - PSM accesses the pref service off the main thread

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Kai Engert (:KaiE:)]

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

Comment 1

[Benjamin Smedberg]

Or remove the pref, if it isn't worth keeping.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Removing dependency on bug 624490 because I can't add correct dependencies of other bugs due to it.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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

Comment 4

[(no longer active)]

I have backed out bug 619487 which is the reason behind this bug, so this doesn't need to hold off beta9.

Comment 5

[Kaspar]

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).

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 8

[u279076]

(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.

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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

[Sheila Mooney]

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?

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I agree with the current assessment (final+ softblocker).

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 18

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Not going to take bug 619487 for 4, so this doesn't block.

Comment 19

[Shawn Wilsher :sdwilsh]

(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?

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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.

Comment 21

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, marking blocking .x then as that seems to be how we flag "let's make it a priority after ff4 ships" right now.

Comment 23

[Shawn Wilsher :sdwilsh]

I would really like to get bug 619487 in the tree.  Can we get this in soonish?

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I will write the new patch in the next day.

Comment 25

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

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.

Comment 26

[Doug Turner (:dougt)]

clearly flag.  it isn't clear that this should block a release.

Comment 27

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: 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.

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 30

[Kai Engert (:KaiE:)]

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 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 32

[Honza Bambas (:mayhemer)]

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.

Comment 33

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

So, can we land this?

Comment 34

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

This also depends on bug 714477 to be fully fixed. I will check in both patches at the same time.

Comment 35

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46726c3ab4e1

Comment 36

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/46726c3ab4e1