Skip to content
This repository has been archived by the owner on Dec 19, 2023. It is now read-only.

Fix Post based CSRF #1

Merged
merged 2 commits into from
Sep 4, 2020
Merged

Fix Post based CSRF #1

merged 2 commits into from
Sep 4, 2020

Conversation

alromh87
Copy link

@alromh87 alromh87 commented Aug 31, 2020 •
edited
Loading

📊 Metadata *

Bounty URL: https://www.huntr.dev/bounties/1-npm-lets-chat/

⚙️ Description *

Fix CSRF by requiring csrf token for authenticated post routes

💻 Technical Description *

CSRF Token is created during loggin and stored in session, then the token is sent and validated during authorized POST request, if correct token is not provided reuqest is denied.

🐛 Proof of Concept (PoC) *

Install the chat
Create a new user and login
Create a malicious file containing the following CSRF PoC:

<html>
  <body>
  <script>history.pushState('', '', '/')</script>
    <form action="http://localtest.me:5000/account/profile" method="POST">
      <input type="hidden" name="display&#45;name" value="HACKED" />
      <input type="hidden" name="first&#45;name" value="HACKED;" />
      <input type="hidden" name="last&#45;name" value="HACKED" />
      <input type="submit" value="Submit request" />
    </form>
  </body>
</html>

Victim opens the crafted file) and it's name/display name are changed:

Captura de pantalla de 2020-08-31 23-31-47

Captura de pantalla de 2020-08-31 23-32-30

POC for regenerating keys:

<html>
  <body>
  <script>history.pushState('', '', '/')</script>
    <form action="http://localtest.me:5000/account/token/generate" method="POST">
      <input type="submit" value="Submit request" />
    </form>
  </body>
</html>

Opening this the api key created before will be deleted and another one created (invalidates previously connected applications)
Captura de pantalla de 2020-08-31 23-32-15

🔥 Proof of Fix (PoF) *

Request are now flagged as Unauthorized

Captura de pantalla de 2020-08-31 23-30-29
Captura de pantalla de 2020-08-31 23-30-14

👍 User Acceptance Testing (UAT)

Application continue working normally:

Captura de pantalla de 2020-08-31 23-41-35

Mik317
Mik317 approved these changes Sep 1, 2020
View reviewed changes
Copy link

@Mik317 Mik317 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 😄 🎉

Cheers,
Mik

@ghost
Copy link

ghost commented Sep 4, 2020

I'm curious why we aren't using a library for CSRF tokens? Since that way all of the token generation is covered (edge cases like collisions etc). Could use csurf or just plain csrf

@ghost ghost mentioned this pull request Sep 4, 2020
Open
@alromh87
Copy link
Author

alromh87 commented Sep 4, 2020

Tried csurf but it wasn't validating the token

@ghost
Copy link

ghost commented Sep 4, 2020

Any luck with csrf?

@alromh87
Copy link
Author

alromh87 commented Sep 4, 2020

Done 😉

Thanks I was looking for something like this

@ghost
Copy link

ghost commented Sep 4, 2020

Nice one thanks! Looks good to me - just waiting for a sheriff to check and we'll merge it 😄

@ghost ghost requested a review from Mik317 September 4, 2020 10:47
Mik317
Mik317 approved these changes Sep 4, 2020
View reviewed changes
Copy link

@Mik317 Mik317 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good fix 😄 🎉

Cheers,
Mik

@JamieSlome JamieSlome merged commit aba245f into 418sec:master Sep 4, 2020
@huntr-helper
Copy link
Member

Congratulations alromh87 - your fix has been selected! 🎉

Thanks for being part of the community & helping secure the world's open source code.
If you have any questions, please respond in the comments section. Your bounty is on its way - keep hunting!

@huntr-helper huntr-helper mentioned this pull request Sep 4, 2020
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Mik317 Mik317 Mik317 approved these changes

@toufik-airane toufik-airane Awaiting requested review from toufik-airane

@mufeedvh mufeedvh Awaiting requested review from mufeedvh

@bbeale bbeale Awaiting requested review from bbeale

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alromh87 @huntr-helper @Mik317 @JamieSlome