Project

General

Profile

Actions

Bug #4579

closed

SecureRandom + OpenSSL may repeat with fork

Added by normalperson (Eric Wong) almost 13 years ago. Updated almost 13 years ago.

Status:
Closed
Target version:
ruby -v:
-
Backport:
[ruby-core:35765]

Description

=begin
This could arguably be a bug in OpenSSL or the openssl extension, but
I think it's easier to fix in Ruby right now.

The PRNG in OpenSSL uses the PID to seed the PRNG. Since PIDs get
recycled over time on Unix systems, this means independent processes
over a long time span will repeat random byte sequences. This has
security implications, but fortunately very little software forks
very frequently. I am not a security expert.

I am using OpenSSL 0.9.8g-15+lenny11 (Debian Lenny)

Attached is a script that reproduces the issue (takes a while to run).
It'll output two identical lines to illustrate the issue.

=end


Files

test_fork_random.rb (292 Bytes) test_fork_random.rb normalperson (Eric Wong), 04/15/2011 11:46 AM
ossl_rand.patch (848 Bytes) ossl_rand.patch kosaki (Motohiro KOSAKI), 04/16/2011 12:00 AM
ossl_rand2.patch (923 Bytes) ossl_rand2.patch kosaki (Motohiro KOSAKI), 04/16/2011 12:14 AM
securerandom_opensslfree.diff (2.69 KB) securerandom_opensslfree.diff nahi (Hiroshi Nakamura), 06/11/2011 06:36 PM
securerandom-openssl-pid-recycle.patch (543 Bytes) securerandom-openssl-pid-recycle.patch akr (Akira Tanaka), 06/13/2011 01:11 AM
securerandom.rb.diff (4.82 KB) securerandom.rb.diff nahi (Hiroshi Nakamura), 06/16/2011 08:05 PM

Updated by naruse (Yui NARUSE) almost 13 years ago

=begin
SecureRandom.random_bytes is just a wrapper of OpenSSL::Random.random_bytes(n) on systems with openssl.
And OpenSSL::Random.random_bytes is a wrapper of RAND_bytes(3).

You know its result depends pid and openssl ext should use RAND_add(3) or something.
http://www.openssl.org/docs/crypto/RAND_add.html
=end

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

=begin
I don't have an idea how OpenSSL can be 'fork-safe' for your purpose...

Call OpenSSL::Random.load_random_file("/dev/urandom" or "/dev/random" or ENV["RANDFILE"]) after each fork.
=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

=begin
Usually openssl read /dev/urandom only once. But RAND_cleanup() lead to read /dev/urandom again. Thus attached patch fixes this issue.

This is better patch than PAND_add(/dev/urandom) because openssl can use other entropy source internally.
=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

=begin
More paranoia patch is here.
=end

Updated by normalperson (Eric Wong) almost 13 years ago

=begin
I think RAND_cleanup() is enough and simpler. I'm also bringing this up on the openssl-dev mailing list.
=end

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

=begin
Motohiro: I don't know you're serious or not about using pthread_atfork(), we should ask to change OpenSSL's "1 time initialization by RAND_poll() per process when using built-in MD based RPNG engine" strategy if we really want.

Eric: I saw your post at openssl-dev[1]. Let's see how they treat this. I don't know OpenSSL can be 'fork-safe' for your purpose as I wrote. There must be other per-process initializations in it.

[1] http://marc.info/?l=openssl-dev&m=130289811108150&w=2

=end

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

  • ruby -v changed from ruby 1.9.3dev (2011-04-14 trunk 31267) [x86_64-linux] to -

=begin
Hi

Motohiro: I don't know you're serious or not about using pthread_atfork(), we should ask to change OpenSSL's "1 time initialization by RAND_poll() per process when using built-in MD based RPNG engine" strategy if we really want.

It's ruby's spec. We already decided random seed should reinitialize per fork.

void rb_thread_atfork(void)
{
rb_thread_atfork_internal(terminate_atfork_i);
GET_THREAD()->join_list_head = 0;

 /* We don't want reproduce CVE-2003-0900. */
 rb_reset_random_seed();

}

Now, SecureRandom is insecure than normal random from fork issue. It's
rather than unhappy.
=end

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

=begin
I think you're confusing SecureRandom's spec and ext/openssl (formerly ruby-pki) spec. ext/openssl aims to wrap OpenSSL that user's using so if OpenSSL is not 'fork-safe' as Eric expected, so ruby-pki doesn't.

So if OpenSSL can't change this behavior (I bet they can't at least in the near future), why don't we change lib/securerandom.rb?

The reason why I think you're not serious is adding ptherad_atfork() in ext is too ad-hoc-ish. We can't do it from Ruby world if I understand correctly. Adding atfork hook first?

To change (OK, 'fix') this behavior, it could be good to abandon using OpenSSL::Random.random_bytes(). We cannot use OpenSSL's engine which could provide physical random number but OpenSSL::Random.random_bytes must be enough for such user.

Tanaka-san, what do you think about the change?

=end

Updated by normalperson (Eric Wong) almost 13 years ago

=begin
Hiroshi NAKAMURA wrote:

I think you're confusing SecureRandom's spec and ext/openssl (formerly
ruby-pki) spec. ext/openssl aims to wrap OpenSSL that user's using so
if OpenSSL is not 'fork-safe' as Eric expected, so ruby-pki doesn't.

I hope everything in Ruby (including 3rd-party extensions/gems) can be
made fork-safe by default (if they run on a system with fork) one day.
I don't agree with blindly mimicking OpenSSL upstream behavior if Ruby
can be made easier-to-use.

