1312272 - (CVE-2016-9895) "onstart" event for "marquee" bypasses 'unsafe-inline' CSP directive

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Andrew]

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

[Andrew]

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

[:Gijs (he/him)]

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

Christoph, do you know of someone who has cycles to investigate this?

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

[Daniel Veditz [:dveditz]]

new Function() is allowed by 'unsafe-eval', but shouldn't work without that.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

[Christoph Kerschbaumer [:ckerschb]]

(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

[Frederik Braun [:freddy]]

(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

[:Gijs (he/him)]

(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

[:Gijs (he/him)]

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

[Frederik Braun [:freddy]]

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

[Boris Zbarsky [:bzbarsky]]

> 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

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Comment 13

[Frederik Braun [:freddy]]

Attachment: 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

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Comment 15

[Boris Zbarsky [:bzbarsky]]

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

[Frederik Braun [:freddy]]

Attachment: 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

[Christoph Kerschbaumer [:ckerschb]]

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

[Christoph Kerschbaumer [:ckerschb]]

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

[Frederik Braun [:freddy]]

Attachment: 0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Comment 20

[Frederik Braun [:freddy]]

Attachment: 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

[Olli Pettay [:smaug][[e-mail]]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

[Frederik Braun [:freddy]]

Attachment: 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

[Frederik Braun [:freddy]]

Attachment: 0002-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Comment 25

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Missing file.

Comment 26

[Frederik Braun [:freddy]]

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

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

[Olli Pettay [:smaug][[e-mail]]]

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

[Frederik Braun [:freddy]]

(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

[Frederik Braun [:freddy]]

Attachment: 0002-Bug-1312272-marquee-event-handlers-to-adhere-CSP-r-s.patch

Updated patch, carrying over r+.

Comment 32

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1312272-test-that-marquee-event-handlers-are-sub.patch

Updated test, also carrying over r+

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87ae1185df39806f84a0b6069eddcc24febf2bd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c8fa338a908091cabd38345bc765c0eb3f3516f

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/87ae1185df39
https://hg.mozilla.org/mozilla-central/rev/6c8fa338a908

Comment 35

[Frederik Braun [:freddy]]

Nominating for the bounty committee to look at.

Comment 36

[:Gijs (he/him)]

Does this want uplift?

Comment 37

[Frederik Braun [:freddy]]

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

[Daniel Veditz [:dveditz]]

[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

[Gerry Chang [:gchang]]

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

[Carsten Book [:Tomcat]]

the test has conflicts, can you take a look, thanks!

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fa5bb25eb05f

Comment 42

[Andrew]

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

[Frederik Braun [:freddy]]

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

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch

This patch should apply cleanly on Aurora. Thanks!

Comment 45

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cab5e6485c77

Comment 46

[Andrew]

Thanks Frederik, filled a new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1316826

Comment 47

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch

Missing file :(

Comment 48

[Frederik Braun [:freddy]]

Andrew, can you set me to CC on the new bug?

Comment 49

[Carsten Book [:Tomcat]]

backed out again for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=4125053&repo=mozilla-aurora

Comment 50

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1312272-test-that-marquee-event-handlers-are-sub-AURORA.patch

Third time's the charm?

Comment 51

[Gerry Chang [:gchang]]

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

[Daniel Veditz [:dveditz]]

Setting the tracking flags so we take this in 50.1 and ESR-45.5.1

Comment 53

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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 54

[Ryan VanderMeulen [:RyanVM]]

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

Comment 55

[Al Billings [:abillings - ex-MoCo]]

(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

[Frederik Braun [:freddy]]

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

[Al Billings [:abillings - ex-MoCo]]

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

[April King [:April]]

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

[Al Billings [:abillings - ex-MoCo]]

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

[Frederik Braun [:freddy]]

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

[Al Billings [:abillings - ex-MoCo]]

Ok. Cool. Please nominate an ESR45 patch ASAP.

Comment 62

[Liz Henry (:lizzard) (relman/hg->git project)]

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

[Frederik Braun [:freddy]]

Attachment: 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

[Frederik Braun [:freddy]]

I'm on the other side of the planet. Please excuse the delay.

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-esr45/rev/94bd2b43c7660d0b6d4f22e87f5883b1dbf86724