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)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
firefox-esr24 28+ fixed
b2g18 --- fixed
b2g-v1.1hd --- fixed
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: smaug, Assigned: smaug)

Details

(Keywords: csectype-uaf, sec-moderate, Whiteboard: [adv-main28+][adv-esr24.4+][qa-])

Attachments

(2 files)

Attached patch idb_crash.diffSplinter 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.
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.
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+
I guess this doesn't need to be critical as it sounds very difficult to actually exploit.
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.
fixed on central https://hg.mozilla.org/mozilla-central/rev/9539fbbb66ec
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Given how simple the fix is we should take it on Aurora (Fx28) which also gets it into b2g1.3.
Whiteboard: not exposed to content, only addons?
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)
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?
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)
Attachment #8358847 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This is trivial to backport to b2g26 (v1.2), but esr24/b2g18 would require a bit more love.
Simple fix for the older branches. Running through Try for sanity's sake.
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 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?
Actually, at sec-moderate this doesn't meet ESR landing criteria - why should we take this on that branch?
Flags: needinfo?(bugs)
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+
> 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.
Flags: needinfo?(bugs)
Attachment #8366143 - Flags: approval-mozilla-b2g18?
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.
Attachment #8366143 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
Whiteboard: [adv-main28+][adv-esr24.4+]
Whiteboard: [adv-main28+][adv-esr24.4+] → [adv-main28+][adv-esr24.4+][qa-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.