Opened 3 years ago

Closed 3 years ago

#32795 closed Cleanup/optimization (fixed)

Reject requests earlier if the non-cookie CSRF token is missing or has the wrong format

Reported by: Chris Jerdonek Owned by: Chris Jerdonek
Component: CSRF Version: dev
Severity: Normal Keywords:
Cc: Shai Berger, Florian Apolloner Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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 these lines:

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.)

Change History (9)

comment:1 by Chris Jerdonek, 3 years ago

One way to implement this would be to change _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().

Version 0, edited 3 years ago by Chris Jerdonek (next)

comment:2 by Mariusz Felisiak, 3 years ago

Cc: Shai Berger Florian Apolloner added
Triage Stage: UnreviewedAccepted

comment:3 by Florian Apolloner, 3 years ago

The proposed solution sounds good to me!

comment:4 by Chris Jerdonek, 3 years ago

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:6 by Chris Jerdonek, 3 years ago

Summary: Reject requests earlier if the CSRF token is missing or has the wrong formatReject requests earlier if the non-cookie CSRF token is missing or has the wrong format

comment:7 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In ffdee8d2:

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.

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 55775891:

Fixed #32795 -- Changed CsrfViewMiddleware to fail earlier on badly formatted tokens.

Note: See TracTickets for help on using tickets.
Back to Top