#32796: Reject requests earlier if the CSRF cookie token has the wrong format
------------------------------------------------+--------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+--------------------------
(This issue is similar to #32795 but for the cookie token rather than for
the non-cookie token.)
I noticed in `CsrfViewMiddleware.process_view()` that if the CSRF cookie
has the wrong format (i.e. wrong length or contains invalid characters),
then the code will do a fair amount of unnecessary work. Specifically, the
code will proceed inside `_get_token()` at
[https://github.com/django/django/blob/d270dd584e0af12fe6229fb712d0704c232dc7e5/django/middleware/csrf.py#L362
this line] to use Python's `secrets` module twice to generate both a new
token and a mask for the token. But this new token will only be used for
the purposes of later calling `_compare_masked_tokens()` in a way that
will be guaranteed to fail (since the cookie being used will be brand new
and so won't match). And then it will call `_compare_masked_tokens()` with
that value.
Instead, if the CSRF cookie is found at that line to have the wrong
format, the middleware could reject the request outright similar to how
#32795 does it if the token has the wrong format. I think this will
simplify `CsrfViewMiddleware` and make it easier to understand because it
will eliminate a number of steps that aren't needed for security. In
particular, one thing this will do is cut down the number of places where
`_get_new_csrf_token()` is called, which will make it clearer where a new
value is really needed / used. Similar to #32795, it will also make
troubleshooting easier because the rejection messages will be more
specific.
I think this could be implemented as follows. After #32795 is merged,
[https://github.com/django/django/blob/d270dd584e0af12fe6229fb712d0704c232dc7e5/django/middleware/csrf.py#L193
_get_token()] could be changed to allow `InvalidTokenFormat` to bubble up
instead of handling it. Then the `InvalidTokenFormat` exception could be
handled differently in the two places `_get_token()()` is called: (1) In
`process_request()`, it could be handled by calling
`_get_new_csrf_token()` (`_get_token()`'s current behavior). (2) In
`process_view()`, it could be handled similar to how #32795 handles it.
Namely, reject the request using the `InvalidTokenFormat`'s reason string.
--
Ticket URL: <https://code.djangoproject.com/ticket/32796>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to django-updates+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/052.b012f0e13fb8a8ceb0985b86083f1a3d%40djangoproject.com.
