751422 - (CVE-2012-1944) <img onerror="..."> execute even when inline scripts are blocked by CSP

Status: VERIFIED FIXED

(Core :: Security, defect)

Description

[Adam Barth]

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 - ex-MoCo]]

Attachment: 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]

sg:high ?

Comment 3

[Al Billings [:abillings - ex-MoCo]]

Yes, I think so.

Comment 4

[Ian Melven :imelven]

Note that the current CSP tests for inline script only try body onload=...some script... which apparently is blocked.

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 (Exited; not receiving bugmail, old account, do not use)]

Also as far as I can tell there are no tests for unsafe-inline.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Kyle, are you taking this or should I?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

So, is there a testcase for this?
The PoC gives 3 PASSes.

Comment 9

[Ian Melven :imelven]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

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]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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]

(In reply to Ian Melven :imelven from comment #11)
> (In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

(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]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 (Exited; not receiving bugmail, old account, do not use)]

Do we have a bug on file on aligning with the spec?

Comment 16

[Tanvi Vyas[:tanvi]]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #15)
> Do we have a bug on file on aligning with the spec?

That's bug 746978.

Comment 18

[Tanvi Vyas[:tanvi]]

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][[e-mail]]]

Attachment: patch with test

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Comment 21

[Tanvi Vyas[:tanvi]]

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]]

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][[e-mail]]]

(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]]

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][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/e0204c492e54

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

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]]

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][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/22fbb77fc585
https://hg.mozilla.org/releases/mozilla-beta/rev/4c55969f64f9

Comment 30

[Al Billings [:abillings - ex-MoCo]]

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]

please go ahead and nominate for landing on esr10 branch.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

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][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/9619622a25ee

Comment 34

[u279076]

(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][[e-mail]]]

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 - ex-MoCo]]

(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

[u279076]

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 - ex-MoCo]]

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

[u279076]

(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]

Any CVE id for this issue?

Comment 41

[Al Billings [:abillings - ex-MoCo]]

Yes, I just hadn't added it yet. CVE-2012-1944

Comment 42

[u279076]

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]]

rforbes-bugspam-for-setting-that-bounty-flag-20130719