532808 - "ASSERTION: aParentContent must be element" with XBL

Status: RESOLVED WONTFIX

(Core :: XBL, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: aParentContent must be element: 'aParentContent && aParentContent->IsNodeOfType(nsINode::eELEMENT)', file layout/style/nsStyleSet.cpp, line 819

###!!! ASSERTION: non-element leaked into SelectorMatches: '!aContent || aContent->IsNodeOfType(nsINode::eELEMENT)', file layout/style/nsCSSRuleProcessor.cpp, line 904

###!!! ASSERTION: content (if present) must be element: '!aData->mContent || aData->mContent->IsNodeOfType(nsINode::eELEMENT)', file layout/style/nsCSSRuleProcessor.cpp, line 2083

Comment 1

[Boris Zbarsky [:bzbarsky]]

So in nsCSSFrameConstructor::GetInsertionPrevSibling we end up with |container| being an nsTextNode.  

That happens because we have parentFrame being an nsTextFrame in ContentInserted.  And that happens because that's what GetInsertionPoint returns in this case.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The relevant textnode has a single newline in it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

The frame tree:

          Block(body)(3)@0x7f558bf869e0 [content=0x7f558bf753d0] {480,480,35640,0} [state=00100000] sc=0x7f558bf86780(i=0,b=1)<
            line 0x7f558bf91200: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8348] {0,0,35640,0} <
              Block(div)(0)@0x7f558bf86e40 [content=0x7f558bf75560] {0,0,35640,0} [state=00100000] sc=0x7f558bf86458(i=0,b=1)<
                line 0x7f558bf911c8: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x8348] {0,0,35640,0} <
                  Block(div)(-1)@0x7f558bf86ef0 [content=0x7f558bf75fb0] {0,0,35640,0} [state=00100000] sc=0x7f558bf86810(i=1,b=0)<
                    line 0x7f558bf91398: count=2 state=inline,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x10300] {0,0,0,0} <
                      Inline(span)(0)@0x7f558bf912c8 next=0x7f558bf91330 {0,-900,0,1140} [content=0x7f558bf6de20] [sc=0x7f558bf868b0]<>
                      Text(1)@0x7f558bf91330[0,1,T]  {0,0,0,0} [state=48600000] [content=0x7f558ec6e240] sc=0x7f558bf91238 pst=:-moz-non-element<
                        "\n"
                      >

Comment 4

[Boris Zbarsky [:bzbarsky]]

That textframe is created when the <span> is inserted via surroundContents.  Before that it's suppressed, as expected.

The issue is that LocateInstance in this case (called from nsXBLPrototypeBinding::GetInsertionPoint) returns an nsTextNode.  In particular, it returns the textnode that is now the second child of the anonymous <div>.  And it does _that_ because in the binding template the insertion point is under the second child.... and because modifying binding content screws up LocateInstance.  As the nice comment at the top of LocateInstance says:

871   // XXX We will get in trouble if the binding instantiation deviates from the template
872   // in the prototype.

