Closed Bug 993423 Opened 5 years ago Closed 5 years ago

XBL scopes can be hijacked from <svg:use>

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox30 --- unaffected
firefox31 --- fixed
firefox-esr24 --- unaffected

People

(Reporter: marius.mlynski, Assigned: bholley)

References

Details

(Keywords: regression, sec-high, Whiteboard: [qa-])

Attachments

(2 files)

Attached file 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.
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?
Flags: needinfo?(jwatt)
Flags: needinfo?(bugs)
Keywords: sec-high
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.
Flags: needinfo?(bugs)
(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.
Flags: needinfo?(jwatt)
Hm. So should we flag this stuff specially some how and not hoist it into the XBL scope?
Yes. just keeping its old NAC behavior is safest at this point.
Flags: sec-bounty?
(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.
It is not AC, but NAC. Could we just keep the NAC behavior?
(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.
(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.
Oh, wait. AC it is.
(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?
Does turning off event handlers and script elements bring back the old behavior?
(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.
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.
Assignee: nobody → bobbyholley
Attachment #8406402 - Flags: review?(bugs) → review+
(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
landed on m-c https://hg.mozilla.org/mozilla-central/rev/bdccb863d2de
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Flags: sec-bounty? → sec-bounty+
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 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.
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)?
(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.
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.
Whiteboard: [qa-]
Flags: in-testsuite? → in-testsuite+
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.