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

Patch insecure CSRF token crypto vulnerability #1164

Merged
merged 4 commits into from Apr 18, 2017
Merged

Conversation

Arinerron
Copy link
Contributor

I noticed that CSRF tokens are not using a cryptographically secure pseudorandom number generator. An attacker could potentially "guess" the CSRF token, allowing them to control vesta. The current algorithm for generating CSRF tokens is md5(uniqid(mt_rand(), true)), but it should use an algorithm that is considered cryptographically secure (for example, bin2hex(openssl_random_pseudo_bytes(32)) or bytes from /dev/urandom).

This pull request simply replaces the insecure crypto with the secure function.

To test if the code worked, I made this little script:

<?php
echo bin2hex(openssl_random_pseudo_bytes(16));
?>

The output is something like:

da202fd306550e3ad02fcbf2ac0838d4

The insecure algorithm (being used now) generates tokens like this:

11b1ec9936e32e555c7a32046871abc0

Same length, just more secure. Here's a stackoverflow post about it: http://stackoverflow.com/a/2595372

Also,
http://php.net/manual/en/function.mt-rand.php
http://php.net/manual/en/function.rand.php
If you scroll down on either of those documentation pages, you'll see the warning about how it does not generate cryptographically secure values.

Thanks!

@dpeca dpeca merged commit 0cbb361 into outroll:master Apr 18, 2017
@dpeca
Copy link
Collaborator

dpeca commented Apr 18, 2017

Thank you @Arinerron :)

@dpeca
Copy link
Collaborator

dpeca commented Apr 19, 2017

uhhh, looks like you used undefined function... panel is now broken...

PHP Fatal error: Call to undefined function openssl_random_pseudo_bytes() in /usr/local/vesta/web/login/index.php on line 129

I must revert this.

@Arinerron, it would be nice if you setup virtualbox and install Vesta, then you can test your code before you send push requests, cheers :)

dpeca added a commit that referenced this pull request Apr 19, 2017
Reverting #1164, because undefined function is used
@Arinerron
Copy link
Contributor Author

@dpeca Ugh, sorry. You need the OpenSSL PHP module: http://us3.php.net/manual/en/book.openssl.php

It says it come with PHP >=5.3.0, PHP 7 by default.

http://php.net/manual/en/function.openssl-random-pseudo-bytes.php

@dpeca
Copy link
Collaborator

dpeca commented Apr 19, 2017

I think VestaCP has its own seperated PHP parser - /usr/local/vesta/php/bin/php

Only @serghey-rodin can rebuild it.

@anton-reutov
Copy link
Collaborator

Should be rechecked by Sergey Rodin or Dmitry Naumov-Socolov

@Arinerron
Copy link
Contributor Author

This pull request was merged but reverted. The issue still exists in master. Waiting on @serghey-rodin to rebuild with the OpenSSL PHP module.

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.

None yet

4 participants