Closed Bug 1106713 Opened 5 years ago Closed 5 years ago

HTML imports bypasses prohibition of script execution by iframe[sandbox]

Categories

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

37 Branch
x86
Windows 8
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox35 --- disabled
firefox36 --- disabled
firefox37 --- fixed
firefox-esr31 --- disabled

People

(Reporter: sdna.muneaki.nishimura, Assigned: gkrizsanits)

Details

(Keywords: sec-moderate)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2224.3 Safari/537.36

Steps to reproduce:

1. Enable dom.webcomponents.enabled by about:config
2. Launch http://alice.csrf.jp/imports/sandbox.html

Here, sandbox.html loads victim.html in an iframe[sandbox] which prohibits script execution.


Actual results:

The script, http://mallory.csrf.jp/imports/scripts.php, runs on the sandboxed iframe and it shows alert windows.



Expected results:

The script in http://mallory.csrf.jp/imports/scripts.php has to be prohibited by iframe[sandbox] of the master document.

Suppose that the <link rel="import" href="http://mallory.csrf.jp/imports/scripts.php"> in victim.html is the code injected by an attacker.
Then, the script bypasses iframe[sandbox] and it runs on the target origin.
The basic plumbing for this pref landed a year ago, but it's still pref'ed off. Is there any intent to ship webcomponents or is this just a failed experiment left in the tree?
Component: Untriaged → DOM
Flags: sec-bounty?
Flags: needinfo?(wchen)
Product: Firefox → Core
There is still work to do on web components (like fixing the specs and then implement what the specs say).
Also, for the record, while the HTML imports spec is in a bunch of flux, we're actually using non-import (I think) webcomponents in b2g for a bunch of stuff.
Implementation of HTML Imports seems to be finished last month on Bug 877072 and it seems to be landed on next Firefox release.
https://bugzilla.mozilla.org/show_bug.cgi?id=877072#c83

Also, HTML imports and other feature of Web Components are already available on B2G and Gaia uses them like below.
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/index.html#L114
(In reply to Muneaki Nishimura from comment #4)
> Implementation of HTML Imports seems to be finished last month on Bug 877072
> and it seems to be landed on next Firefox release.
> https://bugzilla.mozilla.org/show_bug.cgi?id=877072#c83
> 
> Also, HTML imports and other feature of Web Components are already available
> on B2G and Gaia uses them like below.
> https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/index.html#L114

Hmmm... I was not aware that imports are being used already in b2g. Anyway as far as I know
CSP should not be an issue for them for now, but maybe that changed as well? Freddy, do you
think this bug can be a concern for b2g?
Assignee: nobody → gkrizsanits
So the patch is pretty simple. But there are a bunch of questions, like what we should do if an import has an iframe with sandbox="allow-same-origin"? I don't think that case is defined by the spec at all. Also, I wonder if there are a lot of CSP related checks scattered around the code, that I should find and fix for imports... Finally, I guess it's time to write some CSP related tests for imports.
Attachment #8532140 - Flags: feedback?(mrbkap)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> Hmmm... I was not aware that imports are being used already in b2g. Anyway
> as far as I know
> CSP should not be an issue for them for now, but maybe that changed as well?

As for the b2g, fortunately there may be no problem since HTML imports is allowed only for certified apps, and as you said, in this case CSP prevents loading of HTML fragments.

However my concern is that the issue may be landed on next official Firefox release as Bug 877072.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> (In reply to Muneaki Nishimura from comment #4)
> > Implementation of HTML Imports seems to be finished last month on Bug 877072
> > and it seems to be landed on next Firefox release.
> > https://bugzilla.mozilla.org/show_bug.cgi?id=877072#c83
> > 
> > Also, HTML imports and other feature of Web Components are already available
> > on B2G and Gaia uses them like below.
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/index.html#L114
> 
> Hmmm... I was not aware that imports are being used already in b2g. Anyway
> as far as I know
> CSP should not be an issue for them for now, but maybe that changed as well?
> Freddy, do you
> think this bug can be a concern for b2g?


It's not. We are using a polyfill for imports, which just does XMLHttpRequest and is restricted to on-origin resources within certified apps.
We have rated sandbox bypasses differently over time. This is either sec-moderate or sec-high. Opinions?
Keywords: sec-moderate
(In reply to Frederik Braun [:freddyb] from comment #9)
> We have rated sandbox bypasses differently over time. This is either
> sec-moderate or sec-high. Opinions?

Imports are pref-ed out by default so it should be sec-mod/sec-other depending our policy about pref-ed out features.
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> Created attachment 8532140 [details] [diff] [review]
> fixing up CSP checks for imports. v1
> 

I talked a bit about this with Blake, just I was too jatlagged at the moment to make any sense. So my worry is that the SANDBOXED_SCRIPTS flags is something iframes inherit as far as I can tell from their parent document. But I don't think we inherit them over when the iframe is inside an import (when we want to inherit the flag from the master of the parent and not from the parent which we _probably_ do). Not sure where should we fix that one, or if there are other stuff we should inherit over to iframes from the master document instead of the direct parent which might be an import. Am I making sense?
Comment on attachment 8532140 [details] [diff] [review]
fixing up CSP checks for imports. v1

Review of attachment 8532140 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsScriptLoader.cpp
@@ +434,5 @@
>  static bool
>  CSPAllowsInlineScript(nsIScriptElement *aElement, nsIDocument *aDocument)
>  {
>    nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  nsresult rv = aDocument->MasterDocument()->NodePrincipal()->GetCsp(getter_AddRefs(csp));

Thinking this part over I think I just want to add a comment here instead. This change makes no practical change, and explaining that the principal of the master and the imports are the same is cleaner.
Attachment #8532140 - Flags: feedback?(mrbkap)
This bug is not marked as confirmed, Gabor. Just an oversight?
Flags: needinfo?(gkrizsanits)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(gkrizsanits)
I think this is a cleaner way to fix the problem. This way we always copy the sandbox flags over to the import documents.
Attachment #8532140 - Attachment is obsolete: true
Attachment #8533698 - Flags: review?(mrbkap)
Comment on attachment 8533698 [details] [diff] [review]
sandbox support for imports. v2

Review of attachment 8533698 [details] [diff] [review]:
-----------------------------------------------------------------

I like this a lot better.

In the raw patch, there were a couple "no newline at end of file". Please fix those before checking in.
Attachment #8533698 - Flags: review?(mrbkap) → review+
Target Milestone: --- → mozilla37
Flags: needinfo?(wchen)
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)
> (In reply to Frederik Braun [:freddyb] from comment #9)
> > We have rated sandbox bypasses differently over time. This is either
> > sec-moderate or sec-high. Opinions?
> 
> Imports are pref-ed out by default so it should be sec-mod/sec-other
> depending our policy about pref-ed out features.

Depends on why it's pref'ed off. If it's a super-powered developer-only feature that will always remain pref'd off then we rate it lower. If it's a feature we intend to ship eventually then we usually rate it as if it's turned on because that's the kind of eventual-vuln the reporter is saving us from.
Flags: needinfo?(gkrizsanits)
(In reply to Daniel Veditz [:dveditz] from comment #18)
> Depends on why it's pref'ed off. If it's a super-powered developer-only
> feature that will always remain pref'd off then we rate it lower. If it's a
> feature we intend to ship eventually then we usually rate it as if it's
> turned on because that's the kind of eventual-vuln the reporter is saving us
> from.

Right. Thanks for the explanation. It used to be "we intend to ship" case, but right
now it's in the "wait until we rewrite spec and then re-implement from scratch" state.
So it is unlikely that we will ship it in this form, but also not a super-powered dev-only
feature.
Flags: needinfo?(gkrizsanits)
Giving a smaller bounty as a "thank you" for the work here and the addition of a valuable test for when we re-write this feature. As a sec-moderate rated issue, it wouldn't normally qualify for a bounty.
Flags: sec-bounty? → sec-bounty+
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.