Closed
Bug 958867
Opened 10 years ago
Closed 10 years ago
Make sure to call HoldJSObjects when setting IDBFactory::mOwningObject
Categories
(Core :: Storage: IndexedDB, defect)
Tracking
()
People
(Reporter: smaug, Assigned: smaug)
Details
(Keywords: csectype-uaf, sec-moderate, Whiteboard: [adv-main28+][adv-esr24.4+][qa-])
Attachments
(2 files)
889 bytes,
patch
|
bent.mozilla
:
review+
bajaj
:
approval-mozilla-aurora+
praghunath
:
approval-mozilla-b2g18+
praghunath
:
approval-mozilla-b2g26+
|
Details | Diff | Splinter Review |
1.08 KB,
patch
|
Sylvestre
:
approval-mozilla-esr24+
|
Details | Diff | Splinter Review |
While testing a patch for bug 958315, it revealed an sg:crit bug in IDB. We don't always end up tracing mOwningObject because of the missing HoldJSObjects(). Seems to be a regression from http://hg.mozilla.org/mozilla-central/diff/8b5e83943958/dom/indexedDB/IDBFactory.cpp (Bug 772700 added HoldJSObjects call to another ::Create method)
Attachment #8358847 -
Flags: review?(bent.mozilla)
IIRC I pointed this out at the time and we decided it wasn't necessary.
Assignee | ||
Comment 2•10 years ago
|
||
It is indeed probably very tricky to trigger, unless one tries to access all the JS* member variables. It is bad thing to let a member variable to point to a deleted object.
I believe the justification was something about mOwningObject always pointing to the global object.
Assignee | ||
Comment 4•10 years ago
|
||
So? Globals do get GCed too. Anyhow, the patch is simple, and so far it looks like this is the only missing holdJSobject in the tree.
Comment on attachment 8358847 [details] [diff] [review] idb_crash.diff Review of attachment 8358847 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, mOwningObject is always a global, so I don't know how you could have a problem here really... You'd need a live IDBFactory object that somehow outlived its global? But I'm totally fine with making it bulletproof anyway.
Attachment #8358847 -
Flags: review?(bent.mozilla) → review+
Updated•10 years ago
|
Keywords: sec-critical
Updated•10 years ago
|
Keywords: csectype-uaf
Comment 6•10 years ago
|
||
I guess this doesn't need to be critical as it sounds very difficult to actually exploit.
Keywords: sec-critical → sec-moderate
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9539fbbb66ec
Assignee | ||
Comment 8•10 years ago
|
||
Yeah, so far I haven't found a way for a web page to trigger a crash. Some binary addon could in theory do it.
Comment 9•10 years ago
|
||
fixed on central https://hg.mozilla.org/mozilla-central/rev/9539fbbb66ec
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox29:
--- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 10•10 years ago
|
||
Given how simple the fix is we should take it on Aurora (Fx28) which also gets it into b2g1.3.
status-b2g18:
--- → affected
status-b2g-v1.2:
--- → affected
status-b2g-v1.3:
--- → affected
status-firefox26:
--- → wontfix
status-firefox27:
--- → affected
status-firefox28:
--- → affected
status-firefox-esr24:
--- → affected
tracking-firefox28:
--- → ?
Whiteboard: not exposed to content, only addons?
Comment 11•10 years ago
|
||
Smaug, can you please request Aurora approval? And it sounds like esr24, b2g18, b2g26 are all wontfix? Is there anything in b2g land that might make this easier to hit than on desktop?
Flags: needinfo?(bugs)
Comment 12•10 years ago
|
||
This is exposed to content, it is just that the object that isn't being held alive is the global for the page that the IDBFactory is on, so somehow the factory would have to be alive when the page it is on is dead.
Whiteboard: not exposed to content, only addons?
Assignee | ||
Comment 13•10 years ago
|
||
Comment on attachment 8358847 [details] [diff] [review] idb_crash.diff [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 772700 User impact if declined: Not quite clear. So far I haven't found a way to trigger the possible crash Testing completed (on m-c, etc.): landed to m-c 2014-01-14 Risk to taking this patch (and alternatives if risky): should be safe String or IDL/UUID changes made by this patch: NA
Attachment #8358847 -
Flags: approval-mozilla-aurora?
Flags: needinfo?(bugs)
Updated•10 years ago
|
Attachment #8358847 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Updated•10 years ago
|
tracking-firefox28:
? → ---
Comment 14•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/2cffd1ae3ca3
status-b2g-v1.1hd:
--- → affected
status-b2g-v1.4:
--- → fixed
Comment 15•10 years ago
|
||
This is trivial to backport to b2g26 (v1.2), but esr24/b2g18 would require a bit more love.
Comment 16•10 years ago
|
||
Simple fix for the older branches. Running through Try for sanity's sake.
Comment 17•10 years ago
|
||
Comment on attachment 8358847 [details] [diff] [review] idb_crash.diff NOTE: This flag is now for security issues only. Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings. [Approval Request Comment] Bug caused by (feature/regressing bug #): See comment 13. User impact if declined: See comment 13. Testing completed: On m-c/aurora without issue. Risk to taking this patch (and alternatives if risky): See comment 13. Patch applied cleanly to b2g26 as-is and is green on Try String or UUID changes made by this patch: None
Attachment #8358847 -
Flags: approval-mozilla-b2g26?
Comment 18•10 years ago
|
||
Comment on attachment 8366143 [details] [diff] [review] esr24/b2g18 patch Bug caused by (feature/regressing bug #): See comment 13. User impact if declined: See comment 13. Testing completed: On m-c/aurora without issue. Risk to taking this patch (and alternatives if risky): See comment 13. Patch required minor changes for esr24/b2g18 but is green on Try. String or UUID changes made by this patch: None
Attachment #8366143 -
Flags: approval-mozilla-esr24?
Attachment #8366143 -
Flags: approval-mozilla-b2g18?
Updated•10 years ago
|
tracking-firefox-esr24:
--- → 28+
Comment 19•10 years ago
|
||
Actually, at sec-moderate this doesn't meet ESR landing criteria - why should we take this on that branch?
Flags: needinfo?(bugs)
Updated•10 years ago
|
Comment 20•10 years ago
|
||
Comment on attachment 8358847 [details] [diff] [review] idb_crash.diff Review of attachment 8358847 [details] [diff] [review]: ----------------------------------------------------------------- mccr8: b2g uses IDB a lot so it seems like it would be nice to take it basically hedging that b2g is more prone to possible IDB issues
Attachment #8358847 -
Flags: approval-mozilla-b2g26?
Attachment #8358847 -
Flags: approval-mozilla-b2g26+
Attachment #8358847 -
Flags: approval-mozilla-b2g18+
Comment 21•10 years ago
|
||
> Actually, at sec-moderate this doesn't meet ESR landing criteria - why should we take this on that branch
Well, it is moderate because we don't think it can actually cause a problem, but if we're wrong, it would be a sec-high problem, and this patch is low risk enough that it seems bad to leave things in a possibly dangerous state for a year.
Updated•10 years ago
|
Flags: needinfo?(bugs)
Updated•10 years ago
|
Attachment #8366143 -
Flags: approval-mozilla-b2g18?
Comment 22•10 years ago
|
||
well, that's a risk we wouldn't want to take so tracking this to go in the next ESR24 alongside FF28 - it can land post-merge.
Comment 23•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/e82dbbfe3a66 https://hg.mozilla.org/releases/mozilla-b2g18/rev/a2b7e38ac18c https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/a2b7e38ac18c
Updated•10 years ago
|
Attachment #8366143 -
Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Updated•10 years ago
|
Whiteboard: [adv-main28+][adv-esr24.4+]
Updated•10 years ago
|
Whiteboard: [adv-main28+][adv-esr24.4+] → [adv-main28+][adv-esr24.4+][qa-]
Updated•10 years ago
|
status-b2g-v1.3T:
--- → fixed
Updated•9 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•