Forum: Ruby-core [ruby-trunk - Bug #6928][Open] SecureRandom.random_bytes: assume zero entropy for seed value

Issue #6928 has been reported by MartinBosslet (Martin Bosslet).

----------------------------------------
Bug #6928: SecureRandom.random_bytes: assume zero entropy for seed value
https://bugs.ruby-lang.org/issues/6928

Author: MartinBosslet (Martin Bosslet)
Status: Open
Priority: Normal
Assignee: akr (Akira Tanaka)
Category: lib
Target version: 2.0.0
ruby -v: trunk


If OpenSSL is available SecureRandom.random_bytes uses
OpenSSL::Random.random_bytes and the random generator is reseeded [1]
whenever the current pid changes (due to repeated values when a pid
is reused, cf. #4579).

Since this seeding is also called the first time the method is entered,
using OpenSSL::Random.seed is potentially dangerous. 
OpenSSL::Random.seed
is equal to using OpenSSL::Random.random_add where it is assumed that 
the
string passed to seed possesses full entropy. This is definitely not the
case for pid and time values. In fact, OpenSSL itself assumes an entropy
of 1.0 or even 0.0 when doing similar seeding in RAND_poll [2][3]. 
However,
this seems to have no impact so far, since the OpenSSL random generator
gathers enough entropy on startup even if we seeded with what it would
consider enough bytes of entropy (32 by default). So even if our seed
string is already 32 bytes or larger, OpenSSL's RAND_poll still seems to
collect 32 bytes of entropy on initialization regardless of what has 
been
added/seeded so far, which is a good thing in this case. Still, this 
could
change over time if OpenSSL for example changes internal behaviour and
would decide that enough entropy had been provided while seeding.

Therefore I believe using OpenSSL::Random.random_add with an assumed
entropy of 0.0 might be a more defensive choice. The forking test from
#4579 still passes with the attached patch. What do you think?

[1] https://github.com/ruby/ruby/blob/trunk/lib/secure...
[2] 
https://github.com/plenluno/openssl/blob/master/cr...
[3] 
https://github.com/plenluno/openssl/blob/master/cr...

Issue #6928 has been updated by nahi (Hiroshi Nakamura).


Agreed.  We should fix it because the current usage of 
OpenSSL::Rand.seed in secrerandom.rb is not expected; 
OpenSSL::Rand.seed(bytes) is a wrapper for RAND_seed(), RAND_seed() is 
equivalent to RAND_add() when num == entropy, and the entropy for 
RAND_add() must be a lower bound of an estimate of entropy of the given 
seed.  'ary.to_s' clearly does not have an entropy of 30 bytes.

The patch looks good to me. Though the buf would have 5 bytes or so of 
entropy, we don't need to bother the exact lower bound I think. :-)
----------------------------------------
Bug #6928: SecureRandom.random_bytes: assume zero entropy for seed value
https://bugs.ruby-lang.org/issues/6928#change-29065

Author: MartinBosslet (Martin Bosslet)
Status: Open
Priority: Normal
Assignee: akr (Akira Tanaka)
Category: lib
Target version: 2.0.0
ruby -v: trunk


If OpenSSL is available SecureRandom.random_bytes uses
OpenSSL::Random.random_bytes and the random generator is reseeded [1]
whenever the current pid changes (due to repeated values when a pid
is reused, cf. #4579).

Since this seeding is also called the first time the method is entered,
using OpenSSL::Random.seed is potentially dangerous. 
OpenSSL::Random.seed
is equal to using OpenSSL::Random.random_add where it is assumed that 
the
string passed to seed possesses full entropy. This is definitely not the
case for pid and time values. In fact, OpenSSL itself assumes an entropy
of 1.0 or even 0.0 when doing similar seeding in RAND_poll [2][3]. 
However,
this seems to have no impact so far, since the OpenSSL random generator
gathers enough entropy on startup even if we seeded with what it would
consider enough bytes of entropy (32 by default). So even if our seed
string is already 32 bytes or larger, OpenSSL's RAND_poll still seems to
collect 32 bytes of entropy on initialization regardless of what has 
been
added/seeded so far, which is a good thing in this case. Still, this 
could
change over time if OpenSSL for example changes internal behaviour and
would decide that enough entropy had been provided while seeding.

Therefore I believe using OpenSSL::Random.random_add with an assumed
entropy of 0.0 might be a more defensive choice. The forking test from
#4579 still passes with the attached patch. What do you think?

[1] https://github.com/ruby/ruby/blob/trunk/lib/secure...
[2] 
https://github.com/plenluno/openssl/blob/master/cr...
[3] 
https://github.com/plenluno/openssl/blob/master/cr...

Issue #6928 has been updated by mame (Yusuke Endoh).


Martin, may I postpone this to next minor?
Or must it be fixed immediately?

--
Yusuke Endoh <mame@tsg.ne.jp>

----------------------------------------
Bug #6928: SecureRandom.random_bytes: assume zero entropy for seed value
https://bugs.ruby-lang.org/issues/6928#change-36534

Author: MartinBosslet (Martin Bosslet)
Status: Assigned
Priority: Normal
Assignee: akr (Akira Tanaka)
Category: lib
Target version: 2.0.0
ruby -v: trunk


If OpenSSL is available SecureRandom.random_bytes uses
OpenSSL::Random.random_bytes and the random generator is reseeded [1]
whenever the current pid changes (due to repeated values when a pid
is reused, cf. #4579).

Since this seeding is also called the first time the method is entered,
using OpenSSL::Random.seed is potentially dangerous. 
OpenSSL::Random.seed
is equal to using OpenSSL::Random.random_add where it is assumed that 
the
string passed to seed possesses full entropy. This is definitely not the
case for pid and time values. In fact, OpenSSL itself assumes an entropy
of 1.0 or even 0.0 when doing similar seeding in RAND_poll [2][3]. 
However,
this seems to have no impact so far, since the OpenSSL random generator
gathers enough entropy on startup even if we seeded with what it would
consider enough bytes of entropy (32 by default). So even if our seed
string is already 32 bytes or larger, OpenSSL's RAND_poll still seems to
collect 32 bytes of entropy on initialization regardless of what has 
been
added/seeded so far, which is a good thing in this case. Still, this 
could
change over time if OpenSSL for example changes internal behaviour and
would decide that enough entropy had been provided while seeding.

Therefore I believe using OpenSSL::Random.random_add with an assumed
entropy of 0.0 might be a more defensive choice. The forking test from
#4579 still passes with the attached patch. What do you think?

[1] https://github.com/ruby/ruby/blob/trunk/lib/secure...
[2] 
https://github.com/plenluno/openssl/blob/master/cr...
[3] 
https://github.com/plenluno/openssl/blob/master/cr...

Issue #6928 has been updated by mame (Yusuke Endoh).

Target version changed from 2.0.0 to next minor

I assume that if this is so significant issue, Martin would have 
reported this to security@ruby-lang.org.
So I postpone this to next minor.

--
Yusuke Endoh <mame@tsg.ne.jp>
----------------------------------------
Bug #6928: SecureRandom.random_bytes: assume zero entropy for seed value
https://bugs.ruby-lang.org/issues/6928#change-36659

Author: MartinBosslet (Martin Bosslet)
Status: Assigned
Priority: Normal
Assignee: akr (Akira Tanaka)
Category: lib
Target version: next minor
ruby -v: trunk


If OpenSSL is available SecureRandom.random_bytes uses
OpenSSL::Random.random_bytes and the random generator is reseeded [1]
whenever the current pid changes (due to repeated values when a pid
is reused, cf. #4579).

Since this seeding is also called the first time the method is entered,
using OpenSSL::Random.seed is potentially dangerous. 
OpenSSL::Random.seed
is equal to using OpenSSL::Random.random_add where it is assumed that 
the
string passed to seed possesses full entropy. This is definitely not the
case for pid and time values. In fact, OpenSSL itself assumes an entropy
of 1.0 or even 0.0 when doing similar seeding in RAND_poll [2][3]. 
However,
this seems to have no impact so far, since the OpenSSL random generator
gathers enough entropy on startup even if we seeded with what it would
consider enough bytes of entropy (32 by default). So even if our seed
string is already 32 bytes or larger, OpenSSL's RAND_poll still seems to
collect 32 bytes of entropy on initialization regardless of what has 
been
added/seeded so far, which is a good thing in this case. Still, this 
could
change over time if OpenSSL for example changes internal behaviour and
would decide that enough entropy had been provided while seeding.

Therefore I believe using OpenSSL::Random.random_add with an assumed
entropy of 0.0 might be a more defensive choice. The forking test from
#4579 still passes with the attached patch. What do you think?

[1] https://github.com/ruby/ruby/blob/trunk/lib/secure...
[2] 
https://github.com/plenluno/openssl/blob/master/cr...
[3] 
https://github.com/plenluno/openssl/blob/master/cr...

Issue #6928 has been updated by MartinBosslet (Martin Bosslet).


mame (Yusuke Endoh) wrote:
> I assume that if this is so significant issue, Martin would have reported this 
to security@ruby-lang.org.
> So I postpone this to next minor.
>

Sorry for not responding in time. It is safe to move this to next minor 
- right now, the risk I mentioned is only hypothetical and would only 
affect us if OpenSSL decided to change their internals.

----------------------------------------
Bug #6928: SecureRandom.random_bytes: assume zero entropy for seed value
https://bugs.ruby-lang.org/issues/6928#change-36970

Author: MartinBosslet (Martin Bosslet)
Status: Assigned
Priority: Normal
Assignee: akr (Akira Tanaka)
Category: lib
Target version: next minor
ruby -v: trunk


If OpenSSL is available SecureRandom.random_bytes uses
OpenSSL::Random.random_bytes and the random generator is reseeded [1]
whenever the current pid changes (due to repeated values when a pid
is reused, cf. #4579).

Since this seeding is also called the first time the method is entered,
using OpenSSL::Random.seed is potentially dangerous. 
OpenSSL::Random.seed
is equal to using OpenSSL::Random.random_add where it is assumed that 
the
string passed to seed possesses full entropy. This is definitely not the
case for pid and time values. In fact, OpenSSL itself assumes an entropy
of 1.0 or even 0.0 when doing similar seeding in RAND_poll [2][3]. 
However,
this seems to have no impact so far, since the OpenSSL random generator
gathers enough entropy on startup even if we seeded with what it would
consider enough bytes of entropy (32 by default). So even if our seed
string is already 32 bytes or larger, OpenSSL's RAND_poll still seems to
collect 32 bytes of entropy on initialization regardless of what has 
been
added/seeded so far, which is a good thing in this case. Still, this 
could
change over time if OpenSSL for example changes internal behaviour and
would decide that enough entropy had been provided while seeding.

Therefore I believe using OpenSSL::Random.random_add with an assumed
entropy of 0.0 might be a more defensive choice. The forking test from
#4579 still passes with the attached patch. What do you think?

[1] https://github.com/ruby/ruby/blob/trunk/lib/secure...
[2] 
https://github.com/plenluno/openssl/blob/master/cr...
[3] 
https://github.com/plenluno/openssl/blob/master/cr...