Closed Bug 1316826 Opened 3 years ago Closed 3 years ago

CSP bypass with DOM events and 'strict-dynamic'

Categories

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

52 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox49 --- unaffected
firefox-esr45 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 + verified
firefox53 --- verified

People

(Reporter: buglloc, Assigned: freddyb)

References

Details

(Keywords: csectype-other, regression, sec-moderate, Whiteboard: [post-critsmash-triage][domsecurity-active])

Attachments

(4 files, 1 obsolete file)

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.
Confirmed. This isn't even marquee specific. It fires for img onerror and input onkeyup just as well.
Group: firefox-core-security → core-security
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM: Security
Ever confirmed: true
Flags: sec-bounty?
Product: Firefox → Core
Summary: CSP bypass with "marquee" html events and 'strict-dynamic' → CSP bypass with DOM events and 'strict-dynamic'
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.
Group: core-security → dom-core-security
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.
Assignee: nobody → ckerschb
Priority: -- → P1
Whiteboard: [domsecurity-active]
I've added two test cases, care to take a look, Dan?
Attachment #8811218 - Flags: review?(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)
Attachment #8811218 - Flags: review?(dveditz) → review+
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.
Assignee: ckerschb → fbraun
Status: NEW → ASSIGNED
Attachment #8812707 - Flags: review?(dveditz)
Attachment #8812707 - Flags: approval-mozilla-aurora?
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 :-)
Blocks: 1299483
Keywords: regression
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
Attachment #8812707 - Flags: review?(dveditz)
Attachment #8812707 - Flags: review+
Attachment #8812707 - Flags: approval-mozilla-aurora?
Attachment #8812707 - Flags: approval-mozilla-aurora+
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.
Keywords: checkin-needed
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
Ugh, thank you Andrew!
I'll create a follow-up patch first thing tomorrow. Should be another simple true/false change.
Tanvi, can you take a look? It's a simple change that was missing from the previously landed patches.
Attachment #8813054 - Flags: review?(tanvi)
Test case for JS URLs
Attachment #8813056 - Flags: review?(tanvi)
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
Attachment #8813054 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/2fee4cc0f780
https://hg.mozilla.org/mozilla-central/rev/cfff89d0d8c9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(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.
Attachment #8813054 - Flags: review?(tanvi) → review?(dveditz)
Attachment #8813056 - Flags: review?(tanvi) → review?(dveditz)
(In reply to Tanvi Vyas [:tanvi] from comment #19)
> There are a bunch of whitespace changes here. 

Oops. Removing.
Approval Request see comment 6 and comment 17.
Attachment #8813054 - Attachment is obsolete: true
Attachment #8813054 - Flags: review?(dveditz)
Attachment #8813054 - Flags: approval-mozilla-aurora?
Attachment #8813560 - Flags: review?(dveditz)
Attachment #8813560 - Flags: approval-mozilla-aurora?
Group: dom-core-security → core-security-release
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #8813056 - Flags: review?(dveditz) → review+
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
Attachment #8813560 - Flags: review?(dveditz)
Attachment #8813560 - Flags: review+
Attachment #8813560 - Flags: approval-mozilla-aurora?
Attachment #8813560 - Flags: approval-mozilla-aurora+
Requesting checkin to aurora and central.
Keywords: checkin-needed
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/011a51e21bbb
https://hg.mozilla.org/mozilla-central/rev/df24b83fca7a
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
(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
Depends on: 1322167
Flags: sec-bounty? → sec-bounty+
Flags: qe-verify+
Whiteboard: [domsecurity-active] → [post-critsmash-triage][domsecurity-active]
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'”).
Status: RESOLVED → VERIFIED
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.