887974 - CSP: when script-src has both 'unsafe-inline' and 'unsafe-eval' directives present, eval() is still not allowed

Status: VERIFIED FIXED

(Core :: General, defect)

Description

[Bob Copeland]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20130627 Firefox/25.0 (Nightly/Aurora)
Build ID: 20130627031027

Steps to reproduce:

I'm trying to deploy CSP-1.0 compliant headers where we have used X-CSP in the past.  On Aurora as of today, 6/27, I cannot get unsafe-eval to work.  (Yes, don't do that, but still...)

Simple test case - php script:
-----------------------------------------------------------------
<?php
header("Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'");

?>
<script>
// this works:
//alert("hello world!");
// this fails (works in chrome, unless unsafe-eval is removed)
eval('alert("hello world!")');
</script>
-----------------------------------------------

wget -S shows header being set as:

Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline' 'unsafe-eval'



Actual results:

[15:52:03.217] Error: call to eval() blocked by CSP @ https://[...]:5
[15:52:03.246] Content Security Policy: Directive eval script base restriction violated @ https://[...]:5





Expected results:

Alert worked.

Comment 1

[Ian Melven :imelven]

Garrett has a fix for this, we just spotted the bug together while working on bug 885433 and debugging the test he wrote for that.

Comment 2

[Ian Melven :imelven]

Thank you for reporting this, Bob !

Comment 3

[Garrett Robinson [:grobinson]]

Attachment: Patch 1

Can *you* spot the bug? Props to imelven. This also fixes the weird behavior I was seeing in my Mochitest 885433.

Comment 4

[Ian Melven :imelven]

This bug only manifests when a policy contains BOTH 'unsafe-inline' *and* 'unsafe-eval' btw.

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 768672 [details] [diff] [review]
Patch 1

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

Nit: you didn't take long enough to fix this bug.  I tested the fix locally with the PHP test case provided, r=me.

Comment 6

[Ian Melven :imelven]

The test Garrett has written for bug 885433 specifies both 'unsafe-inline' and 'unsafe-eval' which is how we found the issue. That should land shortly (we want to get bug 886132 fixed) and then we will have a test in the suite to cover this case going forward.

Comment 7

[Ian Melven :imelven]

We will almost definitely want to nominate this for uplift to Fx24 (Aurora) and Fx23 (Beta).

Comment 8

[Ian Melven :imelven]

https://hg.mozilla.org/integration/mozilla-inbound/rev/08aa8ceeb4b2

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/08aa8ceeb4b2

Comment 10

[Ian Melven :imelven]

Comment on attachment 768672 [details] [diff] [review]
Patch 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 746978
User impact if declined: CSP policies with script-src: 'unsafe-inline' 'unsafe-eval' will not work correctly (eval will not be allowed when it should be)
Testing completed (on m-c, etc.): This just landed but it's a small change - there's a lot of CSP tests to minimize regression worry. Comment 6 explains that grobinson is going to add a test that will cover the case of a policy with script-src: 'unsafe-inline' 'unsafe-eval' very shortly
Risk to taking this patch (and alternatives if risky): should be low, see above re testing 
String or IDL/UUID changes made by this patch: nope

We will probably want to uplift this to beta too.

Comment 11

[Ian Melven :imelven]

Thanks, Bhavana ! 

https://hg.mozilla.org/releases/mozilla-aurora/rev/3f9d6dbaf669

How long should we wait before nominating for beta uplift ? This is a pretty bad bug with our CSP 1.0 support and it would be good to get it fixed in Fx23 before that goes to release.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Please nominate for beta uplift so we can get this in next week's builds

Comment 13

[Garrett Robinson [:grobinson]]

Comment on attachment 768672 [details] [diff] [review]
Patch 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 887974
User impact if declined: Incorrect, non spec compliant implementation of CSP 1.0
Testing completed (on m-c, etc.): tested with m-c against the CSP test suite
Risk to taking this patch (and alternatives if risky): Minor (only affects CSP)
String or IDL/UUID changes made by this patch: None

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0708c21a6ed3

Comment 15

[Bob Copeland]

A little late but I can confirm the fix, thanks!

Comment 16

[u279076]

(In reply to Bob Copeland from comment #15)
> A little late but I can confirm the fix, thanks!

Hi Bob, which versions were you able to test? This should now be fixed in Firefox 23, 24, and 25.

Comment 17

[u279076]

Bob, can you please let us know which versions you confirm this to be fixed? It should be fixed in Firefox 23, 24, and 25.

Comment 18

[Bob Copeland]

It was fixed in all versions I could test, and is now deployed on our live site for some time.

Comment 19

[u279076]

Thanks for getting back to me, Bob. Marking this bug verified fixed based on comment 18.