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 recoverPassword.php to use a secure random number generator. #147

Merged
merged 2 commits into from Nov 3, 2015

Conversation

Projects
None yet
3 participants
@ghost

ghost commented Nov 3, 2015

The original implementation of recoverPassword.php uses a timestamp for
seeding the random number generator (via srand). If two people reset
their passwords at the exact same time, their passwords will be
identical. An attacker who resets their password and another users at
almost the same time can determine the approx. int value used as the
seed and then generate a list of possible passwords, iterating through
the seed values. This creates a password list that can be used to
brute-force a victims password.

This attack was tested against CDash and was found to be effective.

If you're happy with this approach, we can replace other uses of
insecure random number generators in the codebase in a similar way.

References:
http://www.openwall.com/php_mt_seed/
https://paragonie.com/blog/2015/07/how-safely-generate-random-strings-and-integers-in-php

@jamiesnape

This comment has been minimized.

Show comment
Hide comment
@jamiesnape

jamiesnape Nov 3, 2015

Contributor

Have you ever considered ircmaxell/random-lib? If using paragonie/random_compat (which is fine), it should be installed using composer, see https://packagist.org/packages/paragonie/random_compat.

Contributor

jamiesnape commented Nov 3, 2015

Have you ever considered ircmaxell/random-lib? If using paragonie/random_compat (which is fine), it should be installed using composer, see https://packagist.org/packages/paragonie/random_compat.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 3, 2015

I didn't want to change too much of the CDash code, so I chose paragonie/random_compat, as it's almost a "drop in" replacement. Plus, it means that users of PHP 7 will use the native random_int function instead of an extra library.

I've added paragonie/random_compat into package.json, so Composer will install it as a dependency.

If anyone has strong preferences to change this approach, then I'm happy to modify my pull request, as long as the insecure random number generator use is fixed. :)

ghost commented Nov 3, 2015

I didn't want to change too much of the CDash code, so I chose paragonie/random_compat, as it's almost a "drop in" replacement. Plus, it means that users of PHP 7 will use the native random_int function instead of an extra library.

I've added paragonie/random_compat into package.json, so Composer will install it as a dependency.

If anyone has strong preferences to change this approach, then I'm happy to modify my pull request, as long as the insecure random number generator use is fixed. :)

@jamiesnape

This comment has been minimized.

Show comment
Hide comment
@jamiesnape

jamiesnape Nov 3, 2015

Contributor

Can you add paragonie/random_compat to composer.json rather than package.json?

Contributor

jamiesnape commented Nov 3, 2015

Can you add paragonie/random_compat to composer.json rather than package.json?

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 3, 2015

Oops! I'll update it soon.

ghost commented Nov 3, 2015

Oops! I'll update it soon.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Nov 3, 2015

Updated. Thanks! :)

ghost commented Nov 3, 2015

Updated. Thanks! :)

@jamiesnape

This comment has been minimized.

Show comment
Hide comment
@jamiesnape

jamiesnape Nov 3, 2015

Contributor

LGTM.

Contributor

jamiesnape commented Nov 3, 2015

LGTM.

zackgalbreath added a commit that referenced this pull request Nov 3, 2015

Merge pull request #147 from ls--/master
Update recoverPassword.php to use a secure random number generator.

@zackgalbreath zackgalbreath merged commit cfd5118 into Kitware:master Nov 3, 2015

1 of 2 checks passed

ci/circleci Your tests failed
Details
StyleCI The StyleCI analysis has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment