591002 - Escaping Needed to Prevent Stored XSS via Django's CSRF Token

Status: VERIFIED FIXED

(addons.mozilla.org :: Security, defect)

Description

[Michael Coates [:mcoates] (acct no longer active)]

Issue

Django's CSRF defense contains a flaw which allows cross site scripting attacks via the CSRF cookie.  This issue is being tracked by django here: http://code.djangoproject.com/ticket/14032. 

This attack could be launched from a subdomain or from a man in the middle attack via cookie forcing. This would allow an attacker to inject a stored XSS attack within the victim's browser. The XSS would fire anytime the victim visits a form using DJANGO's csrf controls within a valid domain for the infected cookie. 


Recommended Resolution

This is a publicly disclosed XSS vulnerability and will need to be resolved on our end as soon as possible. The recommended solution is to modify all uses of django's CSRF defense and remove the "safe" flag. This will ensure that DJANGO safely displays the CSRF token within the HTTP response.  

Note: The attacker does not need access to the django components in order to launch this attack. Once the victim's cookie is infected the attack will launch whenever the victim access the page. Therefore, restricted pages, such as those protected by LDAP auth, or equally vulnerable to attack.

Comment 1

[Jeff Balogh (:jbalogh)]

Attachment: django patch

This fixes Django and Jingo all at once.

Comment 2

[Jeff Balogh (:jbalogh)]

Can we fix this with a local patch?  The alternative is switching vendor to point at copy of django.git with this patch in it, but sumo and input and basket probably aren't on the same rev of Django as us.  I guess we don't have to worry about keeping our fix a secret since there's already a public Django bug.

Comment 3

[James Socol [:jsocol, :james]]

We're running r13302, Django 1.2.1, the latest released version.

Comment 4

[Jeff Balogh (:jbalogh)]

cc'ing more people who might have Django sites in prod.

Comment 5

[Michael Coates [:mcoates] (acct no longer active)]

I filed individual bugs for all django sites that were known to morgamic.

Comment 6

[Jeff Balogh (:jbalogh)]

oremj: how do you want to deploy this?

Comment 7

[Jeff Balogh (:jbalogh)]

Attachment: replacement for context_processors.csrf

Hey everyone, here's a new patch you can drop in your project without editing Django.  It replaces django.core.context_processors.csrf to do escaping.

Comment 8

[Les Orchard [:lorchard]]

(In reply to comment #7)
> Created attachment 470528 [details] [diff] [review]
> replacement for context_processors.csrf
> 
> Hey everyone, here's a new patch you can drop in your project without editing
> Django.  It replaces django.core.context_processors.csrf to do escaping.

I think you attached the wrong file - that was a patch to django/template/defaulttags.py

Comment 9

[Jeff Balogh (:jbalogh)]

Attachment: safe csrf in your project

Comment 10

[Jeff Balogh (:jbalogh)]

(In reply to comment #8)
> I think you attached the wrong file - that was a patch to
> django/template/defaulttags.py

Thanks Les.  Attaching patches is so primitive!

Comment 11

[Wil Clouser [:clouserw]]

Comment on attachment 470535 [details] [diff] [review]
safe csrf in your project

Instead of returning '', shouldn't you return None?  And from middleware/csrf.py:


    A side effect of calling this function is to make the the csrf_protect
    decorator and the CsrfViewMiddleware add a CSRF cookie and a 'Vary: Cookie'
    header to the outgoing response.  For this reason, you may need to use this
    function lazily, as is done by the csrf context processor.

Comment 12

[Jeff Balogh (:jbalogh)]

(In reply to comment #11)
> Comment on attachment 470535 [details] [diff] [review]
> safe csrf in your project
> 
> Instead of returning '', shouldn't you return None?  And from
> middleware/csrf.py:

The lazy() wrapper is expecting a string.  I think it will get upset about NoneType.

It probably won't like '' either.  I'm changing that to u''.

> 
>     A side effect of calling this function is to make the the csrf_protect
>     decorator and the CsrfViewMiddleware add a CSRF cookie and a 'Vary: Cookie'
>     header to the outgoing response.  For this reason, you may need to use this
>     function lazily, as is done by the csrf context processor.

Thanks!  I should make sure I'm not triggering the CSRF cookie.

Comment 13

[Wil Clouser [:clouserw]]

(In reply to comment #12)
> (In reply to comment #11)
> > Comment on attachment 470535 [details] [diff] [review] [details]
> > safe csrf in your project
> > 
> > Instead of returning '', shouldn't you return None?  And from
> > middleware/csrf.py:
> 
> The lazy() wrapper is expecting a string.  I think it will get upset about
> NoneType.

Alright.  I saw their function returning None and was just wanted to make sure that this wouldn't let someone with a blank csrf cookie through, but didn't go through all the functions.  Let's land it on PAMO and have mcoates verify

Comment 14

[Jeff Balogh (:jbalogh)]

http://github.com/jbalogh/zamboni/commit/a43dcb1

If you want to apply this to your project, try:

curl http://github.com/jbalogh/zamboni/commit/a43dcb1.patch | git apply

If that doesn't work, grab the context processor at http://github.com/jbalogh/zamboni/raw/a43dcb1/lib/csrf_context.py and then update your settings.py.

Comment 15

[Michael Coates [:mcoates] (acct no longer active)]

I'll take a look. Let me know when its ready.

Comment 16

[Jeff Balogh (:jbalogh)]

(In reply to comment #15)
> I'll take a look. Let me know when its ready.

It's on preview.amo.

Comment 17

[Michael Coates [:mcoates] (acct no longer active)]

The new changes look good.  HTML is correctly output encoded to prevent XSS via the csrf cookie.  The csrf django control still functions as expected and checks the CSRF token and the refer protocol and domain.

Comment 18

[Wil Clouser [:clouserw]]

Awesome, thanks.  -> verified from comment 17

Comment 19

[Yvan Boily [:ygjb][:yvan]]

Adding keywords to bugs for metrics, no action required.  Sorry about bugmail spam.