462806 - [FIX]Stop initializing PSM on startup when dealing with chrome jars

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

We get a signature verifier on the off chance that the jar is signed.  We should hold off on that until we actually know that it is.  Chrome jars, notably, are not.

Patch attached.  mstoltz is kinda awol, so I guess I'll go with dveditz for the review; no really good reviewers here.

Comment 1

[Daniel Veditz [:dveditz]]

Comment on attachment 346000 [details] [diff] [review]
Like so

looks good, r=dveditz

Comment 2

[Daniel Veditz [:dveditz]]

We should check this into old branches, too. We originally ran across this problem when an XMLHttpRequest security fix coupled with the FoxyProxy addon caused a top-crash (bug 456705). FoxyProxy worked around the problem, but it's a pattern copied by/from other addons and the crash is still there, though at lower levels (FoxyProxy was the popular one). And the workaround--make FoxyProxy use "flat" chrome rather than jars--is pretty ugly.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Daniel, do you think I should try to get this into b2?

Comment 4

[Daniel Veditz [:dveditz]]

I think we should try to get this in b2

Comment 5

[Daniel Veditz [:dveditz]]

One easy way to verify this fix is to go back to an earlier version of foxyproxy and see that it doesn't cause the https crashes.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 346000 [details] [diff] [review]
Like so

OK, trying for approval.  This should be a very safe patch, and we want it on branches.

Comment 7

[Samuel Sidler (old account; do not CC)]

Boris, does this patch apply on 1.9.0 and 1.8.1? If so, can you request approval on it? We can take this before it lands on trunk (though it's not preferred).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 346000 [details] [diff] [review]
Like so

This applies to 1.9 branch (with fuzz=3 because of mMtime not being there on 1.9 branch).

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 346000 [details] [diff] [review]
Like so

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: 1.8 branch merge

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 348232 [details] [diff] [review]
1.8 branch merge

Approved for 1.8.1.19, a=dveditz for release-drivers

Comment 12

[Boris Zbarsky [:bzbarsky]]

Landed on both branches.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346000 [details] [diff] [review]
Like so

a=beltzner for 1.9.1b2

Comment 14

[Samuel Sidler (old account; do not CC)]

fwiw, it looks like this patch dropped Ts by ~50ms on Windows (~4%), ~100ms on Mac (~7%), and maybe ~25ms on Linux (~1.8%), though that's harder to tell because Windows is a little more sporadic right now. Wins all around!

Comment 15

[Boris Zbarsky [:bzbarsky]]

Pushed changeset 0c3300f451d6 to m-c.  Can't think of a way to test this, really, other than Ts.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

This change ostensibly is the cause of 
Bug 465974 Crash during start of firefox with weave installed 

I wonder how many other uses of PSM have historically relied on it being
already initialized by the time they use it, and will now break.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Weave isn't using PSM.  It's using NSS directly without doing anything to make sure it's initialized.  I'd hope that there are very few binary extensions (already a given, btw) that do silly things like that.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Paul, might be a good idea to document this somewhere. In case anyone is using NSS like Weave is.

Comment 19

[timeless]

a quick scan:
http://mxr-test.konigsberg.mozilla.org/addons/search?string=nss_&find=dll&filter=.&hitlimit=1

shows two likely candidates: 4522 and 4471

http://mxr-test.konigsberg.mozilla.org/addons/search?string=nss_&find=%5C.so&filter=.&hitlimit=1

shows three candidates including allpeers (sanity check): 3234, 4522 and 4471

http://mxr-test.konigsberg.mozilla.org/addons/search?string=nss_&find=dylib&filter=.&hitlimit=1

shows two likely candidates: 4522 and 4471

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Is there any way to verify this change for 1.9.0.5 and 1.8.1.19? I see that there is no automated testcase (nor manual one).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Not easily, no.  I verified by setting breakpoints in a debugger and seeing whether browser startup hit them.

This _did_ cause a Ts drop on 1.9.0 talos machines, if that counts.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Perf drops are a great way to validate fixes! :-)

