Обсуждение: [PATCH] Enable CsrfViewMiddleware -- make CSRF protection required by default

Hi list,

I noticed that most of the forms on the Postgres community site don't
use CSRF protection. That's bad -- CSRF should be on by default.

I went through all the views that handle POST data and didn't find any
that should handle input from cross-domain requests. But CSRF
exceptions, if any, should be decorated with @csrf_exempt (from
django.views.decorators.csrf).

Also available from my Github repo: https://github.com/intgr/pgweb

Regards,
Marti

On Wed, Oct 31, 2012 at 7:29 PM, Magnus Hagander <magnus@hagander.net> wrote:
> The diff appears to be reversed. But that's easy enough to deal with during
> commit.

No, it's not reversed. I'm removing the explicit @csrf_protect
decorators because all views are now protected by default.

> Have you verified that it works with django 1.2 as well? The production
> deployment is on that quite old version still...

Yeah, I developed and tested this on Django 1.2

Regards,
Marti

On Fri, Nov 2, 2012 at 1:31 PM, Magnus Hagander <magnus@hagander.net> wrote:
> So, one more thought. Is this going to break if the form is cached? That is,
> the original form at e.g. http://www.postgresql.org/community/ for the
> surveys is cached. That means that the CSRF token that's on the form
> actually ends up being cached. Is the CSRF token going to be valid in those
> cases, and is it actually going to protect us?

Yeah, that's true. But it should be a matter of flushing the Varnish
cache, right? There are no cache policy headers on these responses, so
browsers will generally revalidate the page.

But now that you mention it, there is another caching impact:
accessing this page causes the user's cookies to be changed, and due
to "Vary: Cookie", it will prevent the caching of any subsequent page
fetches for this user in Varnish, even on other pages (for 1 full year
by default).

Of course the above also affects any users who logged in -- since the
csrftoken cookie is served without the "secure" flag, the cookie is
also present in any non-secure requests.

Does this also impair Varnish "grace mode", when the backend server is down?

Regards,
Marti

On Fri, Nov 2, 2012 at 4:32 PM, Magnus Hagander <magnus@hagander.net> wrote:
> No, that's not a problem. We strip cookies in varnish by default. We only
> support them over https...

Ahhh! That explains everything. I wasn't aware of the magic that
happens on the proxy level. I thought you were relying on Django to
not send cookies when not necessary, and the proxy respected the HTTP
headers sent by Django like a conforming HTTP proxy.

The attached patch adds @csrf_exempt to the survey view and removes
csrf_token from the template.

> if we have any other such pages (other than the search, but we can certainly
> disable CSRF for search, right?)

Search uses GET parameters so it already bypasses CSRF.

Regards,
Marti

On Mon, Nov 5, 2012 at 2:12 PM, Magnus Hagander <magnus@hagander.net> wrote:
> On Fri, Nov 2, 2012 at 4:09 PM, Marti Raudsepp <marti@juffo.org> wrote:
>>
>> On Fri, Nov 2, 2012 at 4:32 PM, Magnus Hagander <magnus@hagander.net>
>> wrote:
>> > No, that's not a problem. We strip cookies in varnish by default. We
>> > only
>> > support them over https...
>>
>> Ahhh! That explains everything. I wasn't aware of the magic that
>> happens on the proxy level. I thought you were relying on Django to
>> not send cookies when not necessary, and the proxy respected the HTTP
>> headers sent by Django like a conforming HTTP proxy.
>>
>> The attached patch adds @csrf_exempt to the survey view and removes
>> csrf_token from the template.
>
>
> Thanks - applied. Please help me keep an extra eye out on things the next
> couple of days to see if we broke something :)

Ugh.

This broke the admin interface form to access varnish. I've mad eit
exempt. Is there any actual reason why we need it in the admin
interface, since you need to have a session logged in as an
administrator already to access it?

It also broke the purging API. Also made exempt, but that appears to
not solve the problem. Do I need to do something more than add
@csrf_exempt to a view functoin to make it not broken? The error
message talks about the referrer header - but surely that shouldn't be
a requirement when oyu've set @csrf_exempt?

And it broke the bug reporting form, also fixed in a separate commit.


