Last Comment Bug 751422 - (CVE-2012-1944) <img onerror="..."> execute even when inline scripts are blocked by CSP
(CVE-2012-1944)
: <img onerror="..."> execute even when inline scripts are blocked by CSP
Status: VERIFIED FIXED
[sg:high][advisory-tracking+][qa?]
: regression, sec-high
Product: Core
Classification: Components
Component: Security (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug] (high review load, please consider other reviewers)
:
Mentors:
Depends on:
Blocks: 682420
  Show dependency treegraph
 
Reported: 2012-05-02 16:49 PDT by Adam Barth
Modified: 2015-05-07 15:29 PDT (History)
14 users (show)
rforbes: sec‑bounty+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
13+
fixed


Attachments
PoC from linked checkin (PHP) (1.07 KB, text/plain)
2012-05-02 17:09 PDT, Al Billings [:abillings]
no flags Details
patch with test (4.73 KB, patch)
2012-05-03 12:08 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
no flags Details | Diff | Review
patch (924 bytes, patch)
2012-05-03 12:09 PDT, Olli Pettay [:smaug] (high review load, please consider other reviewers)
khuey: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Review

Description Adam Barth 2012-05-02 16:49:11 PDT
See http://dvcs.w3.org/hg/webappsec/file/tip/tests/csp/submitted/webkit/CSP_default-src-inline-allowed.php

I also reported this in person at the W3C WebAppSec working group face-to-face.
Comment 1 Al Billings [:abillings] 2012-05-02 17:09:23 PDT
Created attachment 620531 [details]
PoC from linked checkin (PHP)

Please attach any proof of concepts when opening a bug since external resources may disappear.
Comment 2 Ian Melven :imelven 2012-05-02 17:29:21 PDT
sg:high ?
Comment 3 Al Billings [:abillings] 2012-05-02 17:36:24 PDT
Yes, I think so.
Comment 4 Ian Melven :imelven 2012-05-02 17:38:25 PDT
Note that the current CSP tests for inline script only try body onload=...some script... which apparently is blocked.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-02 19:45:24 PDT
I think this was regressed by http://hg.mozilla.org/mozilla-central/diff/9e6aa5ee6425/content/events/src/nsEventListenerManager.cpp#l1.24.  By not setting 'doc' we never descend into the block that checks the CSP.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-02 19:46:32 PDT
Also as far as I can tell there are no tests for unsafe-inline.
Comment 7 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-03 01:51:49 PDT
Kyle, are you taking this or should I?
Comment 8 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-03 04:09:36 PDT
So, is there a testcase for this?
The PoC gives 3 PASSes.
Comment 9 Ian Melven :imelven 2012-05-03 07:52:47 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #6)
> Also as far as I can tell there are no tests for unsafe-inline.

what about http://mxr.mozilla.org/mozilla-central/source/content/base/test/test_CSP_inlinescript.html?force=1 ?
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-03 07:57:16 PDT
All I see is http://mxr.mozilla.org/mozilla-central/source/content/base/test/file_CSP_inlinescript_main.html%5Eheaders%5E

Again, I don't see 'unsafe-inline' anywhere in mozilla-central.
Comment 11 Ian Melven :imelven 2012-05-03 09:10:34 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> All I see is
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/
> file_CSP_inlinescript_main.html%5Eheaders%5E
> 
> Again, I don't see 'unsafe-inline' anywhere in mozilla-central.

ah ok - i'm a little confused - the bug title states that img onerror shouldn't run when inline scripts are blocked (which they are by default) - but the test case looks like it is testing that event handler inline scripts can run when unsafe-inline is specified. i didn't realize unsafe-inline is actually a CSP directive, i thought you were referring to there being no tests that inline event handlers were blocked, sorry ! :)
Comment 12 Ian Melven :imelven 2012-05-03 09:12:15 PDT
(In reply to Ian Melven :imelven from comment #11)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> > All I see is
> > http://mxr.mozilla.org/mozilla-central/source/content/base/test/
> > file_CSP_inlinescript_main.html%5Eheaders%5E
> > 
> > Again, I don't see 'unsafe-inline' anywhere in mozilla-central.
> 
> ah ok - i'm a little confused - the bug title states that img onerror
> shouldn't run when inline scripts are blocked (which they are by default) -
> but the test case looks like it is testing that event handler inline scripts
> can run when unsafe-inline is specified. i didn't realize unsafe-inline is
> actually a CSP directive, i thought you were referring to there being no
> tests that inline event handlers were blocked, sorry ! :)

