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

Update csrf token function, provide support for those without openssl #7

Merged
merged 1 commit into from
Apr 18, 2014

Conversation

r3wt
Copy link

@r3wt r3wt commented Apr 15, 2014

I've made some minor improvements on the csrf token function of the user class. i also attempted to provide support for those without openssl on their box. I attempted to make a Roll-your-own PSRNG. it is my belief that this solution is sufficiently random enough for something as trivial as csrf tokens. This being said, i decided to up the hash function to whirlpool as a counterbalance to the potentially weaker PSRNG. I encourage some review of this prior to merging the commit.

r3wt

@alexweissman
Copy link
Member

Cool, thank you. I actually don't know that much about CSRF vulnerabilities - would you be willing to write up a paragraph or two explaining the vulnerability, how your implementation addresses the issue, and how that compares to other possible solutions? We can put a "security" section in the README.

@alexweissman
Copy link
Member

Based on my own (admittedly limited) expertise in cryptography, this looks pretty good. My only concern is that including the hashed user password in the CSRF token would potentially create a way for an attacker to recover a user's password:

A) Attacker recovers a user's CSRF token. Easy enough if the site is not using HTTPS/SSL.
B) The attacker manages to figure out the preimage of said token.
C) The attacker extracts the password portion of the preimage token, and then finds its preimage.

However, this all relies on the idea that an attacker could recover the CSRF token to begin with. In which case, they'd probably be able to intercept the user password directly during login, anyway.

I'm going to go ahead and approve the pull request, but we might consider using a less sensitive (but still user-supplied) piece of information instead.

alexweissman added a commit that referenced this pull request Apr 18, 2014
Update csrf token function, provide support for those without openssl
@alexweissman alexweissman merged commit c3af073 into userfrosting:master Apr 18, 2014
@r3wt
Copy link
Author

r3wt commented Apr 18, 2014

i agree alex. i think we can replace the password with the username, since the username is unique anyway. i also need to go ahead and push the rest of the token system, required for verifying the tokens. the way it works is, you just include/require_once the file in any form processing script that requires a user to be logged in, for example the user settings page.

a csrf token failure would result in an $error[] being added to the superglobal with a message like "A security error occured", and the token would be regenerated, making bruteforcing the tokens practically impossible, since the attacker doesn't have access to the user session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants