Closed Bug 1277475 (CVE-2016-5262) Opened 8 years ago Closed 8 years ago

XSS out of iframe sandbox, iframe disabled javascript. marquee

Categories

(Core :: DOM: Security, defect)

Product:
Core
Component:
DOM: Security
Version:
46 Branch
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox48 --- verified
firefox49 --- verified
firefox50 --- verified

People

(Reporter: nikita.arykov, Assigned: bobowen)

Details

(Keywords: csectype-other, sec-moderate, testcase, Whiteboard: [domsecurity-active][adv-main48+])

Attachments

(4 files, 3 obsolete files)

security bug.zip
471 bytes, application/java-archive
Details
Part 2: Ignore marquee attribute event handlers when sandboxed scripts flag is set on its owning Document
1.27 KB, patch
bzbarsky
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Part 1: Add a chrome and XBL accessor for the sandboxed scripts flag to Document WEBIDL
6.21 KB, patch
bobowen
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Part 3: Ensure marquee attribute event handlers don't run when sandboxed scripts flag is set on its owning Document
6.35 KB, patch
bobowen
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file security bug.zip — 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2661.102 Safari/537.36

Steps to reproduce:

1) create first file index.html with
<html>
<head></head>
<body>
<marquee loop=1 width=0 onfinish=alert(1)>
</body>
</html>
2) create second file iframe.html with
<html>
<body>
	<iframe frameborder="0" sandbox="allow-same-origin" src="./index.html">
	</iframe>
</body>
</html>
3) Open iframe.html at firefox last version 46.0.1


Actual results:

XSS. javascript executed.
At chrome and safari problem not occur.


Expected results:

XSS should not be executed

I think this issue is part of bug bounty program https://www.mozilla.org/en-US/security/client-bug-bounty/
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86
Bob, can you take a look or point to someone who can?
Group: firefox-core-security → core-security
Component: Untriaged → DOM: Security
Flags: needinfo?(bobowen.code)
Keywords: testcase
Product: Firefox → Core
Flags: sec-bounty?
Flags: sec-bounty?
Flags: sec-bounty?
Yes it would appear this is not caught by whatever checks we have to prevent script running in the iframe sandbox.
I'll dig a bit further, might be tomorrow though now.
Attachment #8759064 - Attachment mime type: application/zip → application/java-archive
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
Flags: needinfo?(bobowen.code)
I need to write a test for this, but thought I'd get the patch up for review, because I'm slightly concerned this might break things other than marquee.

This blocks listeners similar to the check we have in SetEventHandler [1], but not when they are from chrome/native.

This in fact stops the marquee from working at all in the sandbox, because it uses onload to initialise itself [2].
This is probably no bad thing for marquee, but as I say, my concern is this will break other things as well.


[1] http://searchfox.org/mozilla-central/rev/cd94cb207b1109575211bc6681c164f5298689bb/dom/events/EventListenerManager.cpp#827
[2] http://searchfox.org/mozilla-central/rev/cd94cb207b1109575211bc6681c164f5298689bb/layout/style/xbl-marquee/xbl-marquee.xml#651
bz - any chance you could take a look at reviewing this, smaug has a comment that he has a really heavy load at the moment.

See comment 3 for details.
Flags: needinfo?(bzbarsky)
Damn, forgot about pushing to try on sec bugs again.
This doesn't seem like the right fix...

The right fix would be to change the marquee binding to ignore the attribute event handlers if sandboxed without allow-scripts, I would think.  That probably requires an accessor for the sandbox state on Document (probably conditioned on IsChromeOrXBL).
Flags: needinfo?(bzbarsky)
Attachment #8759734 - Attachment is obsolete: true
Group: core-security → dom-core-security
Whiteboard: [domsecurity-active]
MozReview-Commit-ID: Cv0vJCdLH4D
Comment on attachment 8760321 [details] [diff] [review]
Part 1: Add a chrome and XBL accessor for the sandboxed scripts flag to Document WEBIDL

It would make sense to use this new accessor in nsDocument::IsScriptEnabled, nsJSThunk::EvaluateScript, EventListenerManager::SetEventHandler, nsScriptLoader::StartLoad, and nsScriptLoader::ProcessScriptElement instead of directly poking the flags.

r=me with that.
Attachment #8760321 - Flags: review+
Comment on attachment 8760322 [details] [diff] [review]
Part 2: Ignore marquee attribute event handlers when sandboxed scripts flag is set on its owning Document

r=me
Attachment #8760322 - Flags: review+
Comment on attachment 8760323 [details] [diff] [review]
Part 3: Ensure marquee attribute event handlers don't run when sandboxed scripts flag is set on its owning Document

This is theoretically racy in the sense that it might pass if all the if2 messages show up before any of the if1 messages.

Could we address that by adding capturing listeners from the parent page for the relevant events on document.getElementById("if1").contentWindow before we start the load of "file_marquee_event_handlers.html" in that iframe?  Then we could end the test when we get 3 messages from if2 _and_ all three of those capturing listeners fired _and_ an event loop tick (e.g. executeSoon) passed since the firing of the last listener, _and_ a message we posted to if1 after the last listener fired was echoed back to us (to ensure that we wait for the async message events to come through)?

r=me either way, but I think doing something like that would make the test guaranteed non-racy.
Attachment #8760323 - Flags: review+
Once I've addressed bz's comments is it OK for me to push this to try given that this is sec-moderate?

(I've already pushed an old patch, but it wasn't as obvious what the issue is with that one.)
Flags: needinfo?(abillings)
Bob, yes, that is fine.
Flags: needinfo?(abillings)
Carrying r=bz from comment 11.

