Bug #19230
closedThe openssl backend of securerandom is no longer needed
Description
securerandom first checks if Random.urandom is available (Line 77), and if not available, it uses the openssl backend as a degeneration.
However, the openssl backend does not work because it internally uses Random.urandom (Line 55) to create a seed.
This issue is found by @hanachin (Seiei Miyagi).
$ ruby -ve 'def Random.urandom(*); raise; end; require "securerandom"; p SecureRandom.bytes(10)'
ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [x86_64-linux]
-e:1: warning: method redefined; discarding old urandom
-e:1:in `urandom': unhandled exception
from /home/mame/local/lib/ruby/3.1.0/securerandom.rb:75:in `singleton class'
from /home/mame/local/lib/ruby/3.1.0/securerandom.rb:42:in `<module:SecureRandom>'
from /home/mame/local/lib/ruby/3.1.0/securerandom.rb:41:in `<top (required)>'
from <internal:/home/mame/local/lib/ruby/3.1.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
from <internal:/home/mame/local/lib/ruby/3.1.0/rubygems/core_ext/kernel_require.rb>:85:in `require'
from -e:1:in `<main>'
There has been this bug since abae70d6ed63054d7d01bd6cd80c1b5b98b93ba3, which made the urandom backend as default and left the openssl backend just for degeneration. I think no one need the openssl anymore because no one has reported this bug for such a long time.
How about removing it?
diff --git a/lib/securerandom.rb b/lib/securerandom.rb
index 07ae048634..32b76a2137 100644
--- a/lib/securerandom.rb
+++ b/lib/securerandom.rb
@@ -14,7 +14,6 @@
#
# It supports the following secure random number generators:
#
-# * openssl
# * /dev/urandom
# * Win32
#
@@ -46,21 +45,6 @@ def bytes(n)
private
- def gen_random_openssl(n)
- @pid = 0 unless defined?(@pid)
- pid = $$
- unless @pid == pid
- now = Process.clock_gettime(Process::CLOCK_REALTIME, :nanosecond)
- OpenSSL::Random.random_add([now, @pid, pid].join(""), 0.0)
- seed = Random.urandom(16)
- if (seed)
- OpenSSL::Random.random_add(seed, 16)
- end
- @pid = pid
- end
- return OpenSSL::Random.random_bytes(n)
- end
-
def gen_random_urandom(n)
ret = Random.urandom(n)
unless ret
@@ -77,13 +61,7 @@ def gen_random_urandom(n)
Random.urandom(1)
alias gen_random gen_random_urandom
rescue RuntimeError
- begin
- require 'openssl'
- rescue NoMethodError
- raise NotImplementedError, "No random device"
- else
- alias gen_random gen_random_openssl
- end
+ raise NotImplementedError, "No random device"
end
public :gen_random
Updated by zzak (zzak _) almost 2 years ago
Good catch!
I did a quick GitHub search to see if anyone might be calling this method directly, but given it's private maybe we shouldn't care? The first 10 pages showed no interesting results.
It seems this test will have to be removed, also in the gem, which seems like mame-san added :)
There was also this ticket in JRuby, which I guess means they Charlie can remove that exclusion from their test suite..
Updated by dentarg (Patrik Ragnarsson) over 1 year ago
Here's actually an user running into the above bug: https://github.com/stefansundin/rssbox/issues/67, https://github.com/sinatra/sinatra/pull/1888
but removing this code would address the issue too as Sinatra has some sort of fallback implemented
Updated by mame (Yusuke Endoh) over 1 year ago
- Status changed from Open to Assigned
- Assignee set to shyouhei (Shyouhei Urabe)
Updated by shyouhei (Shyouhei Urabe) over 1 year ago
Proposed fix: https://github.com/ruby/ruby/pull/7705
Updated by jeremyevans0 (Jeremy Evans) about 1 year ago
- Status changed from Assigned to Closed