Closed Bug 736589 Opened 8 years ago Closed 8 years ago

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

Categories

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

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla14
Tracking Status
firefox12 + verified
firefox13 + verified
firefox14 + verified
firefox-esr10 12+ verified

People

(Reporter: jruderman, Assigned: mayhemer)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, testcase, Whiteboard: [sg:critical][qa!])

Crash Data

Attachments

(4 files, 1 obsolete file)

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
Attached file stack trace
Assignee: nobody → honzab.moz
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
Status: NEW → ASSIGNED
Whiteboard: [sg:critical]
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".
I have a wip patch that needs testing and actually a separate slightly modified version for each channel.  But it is doable.
Try run for mozilla-central (just fresh):

https://tbpl.mozilla.org/?tree=Try&rev=42c9415848a2
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).
Attachment #609555 - Flags: review?(jst)
Attached patch v1 for m-b and m-r (obsolete) — Splinter Review
This preserves security check for globalStorage and sessionStorage using just domain, as we currently do on Beta and Release channels.
Attachment #609571 - Flags: review?(jst)
(I cannot push to try, some ssh server issues)
Attachment #609555 - Flags: review?(jst) → review+
Attachment #609571 - Flags: review?(jst) → review+
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)
Attachment #609571 - Attachment is obsolete: true
Attachment #609929 - Flags: review+
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
Attachment #609555 - Flags: approval-mozilla-aurora?
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
Attachment #609929 - Flags: approval-mozilla-beta?
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.
(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
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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.
Attachment #609929 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #609555 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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).
Attachment #609929 - Flags: approval-mozilla-esr10?
Comment on attachment 609929 [details] [diff] [review]
v1.1 for m-b and m-r

[Triage Comment]
post-facto a+ :)
Attachment #609929 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Whiteboard: [sg:critical] → [sg:critical][qa+]
any CVE for this one?
Group: core-security
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
If no crash occurred when clicking the "Start test" button of the test case, then that is verified fixed.
Status: RESOLVED → VERIFIED
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Verified fixed in Firefox 10.0.6esrpre 2012-06-21.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.