Closed Bug 1033253 Opened 8 years ago Closed 8 years ago

"Assertion failure: uintptr_t(str) > 0x1000" with a specific WebRTC constraints object due to function Xrays not handling .name on anonymous functions properly

Categories

(Core :: XPConnect, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla33
Tracking Status
firefox33 + verified
firefox-esr24 --- wontfix
firefox-esr31 --- wontfix

People

(Reporter: jruderman, Assigned: bholley)

References

Details

(4 keywords, Whiteboard: [adv-main33-])

Attachments

(3 files, 1 obsolete file)

Attached file testcase
Assertion failure: uintptr_t(str) > 0x1000, at ../../../dist/include/js/Value.h:721
Attached file stack
This testcase doesn't crash, but I don't trust anything that gets near jsval_layout.asBits.
Very likely (to be confirmed) this is a trying to set a nullptr into a JS string; it's possible it's an uninitialized value.  Once we take a look at the case it should become clear which one is happening. 

Jesse: I'm guessing if it's a guaranteed nullptr, that's likely not a sec issue.
I'm a bit surprised that the webidl bindings let a function through as a webidl object argument, but here you go.

This part of the code needs updating anyways to match advancements in the spec (CreateOffer/Answer no longer takes constraints but a dictionary). I'll open a separate bug.
Assignee: nobody → jib
Attachment #8449511 - Flags: review?(rjesup)
Attachment #8449511 - Flags: feedback?(bzbarsky)
> I'm a bit surprised that the webidl bindings let a function through as a webidl object
> argument

Web IDL "object" means any JS object.  Functions are objects in JS.

I don't care that much whether you want to filter them out here or not (in a separate bug, please), though per spec I assume you shouldn't.  But the real bug here is in the Xray code, which for a function Xray does this:


  } else if (id == GetRTIdByIndex(cx, XPCJSRuntime::IDX_NAME)) {
     FillPropertyDescriptor(desc, wrapper, JSPROP_PERMANENT | JSPROP_READONLY,
                            StringValue(JS_GetFunctionId(JS_GetObjectFunction(target))));

But for an anonymous function JS_GetFunctionId will return null, which is what's triggering the assert.  WebRTC is just a way of triggering this codepath.  Here are simpler STR:

1)  Load data:text/html,<script>document.documentElement.onclick = function() {}</script>
2)  In a browser-environment scratchpad, evaluate:

       content.document.documentElement.onclick.name

This code needs to null-check the return value of JS_GetFunctionId.
Assignee: jib → bobbyholley
Blocks: 976148
Component: WebRTC → XPConnect
Keywords: regression
Attachment #8449511 - Flags: feedback?(bzbarsky) → feedback-
Summary: "Assertion failure: uintptr_t(str) > 0x1000" with a specific WebRTC constraints object → "Assertion failure: uintptr_t(str) > 0x1000" with a specific WebRTC constraints object due to function Xrays not handling .name on anonymous functions properly
Attachment #8449511 - Flags: review?(rjesup)
Comment on attachment 8449574 [details] [diff] [review]
Null-check the result of JS_GetFunctionId. v1

r=me
Attachment #8449574 - Flags: review?(bzbarsky) → review+
Sounds like a null-deref.  I'm going to leave it hidden for now because I'm not sure if any of the other implications of the test case are worth hiding or not.
Keywords: sec-other
Comment on attachment 8449511 [details] [diff] [review]
Throw if mandatory.constraint is not an object

(In reply to Boris Zbarsky [:bz] from comment #5)
> I don't care that much whether you want to filter them out here or not (in a
> separate bug, please), though per spec I assume you shouldn't.

Thanks for spotting the real bug. I've filed Bug 1033833 to use dictionaries instead of constraints here, so that takes care of the filtering question.
Attachment #8449511 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ffddc07896d7
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Flags: in-testsuite? → in-testsuite-
Whiteboard: [adv-main33-]
Confirmed assert in Fx33, 2014-07-01.
Verified fixed in Fx33, 2014-10-06.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.