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.

Sign up for GitHub

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

Jump to bottom

Rescue RuntimeError when trying to use SecureRandom #1888

Merged
merged 2 commits into from
Feb 26, 2023
Merged

Rescue RuntimeError when trying to use SecureRandom #1888

merged 2 commits into from
Feb 26, 2023

Conversation

stefansundin
Copy link
Contributor

Hello,

I got a report from a user that he couldn't run my app on his Synology. Turns out it doesn't have urandom. stefansundin/rssbox#67

Relevant part of the log:

2023-02-25 15:21:22,stdout,	from /app/app.rb:4:in `require'
2023-02-25 15:21:22,stdout,	from /app/vendor/bundle/ruby/3.2.0/gems/sinatra-3.0.5/lib/sinatra.rb:3:in `<top (required)>'
2023-02-25 15:21:22,stdout,	from /app/vendor/bundle/ruby/3.2.0/gems/sinatra-3.0.5/lib/sinatra.rb:3:in `require'
2023-02-25 15:21:22,stdout,	from /app/vendor/bundle/ruby/3.2.0/gems/sinatra-3.0.5/lib/sinatra/main.rb:3:in `<top (required)>'
2023-02-25 15:21:22,stdout,	from /app/vendor/bundle/ruby/3.2.0/gems/sinatra-3.0.5/lib/sinatra/main.rb:28:in `<module:Sinatra>'
2023-02-25 15:21:22,stdout,	from /app/vendor/bundle/ruby/3.2.0/gems/sinatra-3.0.5/lib/sinatra/main.rb:28:in `require'
2023-02-25 15:21:22,stdout,	from /app/vendor/bundle/ruby/3.2.0/gems/sinatra-3.0.5/lib/sinatra/base.rb:20:in `<top (required)>'
2023-02-25 15:21:22,stdout,	from /app/vendor/bundle/ruby/3.2.0/gems/sinatra-3.0.5/lib/sinatra/base.rb:908:in `<module:Sinatra>'
2023-02-25 15:21:22,stdout,	from /app/vendor/bundle/ruby/3.2.0/gems/sinatra-3.0.5/lib/sinatra/base.rb:1840:in `<class:Base>'
2023-02-25 15:21:22,stdout,	from /usr/local/ruby/lib/ruby/3.2.0/random/formatter.rb:94:in `hex'
2023-02-25 15:21:22,stdout,	from /usr/local/ruby/lib/ruby/3.2.0/random/formatter.rb:74:in `random_bytes'
2023-02-25 15:21:22,stdout,	from /usr/local/ruby/lib/ruby/3.2.0/securerandom.rb:55:in `gen_random_openssl'
2023-02-25 15:21:22,stdout,/usr/local/ruby/lib/ruby/3.2.0/securerandom.rb:55:in `urandom': �[1mfailed to get urandom (�[1;4mRuntimeError�[m�[1m)�[m
2023-02-25 15:21:22,stdout,�[31mbundler: failed to load command: puma (/app/vendor/bundle/ruby/3.2.0/bin/puma)�[0m
2023-02-25 15:21:22,stdout,! Unable to load application: RuntimeError: failed to get urandom

Where the exception is raised in the Ruby code: https://github.com/ruby/ruby/blob/381a373ab92e2a5869e75f43815993cef39d32cf/random.c#L767

I can't find anything that supports that NotImplementedError is raised, but perhaps that is if both random and urandom is missing?

Maybe it would be better to just rescue all errors instead?

I have not tested this fix myself since I don't know if it is simple to get rid of urandom in a way that accurately represents the issue. I'll check with the user if he's willing to try out the change.. Maybe I'll build a special docker image for him to test.

Please let me know if you want me to change anything, or feel free to make changes on this branch directly. Thanks!

@stefansundin stefansundin mentioned this pull request Feb 25, 2023
Closed
@dentarg
Copy link
Member

dentarg commented Feb 26, 2023 •
edited
Loading

Hmm

From the stack trace above, we can see that it is the call to .hex that fails:

sinatra/lib/sinatra/base.rb

Lines 1837 to 1844 in 186106d

# explicitly generating a session secret eagerly to play nice with preforking
begin
require 'securerandom'
set :session_secret, SecureRandom.hex(64)
rescue LoadError, NotImplementedError
# SecureRandom raises a NotImplementedError if no random device is available
set :session_secret, format('%064x', Kernel.rand((2**256) - 1))
end

Ruby, from 3.2.0, does check for, and accounts for, the RuntimeError raised when urandom isn't available, when securerandom is loaded: https://github.com/ruby/ruby/blob/v3_2_0/lib/securerandom.rb#L75-L87

It looks like your user then manages to load openssl, and it is Random.urandom(16) in gen_random_openssl that fails: https://github.com/ruby/ruby/blob/v3_2_0/lib/securerandom.rb#L55

Not 100% we should be rescuing the RuntimeError in this case? It might hide errors that should be addressed by the end-user?

@dentarg
Copy link
Member

dentarg commented Feb 26, 2023

It looks like your user then manages to load openssl, and it is Random.urandom(16) in gen_random_openssl that fails: https://github.com/ruby/ruby/blob/v3_2_0/lib/securerandom.rb#L55

That has actually been reported in Ruby: https://bugs.ruby-lang.org/issues/19230

dentarg
dentarg reviewed Feb 26, 2023
View reviewed changes
lib/sinatra/base.rb Outdated Show resolved Hide resolved
@dentarg
Copy link
Member

dentarg commented Feb 26, 2023

I have not tested this fix myself since I don't know if it is simple to get rid of urandom in a way that accurately represents the issue.

Yeah, same here, did not find a way. I saw this Dockerfile linked from another Ruby bug, just doing RUN rm /dev/urandom, so I tried it in ruby:3.2.0 but I could not reproduce the failure.

dentarg
dentarg approved these changes Feb 26, 2023
View reviewed changes
Copy link
Member

@dentarg dentarg left a comment

Choose a reason for hiding this comment

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

I think we should merge this. Would appreciate input from someone more from @sinatra/sinatras-helpers

@jkowens
Copy link
Member

jkowens commented Feb 26, 2023

@dentarg I think this is ok to merge as well. Normally it would be good to let the Runtime exception be raised to indicate a system error, but this is very specific to generating a secret token which has a fallback.

@dentarg dentarg changed the title Rescue RuntimeError when urandom is not available Rescue RuntimeError when trying to use SecureRandom` Feb 26, 2023
@dentarg dentarg changed the title Rescue RuntimeError when trying to use SecureRandom` Rescue RuntimeError when trying to use SecureRandom Feb 26, 2023
@dentarg dentarg merged commit 055087c into sinatra:master Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dentarg dentarg dentarg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stefansundin @dentarg @jkowens