852134 - SmsRequest DROPs without clearing its wrappercache

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Andrew McCreight [:mccr8]]

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?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

It should be b2g only.

Comment 2

[Andrew McCreight [:mccr8]]

I see at least one SMS related bug on Android (bug 719795).

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: DROP less aggressively

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: DROP less aggressively

Comment 5

[Andrew McCreight [:mccr8]]

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.

Comment 6

[Mounir Lamouri (: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?

Comment 7

[Andrew McCreight [:mccr8]]

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 8

[Andrew McCreight [:mccr8]]

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

Comment 9

[Andrew McCreight [:mccr8]]

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.

Comment 10

[Andrew McCreight [:mccr8]]

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

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/561ac0327dc9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/2de7d72b757a

What about v1.0.1?

Comment 13

[Andrew McCreight [:mccr8]]

1.0.1 is certainly affected, I don't know how landing goes for that.

Comment 14

[Alex Keybl [:akeybl]]

We still have the opportunity to get this all the way up to v1.0.1. Marking as tef+ and CCing Ryan.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/a81bc014d926