Last Comment Bug 1312272 - (CVE-2016-9895) "onstart" event for "marquee" bypasses 'unsafe-inline' CSP directive
(CVE-2016-9895)
: "onstart" event for "marquee" bypasses 'unsafe-inline' CSP directive
Status: RESOLVED FIXED
[domsecurity-backlog1][adv-main50.1+]...
: sec-high
Product: Core
Classification: Components
Component: DOM: Security (show other bugs)
: 49 Branch
: Unspecified Unspecified
P2 normal (vote)
: mozilla52
Assigned To: Frederik Braun [:freddyb] (off until March 6th)
:
: Christoph Kerschbaumer [:ckerschb]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-23 08:45 PDT by Andrew
Modified: 2017-02-09 08:03 PST (History)
13 users (show)
dveditz: sec‑bounty+
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
+
fixed
+
fixed
+
fixed
50+
fixed


Attachments
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch (1.07 KB, patch)
2016-11-01 04:06 PDT, Frederik Braun [:freddyb] (off until March 6th)
no flags Details | Diff | Splinter Review
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r.patch (2.96 KB, patch)
2016-11-01 04:07 PDT, Frederik Braun [:freddyb] (off until March 6th)
bzbarsky: feedback+
Details | Diff | Splinter Review
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch (4.07 KB, patch)
2016-11-01 05:53 PDT, Frederik Braun [:freddyb] (off until March 6th)
ckerschb: feedback+
Details | Diff | Splinter Review
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r.patch (8.34 KB, patch)
2016-11-02 01:27 PDT, Frederik Braun [:freddyb] (off until March 6th)
ckerschb: feedback+
Details | Diff | Splinter Review
0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch (4.25 KB, patch)
2016-11-02 04:37 PDT, Frederik Braun [:freddyb] (off until March 6th)
bugs: review-
Details | Diff | Splinter Review
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-c.patch (8.37 KB, patch)
2016-11-02 04:41 PDT, Frederik Braun [:freddyb] (off until March 6th)
bugs: review-
Details | Diff | Splinter Review
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-c.patch (3.42 KB, patch)
2016-11-02 09:01 PDT, Frederik Braun [:freddyb] (off until March 6th)
bugs: review+
Details | Diff | Splinter Review
0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch (5.84 KB, patch)
2016-11-02 09:02 PDT, Frederik Braun [:freddyb] (off until March 6th)
no flags Details | Diff | Splinter Review
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch (6.39 KB, patch)
2016-11-02 09:13 PDT, Frederik Braun [:freddyb] (off until March 6th)
bugs: review+
Details | Diff | Splinter Review
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-s.patch (3.13 KB, patch)
2016-11-03 13:14 PDT, Frederik Braun [:freddyb] (off until March 6th)
fbraun: review+
gchang: approval‑mozilla‑aurora+
rkothari: approval‑mozilla‑release+
Details | Diff | Splinter Review
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch (4.07 KB, patch)
2016-11-03 13:15 PDT, Frederik Braun [:freddyb] (off until March 6th)
fbraun: review+
Details | Diff | Splinter Review
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch (3.13 KB, patch)
2016-11-11 01:46 PST, Frederik Braun [:freddyb] (off until March 6th)
no flags Details | Diff | Splinter Review
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch (4.33 KB, patch)
2016-11-11 03:47 PST, Frederik Braun [:freddyb] (off until March 6th)
no flags Details | Diff | Splinter Review
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch (3.97 KB, patch)
2016-11-11 04:14 PST, Frederik Braun [:freddyb] (off until March 6th)
no flags Details | Diff | Splinter Review
0001-Bug-1312272-marquee-event-handlers-know-CSP-for-esr45.patch (3.05 KB, patch)
2016-12-08 00:36 PST, Frederik Braun [:freddyb] (off until March 6th)
lhenry: approval‑mozilla‑esr45+
Details | Diff | Splinter Review

Description User image Andrew 2016-10-23 08:45:29 PDT
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

Open the following URL: data:text/html,<meta http-equiv="Content-Security-Policy" content="default-src 'none'; script-src 'unsafe-eval'"><marquee onstart="alert('test')"></marquee>


Actual results:

JavaScript code "alert('test')" will be executed, while "'inline-inline'" not specified in CSP policy.


