Bug 751422 (CVE-2012-1944)

<img onerror="..."> execute even when inline scripts are blocked by CSP

VERIFIED FIXED

Status

()

Core
Security
VERIFIED FIXED
5 years ago
2 years ago

People

(Reporter: Adam Barth, Assigned: smaug)

Tracking

({regression, sec-high})

unspecified
regression, sec-high
Points:
---
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox13 fixed, firefox14 fixed, firefox-esr1013+ fixed)

Details

(Whiteboard: [sg:high][advisory-tracking+][qa?])

Attachments

(3 attachments)

(Reporter)

Description

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

5 years ago
sg:high ?
Whiteboard: [sg:high]
Yes, I think so.

Comment 4

5 years ago
Note that the current CSP tests for inline script only try body onload=...some script... which apparently is blocked.
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.
Depends on: 682420
Blocks: 682420
No longer depends on: 682420
Also as far as I can tell there are no tests for unsafe-inline.
(Assignee)

Comment 7

5 years ago
Kyle, are you taking this or should I?
(Assignee)

Comment 8

5 years ago
So, is there a testcase for this?
The PoC gives 3 PASSes.
(Assignee)

Updated

5 years ago
Assignee: nobody → bugs

Comment 9

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

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

5 years ago
(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 ?
(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.
(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
Do we have a bug on file on aligning with the spec?
(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.)
(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.
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
Keywords: sec-high
(Assignee)

Comment 19

5 years ago
Created attachment 620805 [details] [diff] [review]
patch with test
(Assignee)

Comment 20

5 years ago
Created attachment 620806 [details] [diff] [review]
patch
Attachment #620806 - Flags: review?(khuey)
Attachment #620806 - Flags: review?(khuey) → review+
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
This effects ff 10, 11, and 12.  Anyway we can put this patch into 13 even though its in beta?
(Assignee)

Comment 23

5 years ago
(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.
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.
status-firefox-esr10: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox-esr10: --- → ?
tracking-firefox13: --- → +
tracking-firefox14: --- → +
tracking-firefox15: --- → +
Keywords: regression
status-firefox-esr10: affected → ---
status-firefox13: affected → ---
status-firefox14: affected → ---
status-firefox15: affected → ---
tracking-firefox-esr10: ? → ---
tracking-firefox13: + → ---
tracking-firefox14: + → ---
tracking-firefox15: + → ---
Whiteboard: [sg:high] → [sg:moderate]
Keywords: sec-high → sec-moderate
(Assignee)

Comment 26

5 years ago
https://hg.mozilla.org/mozilla-central/rev/e0204c492e54
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 27

5 years ago
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
Attachment #620806 - Flags: approval-mozilla-beta?
Attachment #620806 - Flags: approval-mozilla-aurora?
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.
Attachment #620806 - Flags: approval-mozilla-beta?
Attachment #620806 - Flags: approval-mozilla-beta+
Attachment #620806 - Flags: approval-mozilla-aurora?
Attachment #620806 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 29

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/22fbb77fc585
https://hg.mozilla.org/releases/mozilla-beta/rev/4c55969f64f9
status-firefox13: --- → fixed
status-firefox14: --- → fixed
Keywords: sec-moderate → sec-high
Whiteboard: [sg:moderate] → [sg:high]
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 13+
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.)
Status: RESOLVED → VERIFIED
status-firefox-esr10: affected → ---
tracking-firefox-esr10: 13+ → ---
please go ahead and nominate for landing on esr10 branch.
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → 13+
(Assignee)

Updated

5 years ago
Attachment #620806 - Flags: approval-mozilla-esr10?
Comment on attachment 620806 [details] [diff] [review]
patch

approved for esr to fix sg:high regression that is fixed on 13.
Attachment #620806 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 33

5 years ago
https://hg.mozilla.org/releases/mozilla-esr10/rev/9619622a25ee
status-firefox-esr10: affected → fixed
(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.
(Assignee)

Comment 35

5 years ago
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.
(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.
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.
Whiteboard: [sg:high] → [sg:high][advisory-tracking+]
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.
(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

5 years ago
Any CVE id for this issue?
Yes, I just hadn't added it yet. CVE-2012-1944
Alias: CVE-2012-1944
Attachment #620531 - Attachment description: PoC from linked checkin → PoC from linked checkin (PHP)
Attachment #620531 - Attachment mime type: text/html → text/plain
Making one more pass at this bug. Can someone provide some direction as to how QA can verify this fix? Perhaps using the PoC?
Whiteboard: [sg:high][advisory-tracking+] → [sg:high][advisory-tracking+][qa?]
Group: core-security
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
You need to log in before you can comment on or make changes to this bug.