Bug 1277475 (CVE-2016-5262)

XSS out of iframe sandbox, iframe disabled javascript. marquee

VERIFIED FIXED in Firefox 48

Status

()

Core
DOM: Security
VERIFIED FIXED
2 years ago
10 months ago

People

(Reporter: Nikita, Assigned: bobowen)

Tracking

({csectype-other, sec-moderate, testcase})

46 Branch
mozilla50
x86
Mac OS X
csectype-other, sec-moderate, testcase
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox48 verified, firefox49 verified, firefox50 verified)

Details

(Whiteboard: [domsecurity-active][adv-main48+])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8759064 [details]
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/
(Reporter)

Updated

2 years ago
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86

Comment 1

2 years ago
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?
(Reporter)

Updated

2 years ago
Flags: sec-bounty?
(Reporter)

Updated

2 years ago
Flags: sec-bounty?
Keywords: csectype-other, sec-moderate
(Assignee)

Comment 2

2 years ago
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)

Updated

2 years ago
Assignee: nobody → bobowen.code
Status: NEW → ASSIGNED
Flags: needinfo?(bobowen.code)
(Assignee)

Comment 3

2 years ago
Created attachment 8759734 [details] [diff] [review]
Don't allow non-chrome/native event listeners when sandboxed without scripts

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
(Assignee)

Comment 4

2 years ago
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)
(Assignee)

Comment 6

2 years ago
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)
(Assignee)

Updated

2 years ago
Attachment #8759734 - Attachment is obsolete: true
Group: core-security → dom-core-security
Whiteboard: [domsecurity-active]
(Assignee)

Comment 8

2 years ago
Created attachment 8760321 [details] [diff] [review]
Part 1: Add a chrome and XBL accessor for the sandboxed scripts flag to Document WEBIDL

MozReview-Commit-ID: Cv0vJCdLH4D
(Assignee)

Comment 9

2 years ago
Created attachment 8760322 [details] [diff] [review]
Part 2: Ignore marquee attribute event handlers when sandboxed scripts flag is set on its owning Document

MozReview-Commit-ID: IvUAvkEQcSW
(Assignee)

Comment 10

2 years ago
Created 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

MozReview-Commit-ID: 4vWafBniv4R
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+
(Assignee)

Comment 14

2 years ago
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)
(Assignee)

Comment 16

2 years ago
Created attachment 8761163 [details] [diff] [review]
Part 1: Add a chrome and XBL accessor for the sandboxed scripts flag to Document WEBIDL

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+
(Assignee)

Comment 17

2 years ago
Created 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

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+
https://hg.mozilla.org/mozilla-central/rev/af451fc9572a
https://hg.mozilla.org/mozilla-central/rev/4a0706b3d080
https://hg.mozilla.org/mozilla-central/rev/963e5b62da1b
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Group: dom-core-security → core-security-release
(Assignee)

Comment 21

2 years ago
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?
(Assignee)

Comment 22

2 years ago
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?
(Assignee)

Comment 23

2 years ago
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?
(Assignee)

Comment 24

2 years ago
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.
(Assignee)

Comment 26

2 years ago
(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+
(Assignee)

Comment 28

2 years ago
(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+]

Comment 31

2 years ago
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.