[Django] #32796: Reject requests earlier if the CSRF cookie token has the wrong format
Django
------------------------------------------------+--------------------------
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.
Django
Reporter: Chris Jerdonek | Owner: nobody
Cleanup/optimization |
Component: CSRF | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Description changed by Chris Jerdonek:
Old description:
New description:
(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 (as well as similar to
how the code currently handles a ''missing'' cookie in the
[https://github.com/django/django/blob/d270dd584e0af12fe6229fb712d0704c232dc7e5/django/middleware/csrf.py#L363-L367
lines after]). 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 on 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#comment:1>
Django
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* cc: Shai Berger, Florian Apolloner (added)
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/32796#comment:2>
Django
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* owner: nobody => Chris Jerdonek
--
Ticket URL: <https://code.djangoproject.com/ticket/32796#comment:3>
Django
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Chris Jerdonek):
PR: https://github.com/django/django/pull/14471
--
Ticket URL: <https://code.djangoproject.com/ticket/32796#comment:4>
Django
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Chris Jerdonek):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32796#comment:5>
Django
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: CSRF | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/32796#comment:6>
Django
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: CSRF | Version: dev
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak <felisiak.mariusz@…>):
In [changeset:"623cec0879e09e8ad804c213157e51e764f5d826" 623cec08]:
{{{
#!CommitTicketReference repository=""
revision="623cec0879e09e8ad804c213157e51e764f5d826"
Refs #32796 -- Added CsrfViewMiddleware tests for incorrectly formatted
cookie tokens.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32796#comment:7>
Django
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: CSRF | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"cd19db10df6225e01b77685397a88c9cdf216dd1" cd19db1]:
{{{
#!CommitTicketReference repository=""
revision="cd19db10df6225e01b77685397a88c9cdf216dd1"
Fixed #32796 -- Changed CsrfViewMiddleware to fail earlier on badly
formatted cookie tokens.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32796#comment:8>