Opened 19 months ago

Closed 12 months ago

Last modified 12 months ago

#32800 closed Cleanup/optimization (fixed)

CsrfViewMiddleware unnecessarily masks CSRF cookie

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)

I noticed that CsrfViewMiddleware unnecessarily masks the CSRF cookie. See, for example:
https://github.com/django/django/blob/d270dd584e0af12fe6229fb712d0704c232dc7e5/django/middleware/csrf.py#L91

My understanding of the BREACH attack is that the vulnerability comes from not masking the CSRF token in the response body (e.g. what is included in the HTML). Masking the cookie itself doesn't help with this. (Django also doesn't change the mask of the cookie with each request, so the mask wouldn't help in this regard anyways.)

Some advantages of not masking the cookie are: It would simplify the code in CsrfViewMiddleware because it would remove some complexity and operations that aren't needed for security. Currently, masking the CSRF cookie is a red herring for someone wanting to understand the various security features. Also, not masking the cookie would make requests and responses smaller when CSRF_USE_SESSIONS is false or when true and cookie-based sessions are used. This is because masking doubles the length of the string.

This can be changed fairly easily while (1) continuing to respect masked cookie values, and (2) not forcing session stores to update their cookie in its unmasked form if they are currently storing it masked.

Change History (34)

comment:1 Changed 19 months ago by Chris Jerdonek

Description: modified (diff)

comment:2 Changed 19 months ago by Mariusz Felisiak

Cc: Shai Berger Florian Apolloner added

comment:3 Changed 19 months ago by Florian Apolloner

I am a bit worried about backwards compat. What happens if JS reads the short unmasked secret from the Cookie and then sends that to the server (as opposed to reading it from the html)?

comment:4 Changed 19 months ago by Chris Jerdonek