i'm guessing this is also the reason Olli was asking about a test case for the bug in the title in comment 8 - Adam or Tanvi, can you clarify what's going on ?
Comment 13 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-03 09:12:52 PDT
(In reply to Ian Melven :imelven from comment #12)
> i'm guessing this is also the reason Olli was asking about a test case for
> the bug in the title in comment 8 - Adam or Tanvi, can you clarify what's
> going on ?

Right.  As far as I can tell the testcase is testing the opposite of what the bug report is about.
Comment 14 Tanvi Vyas[:tanvi] 2012-05-03 10:07:20 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #10)
> All I see is
> http://mxr.mozilla.org/mozilla-central/source/content/base/test/
> file_CSP_inlinescript_main.html%5Eheaders%5E
> 
> Again, I don't see 'unsafe-inline' anywhere in mozilla-central.

That's because mozilla calls it "inline-script" instead of "unsafe-inline":
mxr.mozilla.org/mozilla-central/source/content/base/src/CSPUtils.jsm#246
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-05-03 10:09:15 PDT
Do we have a bug on file on aligning with the spec?
Comment 16 Tanvi Vyas[:tanvi] 2012-05-03 10:12:25 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #15)
> Do we have a bug on file on aligning with the spec?

Working on it.  And will also look at the other comments in this bug.  (In an all day meeting today, so my comments may be a bit delayed.)
Comment 17 Sid Stamm [:geekboy or :sstamm] 2012-05-03 10:29:09 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #15)
> Do we have a bug on file on aligning with the spec?

That's bug 746978.
Comment 18 Tanvi Vyas[:tanvi] 2012-05-03 11:00:14 PDT
So the test case sets "unsafe-inline" which means nothing to firefox.  Hence, inline-scripts should be blocked (by default).  They are blocked in a <script> tag, but for some reason don't seem to be blocked in on* handlers.

Here is another proof of concept (trimmed down to show the exact issue).  The alerts for script and onload don't fire on FF12, but the alerts for onerror and onclick do.

<?php
header("X-Content-Security-Policy: default-src 'none'");
?>
<!DOCTYPE html>
<html>
<head>
<title>CSP Test: default-src 'self' about: 'unsafe-inline'</title>
</head>
<body onload="alert('onload');">
<div id="log"></div>
<script>
alert("script tag");
</script>
<img style="display:none"
     onerror="alert('onerror');"
     src="about:blank">
<a href="http://www.google.com" onclick="alert('onclick');">link</a>
</body>
</html>


Note that it appears as if we do have a test for the onload handler.  Not sure why/how we block scripts in onload but not other on* handlers: 
http://mxr.mozilla.org/mozilla-central/source/content/base/test/file_CSP_inlinescript_main.html?force=1#5
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-03 12:08:37 PDT
Created attachment 620805 [details] [diff] [review]
patch with test
Comment 20 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-03 12:09:48 PDT
Created attachment 620806 [details] [diff] [review]
patch
Comment 21 Tanvi Vyas[:tanvi] 2012-05-03 12:51:51 PDT
What does this do/mean.  When is global not defined?  For data: urls?

566   if (!global) {
567     // This can happen; for example this document might have been
568     // loaded as data.
569     return NS_OK;
570   }

mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventListenerManager.cpp#566
Comment 22 Tanvi Vyas[:tanvi] 2012-05-03 12:52:37 PDT
This effects ff 10, 11, and 12.  Anyway we can put this patch into 13 even though its in beta?
Comment 23 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-03 12:55:41 PDT
(In reply to Tanvi Vyas from comment #21)
> What does this do/mean.  When is global not defined?  For data: urls?
data: url may or may not have global. URL doesn't matter.

But document from XHR doesn't have a global. Nor document.implement.createHTMLDocument() etc.
Comment 24 Daniel Veditz [:dveditz] 2012-05-03 12:58:35 PDT
doesn't sec-high overrate this bug? The page already has an XSS problem in the absence of CSP (old browsers), this Firefox bug just means our protection fails. In a world of ubiquitous CSP support I support sec-high would be fair, but current day feels more "moderate" to me. I suppose it doesn't matter that much.
Comment 26 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-04 09:27:12 PDT
https://hg.mozilla.org/mozilla-central/rev/e0204c492e54
Comment 27 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-04 09:28:41 PDT
Comment on attachment 620806 [details] [diff] [review]
patch

[Approval Request Comment]
Regression caused by (bug #): bug 682420
User impact if declined: broken CSP
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): should be very low risk
String changes made by this patch: N/A
Comment 28 Alex Keybl [:akeybl] 2012-05-06 19:03:26 PDT
Comment on attachment 620806 [details] [diff] [review]
patch

[Triage Comment]
Given the low risk evaluation, where we are in the cycle, and the fact that this is externally known, we'll uplift this sg:moderate bug early to both Aurora 14 and Beta 13.
Comment 29 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-07 03:02:37 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/22fbb77fc585
https://hg.mozilla.org/releases/mozilla-beta/rev/4c55969f64f9
Comment 30 Al Billings [:abillings] 2012-05-07 17:29:56 PDT
Verified fixed using the test in comment 18 against Firefox 12 and the 5/6 nightly build of mozilla central. 

(Note to QA: You'll need to host this on a PHP capable webserver to test.)
Comment 31 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-10 16:31:07 PDT
please go ahead and nominate for landing on esr10 branch.
Comment 32 Lukas Blakk [:lsblakk] use ?needinfo 2012-05-16 11:39:40 PDT
Comment on attachment 620806 [details] [diff] [review]
patch

approved for esr to fix sg:high regression that is fixed on 13.
Comment 33 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-16 11:53:19 PDT
https://hg.mozilla.org/releases/mozilla-esr10/rev/9619622a25ee
Comment 34 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-22 15:31:57 PDT
(In reply to Al Billings [:abillings] from comment #30)
> Verified fixed using the test in comment 18 against Firefox 12 and the 5/6
> nightly build of mozilla central. 
> 
> (Note to QA: You'll need to host this on a PHP capable webserver to test.)

@adambarth, could you please assist QA in verifying this is fixed for Firefox 13, 14, and latest-mozilla-esr10? Given our current timeline and the dependency of a PHP webserver, I don't think we will be able to verify this on our own.
Comment 35 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-05-22 15:44:29 PDT
Eh, https://bug751422.bugzilla.mozilla.org/attachment.cgi?id=620805&t=VsZ7FM1iS0 has a test
for this running on our https.js. So you could extract the test from that and test a build
with and without the patch.
Comment 36 Al Billings [:abillings] 2012-05-22 15:57:28 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #34)
> 
> @adambarth, could you please assist QA in verifying this is fixed for
> Firefox 13, 14, and latest-mozilla-esr10? Given our current timeline and the
> dependency of a PHP webserver, I don't think we will be able to verify this
> on our own.

Or you could use mozqa.com, which runs PHP.
Comment 37 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-22 16:16:46 PDT
I'd rather let someone who is already set up to test this have a go at it first; it's also much more efficient.
Comment 38 Al Billings [:abillings] 2012-05-22 17:48:16 PDT
QA, the reporter is a member of the Chrome team who was nice enough to open this bug after verbally reporting it to us. He had Firefox in a VM when he found this while doing other work. I really doubt that it is more efficient to ask him to download three different unreleased Firefox builds to verify this for us. 

Since, per e-mail exchanges with others about this bug, QA is unwilling to verify this issue, I'll go ahead and do it.
Comment 39 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-05-22 18:45:52 PDT
(In reply to Al Billings [:abillings] from comment #38)
> per e-mail exchanges with others about this bug, QA is unwilling to
> verify this issue, I'll go ahead and do it.

For the record, I never said that QA was unwilling to verify this fix.  I clearly stated in my email that if the reporter was unwilling, unable, or unresponsive that QA would step in and verify the fix. I always prefer to give the reporter opportunity for ownership of their bugs. I was merely giving the reporter opportunity to verify his bug was fixed before QA went to the trouble of learning how to correctly reproduce the environment, reproduce the bug, and then verify the fix. 

Al, if you are already set up to reproduce this bug, feel free to verify it. If not, I would appreciate you sharing some knowledge so that I know how to set up a test environment.
Comment 40 Huzaifa Sidhpurwala 2012-05-31 21:15:36 PDT
Any CVE id for this issue?
Comment 41 Al Billings [:abillings] 2012-05-31 22:44:41 PDT
Yes, I just hadn't added it yet. CVE-2012-1944
Comment 42 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-21 16:54:48 PDT
Making one more pass at this bug. Can someone provide some direction as to how QA can verify this fix? Perhaps using the PoC?
Comment 43 Raymond Forbes[:rforbes] 2013-07-19 18:22:50 PDT
rforbes-bugspam-for-setting-that-bounty-flag-20130719

Note You need to log in before you can comment on or make changes to this bug.