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

Remove bind method from Safemode::Scope #47

Merged
merged 2 commits into from
May 5, 2023
Merged

Remove bind method from Safemode::Scope #47

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4edd6d1
Remove bind method from Safemode::Scope
adamruzicka Mar 27, 2023
69d703f
Fix Box#output
adamruzicka May 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions lib/safemode.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,16 @@ def find_jail_class(klass)

class Box
def initialize(delegate = nil, delegate_methods = [], filename = nil, line = nil)
@scope = Scope.new(delegate, delegate_methods)
adamruzicka marked this conversation as resolved.
Show resolved Hide resolved
@delegate = delegate
@delegate_methods = delegate_methods
@filename = filename
@line = line
end

def eval(code, assigns = {}, locals = {}, &block)
code = Parser.jail(code)
binding = @scope.bind(assigns, locals, &block)
result = Kernel.eval(code, binding, @filename || __FILE__, @line || __LINE__)
scope = Scope.new(@delegate, @delegate_methods, instance_vars: assigns, locals: locals, &block)
result = Kernel.eval(code, scope.get_binding, @filename || __FILE__, @line || __LINE__)
end

def output
Expand Down
12 changes: 6 additions & 6 deletions lib/safemode/scope.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
module Safemode
class Scope < Blankslate
def initialize(delegate = nil, delegate_methods = [])
def initialize(delegate = nil, delegate_methods = [], instance_vars: {}, locals: {}, &block)
@delegate = delegate
@delegate_methods = delegate_methods
@locals = {}
end

def bind(instance_vars = {}, locals = {}, &block)
@locals = symbolize_keys(locals) # why can't I just pull them to local scope in the same way like instance_vars?
instance_vars = symbolize_keys(instance_vars)
instance_vars.each {|key, obj| eval "@#{key} = instance_vars[:#{key}]" }
@_safemode_output = ''
binding
@binding = binding
end

def get_binding
@binding
end

def to_jail
Expand Down
4 changes: 4 additions & 0 deletions test/test_safemode_eval.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,10 @@ def test_interpolated_xstr_should_raise_security
assert_raise_security '"#{`ls -a`}"'
end

def test_should_not_allow_access_to_bind
assert_raise_security "self.bind('an arg')"
Copy link
Member

@ofedoren ofedoren Apr 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Even though there is no longer bind method, is this possible to misuse get_binding directly instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how.

The original issue was that one could call bind from a template and pass arguments to it, which were not treated safely. The arguments were just evaluated as-is to set values inside the binding. As far as I understand there wasn't anything inherently unsafe about being able to retrieve the binding, apart from the way how it could be obtained.

end

TestHelper.no_method_error_raising_calls.each do |call|
call.gsub!('"', '\\\\"')
class_eval %Q(
Expand Down