CsrfViewMiddleware unnecessarily masks CSRF cookie

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

[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)?

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

[Florian Apolloner]

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

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

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 2523c32d:

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

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 5e60c394:

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

[Chris Jerdonek]

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

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 71323412:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 795051b:

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

[Chris Jerdonek]

PR: ​https://github.com/django/django/pull/14761

[Carlton Gibson <carlton@…>]

In 26d8e3f:

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

[Carlton Gibson <carlton@…>]

In f10553ec:

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

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

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

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

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

[Chris Jerdonek]

PR: ​https://github.com/django/django/pull/14776

[Mariusz Felisiak <felisiak.mariusz@…>]

In 0820175:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3f0025c1:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

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@…>

[Mariusz Felisiak <felisiak.mariusz@…>]

In 3ff7f6cf:

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

[Mariusz Felisiak <felisiak.mariusz@…>]

In e01970e:

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