Skip to content

SecureRandom documentation does not cover a common use case #4328

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.

Sign up for GitHub

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

Jump to bottom
Closed
gzankevich opened this issue Oct 15, 2014 · 3 comments
Closed

SecureRandom documentation does not cover a common use case #4328

gzankevich opened this issue Oct 15, 2014 · 3 comments
Labels
actionable Clear and specific issues ready for anyone to take them. good first issue Ideal for your first contribution! (some Symfony experience may be required) hasPR A Pull Request has already been submitted for this issue. Security

Comments

@gzankevich
Copy link
Contributor

As I understand it, if you're trying to generate a token for something which you plan to store in the database then you need to hash the result of nextBytes. It would be handy if the docs mentioned this in:

http://symfony.com/doc/master/book/security.html#generating-a-secure-random-number
@javiereguiluz
Copy link
Member

I don't understand this issue. The behavior of nextBytes() depends on the configuration of your system:

  1. If OpenSSL is supported, the returned bytes are generated with openssl_random_pseudo_bytes() PHP function.

  2. If OpenSSL is not supported, Symfony generates the bytes itself and returns them after applying the sha512 hash algorithm.

In the second case, there is no need for the hash. In the first case, the returned value is a string (although it can contain only digits and be interpreted as a number). So, do you really think we need to add a note about hashing this value?

@stof
Copy link
Member

stof commented Jul 2, 2015

@javiereguiluz nextBytes returns a binary string. This may contain \0, so you should indeed hash it/convert it if you want to store it in a DB as they generally don't like NUL bytes.
And if you plan to use the token in a URL, you should restrict the string to URL-safe chars. Using it in form submissions does not play well either with binary strings (which is why CSRF tokens are converting the random bytes for instance)

@xabbuh xabbuh added good first issue Ideal for your first contribution! (some Symfony experience may be required) actionable Clear and specific issues ready for anyone to take them. and removed needs comments labels Jul 2, 2015
@javiereguiluz javiereguiluz mentioned this issue Jul 2, 2015
Merged
@javiereguiluz
Copy link
Member

Now I understand the problem. I'm trying to fix it in #5472. Thanks for the explanation.

@xabbuh xabbuh added the hasPR A Pull Request has already been submitted for this issue. label Jul 2, 2015
weaverryan added a commit that referenced this issue Jul 16, 2015
@weaverryan
feature #5472 Added a tip about hashing the result of nextBytes() (ja…
c520e77 
…viereguiluz)

This PR was merged into the 2.3 branch.

Discussion
----------

Added a tip about hashing the result of nextBytes()

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | all
| Fixed tickets | #4328

Commits
-------

1a4b5fa Reword
64460d5 Added a tip about hashing the result of nextBytes()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
actionable Clear and specific issues ready for anyone to take them. good first issue Ideal for your first contribution! (some Symfony experience may be required) hasPR A Pull Request has already been submitted for this issue. Security
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@javiereguiluz @weaverryan @gzankevich @stof @xabbuh