1316826 - CSP bypass with DOM events and 'strict-dynamic'

Status: VERIFIED FIXED

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

Description

[Andrew]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0
Build ID: 20161110030211

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 due to 'strict-dynamic



Expected results:

_inline_ javascript code not executed.

Comment 1

[Frederik Braun [:freddy]]

Confirmed. This isn't even marquee specific. It fires for img onerror and input onkeyup just as well.

Comment 2

[Frederik Braun [:freddy]]

Well, what I did was not a great way to test, actually. We'd need something to compare parser inserted versus non-parser inserted. In any case, the marquee tag in comment 0 should not work. Investigating.

Comment 3

[Daniel Veditz [:dveditz]]

Freddy: assigning this to ckerschb, but based on your interest if you want to take it feel free.

We need to uplift this to Fx52 when we've fixed it.

Comment 4

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1316826-test-case-strict-dynamic-blocks-inline-e.patch

I've added two test cases, care to take a look, Dan?

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 8811218 [details] [diff] [review]
0001-Bug-1316826-test-case-strict-dynamic-blocks-inline-e.patch

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

r=dveditz

The marquee test is only valid when bug 1312272 is fixed (which it is on beta and below)

Comment 6

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1316826-parserCreated-should-be-false-for-inline.patch

Approval Request Comment
[Feature/regressing bug #]:

bug 1299483 introduced strict-dynamic, which was implemented incorrectly (causing CSP bypasses). It is already in Aurora, so we'd want this uplifted

[User impact if declined]:

CSP is broken for websites using strict-dynamic. If we do not take this bug, we'd need to uplift a patch that disables the feature (it's behidn a pref)

[Describe test coverage new/current, TreeHerder]:

strict-dynamic has good test coverage, I've added tests for this specific bug in another patch set.

[Risks and why]: 

risk seems very low, I'm changing two "false" literals into "true".

[String/UUID change made/needed]:

No string/uuid changes.

Comment 7

[Frederik Braun [:freddy]]

Comment for the bounty team: This bug was filed against <marquee> elements (XBL code). Investigating this it turned out to be more problematic as it affects all attribute event handlers. I want to highlight this as a useful submission :-)

Comment 8

[Frederik Braun [:freddy]]

Successful try run with a minimized version of the patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5778206a5b7118d084c1c629f8072300ffd3c5

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 8812707 [details] [diff] [review]
0001-Bug-1316826-parserCreated-should-be-false-for-inline.patch

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

r=dveditz
a=dveditz

Comment 10

[Frederik Braun [:freddy]]

Please check in.
Landing the test means telling everyone about the exploit. But this isn't in release yet.
AFAIU it's fine to land as long as we land in aurora and central at the same time.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fee4cc0f780
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfff89d0d8c9

Comment 12

[Andrew]

Frederik: the "aParserCreated" must be set for javascript: URIs [1] ?
I didn't check with your fix, but I assume that cases are still bypasses CSP with 'strict-dynamic' policy:
  - data:text/html,<meta http-equiv="Content-Security-Policy" content="default-src 'none'; script-src 'nonce-123' 'strict-dynamic'"><iframe src="javascript:alert('test')"></iframe>
  - data:text/html,<meta http-equiv="Content-Security-Policy" content="default-src 'none'; script-src 'nonce-123' 'strict-dynamic'"><a href="javascript:alert('test')">Click me</a>

Can you check it?

[1] http://searchfox.org/mozilla-central/source/dom/jsurl/nsJSProtocolHandler.cpp#186

Comment 13

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

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/706ad7566a0838290157ad85b7458fda875190e2
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d49013ebf0df7f07ecb12467f193583272bc4a00

Comment 14

[Frederik Braun [:freddy]]

Ugh, thank you Andrew!
I'll create a follow-up patch first thing tomorrow. Should be another simple true/false change.

Comment 15

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1316826-fix-up-for-JS-URLs-r-tanvi.patch

Tanvi, can you take a look? It's a simple change that was missing from the previously landed patches.

Comment 16

[Frederik Braun [:freddy]]

Attachment: 0002-Bug-1316826-test-for-JS-URLs-and-strict-dynamic-r-ta.patch

Test case for JS URLs

Comment 17

[Frederik Braun [:freddy]]

Comment on attachment 8813054 [details] [diff] [review]
0001-Bug-1316826-fix-up-for-JS-URLs-r-tanvi.patch

