736589 - Crash [@ nsDOMStorage::GetNamedItem] with sessionStorage, GC

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires extension for GC)

1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase
3. Click the button

Result: Firefox crashes in one of the following ways:
  [@ nsDOMStorage::GetNamedItem] calling a random address
  [@ nsDOMStorage::CacheStoragePermissions] accessing null

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Honza Bambas (:mayhemer)]

Thanks a lot for the test case!

I identified the cause and I more or less know how to fix this.

I can crash consistently on access to deleted mSecurityChecker that is a raw non-addrefing pointer.

I can see we create nsDOMStorage2 and set its mStorage to a new nsDOMStorage that gets set mSecurityChecker to the nsDOMStorage2 instance.  Later we fork nsDOMStorage2 to a new nsDOMStorage2 instance (the forking mechanism is part of storage event implementation).  Later the first nsDOMStorage2 instance get released (no longer needed).  But we still refer it in nsDOMStorage->mSecurityChecker.

A valid fix is to fix bug 732708.  But that is not acceptable on branches since depends on removal of globalStorage (bug 687579).

Solutions for branches:
- change the mSecurityChecker referencing (either addref it when not |this| or let forks ref the original instance) / easy+dangerous
- change the algorithm for security decision ; there is also mEventBroadcaster (also about to be removed as part of bug 732708) that could suffer the same way, so needs to be changed as well / more work+safer

Comment 3

[Daniel Veditz [:dveditz]]

If bug 732708 is coming soon (say Firefox 13, the release globalStorage was removed) we could go with easy+dangerous for Firefox 12. If bug 732708 is going to take a while then we should go with "more work+safer".

Comment 4

[Honza Bambas (:mayhemer)]

I have a wip patch that needs testing and actually a separate slightly modified version for each channel.  But it is doable.

Comment 5

[Honza Bambas (:mayhemer)]

Try run for mozilla-central (just fresh):

https://tbpl.mozilla.org/?tree=Try&rev=42c9415848a2

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1 for m-c and m-a

This applies cleanly to aurora (13) as well.  There had been two bugs interfering with this landed on version 13: bug 495337 and bug 687579.

So I need a bit different patch for version 12 and 11 (beta and release).

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1 for m-b and m-r

This preserves security check for globalStorage and sessionStorage using just domain, as we currently do on Beta and Release channels.

Comment 8

[Honza Bambas (:mayhemer)]

(I cannot push to try, some ssh server issues)

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 609571 [details] [diff] [review]
v1 for m-b and m-r

https://tbpl.mozilla.org/?tree=Try&rev=1d2d565ecd0d

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 609571 [details] [diff] [review]
v1 for m-b and m-r

https://tbpl.mozilla.org/?tree=Try&rev=d368596306d3 (m-r)

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v1.1 for m-b and m-r

Linux didn't like missing return for default: case.  Otherwise identical to v1.

Try for this particular patch: https://tbpl.mozilla.org/?tree=Try&rev=37902a4693f9 (on m-b branch)

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 609555 [details] [diff] [review]
v1 for m-c and m-a

[Approval Request Comment]
Regression caused by (bug #): 495112
User impact if declined: potentially exploitable crash
Testing completed (on m-c, etc.): none so far landed, now having only try results
Risk to taking this patch (and alternatives if risky): low, the logic is identical, just not using unsafe dereference anymore
String changes made by this patch: none

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

[Approval Request Comment]
Regression caused by (bug #): 495112
User impact if declined: potentially exploitable crash
Testing completed (on m-c, etc.): none so far, now having only try results (this patch is for beta and release only, so it cannot be tested on m-c or m-a)
Risk to taking this patch (and alternatives if risky): low, the logic is identical, just not using unsafe dereference anymore
String changes made by this patch: none

Comment 14

[Honza Bambas (:mayhemer)]

Side note: there is also mEventBroadcaster member (raw ptr).  That is safe to use, since it is either null or set immediately before use to an object valid for the time of the operation with mEventBroadcaster.

Comment 15

[Honza Bambas (:mayhemer)]

Comment on attachment 609555 [details] [diff] [review]
v1 for m-c and m-a

https://hg.mozilla.org/integration/mozilla-inbound/rev/3eea0725665e

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

https://tbpl.mozilla.org/?tree=Try&rev=e011283d7a2e (m-b, android, xul)

Comment 17

[Ed Morley [:emorley]]

(In reply to Honza Bambas (:mayhemer) from comment #15)
> Comment on attachment 609555 [details] [diff] [review]
> v1 for m-c and m-a
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3eea0725665e

https://hg.mozilla.org/mozilla-central/rev/3eea0725665e

Comment 18

[Alex Keybl [:akeybl]]

Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

[Triage Comment]
Approved for Beta 12 and Aurora 13 - we need to make sure to land this on the ESR branch as well, since 10.0.4 will be shipped simultaneously with FF12.

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 609555 [details] [diff] [review]
v1 for m-c and m-a

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5ac0b502079

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

https://hg.mozilla.org/releases/mozilla-beta/rev/be73dfc96f93

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

Try on esr10:
https://tbpl.mozilla.org/?tree=Try&rev=400d24cd8799

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

https://hg.mozilla.org/releases/mozilla-esr10/rev/cea3a82f3f24

(In reply to Alex Keybl [:akeybl] from comment #18)
> Comment on attachment 609929 [details] [diff] [review]
> v1.1 for m-b and m-r
> 
> [Triage Comment]
> Approved for Beta 12 and Aurora 13 - we need to make sure to land this on
> the ESR branch as well, since 10.0.4 will be shipped simultaneously with
> FF12.

I took this as a+ for esr10, but now I spot the flag.  I presume it should be set (postfacto, sorry).

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

[Triage Comment]
post-facto a+ :)

Comment 24

[Huzaifa Sidhpurwala]

any CVE for this one?

Comment 25

[Vlad [QA]]

This bug has to be verified on a debug build?
I've followed the steps from the description on Firefox 11 beta and I didn't got no crash, so I'm guessing that I'm doing something wrong.

Tracy can you help with some clarifications?

Thanks

Comment 26

[Tracy Walker [:tracy]]

If no crash occurred when clicking the "Start test" button of the test case, then that is verified fixed.

Comment 27

[u279076]

Verified fixed in Firefox 10.0.6esrpre 2012-06-21.