All,
With PHP 7 comes random_bytes and random_int. This duplicates some of
the logic internally that password_hash uses to generate its salt.
I would like to refactor this to unify generation. I've opened a PR
against master: https://github.com/php/php-src/pull/1585
I don't feel comfortable pulling against 7 this far into RC status.
Perhaps wait until after it goes gold? Or should this target 7.1? It's
not a big deal in either direction. Though it does add a side-effect,
where if it can't gather enough entropy it will throw an exception and
return failure (where prior it would simply make a "best effort".
Thoughts?
Anthony
+1 for 7.0.x security patch release, best effort sounds scary.
All,
With PHP 7 comes random_bytes and random_int. This duplicates some of
the logic internally that password_hash uses to generate its salt.I would like to refactor this to unify generation. I've opened a PR
against master: https://github.com/php/php-src/pull/1585I don't feel comfortable pulling against 7 this far into RC status.
Perhaps wait until after it goes gold? Or should this target 7.1? It's
not a big deal in either direction. Though it does add a side-effect,
where if it can't gather enough entropy it will throw an exception and
return failure (where prior it would simply make a "best effort".Thoughts?
Anthony
If I'm understanding this correctly, this change doesn't effect actual
behavior, right? It's just taking advantage of reusing code for
random_bytes / random_int ?
If that is true I don't think it much matters whether the change goes
through 7.0 or 7.1 since it has no real end-user impact.
On Sun, Oct 18, 2015 at 6:59 PM, Anthony Ferrara ircmaxell@gmail.com
wrote:
All,
With PHP 7 comes random_bytes and random_int. This duplicates some of
the logic internally that password_hash uses to generate its salt.I would like to refactor this to unify generation. I've opened a PR
against master: https://github.com/php/php-src/pull/1585I don't feel comfortable pulling against 7 this far into RC status.
Perhaps wait until after it goes gold? Or should this target 7.1? It's
not a big deal in either direction. Though it does add a side-effect,
where if it can't gather enough entropy it will throw an exception and
return failure (where prior it would simply make a "best effort".Thoughts?
Anthony
I don't feel comfortable pulling against 7 this far into RC status.
Perhaps wait until after it goes gold? Or should this target 7.1? It's
not a big deal in either direction. Though it does add a side-effect,
where if it can't gather enough entropy it will throw an exception and
return failure (where prior it would simply make a "best effort".Thoughts?
Anthony
It's a clean patch. It doesn't really seem like a problem pulling it.
Korvin wrote:
+1 for 7.0.x security patch release, best effort sounds scary.
This is a salt. It doesn't need to be cryptographically secure. Using
php_rand()
there should pose no problem.
I would actually include that into the patch (move old lines 154-156
into the
FAILURE if).
Korvin wrote:
+1 for 7.0.x security patch release, best effort sounds scary.
This is a salt. It doesn't need to be cryptographically secure. Using
php_rand()
there should pose no problem.
I would actually include that into the patch (move old lines 154-156
into the
FAILURE if).
A password salt needs to be unique. It does not need to be drawn from a
CSPRNG but that is one of the few ways we can be reasonably confident of
uniqueness (since, as usual, we assume the platform RNG is properly seeded).
I can seed php_rand() from my script but, other than using the platform
RNG, I have no idea how. Or I can let PHP seed it but its algorithm, a
function of time and PID, shows PHP doesn't know how either.
As PHP's version numbers increase, so should it's rigor in using best
practices. I've no problem with apps breaking in the 5 -> 7 upgrade if
they have no access to platform RNG. So doing Anthony's proposed change
as early as possible in 7.0.x is best.
Tom
Korvin wrote:
+1 for 7.0.x security patch release, best effort sounds scary.
This is a salt. It doesn't need to be cryptographically secure. Using
php_rand()
there should pose no problem.
I would actually include that into the patch (move old lines 154-156
into the
FAILURE if).A password salt needs to be unique. It does not need to be drawn from a
CSPRNG but that is one of the few ways we can be reasonably confident of
uniqueness (since, as usual, we assume the platform RNG is properly seeded).
A password salt should not be predictable, allowing a salt to potentially
become predictable is a bad idea. Solution is to use a CSPRNG for
generation of salts.
Korvin wrote:
+1 for 7.0.x security patch release, best effort sounds scary.
This is a salt. It doesn't need to be cryptographically secure. Using
php_rand()
there should pose no problem.
I would actually include that into the patch (move old lines 154-156
into the
FAILURE if).A password salt needs to be unique. It does not need to be drawn from a
CSPRNG but that is one of the few ways we can be reasonably confident of
uniqueness (since, as usual, we assume the platform RNG is properly seeded).A password salt should not be predictable, allowing a salt to potentially
become predictable is a bad idea. Solution is to use a CSPRNG for
generation of salts.
Hi all,
I'd like to weigh in by quantifying the predictability of rand(). If
you're well aware of statistics or versed in cryptography, you might
be able to skip this email. (You might still want to read it to help
verify or challenge my conclusion.)
First, for the sake of generosity, let's assume that rand() is
properly seeded, e.g. by 4 bytes from /dev/urandom, on each invocation
so that the pid + time() issue doesn't come into play. (In reality,
this is very important.)
After seeding rand, there are 2^32 possible outputs. That's a little
over 4 billion, and it's highly unlikely that your website has 4
billion user accounts with a hashed password, so that's good enough
right? Well, not quite.
For a moment, imagine you are in a room with 22 other people born in
the same year as you. What is the probability that any two people in
the room share the exact same birthday? Well, it turns out that the
answer is 50%. We call this the Birthday Problem.
From studying this problem, we can estimate the number of random
samples to generate a collision from an n-bit keyspace by a simple
formula: G = 2^(n/2). This is called the Birthday Bound.
If you have a keyspace of 2^32 possible output sequences like we do
from rand(), we can say that after 65,536 there is a 50% probability
of finding at least one collision.
It should go without saying, but if users have weak/common password
choices and their salts collide, they will end up with duplicate
bcrypt hashes.
My conclusion is thus: Yes, we do need a CSPRNG here, especially if we
want to encourage the use of the password_* API for any large system.
https://en.wikipedia.org/wiki/Birthday_problem
https://en.wikipedia.org/wiki/Birthday_attack
http://eprint.iacr.org/2005/004
Scott Arciszewski
Chief Development Officer
Paragon Initiative Enterprises <https://paragonie.com
Korvin wrote:
+1 for 7.0.x security patch release, best effort sounds scary.
This is a salt. It doesn't need to be cryptographically secure. Using
php_rand()
there should pose no problem.
I would actually include that into the patch (move old lines 154-156
into the
FAILURE if).A password salt needs to be unique. It does not need to be drawn from a
CSPRNG but that is one of the few ways we can be reasonably confident of
uniqueness (since, as usual, we assume the platform RNG is properly seeded).A password salt should not be predictable, allowing a salt to potentially
become predictable is a bad idea. Solution is to use a CSPRNG for
generation of salts.Hi all,
I'd like to weigh in by quantifying the predictability of
rand(). If
you're well aware of statistics or versed in cryptography, you might
be able to skip this email. (You might still want to read it to help
verify or challenge my conclusion.)First, for the sake of generosity, let's assume that
rand()is
properly seeded, e.g. by 4 bytes from /dev/urandom, on each invocation
so that the pid +time()issue doesn't come into play. (In reality,
this is very important.)After seeding rand, there are 2^32 possible outputs. That's a little
over 4 billion, and it's highly unlikely that your website has 4
billion user accounts with a hashed password, so that's good enough
right? Well, not quite.For a moment, imagine you are in a room with 22 other people born in
the same year as you. What is the probability that any two people in
the room share the exact same birthday? Well, it turns out that the
answer is 50%. We call this the Birthday Problem.From studying this problem, we can estimate the number of random
samples to generate a collision from an n-bit keyspace by a simple
formula: G = 2^(n/2). This is called the Birthday Bound.If you have a keyspace of 2^32 possible output sequences like we do
fromrand(), we can say that after 65,536 there is a 50% probability
of finding at least one collision.It should go without saying, but if users have weak/common password
choices and their salts collide, they will end up with duplicate
bcrypt hashes.My conclusion is thus: Yes, we do need a CSPRNG here, especially if we
want to encourage the use of the password_* API for any large system.
I don't follow the logic of this hypothetical. php_rand() is only
relevant to this discussion if PHP can't read /dev/random or
CryptGenRandom, in which case we know that the state of php_rand()'s LCG
is not randomized (it's open to tampering) so this statistical analysis
is not possible.
I've verified that password_hash() without /dev/urandom can produce
systematically predictable salts, repeating a sequence of just two
salts. There's nothing statistical involved. Reported in bug 70743.
Seems crypt() is similarly afflicted.
Tom
(...)
If you have a keyspace of 2^32 possible output sequences like we do
fromrand(), we can say that after 65,536 there is a 50% probability
of finding at least one collision.It should go without saying, but if users have weak/common password
choices and their salts collide, they will end up with duplicate
bcrypt hashes.My conclusion is thus: Yes, we do need a CSPRNG here, especially if we
want to encourage the use of the password_* API for any large system.
That assumes that:
(a)rand()has the same internal state on each password generation. Given
that it is seeded once per minit, that suggests they are using something
like cgi.
Which is not too suitable for large systems.
(b) You have people sharing the same password and colliding in the
same salt bucket.
Supposing that you have an equiprobable 4-byte salt (which is actually
the case to which
we fall if (a) holds), and 152 million users (the amount of the adobe
leak, biggest to date)
we would only have collisions on the top-13 passwords… not even
colliding on 1234 (maybe
relax it to the top-16 or top-25 to account for not being completely
fairy distributed).Which
are passwords in the really really bad zone, and would be cracked even
with no help of collisions.
And that's another point, a salt used by two users (ie. finding one
collision) won't speed you
much in terms of cracking their respective passwords.
Undesirable? Sure. Broken? Not really.
Tom Worster wrote:
I don't follow the logic of this hypothetical. php_rand() is only
relevant to this discussion if PHP can't read /dev/random or
CryptGenRandom, in which case we know that the state of php_rand()'s
LCG is not randomized (it's open to tampering) so this statistical
analysis is not possible.I've verified that
password_hash()without /dev/urandom can produce
systematically predictable salts, repeating a sequence of just two
salts. There's nothing statistical involved. Reported in bug 70743.Seems
crypt()is similarly afflicted.
Only providing two hashes would be very worrisome. Note however that
you are cripplingrand()by always resetting the seed (those two
different outputs appear because it is xoring with the same sequence).
It's also reading unitialized memory in line 155, which is probably
accounting for the salts being different every time. And a change in
allocations for the first run being different.
Whereas on a real world usage, even if a new php instance was used every
time, you would need a GENERATE_SEED() collision. It initializes the
seed using the timestamp, pid, microseconds and thread id (plus any
state change caused if different php scripts were run).
You have a very good point in that it would fail only if /dev/urandom
does not exist at all, my initial concerns came from thinking that it
could fail on a low-entropy case, but that won't happen on Linux
(urandom always works), and even a urandom blocking OS (eg. on FreeBSD)
wouldn't make it use the fallback code. Seems it would only be used if
(a) you don't have a /dev/urandom at all or (b) it isn't the special
file it is supposed to be (eg. it's a regular file). In which case you
are running a broken OS :)
So, changing my previous opinion, it'd be acceptable to drop it (I'd
prefer it being done in the point release, though).
Best regards
Tom Worster wrote:
I've verified that `password_hash()` without /dev/urandom can produce systematically predictable salts, repeating a sequence of just two salts. There's nothing statistical involved. Reported in bug 70743. Seems `crypt()` is similarly afflicted.Only providing two hashes would be very worrisome. Note however
that you are cripplingrand()by always resetting the seed (those
My point was to show that password_hash()'s promises of security are
nullified by use of an unrelated API. The docs do not explain how
password_hash() can depend on the use of other functions, like the
old rand() LCG, and there's no reason the user should suspect it.
two different outputs appear because it is xoring with the same
sequence). It's also reading unitialized memory in line 155, which
is probably accounting for the salts being different every time. And
a change in allocations for the first run being different.Whereas on a real world usage, even if a new php instance was used
every time, you would need a GENERATE_SEED() collision. It
initializes the seed using the timestamp, pid, microseconds and
thread id (plus any state change caused if different php scripts
were run).
Security BCPs do not allow arguments of this nature. Security-related
APIs need to be robust in all reasonable circumstances, not just in what
you consider typical circumstances.
You have a very good point in that it would fail only if
/dev/urandom does not exist at all, my initial concerns came from
thinking that it could fail on a low-entropy case, but that won't
happen on Linux (urandom always works), and even a urandom blocking
OS (eg. on FreeBSD) wouldn't make it use the fallback code. Seems it
would only be used if (a) you don't have a /dev/urandom at all or
(b) it isn't the special file it is supposed to be (eg. it's a
regular file). In which case you are running a broken OS :)
So, changing my previous opinion, it'd be acceptable to drop it (I'd
prefer it being done in the point release, though).
There's no such thing as "a low-entropy case" for our purposes. Once the
system's CSPRNG is seeded it will produce an endless supply of random
bytes. PHP has no alternative but to trust this. Anthony recently made
this point in a humorously obscene manner on Twitter.
/dev/random and /dev/urandom are the same thing on FreeBSD. The situation
on OpenBSD, NetBSD and OS X is similar.
FreeBSD's random device does not block except during a reseeding event.
This happens immediately after boot for a short period but PHP can safely
ignore that. The other causes for a reseeding event are very unlikely (and
have nothing to do with "how much entropy is consumed" or any such
nonsense)
but if they do happen then the best thing PHP can do is hang and wait for
the result. I believe PHP can take the same attitude on OpenBSD, NetBSD
and
OS X, although I admit I know less about these.
We do not need to consider low-entropy cases. If we read from urandom and
allow it to block, we should be safe. There is danger in libc arc4random
but that's for another thread #70744.
Tom
Hi!
With PHP 7 comes random_bytes and random_int. This duplicates some of
the logic internally that password_hash uses to generate its salt.I would like to refactor this to unify generation. I've opened a PR
against master: https://github.com/php/php-src/pull/1585I don't feel comfortable pulling against 7 this far into RC status.
Perhaps wait until after it goes gold? Or should this target 7.1? It's
If functionality does not change and it's just internal refactoring not
breaking BC (both language and binary) then it can go into 7.0.x. From
what I can see, it is pretty unintrusive, so I wouldn't mind too much
even getting it into 7.0 but that's on RM to decide. In fact, at least
making php_random_bytes() public API should be in 7.0 as that makes for
much less compatibility problems for extensions later.
Generally speaking, having public random generating function sounds like
a very prudent thing, even if we end up not merging the rest of the
patch into 7.0.
not a big deal in either direction. Though it does add a side-effect,
where if it can't gather enough entropy it will throw an exception and
return failure (where prior it would simply make a "best effort".
From what I can see, the system that can't return enough random bytes
for what php_random_bytes() wants is deeply fubar, so on this scenario
failing fast is the best option.
--
Stas Malyshev
smalyshev@gmail.com
Hi Anthony,
-----Original Message-----
From: Anthony Ferrara [mailto:ircmaxell@gmail.com]
Sent: Monday, October 19, 2015 1:00 AM
To: internals@lists.php.net
Subject: [PHP-DEV] Password_hash salt generation refactorAll,
With PHP 7 comes random_bytes and random_int. This duplicates some of
the logic internally that password_hash uses to generate its salt.I would like to refactor this to unify generation. I've opened a PR against
master: https://github.com/php/php-src/pull/1585I don't feel comfortable pulling against 7 this far into RC status.
Perhaps wait until after it goes gold? Or should this target 7.1? It's not a big
deal in either direction. Though it does add a side-effect, where if it can't
gather enough entropy it will throw an exception and return failure (where
prior it would simply make a "best effort".Thoughts?
As much as it could be an improvement of the password_hash(), one would better avoid any behavior change at this stage. I was thinking about it and came to an idea that maybe could work for 7.0 - one should just make php_random_bytes() that side effect free.
Could php_random_bytes() be extended with a flag that would tell exceptions to pass? Or maybe exceptions could be moved out from the php_random_bytes() and thrown directly in the new functions that was undergoing the RFC but not password_hash() ? I'd be actually for the second variant.
That ways FAILURE would be returned, but a decision about what to do about it would be made outside. IMHO it were also useful as the API is intended to be exported. So for the cases where php_random_bytes() came to use as a library function outside immediate PHP userspace (for example in SAPI), it should not throw exceptions from itself.
Do this suggestions sound eligible?
Regards
Anatol
All,
Hi Anthony,
-----Original Message-----
From: Anthony Ferrara [mailto:ircmaxell@gmail.com]
Sent: Monday, October 19, 2015 1:00 AM
To: internals@lists.php.net
Subject: [PHP-DEV] Password_hash salt generation refactorAll,
With PHP 7 comes random_bytes and random_int. This duplicates some of
the logic internally that password_hash uses to generate its salt.I would like to refactor this to unify generation. I've opened a PR against
master: https://github.com/php/php-src/pull/1585I don't feel comfortable pulling against 7 this far into RC status.
Perhaps wait until after it goes gold? Or should this target 7.1? It's not a big
deal in either direction. Though it does add a side-effect, where if it can't
gather enough entropy it will throw an exception and return failure (where
prior it would simply make a "best effort".Thoughts?
As much as it could be an improvement of the
password_hash(), one would better avoid any behavior change at this stage. I was thinking about it and came to an idea that maybe could work for 7.0 - one should just make php_random_bytes() that side effect free.Could php_random_bytes() be extended with a flag that would tell exceptions to pass? Or maybe exceptions could be moved out from the php_random_bytes() and thrown directly in the new functions that was undergoing the RFC but not
password_hash()? I'd be actually for the second variant.That ways FAILURE would be returned, but a decision about what to do about it would be made outside. IMHO it were also useful as the API is intended to be exported. So for the cases where php_random_bytes() came to use as a library function outside immediate PHP userspace (for example in SAPI), it should not throw exceptions from itself.
Do this suggestions sound eligible?
I have created a PR for this: https://github.com/php/php-src/pull/1614
It changes the public API of the internal php_random_bytes function to
have a third "should_throw" parameter, as well as defining two macros:
php_random_bytes_throw(dest, len) and php_random_bytes_silent(dest,
len).
If this looks acceptable, it can be pulled into 7.0, and then the move
for password_hash can be done at a later date.
Thanks
Anthony
Hi Anthony,
-----Original Message-----
From: Anthony Ferrara [mailto:ircmaxell@gmail.com]
Sent: Friday, October 30, 2015 11:58 AM
To: Anatol Belski anatol.php@belski.net
Cc: internals@lists.php.net; Kalle Sommer Nielsen kalle@php.net
Subject: Re: [PHP-DEV] Password_hash salt generation refactorAll,
On Tue, Oct 20, 2015 at 11:35 PM, Anatol Belski anatol.php@belski.net
wrote:Hi Anthony,
-----Original Message-----
From: Anthony Ferrara [mailto:ircmaxell@gmail.com]
Sent: Monday, October 19, 2015 1:00 AM
To: internals@lists.php.net
Subject: [PHP-DEV] Password_hash salt generation refactorAll,
With PHP 7 comes random_bytes and random_int. This duplicates some of
the logic internally that password_hash uses to generate its salt.I would like to refactor this to unify generation. I've opened a PR
against
master: https://github.com/php/php-src/pull/1585I don't feel comfortable pulling against 7 this far into RC status.
Perhaps wait until after it goes gold? Or should this target 7.1?
It's not a big deal in either direction. Though it does add a
side-effect, where if it can't gather enough entropy it will throw an
exception and return failure (where prior it would simply make a "best
effort".Thoughts?
As much as it could be an improvement of the
password_hash(), one would
better avoid any behavior change at this stage. I was thinking about it and came
to an idea that maybe could work for 7.0 - one should just make
php_random_bytes() that side effect free.Could php_random_bytes() be extended with a flag that would tell exceptions
to pass? Or maybe exceptions could be moved out from the
php_random_bytes() and thrown directly in the new functions that was
undergoing the RFC but notpassword_hash()? I'd be actually for the second
variant.That ways FAILURE would be returned, but a decision about what to do about
it would be made outside. IMHO it were also useful as the API is intended to be
exported. So for the cases where php_random_bytes() came to use as a library
function outside immediate PHP userspace (for example in SAPI), it should not
throw exceptions from itself.Do this suggestions sound eligible?
I have created a PR for this: https://github.com/php/php-src/pull/1614
It changes the public API of the internal php_random_bytes function to have a
third "should_throw" parameter, as well as defining two macros:
php_random_bytes_throw(dest, len) and php_random_bytes_silent(dest, len).If this looks acceptable, it can be pulled into 7.0, and then the move for
password_hash can be done at a later date.
IMHO this looks fine for 7.0. The API is new and was not exported, so no ABI breach and no behavior change when used for internal refactoring.
Regards
Anatol
Hi Anthony,
-----Original Message-----
From: Anthony Ferrara [mailto:ircmaxell@gmail.com]
Sent: Friday, October 30, 2015 11:58 AM
All,On Tue, Oct 20, 2015 at 11:35 PM, Anatol Belski anatol.php@belski.net
wrote:Could php_random_bytes() be extended with a flag that would tell exceptions
to pass? Or maybe exceptions could be moved out from the
php_random_bytes() and thrown directly in the new functions that was
undergoing the RFC but notpassword_hash()? I'd be actually for the second
variant.That ways FAILURE would be returned, but a decision about what to do about
it would be made outside. IMHO it were also useful as the API is intended to be
exported. So for the cases where php_random_bytes() came to use as a library
function outside immediate PHP userspace (for example in SAPI), it should not
throw exceptions from itself.Do this suggestions sound eligible?
I have created a PR for this: https://github.com/php/php-src/pull/1614
It changes the public API of the internal php_random_bytes function to have a
third "should_throw" parameter, as well as defining two macros:
php_random_bytes_throw(dest, len) and php_random_bytes_silent(dest, len).If this looks acceptable, it can be pulled into 7.0, and then the move for
password_hash can be done at a later date.IMHO this looks fine for 7.0. The API is new and was not exported, so no ABI breach and no behavior change when used for internal refactoring.
This change to php_random_bytes() should make it easier to migrate
various old stuff (e.g. Leigh proposed sessions) to the new source, and
I like it for that.
But silenced failure should probably not be used for anything new and it
would be good if a developer refactoring an old API would comment how
the new code processes FAILURE together with a justification. Could
comments be included in this PR to encourage it? As it stands, this
change allows hazardous use without mentioning it.
Tom