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

The implementation probably defeats the purpose of CSRF protection #49

Open
sha256 opened this issue Jul 17, 2022 · 3 comments
Open

The implementation probably defeats the purpose of CSRF protection #49

sha256 opened this issue Jul 17, 2022 · 3 comments
Labels
bug Something isn't working
Milestone

Comments

@sha256
Copy link

sha256 commented Jul 17, 2022

In the server side, if you read the CSRF token value from cookie and do the validation, I don't think it protects you from CSRF attacks.

Let's say, on attacker's website, they have a form like this.

<!--- A form on https://attackers-site.com -->
<form action="https://yoursite.com/api/protected" method="POST">
    ....
</form>

When this form is submitted, the browser will send the actual cookies of yoursite.com (including the CSRF related cookies) with the request. The server will always consider it valid because it's sending the previously set CSRF (valid) cookie.

I recommend the following changes:

  • Not validating CSRF token from cookie, instead provide a way to pass the token to the client.
  • Client side can can decide how to send the token along with a POST request and server should handle it accordingly. For example, <form> can send it as hidden input. Ajax requests can send it in the header.
  • Making the corresponding cookie HttpOnly should be optional. This way client can access it using JS and add it to the headers of Ajax requests.
@divinity76
Copy link

image

@alec-bfa
Copy link

Couldn't you set 'sameSite' as 'Strict' on the cookie, meaning the cookie would not be attached?

@juanbzpy
Copy link
Owner

juanbzpy commented Mar 29, 2023

@sha256 you are right.

As an additional security layer, if the user provides a secret, the token is HMAC signed, but only If a secret is provided.

The changes you suggest align with the latest OWASP CSRF cheatsheet recommendation. I was following something closer to Understanding CSRF, but for v1, I'm moving entirely to OWASP's.

Thanks for taking the time to submit the issue, and apologies for the late reply.

@juanbzpy juanbzpy added the bug Something isn't working label Mar 29, 2023
@juanbzpy juanbzpy added this to the v1 milestone Mar 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants