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

VERIFIED FIXED in Firefox 12

Status

()

Core
DOM
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: mayhemer)

Tracking

(Blocks: 2 bugs, {crash, testcase})

Trunk
mozilla14
x86_64
Mac OS X
crash, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox12+ verified, firefox13+ verified, firefox14+ verified, firefox-esr1012+ verified)

Details

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

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 606719 [details]
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
(Reporter)

Comment 1

5 years ago
Created attachment 606720 [details]
stack trace
(Assignee)

Updated

5 years ago
Assignee: nobody → honzab.moz
(Assignee)

Comment 2

5 years ago
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]
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox12: --- → ?
tracking-firefox13: --- → +
tracking-firefox14: --- → +
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".
(Assignee)

Comment 4

5 years ago
I have a wip patch that needs testing and actually a separate slightly modified version for each channel.  But it is doable.
(Assignee)

Comment 5

5 years ago
Try run for mozilla-central (just fresh):

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

Updated

5 years ago
tracking-firefox12: ? → +
(Assignee)

Comment 6

5 years ago
Created attachment 609555 [details] [diff] [review]
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).
Attachment #609555 - Flags: review?(jst)
(Assignee)

Comment 7

5 years ago
Created attachment 609571 [details] [diff] [review]
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.
Attachment #609571 - Flags: review?(jst)
(Assignee)

Comment 8

5 years ago
(I cannot push to try, some ssh server issues)
(Assignee)

Comment 9

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

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

Comment 10

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

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

Updated

5 years ago
Attachment #609555 - Flags: review?(jst) → review+

Updated

5 years ago
Attachment #609571 - Flags: review?(jst) → review+
(Assignee)

Comment 11

5 years ago
Created attachment 609929 [details] [diff] [review]
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)
Attachment #609571 - Attachment is obsolete: true
Attachment #609929 - Flags: review+
(Assignee)

Comment 12

5 years ago
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?
(Assignee)

Comment 13

5 years ago
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?
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

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

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

Comment 16

5 years ago
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)
(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
Last Resolved: 5 years ago
status-firefox14: affected → fixed
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+

Updated

5 years ago
Attachment #609555 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox-esr10: ? → 12+
(Assignee)

Comment 19

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/b5ac0b502079
Attachment #609555 - Flags: checkin+
(Assignee)

Comment 20

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

https://hg.mozilla.org/releases/mozilla-beta/rev/be73dfc96f93
Attachment #609929 - Flags: checkin+
(Assignee)

Comment 21

5 years ago
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
(Assignee)

Comment 22

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

Updated

5 years ago
status-firefox12: affected → fixed
status-firefox13: affected → fixed
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+]

Updated

5 years ago
status-firefox-esr10: affected → fixed

Comment 24

5 years ago
any CVE for this one?

Updated

5 years ago
status-firefox12: fixed → verified
Group: core-security

Comment 25

5 years ago
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
status-firefox13: fixed → verified
status-firefox14: fixed → verified

Updated

5 years ago
Whiteboard: [sg:critical][qa+] → [sg:critical][qa!]
Verified fixed in Firefox 10.0.6esrpre 2012-06-21.
status-firefox-esr10: fixed → verified
You need to log in before you can comment on or make changes to this bug.