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

Tweak CSPRNG error conditions & add exceptions #1398

Closed
wants to merge 3 commits into from

Conversation

4 participants
@SammyK
Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2015

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" notifications@github.com 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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2015

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" notifications@github.com 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

This comment has been minimized.

Copy link
Member

commented Jul 13, 2015

That's an argument I can buy ;)

@SammyK

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2015

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

@SammyK

This comment has been minimized.

Copy link
Contributor Author

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
You can’t perform that action at this time.