Expected results:

_inline_ javascript code not executed.
Comment 1 User image Andrew 2016-10-23 08:47:55 PDT
Oh, I'm sorry for the typo:
JavaScript code "alert('test')" will be executed, while "'unsafe-inline'" is not specified in the CSP policy.
Comment 2 User image :Gijs 2016-10-26 03:42:15 PDT
Argument #123534512 to not use XBL for this stuff. Ugh. :-(

Christoph, do you know of someone who has cycles to investigate this?
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-26 10:13:08 PDT
This is much like bug 1277475.

I assume unsafe-inline doesn't affect |new Function()| invocations?  If so, _setEventListener needs to directly check the unsafe-inline CSP in the string case.
Comment 4 User image Daniel Veditz [:dveditz] 2016-10-27 10:50:56 PDT
new Function() is allowed by 'unsafe-eval', but shouldn't work without that.
Comment 5 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-27 11:24:19 PDT
It's also possible we're not applying CSP to the expanded principal the XBL runs with.  In any case, to do unsafe-inline here we would need an explicit check.
Comment 6 User image Christoph Kerschbaumer [:ckerschb] 2016-10-31 08:36:10 PDT
(In reply to :Gijs Kruitbosch from comment #2)
> Christoph, do you know of someone who has cycles to investigate this?

Unfortunately, I don't have the cycles to investigate at the moment. Freddy, can you take a look by any chance? If not, let me know and I'll try to find someone else.

In either case I agree with Boris and we have to perform an explicit csp->GetAllowsInline(nsIContentPolicy::TYPE_SCRIPT) check.
Comment 7 User image Frederik Braun [:freddyb] (off until March 6th) 2016-10-31 09:03:50 PDT
(In reply to :Gijs Kruitbosch from comment #2)
> Argument #123534512 to not use XBL for this stuff. Ugh. :-(
> Christoph, do you know of someone who has cycles to investigate this?

marquee/XBL has bitten us before (bug 863933 and bug 1277475).
How hard would it be to rewrite this without XBL instead of fixing this specific bug?
Gijs, would you be willing to guide me?
Comment 8 User image :Gijs 2016-10-31 09:07:07 PDT
(In reply to Frederik Braun [:freddyb] from comment #7)
> (In reply to :Gijs Kruitbosch from comment #2)
> > Argument #123534512 to not use XBL for this stuff. Ugh. :-(
> > Christoph, do you know of someone who has cycles to investigate this?
> 
> marquee/XBL has bitten us before (bug 863933 and bug 1277475).
> How hard would it be to rewrite this without XBL instead of fixing this
> specific bug?
> Gijs, would you be willing to guide me?

Willing, but not able, I fear (in the sense that it'd be the blind leading the blind). You would have to write a new HTML element implementation. You could use native anonymous content (like <input type=text> and <video> do) and use CSS to handle the actual behaviour of the marquee. I believe it's possible, just not entirely trivial. I don't know if bz (knows someone else who) has time to do better leading than I would...
Comment 9 User image :Gijs 2016-10-31 09:08:41 PDT
It'd certainly be a simpler and more short-term and upliftable fix to write a fix that addresses the evaluation of its attributes directly as suggested earlier, though I agree that long-term / in-depth fixes might be better off by ditching XBL.
Comment 10 User image Frederik Braun [:freddyb] (off until March 6th) 2016-10-31 12:32:01 PDT
Thanks Gijs! Fixing the vulnerability first seems relatively easy, having read up on the code in question.
I've sketched out a plan on how to tackle this (mainly for myself) but would be very happy if someone who knows the code wants to provide some feedback:

From what I understand, I will modify the _setEventListner method [1] to find a CSP and (if existing) query whether inline styles are allowed. I'll use csp->GetAllowsInline(nsIContentPolicy::TYPE_SCRIPT) as advised in comment 6, but I have yet to find out how to get the NodePrincipal or document from the existing code. (I am relatively confident that I will find related code somewhere.)

I will generate a script sample as can be seen in [2].


[1] http://searchfox.org/mozilla-central/source/layout/style/xbl-marquee/xbl-marquee.xml#275
[2] http://searchfox.org/mozilla-central/source/dom/events/EventListenerManager.cpp#867
Comment 11 User image Boris Zbarsky [:bz] (still a bit busy) 2016-10-31 13:08:08 PDT
> How hard would it be to rewrite this without XBL instead of fixing this specific bug?

It's a bunch of work, and the result would involve a lot of fiddly C++ code, which would have a tendency to have its own vulnerabilities.  The security tradeoff is entirely unclear; this is one of the reasons we implemented this in XBL to start with...

> but I have yet to find out how to get the NodePrincipal or document from the existing code.

You can't get a NodePrincipal (it's chrome-only), which is OK, because .csp on nsIPrincipal is noscript anyway.

You can get a document by doing "document" like we already do in _SetEventListener.

What you probably want to do is add a new IsChromeOrXBL method on Document (in Document.webidl) and then implement it in C++ and have it poke at the CSP.  See how "hasScriptsBlockedBySandbox" works.
Comment 12 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-01 04:06:49 PDT
Created attachment 8806288 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
Comment 13 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-01 04:07:39 PDT
Created attachment 8806289 [details] [diff] [review]
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r.patch

There are some remaining TODOs with CSP reporting and logging, but I wanted to get this initial patch out for some early feedback.
Comment 14 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-01 05:53:01 PDT
Created attachment 8806322 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2016-11-01 14:03:40 PDT
Comment on attachment 8806289 [details] [diff] [review]
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r.patch

>+  // no blocking if without CSP or inline allowed by CSP:
>+  if (!mHasCSP && !mHasUnsafeInlineCSP) {

The comment says "or", the code says "and".  Which is it?

>+  rv = csp->GetAllowsInline(nsIContentPolicy::TYPE_SCRIPT,
>+                       EmptyString(), //aNonce

Weird indent here.

>+  if (NS_FAILED(rv)) {
>+    return false;
>+  }

Wouldn't returning true be safer?

>+  bool InlineBlockedByCSP();

Should this perhaps be "InlineScriptBlockedByCSP"?  Or is it not limited to script?

The rest looks good.
Comment 16 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-02 01:27:37 PDT
Created attachment 8806626 [details] [diff] [review]
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r.patch

Thanks for taking a look, Boris.

I now notice that the script sample somehow has to come from the XBL script, if we want to make a proper CSP report.
Maybe inlineScriptBlockedByCSP ought to be a method that is given some context on the attempted event handler?
I was thinking along the lines ofcheckAllowedEventHandler(aEventName, aEventText).

Other ideas?
Comment 17 User image Christoph Kerschbaumer [:ckerschb] 2016-11-02 02:57:35 PDT
Comment on attachment 8806626 [details] [diff] [review]
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r.patch

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

Looks already pretty good to me. I wonder how hard it is to get actually get a script sample :-)

::: dom/base/nsDocument.cpp
@@ +12258,5 @@
>    return mSandboxFlags & SANDBOXED_SCRIPTS;
>  }
>  
> +bool
> +nsIDocument::InlineScriptBlockedByCSP()

would it make more sense to rename that method to:
bool CSP_AllowInlinceScript() or something?

@@ +12263,5 @@
> +{
> +  // no blocking if without CSP or inline allowed by CSP:
> +  if (!mHasCSP || !mHasUnsafeInlineCSP) {
> +    return false;
> +  }

mHasCSP and mHasUnsafeInlineCSP are mostly used for gathering telemtry. Even though they should hold the correct values at the moment, I am worried if someone is going to mess with those we end up doing the wrong thing here. I think you can just remove that check and rely on the actual CSP underneath.

@@ +12266,5 @@
> +    return false;
> +  }
> +  nsresult rv;
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  rv = NodePrincipal()->GetCsp(getter_AddRefs(csp));

nit:
nsresult rv = ...

@@ +12269,5 @@
> +  nsCOMPtr<nsIContentSecurityPolicy> csp;
> +  rv = NodePrincipal()->GetCsp(getter_AddRefs(csp));
> +  if (NS_FAILED(rv)) {
> +    return true;
> +  }

nit: NS_ENSURE_SUCCESS(rv, true);

@@ +12270,5 @@
> +  rv = NodePrincipal()->GetCsp(getter_AddRefs(csp));
> +  if (NS_FAILED(rv)) {
> +    return true;
> +  }
> +  bool allowsInlineScript;

nit: please init to false

@@ +12278,5 @@
> +                            0,             // aLineNumber
> +                            &allowsInlineScript);
> +  if (NS_FAILED(rv)) {
> +    return true;
> +  }

nit, same here: NS_ENSURE_SUCCESS(rv, true);
btw, do we really want to return true here? What do we do in other cases where GetAllowsInline() returns an error?

::: dom/webidl/Document.webidl
@@ +443,5 @@
>  
> +// Extension to give chrome and XBL JS the ability to determine if
> +// inline scripts are allowed by the CSP
> +partial interface Document {
> +  [Func="IsChromeOrXBL"] readonly attribute boolean inlineScriptBlockedByCSP;

I suppose you could pair that line with hasScriptsBlockedBySandbox. no need to define an additional partial interface, right?

::: layout/style/xbl-marquee/xbl-marquee.xml
@@ +285,4 @@
>              return true;
>            }
>  
> +          if (document.inlineScriptBlockedByCSP) {

looks good, please add a small comment above like hasScriptsBlockedBySandbox does.
Comment 18 User image Christoph Kerschbaumer [:ckerschb] 2016-11-02 03:03:10 PDT
Comment on attachment 8806322 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

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

Looks good from a birds eye of view :-)

::: dom/security/test/csp/mochitest.ini
@@ +80,5 @@
>    file_bug1229639.html
>    file_bug1229639.html^headers^
> +  file_bug1312272.html
> +  file_bug1312272.js
> +  file_bug1312272.html^headers^

nit: i rather prefer descriptive files names for tests instead of using bug numbers.

::: dom/security/test/csp/test_bug1312272.html
@@ +9,5 @@
> +</head>
> +<body>
> +<iframe id="cspframe" style="width:100%"></iframe>
> +
> +<script type="text/javascript">

nit: please add a small description of the test here. e.g. why you are not doing a positive and negative test and what we are testing exactly, also why we are using 'unsafe-eval' in the policy, etc.

@@ +19,5 @@
> +    SimpleTest.finish();
> +    //removeEventListener('message', handler);
> +  } else {
> +    ok(false, "Should not get any other message")
> +  }

looks good, please remove the console.logs everywhere and also remove the event listener. And as usual, early returns :-) no need for the else, right?
Comment 19 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-02 04:37:15 PDT
Created attachment 8806687 [details] [diff] [review]
0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
Comment 20 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-02 04:41:57 PDT
Created attachment 8806688 [details] [diff] [review]
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-c.patch

Thanks for taking a look, Christoph.

An "allow" function called by a function called "block" was increasing the mental overherad indeed. I turned the logic around, the function is now called "allow"

Other code in EventListenerManager.cpp defaults to allowing the event handler[1], when csp->GetAllowsInline doesn't succeed so I will go with that. This is especially fine given that GetAllowsInline can't really fail (unless you give it an unexpected aContentType[2], which we do not).

[1] http://searchfox.org/mozilla-central/source/dom/events/EventListenerManager.cpp
[2] http://searchfox.org/mozilla-central/source/dom/security/nsCSPContext.cpp#485

Christoph said he can review this week but is not taking review requests on Bugzilla, because of travel.
This still needs a peer review for the changes on 'Document' but feel free to ignore the test cases, Olli.
Comment 21 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-11-02 05:02:51 PDT
Comment on attachment 8806688 [details] [diff] [review]
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-c.patch

>+++ b/dom/webidl/Document.webidl
>@@ -437,8 +437,10 @@ partial interface Document {
> 
> // Extension to give chrome and XBL JS the ability to determine whether
> // the document is sandboxed without permission to run scripts.
>+// and whether inline scripts are blocked by the document's CSP.
> partial interface Document {
>   [Func="IsChromeOrXBL"] readonly attribute boolean hasScriptsBlockedBySandbox;
>+  [Func="IsChromeOrXBL"] readonly attribute boolean InlineScriptAllowedByCSP;
So you have InlineScriptAllowedByCSP here in the interface, exposed to JS. Note capital 'I'
(usually methods and attributes in interfaces start with lowercase letters, so please fix.)

>+          // attribute event handlers should only be added if the
>+          // document's CSP allows it.
>+          if (!document.inlineScriptAllowedByCSP) {
but here you use lowercase 'i'. So, '!document.inlineScriptAllowedByCSP' is always true. Something hasn't been tested here.


Also, could you add a test where web page itself adds property called inlineScriptAllowedByCSP to document and ensure that we don't read that value in XBL.


And could you possibly not do all the random whitespace fixes.
Comment 22 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-11-02 05:06:09 PDT
Comment on attachment 8806687 [details] [diff] [review]
0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

So, add also a test where web page sets hdocument.inlineScriptAllowedByCSP.
The value XBL sees shouldn't be overridden, but better to have a test.

I'm surprised if the previous patch didn't break any other marquee tests, but if it didn't, we need some more tests for the case when inlineScriptAllowedByCSP is true.
Comment 23 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-02 09:01:44 PDT
Created attachment 8806760 [details] [diff] [review]
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-c.patch

(In reply to Olli Pettay [:smaug] from comment #22)
> I'm surprised if the previous patch didn't break any other marquee tests,
> but if it didn't, we need some more tests for the case when
> inlineScriptAllowedByCSP is true.

It might, I have yet to do see a finished try run.
(Waiting for https://treeherder.mozilla.org/#/jobs?repo=try&revision=f196a311f09f)

(In reply to Olli Pettay [:smaug] from comment #21)
> Something hasn't been tested here.

Right, my test was racy. I found the problem, fortunately.
Comment 24 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-02 09:02:15 PDT
Created attachment 8806762 [details] [diff] [review]
0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
Comment 25 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-02 09:13:21 PDT
Created attachment 8806765 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Missing file.
Comment 26 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-02 09:14:33 PDT
New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13985e007e340ab13c9b87f1eacc92eaab314dda
Comment 27 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-11-02 09:30:10 PDT
Comment on attachment 8806760 [details] [diff] [review]
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-c.patch

> partial interface Document {
>   [Func="IsChromeOrXBL"] readonly attribute boolean hasScriptsBlockedBySandbox;
>+  [Func="IsChromeOrXBL"] readonly attribute boolean InlineScriptAllowedByCSP;
This is still wrong. You have capital 'I' here

> 
>+          // attribute event handlers should only be added if the
>+          // document's CSP allows it.
>+          if (!document.inlineScriptAllowedByCSP) {
but lowercase 'i' here.

If your tests didn't catch this issue, clearly tests miss some case.
!document.inlineScriptAllowedByCSP is always true, since document doesn't have property called "inlineScriptAllowedByCSP"
Comment 28 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-11-02 09:31:36 PDT
Comment on attachment 8806760 [details] [diff] [review]
0001-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-c.patch

ahaa, you fix that in the other patch. confusing. Could you please have just one patch for code changes, since that is what we'll probably land to branches, not necessarily the test.

But r+ for the code changes if you do that.
Comment 29 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-11-02 09:44:35 PDT
Comment on attachment 8806765 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

>+// the file has a CSP that should disallow inline event handlers
>+// on a side-note the CSP allows unsafe-inline because the XUL script
>+// uses the Function constructor which would otherwise be disallowed.
I have no idea what this sentence means. Does it miss some '.' somewhere.
And what does XUL script mean here? Please fix the comment.

> // and whether inline scripts are blocked by the document's CSP.
> partial interface Document {
>   [Func="IsChromeOrXBL"] readonly attribute boolean hasScriptsBlockedBySandbox;
>-  [Func="IsChromeOrXBL"] readonly attribute boolean InlineScriptAllowedByCSP;
>+  [Func="IsChromeOrXBL"] readonly attribute boolean inlineScriptAllowedByCSP;
> };
So this really should be in the other patch
Comment 30 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-02 11:28:21 PDT
(In reply to Olli Pettay [:smaug] from comment #29)
> Comment on attachment 8806765 [details] [diff] [review]
> 0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
> 
> >+// the file has a CSP that should disallow inline event handlers
> >+// on a side-note the CSP allows unsafe-inline because the XUL script
> >+// uses the Function constructor which would otherwise be disallowed.
> I have no idea what this sentence means. Does it miss some '.' somewhere.
> And what does XUL script mean here? Please fix the comment.

The XBL(sic!) script that implements the marquee element uses the Function constructor to create event handlers. The Function constructor is blocked by CSP. This means that CSP + <marquee> events _never work_. This is a follow-up bug.
To workaround that, we allow the Function constructor via unsafe-eval(sic!) in the CSP. I will reword the comment to reflect that.
 
> > // and whether inline scripts are blocked by the document's CSP.
> > partial interface Document {
> >   [Func="IsChromeOrXBL"] readonly attribute boolean hasScriptsBlockedBySandbox;
> >-  [Func="IsChromeOrXBL"] readonly attribute boolean InlineScriptAllowedByCSP;
> >+  [Func="IsChromeOrXBL"] readonly attribute boolean inlineScriptAllowedByCSP;
> > };
> So this really should be in the other patch

All my rebasing must have mixed the two commits up. Sorry this is so confusing!
Comment 31 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-03 13:14:39 PDT
Created attachment 8807286 [details] [diff] [review]
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-s.patch

Updated patch, carrying over r+.
Comment 32 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-03 13:15:13 PDT
Created attachment 8807287 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Updated test, also carrying over r+
Comment 35 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-08 03:46:29 PST
Nominating for the bounty committee to look at.
Comment 36 User image :Gijs 2016-11-08 04:26:31 PST
Does this want uplift?
Comment 37 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-08 05:06:25 PST
Comment on attachment 8807286 [details] [diff] [review]
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-s.patch

Approval Request Comment
[Feature/regressing bug #]:
bug 1312272 (this bug).

[User impact if declined]:
The security bug will take more time to get to end users. It's only sec-moderate though.

[Describe test coverage new/current, TreeHerder]:
Tests went fine on try. I'm not sure if we'd usually uplift the test (other patch).

[Risks and why]:
Taking this seems low risk to me. It *does* touch the Document interface, but only for Chrome/XBL scripts.

[String/UUID change made/needed]:
none
Comment 38 User image Daniel Veditz [:dveditz] 2016-11-08 10:58:58 PST
[Tracking Requested - why for this release]:
We definitely want to plug this CSP bypass for 51. It would be good to uplift to 50.1 and ESR-45 as well.
Comment 39 User image Gerry Chang [:gchang] 2016-11-09 01:20:40 PST
Comment on attachment 8807286 [details] [diff] [review]
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-s.patch

Fix a sec-moderate. Take it in 51 aurora.
Comment 40 User image Carsten Book [:Tomcat] 2016-11-10 07:35:13 PST
the test has conflicts, can you take a look, thanks!
Comment 42 User image Andrew 2016-11-11 00:18:34 PST
After implementing 'strict-dynamic' (see https://bugzilla.mozilla.org/show_bug.cgi?id=1299483) <marquee> can strikes again:(

Steps to reproduce:
  - Enable 'strict-dynamic' if needed ("security.csp.enableStrictDynamic")
  - Open the following URL: data:text/html,<meta http-equiv="Content-Security-Policy" content="default-src 'none'; script-src 'nonce-123' 'strict-dynamic'"><marquee onstart="alert('test')"></marquee>

Actual results:
  JavaScript code "alert('test')" will be executed. I suppose that XBL runs in JavaScript context so CSP allows inline and evaluate any scripts, due to 'strict-dynamic' policy.  


Expected results:
  _inline_ javascript code not executed.





Should I create a new report for this?
Comment 43 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-11 01:27:09 PST
Andrew: Please file a new bug. The new bug would only affect Nightly, because strict-dynamic is not going to make beta/aurora.
Comment 44 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-11 01:46:40 PST
Created attachment 8809746 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch

This patch should apply cleanly on Aurora. Thanks!
Comment 46 User image Andrew 2016-11-11 02:50:02 PST
Thanks Frederik, filled a new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1316826
Comment 47 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-11 03:47:31 PST
Created attachment 8809768 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch

Missing file :(
Comment 48 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-11 03:48:09 PST
Andrew, can you set me to CC on the new bug?
Comment 49 User image Carsten Book [:Tomcat] 2016-11-11 04:08:04 PST
backed out again for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4125053&repo=mozilla-aurora
Comment 50 User image Frederik Braun [:freddyb] (off until March 6th) 2016-11-11 04:14:47 PST
Created attachment 8809769 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch

Third time's the charm?
Comment 51 User image Gerry Chang [:gchang] 2016-11-16 00:29:29 PST
Comment on attachment 8807286 [details] [diff] [review]
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-s.patch

Since merge day is passed, move approval‑mozilla‑beta? to approval‑mozilla‑release?
Comment 52 User image Daniel Veditz [:dveditz] 2016-11-16 12:32:23 PST
Setting the tracking flags so we take this in 50.1 and ESR-45.5.1
Comment 53 User image Ritu Kothari (:ritu) 2016-11-28 14:18:09 PST
Comment on attachment 8807286 [details] [diff] [review]
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-s.patch

Sec-high, meets the bar for inclusion in 50.1.0
Comment 55 User image Al Billings [:abillings] 2016-12-07 11:48:47 PST
(In reply to Phil Ringnalda (:philor) from comment #34)
> https://hg.mozilla.org/mozilla-central/rev/87ae1185df39
> https://hg.mozilla.org/mozilla-central/rev/6c8fa338a908

Why did this get checked in without sec-approval+?

I need sec-approval? to be set on this bug and the template questions for it answered so we can understand the security impact and work on the advisory.
Comment 56 User image Frederik Braun [:freddyb] (off until March 6th) 2016-12-07 12:14:40 PST
It was rated sec-high (below comment 50) 1only after it was landed in central (comment 34).
Maybe dveditz increased the rating merely as a trick to get this into release?
As a CSP bypass it needs another vulnerability (XSS) to be effective, which somewhat matches what I am reading under "sec-moderate" in <https://wiki.mozilla.org/Security_Severity_Ratings>
Comment 57 User image Al Billings [:abillings] 2016-12-07 12:20:48 PST
Ah right, ok. Well, I still need the template questions answered now as I have to figure out advisories.

As it is, it is fixed in Firefox 50.1 but still present in ESR45 so I can't publish an advisory for it until we fix it in ESR45 which won't be until late January.
Comment 58 User image April King [:April] 2016-12-07 12:26:13 PST
freddyb: XSS varies widely in security impact (from sec-low all the way up to sec-critical) depending on whether it is stored/reflected/self, and what site it is impacting. If this was a reflected XSS + CSP bypass on one of our sites, it may well be rated sec-moderate.

But this is a bypass for every site that uses CSP, opening them up to XSS attacks that they thought would be prevented from their proactive security policies.  Without being patched, it's essentially as if Firefox doesn't support CSP at all, which is why I personally pushed to have it rated sec-high.
Comment 59 User image Al Billings [:abillings] 2016-12-07 12:35:09 PST
Ok. There is a 45.6 ESR release at the same time as 50.1. The advisory for this should be in the release. We need a patch landed for that there.
Comment 60 User image Frederik Braun [:freddyb] (off until March 6th) 2016-12-07 12:39:01 PST
Here you go.

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?

Very easily. I implement a function called "do the CSP check" in the implementation of the marquee element.

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yep.

> Which older supported branches are affected by this flaw?

Probably all.

> If not all supported branches, which bug introduced the flaw?

N/A

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be relatively easy, but I'd have to touch nsDocument.cpp, which is a very busy file.
Mostly annoying, I suppose.

> How likely is this patch to cause regressions; how much testing does it need?
Not sure. rather unlikely. There's testing that the security check is in place, but I'm only 99% sure that I did not break the <marquee> element's scripting capabilities.
Comment 61 User image Al Billings [:abillings] 2016-12-07 15:22:45 PST
Ok. Cool. Please nominate an ESR45 patch ASAP.
Comment 62 User image Liz Henry (:lizzard) (needinfo? me) 2016-12-07 17:21:06 PST
I missed this one for ESR last week (a bit busy with the dot release). I can hold off on building ESR until later tonight.
Comment 63 User image Frederik Braun [:freddyb] (off until March 6th) 2016-12-08 00:36:19 PST
Created attachment 8817362 [details] [diff] [review]
0001-Bug-1312272-marquee-event-handlers-know-CSP-for-esr45.patch

> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
N/A
> User impact if declined: 
CSP easily bypassable for our users on ESR
> Fix Landed on Version:

50, or 50.1?

> Risk to taking this patch (and alternatives if risky): 
Unsure.

> String or UUID changes made by this patch: 
None
Comment 64 User image Frederik Braun [:freddyb] (off until March 6th) 2016-12-08 00:38:01 PST
I'm on the other side of the planet. Please excuse the delay.

Note You need to log in before you can comment on or make changes to this bug.