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

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

Assertion failure: !JSID_IS_VOID(id), at js/src/jsscope.cpp:617

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Daniel Veditz [:dveditz]]

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?

Comment 3

[Andrew McCreight [:mccr8]]

Luke, does this stack trace happen to ring any bells?

Comment 4

[Luke Wagner [:luke]]

I know someone who just touched XBL for a JS change...

Comment 5

[Daniel Veditz [:dveditz]]

jwalden: is this assertion related to your changes, and is it worrying?

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Waldo ping on comment 5. Welcome back :)

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Any updates here? Can we get a suggested security rating?

Comment 8

[Jeff Walden [:Waldo]]

(In reply to Luke Wagner [:luke] from comment #4)
*dagger*

(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.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Possible trunk patch

It's related to my changes.

  bool
  FieldSetterImpl(JSContext *cx, JS::CallArgs args)
  {
    const JS::Value &thisv = args.thisv();
    MOZ_ASSERT(ValueHasISupportsPrivate(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.

https://tbpl.mozilla.org/?tree=Try&rev=99aef074b6a5

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.

Comment 10

[Jeff Walden [:Waldo]]

Ahem:

https://tbpl.mozilla.org/?tree=Try&rev=b41a0b02c09c

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.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Patch

This fixes the instant problem and passes try.

https://tbpl.mozilla.org/?tree=Try&rev=e6a76b7bdf9a

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 673100 [details] [diff] [review]
Patch

r=me

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 673100 [details] [diff] [review]
Patch

[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.

Comment 14

[Daniel Veditz [:dveditz]]

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.

Comment 15

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b6895b0db6

I'll let this sit a bit before going for branches.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6b6895b0db6

Comment 17

[bhavana bajaj [:bajaj]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b6895b0db6
> 
> I'll let this sit a bit before going for branches.

Can you please nominate this for uplift on aurora?

Comment 18

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #17)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #15)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/c6b6895b0db6
> > 
> > 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 19

[Jeff Walden [:Waldo]]

Comment on attachment 673100 [details] [diff] [review]
Patch

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.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 673100 [details] [diff] [review]
Patch

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bd7cf3e4c57f