993423 - XBL scopes can be hijacked from <svg:use>

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mariusz Mlynski]

Attachment: Proof of concept

This is a regression from bug 986730. Now that all anonymous content is hosted in the XBL scope, it is possible to execute scripts in its context by cloning a user-defined node into the anonymous content of <svg:use>. Please see the attached proof of concept.

Comment 1

[Bobby Holley (:bholley)]

Ugh, we should have seen that coming. Nice one, Mariusz.

jwatt, smaug, what do you think we should do here? Is there some way we can avoid using anonymous content to implement this web-facing feature?

Comment 2

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

As of now we I can't think of any other way to implement svg:use.
We have bugs open to implement in the way the spec wants, but that is far from being trivial.
And we'd still probably implement the cloned part using anonymous content.

Comment 3

[Jonathan Watt [:jwatt]]

(In reply to Olli Pettay [:smaug] from comment #2)
> As of now we I can't think of any other way to implement svg:use.
> We have bugs open to implement in the way the spec wants, but that is far
> from being trivial.

Indeed.

> And we'd still probably implement the cloned part using anonymous content.

Actually the idea is to only have one instance that is somehow lightly restyled as each instance is painted...or something. So no cloning would be involved. I don't think it's a solution here though since it's a fairly large project, and it likely has a bunch of issues to work through that we aren't even aware of yet.

Comment 4

[Bobby Holley (:bholley)]

Hm. So should we flag this stuff specially some how and not hoist it into the XBL scope?

Comment 5

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

Yes. just keeping its old NAC behavior is safest at this point.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #5)
> Yes. just keeping its old NAC behavior is safest at this point.

Is there a way that we can distinguish this usage of AC from the others? I'd hope that we don't have to add another Node bit.

Comment 7

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

It is not AC, but NAC. Could we just keep the NAC behavior?

Comment 8

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #7)
> It is not AC, but NAC.

Really. Then why mariusz did Mariusz say that this was a regression from bug 986730, and not bug 825392? Moreover, why did the issue in bug 986730 comment 2 pop up in that bug, and not in bug 825392?

> Could we just keep the NAC behavior?

Do you mean the behavior where NAC lives in the content scope. If so, then not really...hoisting NAC was necessary to get rid of same-compartment security wrappers. We'd have to back out the SOW removal and a bunch of security wrapper simplifications :-(

We could more easily back out bug 986730, but I think that would be a bit of a shame.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #8)
> Moreover, why did the issue in bug 986730
> comment 2 pop up in that bug, and not in bug 825392?

Er, sorry. Bug 986730 comment 7.

Comment 10

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

Oh, wait. AC it is.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #10)
> Oh, wait. AC it is.

Ok. So we could back out bug 986730, but then:
* we lose the other security benefits of having all AC inaccessible to content
* marquee doesn't work when JS is disabled (bug 948000)
* we can't do the refactoring in bug 988842

So I'd like to do something smarter here.

Can we just turn off event handlers and script elements for these <use> things?

Comment 12

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

Does turning off event handlers and script elements bring back the old behavior?

Comment 13

[Bobby Holley (:bholley)]

(In reply to Olli Pettay [:smaug] from comment #12)
> Does turning off event handlers and script elements bring back the old
> behavior?

Behavior in what sense? We'd still be hoisting this content into the XBL scope. We just wouldn't be hoisting any content-defined script.

The alternative is of course to just not hoist this particular kind of AC. But then we have to detect it, and handle this case.

Comment 14

[Jonathan Watt [:jwatt]]

FWIW I've definitely seen content that has event handlers under a <use> element. Not so sure about script elements, so I'd expect that to be very rare.

Comment 15

[Bobby Holley (:bholley)]

Attachment: Don't hoist <svg:use> content into the XBL scope. v1

Comment 16

[Bobby Holley (:bholley)]

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

Comment 17

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #16)
> https://tbpl.mozilla.org/?tree=Try&rev=70c38d7dadd4

This had some orange due to a missing null-check. I _think_ this should be good to go aside from that.

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

Comment 18

[Carsten Book [:Tomcat]]

landed on m-c https://hg.mozilla.org/mozilla-central/rev/bdccb863d2de

Comment 20

[Matt Wobensmith [:mwobensmith][:matt:]]

Bug file shows missing plugin icon when loaded, doesn't appear to do anything else. Can anyone explain how to run this sample? I'd like to verify the fix. Thank you.

Comment 21

[Bobby Holley (:bholley)]

(In reply to Matt Wobensmith from comment #20)
> Bug file shows missing plugin icon when loaded, doesn't appear to do
> anything else. Can anyone explain how to run this sample? I'd like to verify
> the fix. Thank you.

In the regression range, you should see a message about chromeOnlyContent displayed in the DOM.

Comment 22

[Matt Wobensmith [:mwobensmith][:matt:]]

Thank you, Bobby. Using builds from 2014-04-06 and onwards, I don't see any messages to this effect anywhere. I'm looking at the Console (for errors) and Inspector. Am I looking in the right place, or am I missing something critical (likely)?

Comment 23

[Bobby Holley (:bholley)]

(In reply to Matt Wobensmith from comment #22)
> Thank you, Bobby. Using builds from 2014-04-06 and onwards, I don't see any
> messages to this effect anywhere.

You need builds from the revision in bug 986730 comment 21, which would be the 7th or 8th.

> I'm looking at the Console (for errors)
> and Inspector. Am I looking in the right place, or am I missing something
> critical (likely)?

It should appear in the web page itself.

In general this does have an automated test, so I'm not super worried if we're having trouble manually reproducing the original testcase.

Comment 24

[Matt Wobensmith [:mwobensmith][:matt:]]

I built Fx31 with those revisions, no dice for me. Also tried several builds from post-checkin on the 6th through the 9th, no luck.

We'll rely on the existing test. 

I'm marking [qa-] for posterity.

Thanks for the help.