(In reply to Boris Zbarsky [:bz] from comment #11)
> Comment on attachment 8760321 [details] [diff] [review]
> Part 1: Add a chrome and XBL accessor for the sandboxed scripts flag to
> Document WEBIDL
> 
> It would make sense to use this new accessor in nsDocument::IsScriptEnabled,
> nsJSThunk::EvaluateScript, EventListenerManager::SetEventHandler,
> nsScriptLoader::StartLoad, and nsScriptLoader::ProcessScriptElement instead
> of directly poking the flags.
> 
> r=me with that.

Done, should have thought of that thanks.
Attachment #8760321 - Attachment is obsolete: true
Attachment #8761163 - Flags: review+
Carrying r=bz from comment 13.

(In reply to Boris Zbarsky [:bz] from comment #13)
> Comment on attachment 8760323 [details] [diff] [review]
> Part 3: Ensure marquee attribute event handlers don't run when sandboxed
> scripts flag is set on its owning Document
> 
> This is theoretically racy in the sense that it might pass if all the if2
> messages show up before any of the if1 messages.
> 
> Could we address that by adding capturing listeners from the parent page for
> the relevant events on document.getElementById("if1").contentWindow before
> we start the load of "file_marquee_event_handlers.html" in that iframe? 
> Then we could end the test when we get 3 messages from if2 _and_ all three
> of those capturing listeners fired _and_ an event loop tick (e.g.
> executeSoon) passed since the firing of the last listener, _and_ a message
> we posted to if1 after the last listener fired was echoed back to us (to
> ensure that we wait for the async message events to come through)?
> 
> r=me either way, but I think doing something like that would make the test
> guaranteed non-racy.

Once, I'd re-learnt how to do this stuff this worked like a dream.
Attachment #8760323 - Attachment is obsolete: true
Attachment #8761164 - Flags: review+
Group: dom-core-security → core-security-release
Comment on attachment 8761163 [details] [diff] [review]
Part 1: Add a chrome and XBL accessor for the sandboxed scripts flag to Document WEBIDL

Approval Request Comment
[Feature/regressing bug #]:
This would have been an issue in the original iframe sandbox implementation.

[User impact if declined]:
iframes sandboxed from script can be bypassed to run script. 

[Describe test coverage new/current, TreeHerder]:
There are some in tree marquee tests.
New tests added to ensure that the attribute handlers are now blocked, but work with allow-scripts keyword on the iframe sandbox attribute.

[Risks and why]:
Low - the fix is relatively simple. Just exposing whether the document is sandboxed from running scripts to XBL and chrome and then adding a check for this in the marquee XBL when the attribute event handlers would normally be added.

[String/UUID change made/needed]:
None
Attachment #8761163 - Flags: approval-mozilla-beta?
Attachment #8761163 - Flags: approval-mozilla-aurora?
Comment on attachment 8760322 [details] [diff] [review]
Part 2: Ignore marquee attribute event handlers when sandboxed scripts flag is set on its owning Document

See comment 21.
Attachment #8760322 - Flags: approval-mozilla-beta?
Attachment #8760322 - Flags: approval-mozilla-aurora?
Comment on attachment 8761164 [details] [diff] [review]
Part 3: Ensure marquee attribute event handlers don't run when sandboxed scripts flag is set on its owning Document

See comment 21.
Attachment #8761164 - Flags: approval-mozilla-beta?
Attachment #8761164 - Flags: approval-mozilla-aurora?
Just as a follow up to this, bz said the only other element implemented in XBL that could in theory have the same issue would be <video>.
He was pretty sure it would be fine, but we should check.

I've checked the spec for this on MDN and there are no similar attribute event handlers.
I've also looked through the XBL and can't find any issues, all the calls to addEventListener are for internal Listeners within the XBL.
It's just the video controls that use XBL, not <video> itself, iirc.
(In reply to Boris Zbarsky [:bz] from comment #25)
> It's just the video controls that use XBL, not <video> itself, iirc.

Yes, this is just me confusing the terminology.

The file I checked was toolkit\content\widgets\videocontrols.xml
Comment on attachment 8761163 [details] [diff] [review]
Part 1: Add a chrome and XBL accessor for the sandboxed scripts flag to Document WEBIDL

Improve the security, taking it. Should be in 48 beta 3


Bob, I guess ESR 45 is impacted, right?
Flags: needinfo?(bobowen.code)
Attachment #8761163 - Flags: approval-mozilla-beta?
Attachment #8761163 - Flags: approval-mozilla-beta+
Attachment #8761163 - Flags: approval-mozilla-aurora?
Attachment #8761163 - Flags: approval-mozilla-aurora+
(In reply to Sylvestre Ledru [:sylvestre] from comment #27)

> Bob, I guess ESR 45 is impacted, right?

I haven't tested it, but yes it must be.
Flags: needinfo?(bobowen.code)
Attachment #8760322 - Flags: approval-mozilla-beta?
Attachment #8760322 - Flags: approval-mozilla-beta+
Attachment #8760322 - Flags: approval-mozilla-aurora?
Attachment #8760322 - Flags: approval-mozilla-aurora+
Attachment #8761164 - Flags: approval-mozilla-beta?
Attachment #8761164 - Flags: approval-mozilla-beta+
Attachment #8761164 - Flags: approval-mozilla-aurora?
Attachment #8761164 - Flags: approval-mozilla-aurora+
Flags: sec-bounty? → sec-bounty+
Alias: CVE-2016-5262
Whiteboard: [domsecurity-active] → [domsecurity-active][adv-main48+]
Reproduced with Firefox 47.0.1 on Windows 10 64-bit → the alert pops up as soon as iframe.html is loaded.
Verified fixed with Firefox 48.0 build 1, latest Dev Edition 49.0a2 and Nightly 50.0a1 under Windows 10 64-bit, Mac OS X 10.11.1 and Ubuntu 16.04 64-bit
Status: RESOLVED → VERIFIED
status-firefox48: fixed → verified
status-firefox49: fixed → verified
status-firefox50: fixed → verified
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: