Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Django REST Framework does not check CSRF token on endpoints not requiring authentication #7389

Closed
jamalex opened this issue Jul 25, 2020 · 5 comments · Fixed by #11978
Closed
Assignees
Labels
bug Behavior is wrong or broken DEV: backend Python, databases, networking, filesystem... P1 - important Priority: High impact on UX

Comments

@jamalex
Copy link
Member

jamalex commented Jul 25, 2020

Observed behavior

It appears that DRF accomplishes its CSRF-checking as part of the SessionAuthentication authentication class:
https://stackoverflow.com/questions/49275069/csrf-is-only-checked-when-authenticated-in-drf

This means that endpoints that allow anonymous POST requests aren't CSRF-protected. This generally isn't a huge deal, since the point of CSRF protection is to prevent someone from making you do an action as a particular user. But:

  • There might be non-session cookies set that allow you to do something you wouldn't otherwise do (e.g. our "app context" cookie)
  • In principle this could make it easier to conduct a more distributed brute-force attack (while circumventing protections like rate-limiting).
  • There's no reason for us to allow this, for our standard endpoints; in our use cases, we can always set the CSRF header on POST (or other "dangerous method") requests.

Expected behavior

On all endpoints that are intended to be accessed solely from the browser, we should/could be more strongly enforcing CSRF protections.

User-facing consequences

The main (only?) place this is likely to come into play at the moment is the login endpoint. Worst impact is that by visiting a 3rd party site, it could trigger your browser to log into a Kolibri site as an arbitrary user (for which it knows the credentials). If you then visited the Kolibri site, before the session timed out, any activity would be logged to the attacker's account and could later be inspected.

Given our short session timeouts, the hoops to jump through to accomplish this, and the moderately low impact (doesn't give access to user's account, just temporarily logs them in as another user), this doesn't seem like a blocker for 0.14, but would be good to do a broader audit and address in the future. We'll need to be aware that there could be unknown 3rd-party integrations with Kolibri that make use of our login endpoint and don't bother setting CSRF headers (since it works without), so this could also require some of those integrations. But given the login endpoint isn't a "public" endpoint, we haven't committed to forwards-compatibility.

Steps to reproduce

Try:

curl 'http://127.0.0.1:8080/api/auth/session/' -H 'Content-Type: application/json;charset=UTF-8' --data-binary '{"username":"myuser","password":"mypass"}'

and you should see a successfully created session's JSON blob.

@indirectlylit indirectlylit added the P1 - important Priority: High impact on UX label Aug 4, 2020
@indirectlylit indirectlylit added this to the upcoming patch milestone Aug 4, 2020
@indirectlylit indirectlylit added DEV: backend Python, databases, networking, filesystem... bug Behavior is wrong or broken labels Aug 4, 2020
@YuvrajDoshi01
Copy link

Hey if the issue still persists, can I contribute to it? I have made some changes in the core/auth/middleware file which would not allow non-authenticated users to send POST requests.

@rtibbles
Copy link
Member

I have made some changes in the core/auth/middleware file which would not allow non-authenticated users to send POST requests.

This is not the correct solution to this - as pointed out in the issue, our login endpoint needs a POST request, so stopping unauthenticated users from making POST requests would prevent anyone from ever logging in.

What we need to do instead is to enforce that CSRF checks are carried out for POST requests on the login endpoint.

@YuvrajDoshi01
Copy link

okay, i get it. so we need to make sure post requests for non-authenticated users are only allowed on the login page and otherwise restricted?

@thesujai
Copy link
Contributor

I am working on this, may I be assigned to this issue?

@rtibbles
Copy link
Member

Sure thing, assigned!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Behavior is wrong or broken DEV: backend Python, databases, networking, filesystem... P1 - important Priority: High impact on UX
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants