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

array_rand is not cryptographically safe #2

Closed
spfmoby opened this issue Mar 21, 2022 · 3 comments
Closed

array_rand is not cryptographically safe #2

spfmoby opened this issue Mar 21, 2022 · 3 comments
Assignees

Comments

@spfmoby
Copy link

spfmoby commented Mar 21, 2022

Your getRandomWord() function picks a word in the file using the php array_rand() function.

https://www.php.net/manual/en/function.array-rand.php explicitly says that this function should not be used for cryptographic purposes.

@gregor-j
Copy link
Owner

gregor-j commented Mar 21, 2022

The array_rand() function is used in three places:

  1. Dictionaries\DictionaryFile::getRandomWord()
  2. Generators\ManageRandomItemsTrait::remove()
  3. Generators\RandomCharacter::add()

ad 1.
The DictionaryFile class is in this repository for demonstration. I strongly sugguest to use a database full of words instead of files, and a model class implementing DictionaryInterface which chooses a rand() random_int() line from that database.
However, you are right: Even for demonstrational purposes, array_rand() is at a critical position. This has to be replaced.

ad 2.
This only affects the removal of a random word. I don't think that the use of array_rand() at this position has any real-world effects. However I can replace it with rand() random_int() while I'm at it.

ad 3.
This might become a problem, if someone uses this class as a classic password generator and not for generating a separator character. This has to be replaced too.

Thanks for pointing out that problem.

@spfmoby
Copy link
Author

spfmoby commented Mar 21, 2022

rand() is not better, you should use random_int() instead (for php >= 7)
For the array (DictionaryFile) you can count the number of lines then generate a random int between 0 and this number (using random_int() )

@gregor-j
Copy link
Owner

Replaced all functions in question.

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

No branches or pull requests

2 participants