Skip to content

Tweak CSPRNG error conditions & add exceptions #1398

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

Closed
wants to merge 3 commits into from

Conversation

SammyK
Copy link
Contributor

@SammyK SammyK commented Jul 7, 2015

This PR is part 2 of #1397 and this discussion. It tweaks the behavior of random_bytes() and random_int() in the following ways:

  1. random_bytes() will throw an Exception if length < 1 instead of issuing an E_WARNING and returning false.
  2. random_int() now allows for min & max to be the same value for cases that need it (i.e. when picking the last card in a shuffling algorithm)
  3. random_int() will throw an Exception if min > max instead of issuing an E_WARNING and returning false.

There was some discussion of throwing the exceptions as an InvalidArgumentException, but the consensus seems to be "keep it general" for BC friendliness.

CC: @weltling, @KalleZ, @lt

@nikic
Copy link
Member

nikic commented Jul 13, 2015

I question the validity of 2. A properly implemented Fisher-Yates will not require generating random numbers with min=max. If you need this case, it implies that you're doing an additional superfluous iteration.

@paragonie-scott
Copy link
Contributor

A properly implemented Fisher-Yates

Have you seen how most PHP devs write code? Demanding proper
implementations is literally blaming the implementor, which is the same
error in reason that allowed the Sony ECDSA failure to occur.

On Jul 13, 2015 6:00 PM, "Nikita Popov" [email protected] wrote:

I question the validity of 2. A properly implemented Fisher-Yates will
not require generating random numbers with min=max. If you need this case,
it implies that you're doing an additional superfluous iteration.


Reply to this email directly or view it on GitHub.

@nikic
Copy link
Member

nikic commented Jul 13, 2015

I don't understand your analogy. If you get an exception due to an off-by-one error in your shuffling implementation, you will be forced to improve your implementation. Why do you consider that stricter input validation should result in broken crypto-system implementations?

@paragonie-scott
Copy link
Contributor

I'm saying random_int() should behave like mt_rand() so we can encourage
ease of adoption.

http://3v4l.org/WsfED
On Jul 13, 2015 6:14 PM, "Nikita Popov" [email protected] wrote:

I don't understand your analogy. If you get an exception due to an
off-by-one error in your shuffling implementation, you will be forced to
improve your implementation. Why do you consider that stricter input
validation should result in broken crypto-system implementations?


Reply to this email directly or view it on GitHub
#1398 (comment).

@nikic
Copy link
Member

nikic commented Jul 13, 2015

That's an argument I can buy ;)

@SammyK
Copy link
Contributor Author

SammyK commented Jul 21, 2015

FYI: These changes have also been discussed on the internals mailing list.

@SammyK
Copy link
Contributor Author

SammyK commented Sep 8, 2015

Closing this since #1397 now has all the newly-agreed upon logic.

@SammyK SammyK closed this Sep 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants