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

Use crypto/rand instead of math/rand #20

Closed
elithrar opened this issue Apr 25, 2016 · 3 comments
Closed

Use crypto/rand instead of math/rand #20

elithrar opened this issue Apr 25, 2016 · 3 comments

Comments

@elithrar
Copy link

randomBytes in xsrf.go uses math.rand to generate CSRF tokens. This is unsafe/insecure, and because it is seeded with time.UnixNano, generates predictable results that would allow an attacker to bypass CSRF protection.

sessionID in session.go correctly uses crypto/rand to generate a session ID. You should split this code out and re-use it for both session ID generation and CSRF token generation.

(You're also welcome to import or repurpose https://github.com/gorilla/csrf for this as well, which has some additional mitigations against CSRF bypass)

@elithrar
Copy link
Author

(I'll also gladly provide a PR to address this, but wanted to get buy in/explain it first!)

@dinever
Copy link
Owner

dinever commented Apr 25, 2016

@elithrar Thanks for pointing this out. Please feel free to open a PR.

https://github.com/gorilla/csrf looks really nice. I would prefer to repurpose it and put a credit in the README. Thanks again!

@elithrar
Copy link
Author

elithrar commented May 1, 2016

OK, great. To clarify, I plan on:

  • Making sure crypto/rand is the only way tokens are generated throughout the framework (first and foremost)
  • Import gorilla/csrf for generating CSRF tokens (second)

I'd strongly recommend that you import gorilla/csrf—which has a strict API stability guarantee, but would also be vendored by downstream users—rather than copying the code into your library so that any security patches or feature additions are kept in-sync. Ultimately that's up to you though.

bentranter added a commit to bentranter/golf that referenced this issue Jun 3, 2016
Issue: dinever#20

Uses crypto/rand instead of math/rand for XSRF token generation, as
@elithrar suggested. In that issue it was also suggested that we either
use gorilla/csrf or repurpose it for Golf, so this PR may not close the
issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants