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

[WIP] Easy User-land CSPRNG #1119

Closed
wants to merge 23 commits into from
Closed

[WIP] Easy User-land CSPRNG #1119

wants to merge 23 commits into from

Conversation

SammyK
Copy link
Contributor

@SammyK SammyK commented Feb 24, 2015

Submitting as a PR per @nikic's recommendation.

RFC can be found here.


if (php_random_bytes(bytes->val, size) == FAILURE) {
zend_string_release(bytes);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably that should be RETURN_FALSE as well? Same in random_int.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I'll fix.

zend_ulong result;

if (ZEND_NUM_ARGS() == 1) {
php_error_docref(NULL, E_WARNING, "A minimum and maximum value are expected, only minimum given");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since you already have LONG_MIN and LONG_MAX, so if only minimum value min is given. then simply assume it means min to LONG_MAX?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laruence: I thought about the same thing but then I realize that reading:
random_int(10);
might be understood as 10 being the max.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, that could be a doc issue.. but it's not a big deal :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops - this is a dingleberry left over from when min and max were optional args. They are both required in the current spec that's being voted on. I'll remove this check. :)

RETURN_FALSE;
}

umax = max - min;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to make sure the min & max values weren't the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're already checking this on line 126.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh snap. Not. Enough. Coffee! Oh wait, I'm drinking decaf for some reason today. :/

@sarciszewski
Copy link
Contributor

Congratulations on passing unanimously! One thing: Will you ever support the getrandom(2) syscall on newer Linux flavors?

(I didn't see anything in the RFC and I'm not really awake enough to peruse the code.)

@lt
Copy link
Contributor

lt commented Mar 30, 2015

@sarciszewski This is something we have discussed, but decided to leave for future scope. getrandom (and getentropy on BSD) are typically used to seed RNGs, and are not designed for high throughput themselves.

If you compile against LibreSSL and have a Linux Kernel version > 3.17, you will be using getrandom today.

@ircmaxell
Copy link
Contributor

The vote passed, but was never closed. This should be merged soon ;-)

RANDOM_G(fd) = fd;
}

ssize_t n = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be moved into the while look (combined declaration and assignment) to conform with C89.

@SammyK
Copy link
Contributor Author

SammyK commented Apr 9, 2015

Thanks for the push @ircmaxell! :) I'm coordinating with @lt to get this bad boy ready for merge. :)

Return an arbitrary pseudo-random integer */
PHP_FUNCTION(random_int)
{
zend_long min = ZEND_LONG_MIN;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SammyK Since both parameters are required, we don't need these defaults any more.

@SammyK
Copy link
Contributor Author

SammyK commented Apr 10, 2015

Hokay! @lt updated this PR based on feedback. I normalized the return values when errors happen & updated the tests & merged in the lest from master & fixed conflicts. Phew!

The RFC officially passed unanimously and this PR can be merged into master now. W00t! Not sure if @nikic is the one to do that or who we need to ping.

Thanks again @ircmaxell, @lt, @rdlowrey, and all the other kids who help me along on this one! 🐻 Let's do some more of these! :D

@nikic nikic mentioned this pull request May 9, 2015
@nikic
Copy link
Member

nikic commented May 9, 2015

Merged via #1268. Thanks everyone who worked on this!

@nikic nikic closed this May 9, 2015
@SammyK
Copy link
Contributor Author

SammyK commented May 11, 2015

Yay! Thanks @nikic! :)

@SammyK SammyK deleted the rand-bytes branch May 11, 2015 16:02
@CodesInChaos
Copy link

  1. The documentation for random_int could use some improvements:
    • Clarify the behaviour if min<=max is violated
    • Specify if the bounds are inclusive or exclusive (looking at rand and mt_rand it's probably inclusive)
    • It should mention that the distribution is uniform
  2. Why does it return FALSE which can easily treated as 0 instead of something you can't ignore, like an exception when the RNG in unavailable?
  3. What about a secure polyfill?

@SammyK
Copy link
Contributor Author

SammyK commented Jul 6, 2015

Why does it return FALSE which can easily treated as 0 instead of something you can't ignore, like an exception when the RNG in unavailable?

Good point. I think a RuntimeException should be thrown when a proper source of random cannot be detected. That way when errors are turned off we won't have people using false/0 as their CSPRNG. :) Thoughts @lt?

@SammyK
Copy link
Contributor Author

SammyK commented Jul 6, 2015

@lt: I just pushed up a branch with this change, can you take a look? :)

@CodesInChaos
Copy link

@SammyK What should happen if min > max? Also an exception?

@lt
Copy link
Contributor

lt commented Jul 7, 2015

I think when we wrote this, exceptions in the engine hadn't yet passed.

I'd be happy with InvalidArgumentException for min>max and len<=0

@CodesInChaos
Copy link

Should len === 0 be considered invalid? I'd consider it valid and only throw an exception if len < 0.

@tom--
Copy link

tom-- commented Jul 7, 2015

When someone tries to generate an empty string using the CSPRNG, I think it's likely a programming error (if it were my project, I'd define it as a programming error). So I think it is safer if PHP treats this case as invalid.

And, from the other point of view, I think PHP can reasonably reject bug reports that you can't generate empty strings with the CSPRNG, especially if the conditions for these exceptions are documented.

@lt
Copy link
Contributor

lt commented Jul 7, 2015

If it's not a programming error, it's a stupid thing to do. Invalid all the way.

@CodesInChaos
Copy link

At least min == max on random_int shouldn't be an error, since that can be useful in practice. For example when picking the last card in a shuffling algorithm.

@tom--
Copy link

tom-- commented Jul 7, 2015

@CodesInChaos I agree.

@lt
Copy link
Contributor

lt commented Jul 7, 2015

I'll disagree with practically useful. But I'll agree that it should be allowed, in the "spirit of PHP"

If we're going to allow min == max it should be special cased, no point fetching from the rng for it.

    if (min >= max) {
        if (min == max) {
            RETURN_LONG(min);
        }

        zend_throw_exception(spl_ce_InvalidArgumentException, "Maximum value must not be less than the minimum value", 0);
        return;

    }

@SammyK perhaps do something like this for SPL exceptions:

#ifdef HAVE_SPL
So it can fall back to a less specific exception if SPL is not included.

@nikic
Copy link
Member

nikic commented Jul 7, 2015

Please do the change to exceptions separately from the changing any error conditions.

@SammyK
Copy link
Contributor Author

SammyK commented Jul 7, 2015

Sounds good. I'll submit 2 separate PR's - one for throwing general exceptions and one for new error conditions. Then I'll update the docs when they are merged and post here for another once-over to make sure the docs cover all the bases. :)

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.

None yet