CsrfMiddleware false positive after session.flush()

[Glenn Maynard]

If a user loads a form, logs in in another window, and then submits the form, CSRF triggers. The same happens if you're on a page with a form, click a login button, and then browse back to the form and submit it. The same also happens if the user logs out and then logs in as a different user, and submits an old form.

This happens because contrib.auth.login and logout reset the session, which changes the CSRF security token.

(Is submitting a form in this situation a good idea? That's up to the site; CsrfMiddleware should not cause forms to fail in non-CSRF situations.)

CSRFMiddleware should set its own random cookie, independent of the session cookie, and leave it there indefinitely; the CSRF cookie in a form will always remain valid, regardless of the session. (This will also have the side-effect of making CSRF not depend on sessions, which doesn't hurt.)

I can implement this, but I'll wait for feedback first.

[simon]

I like the side-effect of CSRF not depending on sessions.

[Luke Plant]

If the CSRF cookie is always valid and independent of session, how do we know that a request isn't CSRF? Any request will include the cookie, whether cross site or same site in origin, so as soon as a person has the CSRF cookie, any cross site attack their browser makes will then succeed. What am I missing?

[Glenn Maynard]

It's exactly the same as how it works with the session cookie, except it uses a different cookie with a different random value. A remote site won't have access to the user's cookie in order to fill in the CSRF form token.

[simon]

Provided the value of the cookie (or a hash based on the value of the cookie) is included in a hidden form field the should be just as secure as something based on sessions.

[Luke Plant]

Ah ok, you are still using a hidden form field, I get it. (I was probably asleep or something when making my last comment...)

Before you come up with a patch, note that this won't make it for 1.1, and post 1.1 ticket #9977 is the proposed method. (At least I think it still is, the last I heard Jacob didn't like it, but I think his objections were misfounded -- see ​http://groups.google.com/group/django-developers/browse_thread/thread/c23f556b88cedbc7?pli=1 ). I think the implication of this is that the new CsrfViewMiddleware will actually have a process_response() method that adds a persistent CSRF cookie whenever it is not present.

An issue to think about: Before, the middleware skipped the check if there was no session cookie, which will usually be the case if the user is not logged in, since there is no danger in that situation. In this system, what happens if there is no CSRF cookie? One presumes that no CSRF cookie means:

1. The user hasn't visited the site before
2. Or the user visited the site before the new CSRF middleware was enabled - so they are authenticated but don't have the CSRF cookie.
3. Or the user has manually deleted the CSRF cookie.

If (1), we have no problem -- no possibility of CSRF attack (except login CSRF perhaps?). But the others are more problematic - even though they are corner cases, we probably should be protecting against CSRF in this situation, which means that we have to require the CSRF cookie and corresponding token for all POST requests (apart from those exempted using @csrf_view_exempt of course). I don't know if that has implications, I'm just thinking out loud...

More out loud thinking: One slight issue with this method is that users who don't automatically allow cookies will be prompted to accept a cookie as soon as they come to a site, whether they will ever use a POST form or not.

BTW, if you are going to come up with a patch, please remember that the most valuable and important thing is the tests - in fact, a patch consisting of just tests and no implementation would be preferred over the inverse :-) It's usually much better to write the tests first anyway...

[Glenn Maynard]

I've already implemented this (with tests and doc updates), since I keep hitting these false positives in development and I foresee it being real problem in production.

#2: When there is no CSRF cookie, the session cookie is accepted as the CSRF cookie. This should happen only once, the first time a user visits the site after an upgrade. This can be removed eventually.

#3: I don't think this is a problem. If you're manually deleting security cookies, you lose security; we can't protect the user against his deliberately breaking CSRF verification for himself. (If he deletes *all* cookies for a site, this isn't a problem--he'll also delete the session cookie, and goes back to #1.)

One slight issue with this method is that users who don't automatically allow cookies will be prompted to accept a cookie as soon as they come to a site, whether they will ever use a POST form or not.

"Don't set cookies until you log in" isn't a reasonable user expectation. If you turn cookie prompting on, you'll be inundated on almost every site you visit. The session cookie is always being set, too.

For backwards-compatibility, the cookie name is derived from settings.SESSION_COOKIE_NAME, and SESSION_COOKIE_DOMAIN and SESSION_COOKIE_PATH are used. These could be broken out into separate settings, but it'd just be options bloat unless anyone has a real need for it. (Having generic COOKIE_DOMAIN and COOKIE_PATH settings that default for *all* cookies would be useful, though.) The cookie expiry is arbitrarily one year.

I think the patch in #9977 is doing too much at once. I havn't merged any of that, but I've arranged things so it's possible to generate the token before the CsrfResponseMiddleware is run, and exposed get_csrf_token(request) to do so. This should make adding a template tag for it much easier.

[Glenn Maynard]

missed documentation

[Glenn Maynard]

Add a prefix when using SECRET_KEY (see ​http://code.djangoproject.com/ticket/9977#comment:14)

[Luke Plant]

Thanks for that Glenn, looks very good at a first glance.

Ticket #9977 is really doing just one thing - replacing the post-processing with a template tag. There are edge cases and backwards compatibility issues and so-on that it deals with, and some small refactoring in the tests to support it, but I don't think it is doing too much at once. Oh yeah, It also adds a mechanism for a custom view to be called if the mechanism is triggered.

My vote is to merge this patch with #9977 after Django 1.1 is out. Post-processing the response is really nasty, and hardly any of the core devs are happy with the middleware being enabled by default if it is still doing post-processing. Although this complicates the docs a little bit, I think it is much better than changing the middleware twice.

I'm happy to do that merge (which is a bit tricky, and will also involve moving the cookie setting code to a process_response() method in CsrfViewMiddleware ).

[Glenn Maynard]

fix session cookie BC and revise test to catch this; fix exception when the view middleware is skipped

[Luke Plant]

I've attached a patch to #9977 that merges this patch, because I think that is the way forward, and these tickets overlap too much and in too many subtle ways to work on them separately.

[Chris Beaven]

I'm closing this as a dupe of #9977 then (with a note there to reopen this if that design decision fails)