Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32902 closed Bug (fixed)

CsrfViewMiddleware.process_response()'s csrf_cookie_needs_reset and csrf_cookie_set logic isn't right

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 the csrf_cookie_needs_reset and csrf_cookie_set logic inside CsrfViewMiddleware.process_response() isn't right: https://github.com/django/django/blob/fa35c8bdbc6aca65d94d6280fa463d5bc7baa5c0/django/middleware/csrf.py#L439-L451

Consequently--

  1. self._set_token(request, response) can get called twice in some circumstances, even if response.csrf_cookie_set is true at the beginning, and
  2. the cookie can fail to be reset in some circumstances, even if csrf_cookie_needs_reset is true at the beginning.

(I previously let security@djangoproject.com know about this issue, and they said it was okay to resolve this publicly.)

Change History (10)

comment:1 by Chris Jerdonek, 3 years ago

I believe the fix should look something like this:

diff --git a/django/middleware/csrf.py b/django/middleware/csrf.py
index c2a9470ab1..bf97e50146 100644
--- a/django/middleware/csrf.py
+++ b/django/middleware/csrf.py
@@ -437,15 +437,14 @@ class CsrfViewMiddleware(MiddlewareMixin):
         return self._accept(request)
 
     def process_response(self, request, response):
-        if not getattr(request, 'csrf_cookie_needs_reset', False):
-            if getattr(response, 'csrf_cookie_set', False):
-                return response
-
-        if not request.META.get("CSRF_COOKIE_USED", False):
+        # Prevent the cookie from being set twice.
+        if getattr(response, 'csrf_cookie_set', False):
             return response
+        if (getattr(request, 'csrf_cookie_needs_reset', False) or
+                request.META.get("CSRF_COOKIE_USED")):
+            # Set the CSRF cookie even if it's already set so that the
+            # expiry timer gets renewed.
+            self._set_token(request, response)
+            response.csrf_cookie_set = True
 
-        # Set the CSRF cookie even if it's already set, so we renew
-        # the expiry timer.
-        self._set_token(request, response)
-        response.csrf_cookie_set = True
         return response
Version 0, edited 3 years ago by Chris Jerdonek (next)

comment:2 by Chris Jerdonek, 3 years ago

The code under question was originally added 5 years in this large-ish commit here: https://github.com/django/django/commit/5112e65ef2df1dbb95ff83026b6a962fb2688661

Indeed, part of my proposed fix involves restoring the initial csrf_cookie_set lines prior to that commit (csrf_cookie_set was called csrf_processing_done before this commit).

comment:3 by Mariusz Felisiak, 3 years ago

Cc: Shai Berger Florian Apolloner added
Triage Stage: UnreviewedAccepted

comment:5 by Shai Berger, 3 years ago

Triage Stage: AcceptedReady for checkin

Would appreciate at least one more pair of eyes on this, but LGTM.

comment:6 by Mariusz Felisiak, 3 years ago

Triage Stage: Ready for checkinAccepted

comment:7 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

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

In 311401d9:

Refs #32902 -- Added CSRF test when rotate_token() is called between resetting the token and processing response.

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

Resolution: fixed
Status: assignedclosed

In a2e1f1e2:

Fixed #32902 -- Fixed CsrfViewMiddleware.process_response()'s cookie reset logic.

Thanks Florian Apolloner and Shai Berger for reviews.

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

In 3cfcb8cb:

Refs #32902 -- Moved ensure_csrf_cookie_view after protected_view.

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