Opened 3 years ago

Closed 3 years ago

Last modified 21 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 (35)

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

comment:3 by Florian Apolloner, 3 years ago

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 by Chris Jerdonek, 3 years ago

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 by Florian Apolloner, 3 years ago

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

comment:6 by Mariusz Felisiak, 3 years ago

Triage Stage: UnreviewedAccepted

comment:7 by Chris Jerdonek, 3 years ago

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 by Chris Jerdonek, 3 years ago

Has patch: set

comment:9 by Chris Jerdonek, 3 years ago

Owner: changed from nobody to Chris Jerdonek
Status: newassigned

comment:10 by Mariusz Felisiak, 3 years ago

Has patch: unset

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

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 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 2523c32d:

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

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

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 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 5e60c394:

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

comment:15 by Chris Jerdonek, 3 years ago

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

Last edited 3 years ago by Chris Jerdonek (previous) (diff)

comment:16 by Chris Jerdonek, 3 years ago

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 by Chris Jerdonek, 3 years ago

Has patch: set

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

In 71323412:

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

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

In 795051b:

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

comment:20 by Mariusz Felisiak, 3 years ago

Has patch: unset

comment:22 by Carlton Gibson <carlton@…>, 3 years ago

In 26d8e3f:

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

comment:23 by Carlton Gibson <carlton@…>, 3 years ago

In f10553ec:

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

comment:24 by Carlton Gibson <carlton@…>, 3 years ago

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 by Carlton Gibson <carlton@…>, 3 years ago

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 by Carlton Gibson <carlton@…>, 3 years ago

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 by Chris Jerdonek, 3 years ago

Has patch: unset

comment:28 by Chris Jerdonek, 3 years ago

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:30 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 0820175:

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

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

In 3f0025c1:

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

comment:32 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

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

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 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 3ff7f6cf:

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

comment:35 by Mariusz Felisiak <felisiak.mariusz@…>, 21 months ago

In e01970e:

Refs #32800 -- Removed CSRF_COOKIE_MASKED transitional setting per deprecation timeline.

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