Thanks for your comment! I'm pretty sure that scenario is already handled by the current code (I'm just not 100% sure if there are tests for it). For example, you can see in the code that _sanitize_token() is called on the (non-cookie) CSRF token, and _sanitize_token() has special logic to mask the cookie before returning it if it was passed the short version of length CSRF_SECRET_LENGTH. (However, my preference is to put that logic in _compare_masked_tokens() so that we're not relying on any assumptions on the arguments passed to _compare_masked_tokens() like we currently are.). Note also that the code currently has backwards-compat logic to accept cookies that aren't masked (via the same call to _sanitize_token().)

FWIW, I did prepare a patch privately and saw no problems, and on the whole it seems slightly simpler. I even have a comment addressing the point you make:

# Fall back to X-CSRFToken, to make things easier for AJAX, and
# possible for PUT/DELETE.
try:
    # This can have length CSRF_SECRET_LENGTH or CSRF_TOKEN_LENGTH,
    # depending on whether the client obtained the token from
    # the DOM or the cookie (and whether the cookie was masked
    # or unmasked).
    request_csrf_token = request.META[settings.CSRF_HEADER_NAME]
except KeyError:
    return self._reject(request, REASON_CSRF_TOKEN_MISSING)

comment:5 Changed 19 months ago by Florian Apolloner

Ok, then lets make sure we have that situation tested as well and I think we can go ahead with it.

comment:6 Changed 19 months ago by Mariusz Felisiak

Triage Stage: UnreviewedAccepted

comment:7 Changed 18 months ago by Chris Jerdonek

Ok, then lets make sure we have that situation tested as well and I think we can go ahead with it.

I posted a preliminary PR for the purpose only of adding additional tests: https://github.com/django/django/pull/14485

The PR includes tests of all combinations of masked and unmasked secrets, and masked and unmasked tokens (both via POST and the X-CSRFToken header). See test_masked_unmasked_combinations() in both CsrfViewMiddlewareTests and CsrfViewMiddlewareUseSessionsTests.

comment:8 Changed 18 months ago by Chris Jerdonek

Has patch: set

comment:9 Changed 18 months ago by Chris Jerdonek

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:10 Changed 18 months ago by Mariusz Felisiak

Has patch: unset

comment:11 Changed 18 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In c8108591:

Refs #32800 -- Added to csrf_tests/tests.py the unmasked version of the secret.

This also adds tests that the secret is correct, and updates existing
tests to use the value.

comment:12 Changed 18 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 2523c32d:

Refs #32800 -- Eliminated the need for separate _get_POST_bare_secret() methods.

comment:13 Changed 18 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In defa8d3:

Refs #32800 -- Made CsrfViewMiddlewareTestMixin._csrf_id_cookie and _csrf_id_token different.

This also renames CsrfViewMiddlewareTestMixin._csrf_id to _csrf_id_token.

comment:14 Changed 18 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 5e60c394:

Refs #32800 -- Added CsrfViewMiddleware tests for all combinations of masked/unmasked cookies and tokens.

comment:15 Changed 17 months ago by Chris Jerdonek

I'm going to resolve #32902 #32916 before fixing the remainder of this ticket.

Last edited 17 months ago by Chris Jerdonek (previous) (diff)

comment:16 Changed 16 months ago by Chris Jerdonek

PR: https://github.com/django/django/pull/14723

This PR adds more tests prior to the main code change. It also gives one function a better name, and in particular a name that will fit better following the change.

comment:17 Changed 16 months ago by Chris Jerdonek

Has patch: set

comment:18 Changed 16 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 71323412:

Refs #32800 -- Renamed _compare_masked_tokens() to _does_token_match().

comment:19 Changed 16 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 795051b:

Refs #32800 -- Added tests of more CSRF functions.

comment:20 Changed 16 months ago by Mariusz Felisiak

Has patch: unset

comment:21 Changed 16 months ago by Chris Jerdonek

Has patch: set

comment:22 Changed 16 months ago by Carlton Gibson <carlton@…>

In 26d8e3f:

Refs #32800 -- Used the cookie argument to CsrfViewMiddlewareTestMixin._get_request() in more tests.

comment:23 Changed 16 months ago by Carlton Gibson <carlton@…>

In f10553ec:

Refs #32800 -- Renamed _set_token() to _set_csrf_cookie().

comment:24 Changed 16 months ago by Carlton Gibson <carlton@…>

In 7aba820a:

Refs #32800 -- Improved CsrfViewMiddlewareTestMixin._check_token_present().

This changes CsrfViewMiddlewareTestMixin._check_token_present() to give more
detailed information if the check fails, and in particular why it failed. It
also moves CsrfFunctionTests.assertMaskedSecretCorrect() to a separate
CsrfFunctionTestMixin so the helper can be used in CsrfViewMiddlewareTestMixin.

comment:25 Changed 16 months ago by Carlton Gibson <carlton@…>

In be1fd66:

Refs #32800 -- Added test_masked_secret_accepted_and_not_replaced().

This improves test_bare_secret_accepted_and_replaced() by adding a stronger
assertion. It also adds a parallel test for the non-bare (masked) case.

comment:26 Changed 16 months ago by Carlton Gibson <carlton@…>

In 231de68:

Refs #32800 -- Added _add_new_csrf_cookie() helper function.

This centralizes the logic to use when setting a new cookie. It also
eliminates the need for the _get_new_csrf_token() function, which is now
removed.

comment:27 Changed 16 months ago by Chris Jerdonek

Has patch: unset

comment:28 Changed 16 months ago by Chris Jerdonek

I posted a draft PR, FYI, just to get something up more quickly: https://github.com/django/django/pull/14776

I'll mark "has patch" when ready for review.

comment:29 Changed 16 months ago by Chris Jerdonek

Has patch: set

comment:30 Changed 13 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 0820175:

Refs #32800 -- Added CSRF tests for masked and unmasked secrets during GET.

comment:31 Changed 13 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 3f0025c1:

Refs #32800 -- Avoided use of _does_token_match() in some CSRF tests.

comment:32 Changed 13 months ago by Mariusz Felisiak

Triage Stage: AcceptedReady for checkin

comment:33 Changed 12 months ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 5d80843e:

Fixed #32800 -- Changed CsrfViewMiddleware not to mask the CSRF secret.

This also adds CSRF_COOKIE_MASKED transitional setting helpful in
migrating multiple instance of the same project to Django 4.1+.

Thanks Florian Apolloner and Shai Berger for reviews.

Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@…>

comment:34 Changed 12 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 3ff7f6cf:

Refs #32800 -- Renamed _sanitize_token() to _check_token_format().

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