Thanks, hyatt.  :(  I can try to figure out exactly where LocateInstance screws up, but fundamentally it's not possible to accurately determine the right insertion point in the instantiation if all you're given is the insertion point in the template and the instantiation can have a fundamentally different DOM.

Which is to say we need to disallow editing XBL template instantiation DOMs, or something.  We can probably sanity-check here (in LocateInstance, say) to avoid the assertions, but the behavior is fundamentally broken and I can't guarantee that it can't lead to other bugs....

Comment 5

[Boris Zbarsky [:bzbarsky]]

And _why_ again are we allowing untrusted bindings at all?  :(

Comment 6

[Jesse Ruderman]

Attachment: testcase without DOM Range

Comment 7

[Daniel Veditz [:dveditz]]

(In reply to comment #5)
> And _why_ again are we allowing untrusted bindings at all?  :(

You tell me: when can we break it? If we keep promoting each 3.x version as a minor upgrade then never. Can we disable it today on the trunk and see how far we get? (maybe with a pref so it's easy to flip back on if we have to, and easy for a security addon to flip back off if we've turned it on)

Comment 8

[Boris Zbarsky [:bzbarsky]]

I think we should do the following ASAP on trunk:

1)  Make untrusted bindings not apply, period.
2)  Make getAnonymousNodes do a security check.

Any objections?

Comment 9

[Wladimir Palant]

(In reply to comment #8)
> 1)  Make untrusted bindings not apply, period.

What counts as an untrusted binding? If that means "non-chrome" then sure, it will break Adblock Plus, it uses a custom protocol for bindings applied to web pages. I can imagine that a bunch of other extensions use non-chrome protocols for bindings as well. Not to mention the unit tests using data: protocol for bindings.

(In reply to comment #4)
> Which is to say we need to disallow editing XBL template instantiation DOMs, or
> something.  We can probably sanity-check here (in LocateInstance, say) to avoid
> the assertions, but the behavior is fundamentally broken and I can't guarantee
> that it can't lead to other bugs....

That could be a problem. Editing the DOM is something that is occasionally being done because XBL doesn't allow extending content - if you inherit from an XBL template you either have to keep the original content or completely replace it. I know that at the very least TomTom HOME modifies XBL instantiations a lot because of this.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> What counts as an untrusted binding?

One whose principal is not system.

> it uses a custom protocol for bindings

Then you can control the principal of the binding, so should be fine, with a slight change to your code.  We'd need to make nsIScriptSecurityManager::getSystemPrincipal scriptable is all.

> Not to mention the unit tests using data: protocol for bindings.

We can fix the tests; not a problem.  A lot of those tests are actually crashtests for crashes triggered by malicious bindings in the past, so....

> Editing the DOM is something that is occasionally being done

The point is, once you do it your binding is broken.  But I'm not proposing disallowing this in the short term.  I'm just proposing making it impossible for untrusted script to do this to the trusted bindings we would still be attaching to web pages (e.g. Adblock Plus bindings), specifically by never handing out such anonymous nodes to the untrusted script in question.

We'd probably also need to change the way event.originalTarget works in untrusted contexts...

Comment 11

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

(In reply to comment #10)
> We'd probably also need to change the way event.originalTarget works in
> untrusted contexts...
Like never returning anonymous content? Same for relatedTarget.

originalTarget is used also in C++ so we need so easy way to detect if
caller is JS or C++. Or perhaps C++ could use nsIPrivateDOMEvent to get
originalTarget.

IIRC there is actually a bug open to remove originalTarget from content events.

(XBL2 draft uses different event propagation mechanism.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Like never returning anonymous content?

Precisely.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Unfortunately only allowing bindings from chrome doesn't help much as long as getAnonymousChildrenFor exists

Comment 14

[Boris Zbarsky [:bzbarsky]]

Jonas, see comment 8 item 2.  I propose that it stop existing for untrusted code.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But XBL bindings bound to a untrusted page run as untrusted code. It would at the very least break marquee and remote XUL. Dunno how <video> would fair.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It would at the very least break marquee

Ugh.

Can we disallow untrusted script from adding or removing anonymous nodes, then?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Probably not without breaking remote XUL. Which might be ok. Good topic for next week?

Comment 18

[Mike Shaver (:shaver emeritus)]

(In reply to comment #17)
> Probably not without breaking remote XUL. Which might be ok.

Might be better than OK!

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Unless I'm mistaken, we can wait for a security release for this on mozilla-1.9.2, yes?

Comment 20

[Boris Zbarsky [:bzbarsky]]

Sort of.  I'm not seeing an obvious fix here that doesn't break compatibility, but that would be a problem at this stage just as much as after ship, I guess...

Comment 21

[Samuel Sidler (old account; do not CC)]

beltzner doesn't read bugmail. Re-nominating so he sees comment 20.

Comment 22

[Daniel Veditz [:dveditz]]

I can't see much point in blocking 1.9.0's EOL release on this compatibility-breaking change, but once we have a fix that makes the 1.9.2 branch happy we'll try to land it everywhere else at the same time.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Thanks, Sam; decision stands, though. Not blocking final. Adding the term that will help me pick this up for 3.6.x.

Comment 24

[Jesse Ruderman]

The assertions in comment 0 were removed in rev 1b7b064ee77a, "Bug 562688 part 13.  Eliminate eELEMENT usage in layout/style, except the rule processor."

I don't know whether this has a real effect on this bug, or the plans in comment 8.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> The assertions in comment 0 were removed in rev 1b7b064ee77a

While true, they were replaced with asserts in ToElement() that should now trigger here, right?  Are they not firing?

Nothing about the fundamental problem in this bug has changed; it might just become more urgent to fix as we move from calling virtual nsIContent methods in selector matching to non-virtual Element methods on a pointer that's not actually an Element but has been cast to one.

Comment 26

[Jesse Ruderman]

I don't hit any assertions when I load the testcase on trunk now.

Comment 27

[Boris Zbarsky [:bzbarsky]]

That's... quite odd.  Do you know when you last did see asserts?  I can try to bisect+debug, but a good starting range might help.

Comment 28

[Jesse Ruderman]

I don't know, sorry.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Looks like this assertion stopped firing when lazyfc landed.  Trying to figure out why now.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase modified to still assert with lazyfc

Just need to flush frame construction in the middle there...

Comment 31

[Timothy Nikkel (:tnikkel)]

Yeah, lazyfc would construct frames for the nodes in the opposite order that they are added there.

Comment 32

[Boris Zbarsky [:bzbarsky]]

With that last testcase on tip, I get:

###!!! ASSERTION: Not an element?: 'IsElement()', file ../../dist/include/Element.h, line 60

as expected.

Comment 33

[Timothy Nikkel (:tnikkel)]

To address a question from way above: we already disallow binding to data URIs, but you can enable a pref, layout.debug.enable_data_xbl, to allow them, which we do in our testing infrastructure so those old tests still work.

Comment 34

[Damon Sicore (:damons)]

What's next here?

Comment 35

[Jesse Ruderman]

Are we planning to fix this by dropping XBL-on-the-web (bug 546856 + getAnonymousNodes) or in another way?

Comment 36

[Johnny Stenback (:jst)]

Jonas, did this get fixed on trunk by disabling remote XUL/XBL? Or do we still need to worry about this for trunk?

For the branches this still needs a fix either way though.

Comment 37

[Johnny Stenback (:jst)]

Jonas?

Comment 38

[Johnny Stenback (:jst)]

I talked to Jonas about this, this is fixed on trunk by disabling remote XUL and XBL, but it's still an issue on the 1.9.2 branch.

Comment 39

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

With the upcoming End of Life of 1.9.2, is this bug even relevant? 

This is fixed on trunk.

I'm tempted to won't fix this at this point.

Comment 40

[Alex Keybl [:akeybl]]

(In reply to Al Billings [:abillings] from comment #39)
> With the upcoming End of Life of 1.9.2, is this bug even relevant? 
> 
> This is fixed on trunk.
> 
> I'm tempted to won't fix this at this point.

Nope - I suggest you all mark sg:crit bugs that affect 1.9.2 and we don't plan to chemspill FF11 for as wontfix.

Comment 41

[Alex Keybl [:akeybl]]

Sorry, there's no reason to limit that to sg:crit - we can perform this for all 1.9.2 security bugs at this point.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does the fact that this is no longer a *security* bug really mean that it's definitively WONTFIX?

Comment 43

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

David, it is 1.9.2 only and we're not releasing 1.9.2 anymore. Is there a good reason to work on it?

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

See comment 38 -- it's not a security bug anymore because we disabled remote XUL and XBL, not because we know the bug is gone.

Comment 45

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

I understand this but when I look at the top of the bug, this is a 1.9.2 bug.

Should we change this to trunk and make this about fixing the overall security issue?

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What overall security issue.  There's no remaining *security* problem because XBL and XUL have been disabled for remote content.  That doesn't mean there isn't still a bug here.

Comment 47

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

Is this an issue that someone is going to actually fix in the current codebase?

Comment 48

[Timothy Nikkel (:tnikkel)]

Who knows, but that's not the usual criteria we use to leave bugs open.

Comment 49

[Boris Zbarsky [:bzbarsky]]

XBL is now disabled in Firefox (Bug 1583314) and is in the process of being removed from Gecko (Bug 1566221), so closing bugs requesting changes to its implementation as wontfix.