Closed Bug 768750 Opened 12 years ago Closed 12 years ago

"Assertion failure: !JSID_IS_VOID(id)" with XBL proto


(Core :: XBL, defect)

Not set



Tracking Status
firefox17 --- wontfix
firefox18 + fixed
firefox19 + fixed
firefox-esr10 --- unaffected
firefox-esr17 --- wontfix


(Reporter: jruderman, Assigned: Waldo)



(4 keywords, Whiteboard: [adv-main18+])


(3 files, 1 obsolete file)

Assertion failure: !JSID_IS_VOID(id), at js/src/jsscope.cpp:617
Attached file stack trace
Why now? I assume no one's touched XBL in a long time, is this a regression from a JS engine change? If so, how far back does it go?
Luke, does this stack trace happen to ring any bells?
I know someone who just touched XBL for a JS change...
jwalden: is this assertion related to your changes, and is it worrying?
Assignee: nobody → jwalden+bmo
Waldo ping on comment 5. Welcome back :)
Any updates here? Can we get a suggested security rating?
(In reply to Luke Wagner [:luke] from comment #4)

(In reply to Al Billings [:abillings] from comment #7)
> Any updates here? Can we get a suggested security rating?

Just noticed mail for this, will look after I get a browser build spun up.
Attached patch Possible trunk patch (obsolete) — Splinter Review
It's related to my changes.

  FieldSetterImpl(JSContext *cx, JS::CallArgs args)
    const JS::Value &thisv = args.thisv();

    js::Rooted<JSObject*> thisObj(cx, &thisv.toObject());

    bool installed = false;
    js::Rooted<JSObject*> callee(cx, &args.calleev().toObject());
    js::Rooted<jsid> id(cx);
    if (!InstallXBLField(cx, callee, thisObj, id.address(), &installed)) {
      return false;

    js::Rooted<JS::Value> v(cx,
                            args.length() > 0 ? args[0] : JS::UndefinedValue());
    return JS_SetPropertyById(cx, thisObj, id, v.address());

If |InstallXBLField| returns true but doesn't set |id|, it'll be |JSID_VOID|.  That happens when |!installed| after that method returns.  It's correct in the |installed| case to fall through to the current code, but on first glance it doesn't look correct in the |!installed| case.  What appears to be happening is that we fall through to setting the property in this case using JSID_VOID, which is crazytalk and leads to the assertion.

There are a few code paths where this can happen.  I don't know enough about some of them to be confident they're reachable, but one is simple enough -- when |this| has an nsISupports private, but it's not a wrapped native.  In the old code, before bug 758912, this was all in the resolve hook; if you hit that case, we'd merrily continue resolving the property all the way up the prototype chain.  You can't have these semantics in the new code; the resolve hook resolved the property as a [[Get]]/[[Set]] accessor property, now we're just in the middle of calling it, too late to go back and resolve some more.

But looking a little more closely, and thinking harder about one of the under-described comments in the current code, it looks like we can fold that case into the |this|-testing code used by both FieldGetter and FieldSetter to avoid that particular case.  Then, calling the getter/setter function on a prototype object would just throw a TypeError, none of this weird case of needing to "fail" silently.  This patch implements that and is currently running through try.  I'll write up an explanation of why this change is acceptable (and why/how the old, pre-getter/setter code needed it), then do an r? after I get a solid try run.  I did one on an older version of this patch and got back mostly green except for one test, so I'm reasonably optimistic.

Regarding branches: the approach in this patch is cleanup-y and would only be suitable for trunk.  A smaller spot-fix is possible, so no worries about that.  (Perhaps that should be applied to trunk before this cleanup bit, wouldn't be too difficult to do that.)  As for security consequences of the error here...not sure.  I don't know the ramifications of calling a JSAPI method that takes an id and getting JSID_VOID in it.  Maybe nothing observable, maybe oddness, maybe worse.  More investigation needed.

So it looks like this patch is good to go for trunk.  I'll give a look into splitting it up into a mini-fix and the larger cleanup for trunk/branches simplicity.
Attached patch PatchSplinter Review
This fixes the instant problem and passes try.

The further changes I'll punt to another bug.

It's not clear to me what the security implications of this bug are, exactly.  jsids flow a lot of places, and getting JSID_VOID in a property might or might not have consequences.  For example, it'd stop property iteration using JS_NextProperty prematurely, I think, but that probably wouldn't hurt anything except correctness.  Assumptions from those results seem unlikely to be used in security-sensitive ways.

What seems most likely to be worrisome is that TI uses JSID_VOID to represent the "type" of all elements on an object, so maybe -- maybe -- if the id of the added property gets communicated to TI, it'll assume something about elements that might or might not be wrong, and it'll have stale assumptions about properties.  I don't know.  There's an awful lot of TI code that depends on using JSID_VOID this way, and I'm not TI expert.  I think the best option is to play it safe, assume this is a real problem, and fix it relatively close to the end of a cycle, since the problem the patch fixes is fairly obvious.  But we're also protected by XBL not being web-visible, so it's not quite so bad, maybe.
Attachment #671684 - Attachment is obsolete: true
Attachment #673100 - Flags: review?(bzbarsky)
Blocks: 758912
Comment on attachment 673100 [details] [diff] [review]

Attachment #673100 - Flags: review?(bzbarsky) → review+
Comment on attachment 673100 [details] [diff] [review]

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Pretty easily.  Parlaying it into an actual exploit would be a bit more complex, and I dunno if it's even possible, honestly.  See my previous comment.

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

Which older supported branches are affected by this flaw?
Release, aurora, beta.  Not esr10.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Not yet, but they'd be trivial, and no riskier than this patch.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely.  It converts use of an "uninitialized" (a constructor initializes the jsid to JSID_VOID) value to check before using it.  I can't see how anything would go awry.
Attachment #673100 - Flags: sec-approval?
From the description seems unlikely to cause us trouble, and would require an add-on or other chrome code to be doing this -- going with sec-low.
Attachment #673100 - Flags: sec-approval? → sec-approval+

I'll let this sit a bit before going for branches.
OS: Mac OS X → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla19
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> I'll let this sit a bit before going for branches.

Can you please nominate this for uplift on aurora?
(In reply to bhavana bajaj [:bajaj] from comment #17)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> >
> > 
> > I'll let this sit a bit before going for branches.
> Can you please nominate this for uplift on aurora?
only if you think it has had sufficient bake time :)
Comment on attachment 673100 [details] [diff] [review]

Review of attachment 673100 [details] [diff] [review]:

Yeah, I think this is good to go now -- probably would have been a week earlier or so, just too many things to juggle.  If anything were to come up I'd expect it to be a fuzz bug at best, and I haven't seen anything on that front.  Although, given the small size and simplicity of the fix, I wouldn't expect anything anyway.

Probably it's a bit late for beta at this point in the cycle, but no harm in requesting just in case.
Attachment #673100 - Flags: approval-mozilla-beta?
Attachment #673100 - Flags: approval-mozilla-aurora?
Comment on attachment 673100 [details] [diff] [review]

Since it's sec-low, we're not tracking it, and it's late in beta I think we should just keep churn on m-b low and wait for this in 18 but please go ahead with uplift to Aurora.
Attachment #673100 - Flags: approval-mozilla-beta?
Attachment #673100 - Flags: approval-mozilla-beta-
Attachment #673100 - Flags: approval-mozilla-aurora?
Attachment #673100 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main18+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.