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

VERIFIED FIXED in Firefox 23

Status

()

Core
General
VERIFIED FIXED
5 years ago
3 years ago

People

(Reporter: Bob Copeland, Assigned: grobinson)

Tracking

(Blocks: 1 bug)

25 Branch
mozilla25
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22 unaffected, firefox23+ fixed, firefox24+ fixed, firefox25 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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)

Comment 1

5 years ago
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)

Comment 2

5 years ago
Thank you for reporting this, Bob !

Updated

5 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

5 years ago
Created attachment 768672 [details] [diff] [review]
Patch 1

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)

Comment 4

5 years ago
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+
(Assignee)

Updated

5 years ago
Blocks: 885433

Comment 6

5 years ago
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.

Updated

5 years ago
Keywords: checkin-needed

Comment 7

5 years ago
We will almost definitely want to nominate this for uplift to Fx24 (Aurora) and Fx23 (Beta).
status-firefox23: --- → affected
status-firefox24: --- → affected
tracking-firefox23: --- → ?
tracking-firefox24: --- → ?

Comment 8

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

Updated

5 years ago
Keywords: checkin-needed

Updated

5 years ago
Summary: CSP 'unsafe-eval' directive ignored → CSP: when script-src has both 'unsafe-inline' and 'unsafe-eval' directives present, eval() is still not allowed

Updated

5 years ago
Blocks: 663566

Comment 9

5 years ago
https://hg.mozilla.org/mozilla-central/rev/08aa8ceeb4b2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Comment 10

5 years ago
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?

Updated

5 years ago
status-firefox22: --- → unaffected
status-firefox25: --- → fixed

Updated

5 years ago
tracking-firefox23: ? → +
tracking-firefox24: ? → +

Updated

5 years ago
Attachment #768672 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 11

5 years ago
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.
status-firefox24: affected → fixed

Updated

5 years ago
Flags: needinfo?(bbajaj)
Please nominate for beta uplift so we can get this in next week's builds
Flags: needinfo?(bbajaj)
Flags: needinfo?(grobinson)
(Assignee)

Comment 13

4 years ago
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+
https://hg.mozilla.org/releases/mozilla-beta/rev/0708c21a6ed3
status-firefox23: affected → fixed
Flags: needinfo?(grobinson)
(Reporter)

Comment 15

4 years ago
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)
(Reporter)

Comment 18

3 years ago
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.