The default bug view has changed. See this FAQ.
Bug 1312272 (CVE-2016-9895)

"onstart" event for "marquee" bypasses 'unsafe-inline' CSP directive

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
5 months ago
a month ago

People

(Reporter: Andrew, Assigned: freddyb)

Tracking

({sec-high})

49 Branch
mozilla52
sec-high
Points:
---
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox49 wontfix, firefox50+ fixed, firefox51+ fixed, firefox52+ fixed, firefox-esr4550+ fixed)

Details

(Whiteboard: [domsecurity-backlog1][adv-main50.1+][adv-esr45.6+])

Attachments

(4 attachments, 11 obsolete attachments)

3.13 KB, patch
freddyb
: review+
Details | Diff | Splinter Review
4.07 KB, patch
freddyb
: review+
Details | Diff | Splinter Review
3.97 KB, patch
Details | Diff | Splinter Review
3.05 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

5 months ago
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.
(Reporter)

Comment 1

5 months ago
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

5 months ago
Argument #123534512 to not use XBL for this stuff. Ugh. :-(

Christoph, do you know of someone who has cycles to investigate this?
Group: firefox-core-security → core-security
Component: Untriaged → DOM: Security
Flags: needinfo?(ckerschb)
Product: Firefox → Core
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.
new Function() is allowed by 'unsafe-eval', but shouldn't work without that.
Keywords: sec-moderate
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.
Group: core-security → dom-core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
Flags: needinfo?(ckerschb)
Priority: -- → P2
Whiteboard: [domsecurity-backlog1]
(Assignee)

Comment 7

5 months ago
(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

5 months ago
(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...
Flags: needinfo?(bzbarsky)

Comment 9

5 months ago
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.
(Assignee)

Comment 10

5 months ago
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
Assignee: nobody → fbraun
> 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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 12

5 months ago
Created attachment 8806288 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
Attachment #8806288 - Flags: feedback?
(Assignee)

Comment 13

5 months ago
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.
Attachment #8806289 - Flags: feedback?
(Assignee)

Comment 14

5 months ago
Created attachment 8806322 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
Attachment #8806288 - Attachment is obsolete: true
Attachment #8806288 - Flags: feedback?
Attachment #8806322 - Flags: feedback?
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.
Attachment #8806289 - Flags: feedback? → feedback+
(Assignee)

Comment 16

5 months ago
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?
Attachment #8806289 - Attachment is obsolete: true
Attachment #8806626 - Flags: feedback?
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.
Attachment #8806626 - Flags: feedback? → feedback+
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?
Attachment #8806322 - Flags: feedback? → feedback+
(Assignee)

Updated

5 months ago
See Also: → bug 1314567
(Assignee)

Comment 19

5 months ago
Created attachment 8806687 [details] [diff] [review]
0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
Attachment #8806322 - Attachment is obsolete: true
Attachment #8806687 - Flags: review?(bugs)
(Assignee)

Comment 20

5 months ago
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.
Attachment #8806688 - Flags: review?(bugs)
(Assignee)

Updated

5 months ago
Attachment #8806626 - Attachment is obsolete: true

Comment 21

5 months ago
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.
Attachment #8806688 - Flags: review?(bugs) → review-

Comment 22

5 months ago
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.
Attachment #8806687 - Flags: review?(bugs) → review-
(Assignee)

Comment 23

5 months ago
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.
Attachment #8806688 - Attachment is obsolete: true
Attachment #8806760 - Flags: review?(bugs)
(Assignee)

Comment 24

5 months ago
Created attachment 8806762 [details] [diff] [review]
0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch
Attachment #8806687 - Attachment is obsolete: true
Attachment #8806762 - Flags: review?(bugs)
(Assignee)

Comment 25

5 months ago
Created attachment 8806765 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Missing file.
Attachment #8806762 - Attachment is obsolete: true
Attachment #8806762 - Flags: review?(bugs)
Attachment #8806765 - Flags: review?(bugs)
(Assignee)

Comment 26

5 months ago
New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13985e007e340ab13c9b87f1eacc92eaab314dda

Comment 27

5 months ago
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"
Attachment #8806760 - Flags: review?(bugs) → review-

Comment 28

5 months ago
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.
Attachment #8806760 - Flags: review- → review+

Comment 29

5 months ago
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
Attachment #8806765 - Flags: review?(bugs) → review+
(Assignee)

Comment 30

5 months ago
(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!
(Assignee)

Comment 31

5 months ago
Created attachment 8807286 [details] [diff] [review]
0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-s.patch

Updated patch, carrying over r+.
Attachment #8806760 - Attachment is obsolete: true
Attachment #8807286 - Flags: review+
(Assignee)

Comment 32

5 months ago
Created attachment 8807287 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Updated test, also carrying over r+
Attachment #8806765 - Attachment is obsolete: true
Attachment #8807287 - Flags: review+
(Assignee)

Updated

5 months ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/87ae1185df39806f84a0b6069eddcc24febf2bd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8fa338a908091cabd38345bc765c0eb3f3516f
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/87ae1185df39
https://hg.mozilla.org/mozilla-central/rev/6c8fa338a908
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 35

5 months ago
Nominating for the bounty committee to look at.
Flags: sec-bounty?

Comment 36

5 months ago
Does this want uplift?
Flags: needinfo?(fbraun)
(Assignee)

Comment 37

5 months ago
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
Flags: needinfo?(fbraun)
Attachment #8807286 - Flags: approval-mozilla-beta?
Attachment #8807286 - Flags: approval-mozilla-aurora?
[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.
status-firefox49: --- → wontfix
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox-esr45: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → +
tracking-firefox52: --- → +
tracking-firefox-esr45: --- → ?
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.
Attachment #8807286 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Group: dom-core-security → core-security-release
the test has conflicts, can you take a look, thanks!
Flags: needinfo?(fbraun)
https://hg.mozilla.org/releases/mozilla-aurora/rev/fa5bb25eb05f
status-firefox51: affected → fixed
(Reporter)

Comment 42

4 months ago
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?
(Assignee)

Comment 43

4 months ago
Andrew: Please file a new bug. The new bug would only affect Nightly, because strict-dynamic is not going to make beta/aurora.
(Assignee)

Comment 44

4 months ago
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!
Flags: needinfo?(fbraun)
https://hg.mozilla.org/releases/mozilla-aurora/rev/cab5e6485c77
(Reporter)

Comment 46

4 months ago
Thanks Frederik, filled a new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1316826
(Assignee)

Comment 47

4 months ago
Created attachment 8809768 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch

Missing file :(
Attachment #8809746 - Attachment is obsolete: true
(Assignee)

Comment 48

4 months ago
Andrew, can you set me to CC on the new bug?
backed out again for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4125053&repo=mozilla-aurora
Flags: needinfo?(fbraun)
(Assignee)

Comment 50

4 months ago
Created attachment 8809769 [details] [diff] [review]
0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch

Third time's the charm?
Attachment #8809768 - Attachment is obsolete: true
Flags: needinfo?(fbraun)
Flags: sec-bounty? → sec-bounty+
Keywords: sec-moderate → sec-high
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?
Attachment #8807286 - Flags: approval-mozilla-beta? → approval-mozilla-release?
Setting the tracking flags so we take this in 50.1 and ESR-45.5.1
tracking-firefox50: ? → +
tracking-firefox-esr45: ? → 50+

Comment 53

4 months ago
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
Attachment #8807286 - Flags: approval-mozilla-release? → approval-mozilla-release+
https://hg.mozilla.org/releases/mozilla-beta/rev/9b84116a5a68

https://hg.mozilla.org/releases/mozilla-release/rev/3c67ba1912e1
https://hg.mozilla.org/releases/mozilla-release/rev/7d5cb2fa1daa
status-firefox50: affected → fixed
Flags: in-testsuite+
(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.
Flags: needinfo?(fbraun)
(Assignee)

Comment 56

4 months ago
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>
Flags: needinfo?(fbraun) → needinfo?(dveditz)
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.
Flags: needinfo?(fbraun)
Flags: needinfo?(abillings)

Comment 58

4 months ago
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.
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.
(Assignee)

Comment 60

4 months ago
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.
Ok. Cool. Please nominate an ESR45 patch ASAP.
Flags: needinfo?(abillings)
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.
(Assignee)

Comment 63

4 months ago
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
Flags: needinfo?(fbraun)
Attachment #8817362 - Flags: approval-mozilla-esr45?
(Assignee)

Comment 64

4 months ago
I'm on the other side of the planet. Please excuse the delay.
Attachment #8817362 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
https://hg.mozilla.org/releases/mozilla-esr45/rev/94bd2b43c7660d0b6d4f22e87f5883b1dbf86724
status-firefox-esr45: affected → fixed
Whiteboard: [domsecurity-backlog1] → [domsecurity-backlog1][adv-main50.1+][adv-esr45.6+]
Alias: CVE-2016-9895
Flags: needinfo?(dveditz)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.