Add CSRF_COOKIE to prevent csrf when using JWT_AUTH_COOKIE

[bmpenuelas]

To prevent Cross-Site Request Forgery when using JWT_AUTH_COOKIE, the csrftoken cookie will also be set when issuing the JWT authentication token.

[codecov]

Codecov Report

Merging #434 into master will decrease coverage by 0.53%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #434      +/-   ##
==========================================
- Coverage   90.67%   90.13%   -0.54%     
==========================================
  Files          14       12       -2     
  Lines         847      821      -26     
  Branches       29       30       +1     
==========================================
- Hits          768      740      -28     
- Misses         66       67       +1     
- Partials       13       14       +1

Flag	Coverage Δ
#codecov	90.13% <33.33%> (-0.54%)	⬇️
#dj110	86.84% <33.33%> (-0.64%)	⬇️
#dj111	86.84% <33.33%> (-0.64%)	⬇️
#dj18	89.28% <33.33%> (-0.57%)	⬇️
#dj19	89.28% <33.33%> (-0.57%)	⬇️
#drf31	89.28% <33.33%> (-0.57%)	⬇️
#drf32	89.28% <33.33%> (-0.57%)	⬇️
#drf33	89.28% <33.33%> (-0.57%)	⬇️
#drf34	90.13% <33.33%> (-0.54%)	⬇️
#drf35	89.76% <33.33%> (-0.56%)	⬇️
#drf36	89.76% <33.33%> (-0.56%)	⬇️
#py27	90.13% <33.33%> (-0.54%)	⬇️
#py33	88.91% <33.33%> (-0.58%)	⬇️
#py34	89.76% <33.33%> (+0.27%)	⬆️
#py35	86.84% <33.33%> (?)
#py36	86.84% <33.33%> (?)

Impacted Files	Coverage Δ
rest_framework_jwt/settings.py	100% <ø> (ø)	⬆️
rest_framework_jwt/views.py	88.37% <33.33%> (-4.13%)	⬇️
rest_framework_jwt/utils.py
rest_framework_jwt/models.py

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a0bd40...c64a947. Read the comment docs.

[ghost]

I think this changes are not enough for CSRF protection when JWT_AUTH_COOKIE option is enabled. Your patch adds csrf cookie, but looks like it's value will be never checked, because django-rest-framework explicitly wraps every view in csrf_exempt().

https://github.com/encode/django-rest-framework/blob/master/rest_framework/views.py#L134

I believe that BaseJSONWebTokenAuthentication authentication class should enforce csrf validation when needed. Like in rest_framework.authentication.SessionAuthentication, which explicitly call CSRFCheck?

[bmpenuelas]

Hi @yurikulaz, actually, with this patch the csrf value will be checked for every method that you explicitly declare you want checked.

For example, you can try with @method_decorator(csrf_protect)

Nevertheless, enforcing the csrf check by default and declaring exemptions explicitly is the safer way to code. I'll definitely look into adding that too.

[jheld]

@bmpenuelas Thanks for writing this patch! Is this PR a comprehensive fix (with using the csrf decorator)? Is there any reason why I shouldn't make a custom .whl for this before merge? Is there a timeline for merge?

[bmpenuelas]

Yes @jheld , using the csrf decorator, this fix alone can provide the desired protection.

[PaulDFPV]

Does this protect against login csrf?

[bmpenuelas]

@paul110590 CSRF implies that the attack is executed from another domain, if you want to know whether if the user login is protected then yes, the malicious domain cannot read the CSRF token which belongs to your domain, and thus, the request will fail.

[PaulDFPV]

Hi @bmpenuelas , what I mean is the CSRF token is not set until the JWT is issued, i.e. after the user has logged in. Therefore an attacker could log in via CSRF and the user would think they were logged in to their own account. The user would then unwittingly be performing actions on the attacker's account. Or is there some protection against this that I am missing?

[bmpenuelas]

Protection is generally geared toward avoiding forging user requests. That protection is provided by the Anti-CSRF token and the JWT token working together.

In the particular case you mention, at no time is the user session compromised. It's not a threat like user-impersonating CSRF, but it's true it can have privacy implications. I don't want to go off-topic here since this isn't related to the pull-request itself, but CSRFToken can protect your application from this situation too. For example, you can issue an Anti-CSRF token for the login process, and store it as both: a hidden form value, and a cookie. To validate the login request, both values must match, and since an attacker can't read them both, the login will fail.

[PaulDFPV]

It's a different type of csrf to the more common session riding, but it's still a security flaw we need to protect against.

This is where I think it has relevancy to the pull request: To protect against login-csrf we need to decorate our login view with csrf_protect. We then need to create a view that will set the csrftoken cookie, so that we can fetch it prior to login and our login request is not blocked. This being the case, we no longer need to set the csrftoken cookie when we issue the JWT, because it is already set.

Basically, if we follow the approach of protecting ourselves from login-csrf we will also automatically be protecting ourselves from 'normal' csrf (provided we guard all unsafe views with csrf_protect). Whereas if we follow the approach of protecting from 'normal' csrf, we are still leaving ourselves open to login-csrf.

[bmpenuelas]

Yes, this PR already does that. CSRF tokens are issued for every request, not only after login, so you can use them for login, or to protect other requests, the same principles apply.

The problem before this PR was that when you used the JWT token for authentication, the CSRF token was ignored.

[jayvdb]

just a consideration as I saw this referred to at Styria-Digital#4 , maybe it is appropriate to only import this if api_settings.CSRF_COOKIE is enabled, e.g. as a delayed import within the if below.

Then any site not using csrf middleware doesnt get hit with an extra module being import that isnt used. Are there many sites doing that?

Also pep8 tools would complain bitterly about that.

[ghost]

Any ideas when this PR is going to be merged into the package? Does it need work, can I help in any way getting this out the door?

[jayvdb]

@pc-trent offer sacrifices to @gcollazo and @jpadilla . c.f. #449

[ghost]

@pc-trent offer sacrifices to @gcollazo and @jpadilla . c.f. #449

I'll take that as a "this project is dead"

[jheld]

We should hope not. Maybe we can ask for maintainer privileges? Including PyPI. We've been waiting for this fix for a year.