[Django] #32795: Reject requests earlier if the CSRF token is missing or has the wrong format
Django
format
------------------------------------------------+------------------------
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
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 |
------------------------------------------------+------------------------
I noticed that `CsrfViewMiddleware.process_view()` does what seems like
unnecessary work when the non-cookie CSRF token is either missing or has
the wrong format (i.e. has the wrong length or contains characters that
aren't allowed). Specifically, in
[https://github.com/django/django/blob/b746596f5f0e1fcac791b0f7c8bfc3d69dfef2ff/django/middleware/csrf.py#L385-L386
these lines]:
{{{#!python
if request_csrf_token == '':
# Fall back to X-CSRFToken, to make things easier for AJAX, and
# possible for PUT/DELETE.
request_csrf_token = request.META.get(settings.CSRF_HEADER_NAME, '')
request_csrf_token = _sanitize_token(request_csrf_token)
if not _compare_masked_tokens(request_csrf_token, csrf_token):
return self._reject(request, REASON_BAD_TOKEN)
}}}
if the `request_csrf_token` is missing or has the wrong format, the code
will proceed inside `_sanitize_token ()` to use Python's `secrets` module
twice to generate both a new token and a mask for the token, but only for
the purposes of calling `_compare_masked_tokens()` in a way that will be
guaranteed to fail (since the token being passed will be brand new). And
then it will call `_compare_masked_tokens()` with that value.
However, if the non-cookie token is missing or has the wrong format, it
seems like the request can be rejected at that point outright without
needing to do the work above. It doesn't seem like rejecting the request
outright will reveal any sensitive information since the correct token
length and allowed characters aren't secret information. (Django's
security model assumes that information is publicly known.)
Another advantage of rejecting earlier is that the failure message can be
more specific. Namely, instead of just using `REASON_BAD_TOKEN` ("CSRF
token missing or incorrect"), more specific messages can be used like
"CSRF token missing," "CSRF token has wrong length," and "CSRF token
contains invalid characters." That could be useful in troubleshooting CSRF
issues, which can sometimes be hard to troubleshoot.
A third advantage is that this will make the code easier to understand.
This is because currently, it's hard to tell whether calling
`_sanitize_token()` and `_compare_masked_tokens()` are actually needed for
security reasons even when the CSRF token is missing or has the wrong
format. (There currently aren't any comments explaining why it's needed if
in fact it is.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32795>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
format
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
Comment (by Chris Jerdonek):
One way to implement this would be to change
[https://github.com/django/django/blob/b746596f5f0e1fcac791b0f7c8bfc3d69dfef2ff/django/middleware/csrf.py#L109-L123
_sanitize_token()] to raise a new internal `InvalidTokenFormat` exception
with an appropriate reason string if the token has the wrong length or
contains invalid characters, instead of calling `_get_new_csrf_token()`.
Then, the two places that call `_sanitize_token()` can handle the
exception differently: (1) In `process_view()`, the request could be
rejected using the exception's message. This is similar to how
`process_view()` now handles `RejectRequest` exceptions raised by
`_check_referer()`. (2) In `_get_token()`, the exception could be handled
by calling `_get_new_csrf_token()`.
--
Ticket URL: <https://code.djangoproject.com/ticket/32795#comment:1>
Django
format
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
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/32795#comment:2>
Django
format
Reporter: Chris Jerdonek | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: CSRF | Version: dev
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Florian Apolloner):
The proposed solution sounds good to me!
--
Ticket URL: <https://code.djangoproject.com/ticket/32795#comment:3>
Django
format
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Component: CSRF | Version: dev
Severity: Normal | Resolution:
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
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/32795#comment:4>
Django
format
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
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
Comment:
PR: https://github.com/django/django/pull/14464
--
Ticket URL: <https://code.djangoproject.com/ticket/32795#comment:5>
Django
wrong format
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32795#comment:6>
Django
wrong format
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
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/32795#comment:7>
Django
wrong format
-------------------------------------+-------------------------------------
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:"55775891fbfd8679b75336aa2f15ff9190e9f7a8" 55775891]:
{{{
#!CommitTicketReference repository=""
revision="55775891fbfd8679b75336aa2f15ff9190e9f7a8"
Fixed #32795 -- Changed CsrfViewMiddleware to fail earlier on badly
formatted tokens.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32795#comment:9>
Django
wrong format
-------------------------------------+-------------------------------------
Reporter: Chris Jerdonek | Owner: Chris
Type: | Jerdonek
Component: CSRF | Version: dev
Severity: Normal | Resolution:
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:"ffdee8d2645227748ae4061f21fc48cca4d75c22" ffdee8d2]:
{{{
#!CommitTicketReference repository=""
revision="ffdee8d2645227748ae4061f21fc48cca4d75c22"
Refs #32795 -- Added CsrfViewMiddleware tests for rejecting invalid or
missing tokens.
This also improves test names for test_process_request_no_csrf_cookie
and test_process_request_csrf_cookie_no_token. The logic being tested
is actually in process_view() rather than process_request(), and it's
not necessary to include the method name.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/32795#comment:8>