Opened 3 years ago

Closed 3 years ago

#32796 closed Cleanup/optimization (fixed)

Reject requests earlier if the CSRF cookie token 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 (last modified by Chris Jerdonek)

(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 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 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, _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.

Change History (8)

comment:1 by Chris Jerdonek, 3 years ago

Description: modified (diff)

comment:2 by Mariusz Felisiak, 3 years ago

Cc: Shai Berger Florian Apolloner added
Triage Stage: UnreviewedAccepted

comment:3 by Chris Jerdonek, 3 years ago

Owner: changed from nobody to Chris Jerdonek

comment:5 by Chris Jerdonek, 3 years ago

Has patch: set

comment:6 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

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

In 623cec08:

Refs #32796 -- Added CsrfViewMiddleware tests for incorrectly formatted cookie tokens.

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

Resolution: fixed
Status: assignedclosed

In cd19db1:

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

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