So if OpenSSL can't change this behavior (I bet they can't at least in
the near future), why don't we change lib/securerandom.rb?

Yes, I confirmed OpenSSL can't change the current behavior:
http://marc.info/?l=openssl-dev&m=130298304903422&w=2

I'm still hoping to get a list of things that need to be reinitialized
in OpenSSL after fork() from openssl-dev...

The reason why I think you're not serious is adding ptherad_atfork()
in ext is too ad-hoc-ish. We can't do it from Ruby world if I
understand correctly. Adding atfork hook first?

I would be 100% in favor of making something analogous to
pthread_atfork() available in Ruby. It would make it much easier to
manage various resources in a multi-process situation

No comment on the appropriateness of pthread_atfork() inside an ext.

--
Eric Wong
=end

Updated by ko1 (Koichi Sasada) almost 13 years ago

  • Status changed from Open to Assigned
  • Assignee set to nahi (Hiroshi Nakamura)

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

Attached is the patch which removes OpenSSL dependency. Tanaka-san, aside from how OpenSSL.random_bytes should behave, can you accept this change?

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

Eeek. I dislike to remove OpenSSL dependency from SecureRadom. Because /dev/urandom is less secure than OpenSSL.

Updated by akr (Akira Tanaka) almost 13 years ago

This issue seems a OpenSSL issue.

Does someone reported to OpenSSL project?

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

Yes, comment#9 said,

Yes, I confirmed OpenSSL can't change the current behavior:
http://marc.info/?l=openssl-dev&m=130298304903422&w=2

Updated by akr (Akira Tanaka) almost 13 years ago

Hm.

I don't like pthread_atfork because the hook is run even if we don't need random functions in the child process.
(Remember only async signal safe functions are safe in forked child process)

We should delay modifying PRNG state until we really need it.

securerandom-openssl-pid-recycle.patch do it.
It should work until we have very fast machine which pid is recycled in a nano second.

Updated by kosaki (Motohiro KOSAKI) almost 13 years ago

The patch looks good to me.

Thank you.

Updated by normalperson (Eric Wong) almost 13 years ago

Motohiro KOSAKI wrote:

Eeek. I dislike to remove OpenSSL dependency from SecureRadom. Because
/dev/urandom is less secure than OpenSSL.

Is that only the case with poor urandom implementations in some
systems?

I'm fine with securerandom-openssl-pid-recycle.patch but I like
securerandom_opensslfree.diff since it removes OpenSSL dependency.
(I personally never liked OpenSSL)

Updated by akr (Akira Tanaka) almost 13 years ago

I think securerandom_opensslfree.diff is too radical for this issue.
It may decrease working platforms.

However concrete advantage/disadvantage between OpenSSL and /dev/urandom is interesting.
(portability, randomness quality, performance, ...)

Updated by akr (Akira Tanaka) almost 13 years ago

  • Status changed from Assigned to Closed
  • % Done changed from 0 to 100

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

Attached is the patch for 1.8.7. Urabe-san, can I apply it to ruby_1_8_7?

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

On Mon, Jun 13, 2011 at 17:07, Akira Tanaka wrote:

I think securerandom_opensslfree.diff is too radical for this issue.
It may decrease working platforms.

Agreed. Your fix is nice. We should have been aware of that. Thanks.

However concrete advantage/disadvantage between OpenSSL and /dev/urandom is interesting.
(portability, randomness quality, performance, ...)

On Linux, /dev/urandom seems to return the values which are
"theoretically vulnerable to a cryptographic attack on the algorithms
used by the driver" (from man page). I though it returns shorter bytes
than expected. I was wrong.

And using /dev/urandom every time consumes too much entropy that OS
has, so /dev/random users would not like it. We should avoid using
/dev/urandom every time on the env w/o OpenSSL in the future.

Regards,
// NaHi

Updated by shyouhei (Shyouhei Urabe) almost 13 years ago

Hmm, OK. Go ahead.

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

Hmm, OK.  Go ahead.

OK.

And I found that Tanaka-san already fixed it at ruby_1_8 as well as
trunk. :) I'll push it to ruby_1_8_7 with tests.

Regards,
// NaHi

Actions #24

Updated by akr (Akira Tanaka) almost 13 years ago

NaHi:

We should avoid using
/dev/urandom every time on the env w/o OpenSSL in the future.

I'd like to say "Please install OpenSSL" for such request.

Cryptographic algorithms should be implemented/maintained by cryptographic experts but I am not a cryptographic expert.

Updated by nahi (Hiroshi Nakamura) almost 13 years ago

Hi,

On Thu, Jun 23, 2011 at 08:15, Akira Tanaka wrote:

We should avoid using
/dev/urandom every time on the env w/o OpenSSL in the future.

I'd like to say "Please install OpenSSL" for such request.

Reasonable. Why don't you do so? I mean that removing /dev/urandom
fallback from securerandom.rb and letting simply warn "Please install
OpenSSL".

Cryptographic algorithms should be implemented/maintained by cryptographic experts but I am not a cryptographic expert.

You wrote securerandom.rb. I think it's too late. :-) :-) Joking
aside, since there's no cryptography expert around us, delegating PRNG
thing to OpenSSL is good I think.

Regards,
// NaHi

Updated by akr (Akira Tanaka) almost 13 years ago

I don't understand why /dev/urandom fallback should be removed.

Is your reason the "theoretically vulnerable to a cryptographic attack on the algorithms used by the driver" (from Linux man page)?

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0