Approval Request Comment (see comment 6, this is a fixup :/)

[Feature/regressing bug #]:

bug 1299483 introduced strict-dynamic, which was implemented incorrectly (causing CSP bypasses). It is already in Aurora, so we'd want this uplifted

[User impact if declined]:

the fix that we landed previously would be incomplete (CSP bypassable when using strict-dynamic)

[Describe test coverage new/current, TreeHerder]:

test included

[Risks and why]: 

risk seems very low, I'm changing a "false" literal into "true".


[String/UUID change made/needed]: none

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2fee4cc0f780
https://hg.mozilla.org/mozilla-central/rev/cfff89d0d8c9

Comment 19

[Tanvi Vyas[:tanvi]]

(In reply to Frederik Braun [:freddyb] from comment #15)
> Created attachment 8813054 [details] [diff] [review]
> 0001-Bug-1316826-fix-up-for-JS-URLs-r-tanvi.patch
> 
> Tanvi, can you take a look? It's a simple change that was missing from the
> previously landed patches.

There are a bunch of whitespace changes here.  I'm not familiar with this code and am swamped with another bug, so perhaps it is better to flag Dan here.

Comment 20

[Frederik Braun [:freddy]]

(In reply to Tanvi Vyas [:tanvi] from comment #19)
> There are a bunch of whitespace changes here. 

Oops. Removing.

Comment 21

[Frederik Braun [:freddy]]

Attachment: 0001-Bug-1316826-fix-up-for-JS-URLs-r-dveditz.patch

Approval Request see comment 6 and comment 17.

Comment 22

[Daniel Veditz [:dveditz]]

Comment on attachment 8813560 [details] [diff] [review]
0001-Bug-1316826-fix-up-for-JS-URLs-r-dveditz.patch

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

crap, sorry for the bad review in bug 1299483 
r=dveditz
a=dveditz

Comment 23

[Frederik Braun [:freddy]]

Requesting checkin to aurora and central.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/011a51e21bbbb73b83918c798756376cd62601e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/df24b83fca7a5771149f46711841abeaff34adb7

To whoever ends up doing the Aurora uplifts, only these two new patches need uplifting as the original two were uplifted in comment 13. Hopefully we'll use new bugs to track any further issues to avoid over-complicating things.

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/011a51e21bbb
https://hg.mozilla.org/mozilla-central/rev/df24b83fca7a

Comment 26

[Carsten Book [:Tomcat]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)

> To whoever ends up doing the Aurora uplifts, only these two new patches need
> uplifting as the original two were uplifted in comment 13. Hopefully we'll
> use new bugs to track any further issues to avoid over-complicating things.

thanks Ryan :)

uplifted

https://hg.mozilla.org/releases/mozilla-aurora/rev/42a574257cf3
https://hg.mozilla.org/releases/mozilla-aurora/rev/e3e01366879b

Comment 27

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the POC from comment#0 and comment#12 using the following build:

* fx52.0a1, buildid: 20161111030203, changeset: d38d06f85ef5
** https://archive.mozilla.org/pub/firefox/nightly/2016/11/2016-11-11-03-02-03-mozilla-central/

Went through verification using the following builds:
======================================================

* fx54.0a1, buildid: 20170302030206, changeset: e91de6fb2b3d
** https://archive.mozilla.org/pub/firefox/nightly/2017/03/2017-03-02-03-02-06-mozilla-central/

* fx53.0a2, buildid: 20170302004002, changeset: a89019f9eec3
** https://archive.mozilla.org/pub/firefox/nightly/2017/03/2017-03-02-00-40-02-mozilla-aurora/

* fx52.0, buildid: 20170227080736, changeset: 2183f7cb4f88
** https://archive.mozilla.org/pub/firefox/candidates/52.0-candidates/build1/

Platforms Used:
===============

* macOS 10.12.3 x64 - PASSED
* Win 10 Pro x64 - PASSED
* Ubuntu 16.04.2 LTS x64 - PASSED

Test Cases Used:
================

* ensured the issue isn't reproducible under e10s tabs
* ensured the issue isn't reproducible under non-e10s tabs
* ensured the issue isn't reproducible under private tabs
* ensured the issue isn't reproducible under container tabs

Ensured that following error message is displayed under the browser console:
* Content Security Policy: The page’s settings blocked the loading of a resource at self (“script-src 'nonce-123' 'strict-dynamic'”).