Closed Bug 887974 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: General, defect)

25 Branch
x86_64
Linux
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla25
Tracking Status
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed
firefox25 --- fixed

People

(Reporter: bcopeland, Assigned: grobinson)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Flags: needinfo?(sstamm)
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.
Assignee: nobody → grobinson
Flags: needinfo?(sstamm)
Thank you for reporting this, Bob !
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch Patch 1Splinter Review
Can *you* spot the bug? Props to imelven. This also fixes the weird behavior I was seeing in my Mochitest 885433.
Attachment #768672 - Flags: review?(sstamm)
This bug only manifests when a policy contains BOTH 'unsafe-inline' *and* 'unsafe-eval' btw.
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.
Attachment #768672 - Flags: review?(sstamm) → review+
Blocks: 885433
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.
Keywords: checkin-needed
We will almost definitely want to nominate this for uplift to Fx24 (Aurora) and Fx23 (Beta).
Keywords: checkin-needed
Summary: CSP 'unsafe-eval' directive ignored → CSP: when script-src has both 'unsafe-inline' and 'unsafe-eval' directives present, eval() is still not allowed
Blocks: csp-w3c-1.0
https://hg.mozilla.org/mozilla-central/rev/08aa8ceeb4b2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
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.
Attachment #768672 - Flags: approval-mozilla-aurora?
Attachment #768672 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(bbajaj)
Please nominate for beta uplift so we can get this in next week's builds
Flags: needinfo?(bbajaj)
Flags: needinfo?(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
Attachment #768672 - Flags: approval-mozilla-beta?
Attachment #768672 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
A little late but I can confirm the fix, thanks!
(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.
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.
Flags: needinfo?(bcopeland)
It was fixed in all versions I could test, and is now deployed on our live site for some time.
Thanks for getting back to me, Bob. Marking this bug verified fixed based on comment 18.
Status: RESOLVED → VERIFIED
Flags: needinfo?(bcopeland)
You need to log in before you can comment on or make changes to this bug.