Closed Bug 852134 Opened 7 years ago Closed 7 years ago

SmsRequest DROPs without clearing its wrappercache

Categories

(Core :: DOM: Device Interfaces, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23
blocking-b2g tef+
Tracking Status
firefox19 --- disabled
firefox20 --- disabled
firefox21 --- disabled
firefox22 + disabled
firefox23 --- fixed
firefox-esr17 --- disabled
b2g18 + fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

Details

(Keywords: csectype-uaf, sec-critical, Whiteboard: [adv-main23-])

Attachments

(1 file, 1 obsolete file)

I found this while auditing other classes that use NS_IMPL_CYCLE_COLLECTION_TRACE_JSVAL_MEMBER_CALLBACK.  The calls to UnrootResult outside of Unlink and the destructor leave dangling pointers to the wrapper cache.  Same issue as bug 851996, basically the same fix should work.  I'll double check other classes today, though we've already fixed related sec-crits for them, so hopefully they are okay.  Being able to double HOLD and DROP really makes this easier to fix.  Unfortunately this appears to affect everything.

Is SmsRequest something we should fix in ESR17, or is it not web exposed in desktop?
Component: DOM: Events → DOM: Device Interfaces
It should be b2g only.
I see at least one SMS related bug on Android (bug 719795).
Attached patch DROP less aggressively (obsolete) — Splinter Review
Assignee: nobody → continuation
Attachment #726193 - Attachment is obsolete: true
Mounir, do we ship SMS in some kind of active capacity on Android or Desktop?  I'm wondering whether we want to patch this on beta.  If we do, then I may have to wait until next cycle to land this.
Flags: needinfo?(mounir)
WebSMS API on Android is being a pref and the real implementation is behind a compilation flag which is not enabled on any official Mozilla build. That means that turning on the pref will just give you an empty API shell. Though, that bug would apply with the pref turned on but I guess we shouldn't land something on ESR for a pref'd off feature?
Flags: needinfo?(mounir)
Great, thanks!  Yeah, no need to patch it where it is prefed off, so it sounds like b2g and trunk are the only relevant branches here.
Comment on attachment 726205 [details] [diff] [review]
DROP less aggressively

Review of attachment 726205 [details] [diff] [review]:
-----------------------------------------------------------------

Try run looked okay, modulo some X failures in another patch.  I'll retry it at some point.
https://tbpl.mozilla.org/?tree=Try&rev=cda337ecb483
Attachment #726205 - Flags: review?(bugs)
Attachment #726205 - Flags: review?(bugs) → review+
Comment on attachment 726205 [details] [diff] [review]
DROP less aggressively

[Security approval request comment]
How easily could an exploit be constructed based on the patch? probably not too easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? SMS is disabled everywhere aside from b2g, so the only relevant branches for this are b2g18 and trunk.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  Should be trivial to backport to b2g18.

How likely is this patch to cause regressions; how much testing does it need?  It should be pretty safe.
Attachment #726205 - Flags: sec-approval?
Attachment #726205 - Flags: sec-approval? → sec-approval+
Comment on attachment 726205 [details] [diff] [review]
DROP less aggressively

https://hg.mozilla.org/integration/mozilla-inbound/rev/561ac0327dc9

NOTE: 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 #): landing of SMS
User impact if declined: possibly security bugs
Testing completed: try run
Risk to taking this patch (and alternatives if risky): should be pretty low risk
String or UUID changes made by this patch: none
Attachment #726205 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/561ac0327dc9
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Attachment #726205 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
1.0.1 is certainly affected, I don't know how landing goes for that.
blocking-b2g: --- → tef?
We still have the opportunity to get this all the way up to v1.0.1. Marking as tef+ and CCing Ryan.
blocking-b2g: tef? → tef+
Whiteboard: [adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.