We may well have missed more parts :( Clearly neither one of us tested
this patch very well.

If we run into any further issues (assuming we can solve the one
above), we should probably revert the whole thing. But let's hope we
can make it work..

--Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

On Wed, Nov 7, 2012 at 7:49 PM, Magnus Hagander <magnus@hagander.net> wrote:
> This broke the admin interface form to access varnish. I've mad eit
> exempt. Is there any actual reason why we need it in the admin
> interface, since you need to have a session logged in as an
> administrator already to access it?

Yes, you *especially* need CSRF protection in the admin interface.
Anything that performs privileged actions and is authenticated via
cookies without CSRF protection is vulnerable.

Say for example I send you a bug report with a link to
http://my-evil-server/page.html . Not suspecting anything, you follow
the link.
This page contains a hidden <form method=POST
action="https://www.postgresql.org/admin/...">, with custom fields
based on the actions I want to perform. This form is submitted on page
load via JavaScript into a hidden iframe -- all without you realizing
it.

If you have an authenticated session on postgresql.org, then your
browser will happily pass your personal cookie on to postgresql.org,
along with any form fields dictated by the attacker -- thus the
attacker can use your session to perform any actions you are
authenticated to perform. Such as changing your account password.

This is a major vulnerability, not just security masturbation.

> It also broke the purging API. Also made exempt, but that appears to
> not solve the problem. Do I need to do something more than add
> @csrf_exempt to a view functoin to make it not broken? The error
> message talks about the referrer header - but surely that shouldn't be
> a requirement when oyu've set @csrf_exempt?

It seems that the problem is the @ssl_required decorator -- it returns
a new wrapped view without copying over attributes of the original
view, such as "csrf_exempt". Changing the decorator order won't work
either because that will confuse PgMiddleware.

I'll send a patch to fix @ssl_required some time soon.

> We may well have missed more parts :( Clearly neither one of us tested
> this patch very well.

"It all worked on my computer" ;)

But my setup is plain Django. I admit, I should have put more thought
into it, once you told me about the cookie magic that happens in
Varnish.

Regards,
Marti

On Wed, Nov 7, 2012 at 7:59 PM, Marti Raudsepp <marti@juffo.org> wrote:
> On Wed, Nov 7, 2012 at 7:49 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> This broke the admin interface form to access varnish. I've mad eit
>> exempt. Is there any actual reason why we need it in the admin
>> interface, since you need to have a session logged in as an
>> administrator already to access it?
>
> Yes, you *especially* need CSRF protection in the admin interface.
> Anything that performs privileged actions and is authenticated via
> cookies without CSRF protection is vulnerable.

Fair enough. In that case, it really needs to get fixed...

>> It also broke the purging API. Also made exempt, but that appears to
>> not solve the problem. Do I need to do something more than add
>> @csrf_exempt to a view functoin to make it not broken? The error
>> message talks about the referrer header - but surely that shouldn't be
>> a requirement when oyu've set @csrf_exempt?
>
> It seems that the problem is the @ssl_required decorator -- it returns
> a new wrapped view without copying over attributes of the original
> view, such as "csrf_exempt". Changing the decorator order won't work
> either because that will confuse PgMiddleware.
>
> I'll send a patch to fix @ssl_required some time soon.

Thanks.


>> We may well have missed more parts :( Clearly neither one of us tested
>> this patch very well.
>
> "It all worked on my computer" ;)

Really? Because the purging form doesn't work on my local machine...
Which does not go through varnish at any point, for example.

Same goes for the purging API endpoint - doesn't work locally either.

So if those work for you locally, then there is definitely something
else afoot..

(The bug form worked fine on my computer, so that one was pretty hard
to catch in testing - but a good way to test it is to just turn off
cookies and see if things that should work when not logged in still
work)


--Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/

On Wed, Nov 7, 2012 at 9:11 PM, Magnus Hagander <magnus@hagander.net> wrote:
>> "It all worked on my computer" ;)
>
> Really? Because the purging form doesn't work on my local machine...
> Which does not go through varnish at any point, for example.

Well I meant that half-jokingly.

I don't have a complete development environment. When I navigate to
that page, I get "ERROR: schema "pgq" does not exist".

With that said, I can't see why these views/forms wouldn't work with
CSRF. They're not doing cross-domain requests or anything. I will need
to drill deeper.

Regards,
Marti

On Wed, Nov 7, 2012 at 8:35 PM, Marti Raudsepp <marti@juffo.org> wrote:
> On Wed, Nov 7, 2012 at 9:11 PM, Magnus Hagander <magnus@hagander.net> wrote:
>>> "It all worked on my computer" ;)
>>
>> Really? Because the purging form doesn't work on my local machine...
>> Which does not go through varnish at any point, for example.
>
> Well I meant that half-jokingly.
>
> I don't have a complete development environment. When I navigate to
> that page, I get "ERROR: schema "pgq" does not exist".

Hmm. That was *supposed* to be handled by varnish_local.sql. But I see
now that it tries to actually look into the table that doesn't exist.
The actual form would work - it's just the listing of what's in the
queue right now that's now broken. That could just be rendered as a
completely empty listing in the case that there is no pgq installed -
that should be an easy fix.


> With that said, I can't see why these views/forms wouldn't work with
> CSRF. They're not doing cross-domain requests or anything. I will need
> to drill deeper.

Me either - it looked fine when reviewing the patch. Just not when
testing it (in production) :)

--Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/