Closed Bug 1183882 Opened 4 years ago Closed 4 years ago

PushMessageData extends nsWrapperCache, but is not cycle collectable, nor has QI support for nsWrapperCache

Categories

(Core :: DOM: Service Workers, defect)

36 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- unaffected
firefox41 + fixed
firefox42 + fixed
firefox-esr31 --- unaffected
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

(Keywords: sec-high, Whiteboard: [post-critsmash-triage])

Attachments

(1 file)

Run the following in ServiceWorker:
var e = new PushEvent("test");
e.data.foo = e;


#0  0x00007fffed486d4b in nsWrapperCache::PreserveWrapper(nsISupports*) (this=0x7fffbabb09c8, aScriptObjectHolder=0x7fffbabb09c0)
    at /home/smaug/mozilla/hg/push-m-i/js/xpconnect/wrappers/../../../dom/base/nsWrapperCache.h:234
#1  0x00007fffee509ec3 in mozilla::dom::PreserveWrapperHelper<mozilla::dom::workers::PushMessageData, true>::PreserveWrapper(mozilla::dom::workers::PushMessageData*) (aObject=0x7fffbabb09c0)
    at ../../dist/include/mozilla/dom/BindingUtils.h:2639
#2  0x00007fffee509e95 in mozilla::dom::PreserveWrapper<mozilla::dom::workers::PushMessageData>(mozilla::dom::workers::PushMessageData*) (aObject=0x7fffbabb09c0)
    at ../../dist/include/mozilla/dom/BindingUtils.h:2646
#3  0x00007fffee4d93b5 in mozilla::dom::PushMessageDataBinding_workers::_addProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>) (cx=0x7fffc16b3480, obj=..., id=..., val=...)
    at ./PushMessageDataBinding.cpp:148
#4  0x00007ffff2169290 in js::CallJSAddPropertyOp(JSContext*, bool (*)(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>), JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>) (cx=0x7fffc16b3480, op=0x7fffee4d9350 <mozilla::dom::PushMessageDataBinding_workers::_addProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<jsid>, JS::Handle<JS::Value>)>, obj=..., id=..., v=...)
    at /home/smaug/mozilla/hg/push-m-i/js/src/jscntxtinlines.h:330
#5  0x00007ffff2112e22 in CallAddPropertyHook(js::ExclusiveContext*, JS::Handle<js::NativeObject*>, JS::Handle<js::Shape*>, JS::Handle<JS::Value>) (cx=0x7fffc16b3480, obj=..., shape=..., value=...)
    at /home/smaug/mozilla/hg/push-m-i/js/src/vm/NativeObject.cpp:995
#6  0x00007ffff20c752f in AddOrChangeProperty(js::ExclusiveContext*, JS::Handle<js::NativeObject*>, JS::Handle<jsid>, JS::Handle<JSPropertyDescriptor>) (cx=0x7fffc16b3480, obj=..., id=..., desc=...)
    at /home/smaug/mozilla/hg/push-m-i/js/src/vm/NativeObject.cpp:1177



Simple ServiceWorkerConsole https://github.com/smaug----/serviceworkerconsole, but don't know how to actually serve that as
https + html page. github pages don't seem to work for me.
but running that as localhost works.
Blocks: 1153499
Keywords: sec-high
Assignee: nobody → bugs
(note, I'm adding assertions to catch this kind of errors)
Attached patch patchSplinter Review
Attachment #8633791 - Flags: review?(nsm.nikhil)
Comment on attachment 8633791 [details] [diff] [review]
patch


[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Rather easily, I'd say

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Commit message could be
"Bug 1183882, properly implement wrapper caching on PushMessageData, r=nsm"

Which older supported branches are affected by this flaw?
I was told this is exposed in Aurora/Nightly

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should work with aurora too.

How likely is this patch to cause regressions; how much testing does it need?
Not likely
Attachment #8633791 - Flags: sec-approval?
Attachment #8633791 - Flags: approval-mozilla-aurora?
Blocks: 1038811
poiru: it would be interesting to see if your CC analysis stuff would have caught this...
The assertions from Bug 1183604 did catch this.
I'll land that patch once the issues it found have been landed.
Luckily no issues exposed to web in release builds.
Comment on attachment 8633791 [details] [diff] [review]
patch

Approvals given for landing.
Attachment #8633791 - Flags: sec-approval?
Attachment #8633791 - Flags: sec-approval+
Attachment #8633791 - Flags: approval-mozilla-aurora?
Attachment #8633791 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/71541c64c605
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Group: dom-core-security → release-core-security
Whiteboard: [post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.