1439330 - CSP: eval is not blocked if 'strict-dynamic' is enabled

Status: RESOLVED FIXED

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

Description

[Tomi Belan]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36

Steps to reproduce:

This file only prints 123, as expected:

<meta http-equiv="Content-Security-Policy" content="script-src 'nonce-foobar'">
<script nonce="foobar">document.write(123);eval('document.write(456)')</script>

But when we add 'strict-dynamic', it prints 123456:

<meta http-equiv="Content-Security-Policy" content="script-src 'nonce-foobar' 'strict-dynamic'">
<script nonce="foobar">document.write(123);eval('document.write(456)')</script>


Actual results:

First example prints 123. Second example prints 123456.


Expected results:

Both examples should print 123.

In Chrome 64, both examples print 123.

See also: http://w3c-test.org/content-security-policy/script-src/script-src-strict_dynamic_eval.html

Comment 1

[Tomi Belan]

Attachment: good.html

Comment 2

[Tomi Belan]

Attachment: bad.html

Comment 3

[Vinothkumar Nagasayanan [:vinoth]]

There is some phabricator issue. So adding the patch link here for review,

https://phabricator.services.mozilla.com/D859

:ckerschb Please review the patch and add your comments if any.

Comment 4

[Phabricator Automation]

Attachment: Bug 1439330 - condition added to block eval if only strict-dynamic is present without unsafe-eval keyword

Comment 5

[Phabricator Automation]

Attachment: Bug 1439330 - Test added to check if eval is blocked if 'strict-dynamic' is enabled

Comment 6

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8970104 [details]
Bug 1439330 - Test added to check if eval is blocked if 'strict-dynamic' is enabled

Please review the patch and let me know if changes are needed.

Comment 7

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8965731 [details]
Bug 1439330 - condition added to block eval if only strict-dynamic is present without unsafe-eval keyword

Please review the patch and let me know if changes are needed.

Comment 8

[Phabricator Automation]

Comment on attachment 8970104 [details]
Bug 1439330 - Test added to check if eval is blocked if 'strict-dynamic' is enabled

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D1011

Comment 9

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8970104 [details]
Bug 1439330 - Test added to check if eval is blocked if 'strict-dynamic' is enabled

Vino, I think the patch itself for this bug looks good. Could you please also add a test that includes strict-dynamic and unsafe-eval so we have a testcase for the opposite scenario as well?
Thanks!

Comment 10

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8965731 [details]
Bug 1439330 - condition added to block eval if only strict-dynamic is present without unsafe-eval keyword

Please review the patch and let me know if changes are needed.

Comment 11

[Vinothkumar Nagasayanan [:vinoth]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Comment on attachment 8970104 [details]
> Bug 1439330 - Test added to check if eval is blocked if 'strict-dynamic' is
> enabled
> 
> Vino, I think the patch itself for this bug looks good. Could you please
> also add a test that includes strict-dynamic and unsafe-eval so we have a
> testcase for the opposite scenario as well?
> Thanks!

I have also added the opposite scenario and updated the test cases.
Please verify that too,
https://phabricator.services.mozilla.com/D1011

Comment 12

[Phabricator Automation]

Comment on attachment 8965731 [details]
Bug 1439330 - condition added to block eval if only strict-dynamic is present without unsafe-eval keyword

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D859

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9abb3479fdd
Condition added to block eval if only strict-dynamic is present without unsafe-eval keyword. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/254e0c58f80f
Test added to check if eval is blocked if 'strict-dynamic' is enabled. r=ckerschb

Comment 14

[Stefan Hindli [:stefan_hindli]]

Backed out 2 changesets (bug 1439330) for wpt3 failures in /content-security-policy/script-src/script-src-strict_dynamic_eval.html on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=254e0c58f80fd65ad00bcd3b4dfd324a05d93e67&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=176138888
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=eb408f77a028cd93809fadb30f101fbf4aaa4d70&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

07:25:30     INFO - TEST-UNEXPECTED-PASS | /content-security-policy/script-src/script-src-strict_dynamic_eval.html | Script injected via `eval` is not allowed with `strict-dynamic` without `unsafe-eval`. - expected FAIL
07:25:30     INFO - TEST-INFO | expected FAIL
07:25:30     INFO - TEST-OK | /content-security-policy/script-src/script-src-strict_dynamic_eval.html | took 1133ms
07:25:31     INFO - PID 749 | [Child 752, Main Thread] WARNING: nsAppShell::Exit() called redundantly: file /builds/worker/workspace/build/src/widget/cocoa/nsAppShell.mm, line 738

Comment 15

[Vinothkumar Nagasayanan [:vinoth]]

> 07:25:30     INFO - TEST-UNEXPECTED-PASS |
> /content-security-policy/script-src/script-src-strict_dynamic_eval.html |
> Script injected via `eval` is not allowed with `strict-dynamic` without
> `unsafe-eval`. - expected FAIL
> 07:25:30     INFO - TEST-INFO | expected FAIL
> 07:25:30     INFO - TEST-OK |
> /content-security-policy/script-src/script-src-strict_dynamic_eval.html |
> took 1133ms
> 07:25:31     INFO - PID 749 | [Child 752, Main Thread] WARNING:
> nsAppShell::Exit() called redundantly: file
> /builds/worker/workspace/build/src/widget/cocoa/nsAppShell.mm, line 738



Two tests script-src-strict_dynamic_eval.html.ini and script-src-strict_dynamic_new_function.html.ini are previously expected to "FAIL" because of this bug.
Now after this patch those two tests PASSED, hence this wpt failure.
Now I removed both .ini files and updated the same patch,
https://phabricator.services.mozilla.com/D859 

Please verify and let me know your comments if I can again proceed to landing the patch.

Comment 16

[Christoph Kerschbaumer [:ckerschb}]

Yeah, that looks great to me, thanks!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6419ce7979bd
Condition added to block eval if only strict-dynamic is present without unsafe-eval keyword. r=ckerschb
https://hg.mozilla.org/integration/mozilla-inbound/rev/c12ef7d20d6c
Test added to check if eval is blocked if 'strict-dynamic' is enabled. r=ckerschb

Comment 18

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/6419ce7979bd
https://hg.mozilla.org/mozilla-central/rev/c12ef7d20d6c

Comment 19

[Tomi Belan]

Awesome. Thanks, everyone.