Comment 23

[Daniel Veditz [:dveditz]]

(In reply to comment #20)
> Is there any way to verify this change for 1.9.0.5 and 1.8.1.19?

Can't you go back to bug 456705? Get the pre-workaround crashing FoxyProxy (version 2.8.5) and verify it doesn't crash anymore when loading an https page. Or did some other fix in the meantime also stop the crash?

At least to verify in 2.0.0.19; not sure 3.0.x crashed in that case.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

I've verified that bug 456705 is fixed with 1.8.1.19 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.19) Gecko/2008120316 Firefox/2.0.0.19. It definitely reproduces with 1.8.1.18.

Comment 25

[Jayson King]

There is something with this patch 0c3300f451d6 that interacts badly with Flashblock and SSL sites on trunk. I earlier posted bug 470941 about a crash on loading SSL sites, and bisect pointed to the patch here.

Comment 26

[Nelson Bolyard (seldom reads bugmail)]

In reply to comment 25, I think there is probably a lot of code that doesn't check to be sure that PSM is initialized before using SSL.  Instead, it just assumes that PSM has been initialized.  In the past, the probability that 
PSM was initialized was higher.  Now, it's lower, and we're seeing bugs in
software that fails to initialize it.  I view this as a case where latent
bugs in code that fails to ensure initialization have been exposed by a 
valid change.

Comment 27

[Kai Engert (:KaiE:)]

Adding some thoughts.

Any code attempting to use NSS directly at least must trigger NSS init.
Requesting any service offered by PSM is sufficient, as PSM will automatically init NSS at that time.

If PSM service creation fails, then don't use NSS.

For example, getService(nsISecretDecoderRing).


There is another potential problem, runtime NSS re-init.
SeaMonkey's has a feature "tools / switch profile" which requires to shut down NSS and init NSS again in the new profile directory.
However, Firefox' private browsing mode may be another scenario where the same approach is required.

As of today, notification events are used to have PSM control the init/shutdown state of NSS.

If code outside of PSM has a desire to use NSS directly, it must listen to those events, too. It must avoid calling NSS code while NSS is shut down.

Furthermore, code outside of PSM must not hold on to resource handles owned by NSS. Or that code must listen to the events and free resources accordingly.

If code outside of PSM holds on to NSS resources directly, this can prevent NSS shutdown/re-init to function, and therefore disable features like profile switching or maybe switching to/from private browsing mode.

Comment 28

[Benjamin Smedberg]

The Mozilla platform no longer supports profile switching during runtime.

Private browsing does not shut down PSM either. So I think that in our current platform PSM/NSS is init-once and stays initialized until shutdown.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> Private browsing does not shut down PSM either.

It'll need to to fix the bug about it caching user certificates across the switch to private browsing mode, if we plan to fix it.

Comment 30

[Kai Engert (:KaiE:)]

(In reply to comment #28)
> The Mozilla platform no longer supports profile switching during runtime.
> 
> Private browsing does not shut down PSM either. So I think that in our current
> platform PSM/NSS is init-once and stays initialized until shutdown.

But it might be necessary to change that. If a user owns personal certificates, then there is a risk of unintentional SSL client authentication in private browsing mode. The cleanest solution I'm aware of is a shutdown and re-init of NSS with an empty cert database. See my comment in bug 463256 comment 31.

Comment 31

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #28)
> The Mozilla platform no longer supports profile switching during runtime.

Do you mean that Firefox no longer supports it?
Or do you mean that the underlying "platform" code (common to seamonkey also)
no longer supports it?  

Seamonkey has UI to do this.  Does it now fail?

Comment 32

[Philip Chee]

> Or do you mean that the underlying "platform" code (common to seamonkey also)
> no longer supports it?  

The underlying code no longer supports it.

> Seamonkey has UI to do this.  Does it now fail?
AFAIK on trunk we fake it by doing a restart.