From 1e4316be886ed2c2629f7da5d6110c148a7ff222 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Mon, 20 Nov 2023 19:15:36 -0800 Subject: [PATCH] (PUP-11772) Resolve Security/Eval Both actions and functions/data types already define arbitrary code and are loaded from trusted locations, so using eval isn't any worse. I updated the ActionBuilder to delegate specific methods to the action. For example, if an action calls the DSL method `summary "something"`, then the ActionBuilder will call the corresponding setter on the Action, e.g. Action#summary = "something". The Action code is bit more complicated because the arity of the block passed to `when_invoked=` may be 0, positive or negative, depending on whether it accepts optional arguments. Since we don't support Ruby 1.8 - 2.6, it could be improved in the future to not call `eval`, but I didn't feel like bothering. --- .rubocop_todo.yml | 8 -------- lib/puppet/interface/action.rb | 6 ++++-- lib/puppet/interface/action_builder.rb | 13 ++++--------- .../pops/loader/ruby_data_type_instantiator.rb | 2 +- .../pops/loader/ruby_function_instantiator.rb | 2 +- .../loader/ruby_legacy_function_instantiator.rb | 2 +- 6 files changed, 11 insertions(+), 22 deletions(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index c1a1f67dcc5..6b79b03b89b 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -918,14 +918,6 @@ Naming/VariableName: Naming/VariableNumber: Enabled: false -Security/Eval: - Exclude: - - 'lib/puppet/interface/action.rb' - - 'lib/puppet/interface/action_builder.rb' - - 'lib/puppet/pops/loader/ruby_data_type_instantiator.rb' - - 'lib/puppet/pops/loader/ruby_function_instantiator.rb' - - 'lib/puppet/pops/loader/ruby_legacy_function_instantiator.rb' - # Configuration parameters: EnforcedStyle, AllowModifiersOnSymbols. # SupportedStyles: inline, group Style/AccessModifierDeclarations: diff --git a/lib/puppet/interface/action.rb b/lib/puppet/interface/action.rb index bb769ce466a..a30fcf78b76 100644 --- a/lib/puppet/interface/action.rb +++ b/lib/puppet/interface/action.rb @@ -265,12 +265,14 @@ def #{@name}(#{decl.join(", ")}) end WRAPPER + # It should be possible to rewrite this code to use `define_method` + # instead of `class/instance_eval` since Ruby 1.8 is long dead. if @face.is_a?(Class) - @face.class_eval do eval wrapper, nil, file, line end + @face.class_eval do eval wrapper, nil, file, line end # rubocop:disable Security/Eval @face.send(:define_method, internal_name, &block) @when_invoked = @face.instance_method(name) else - @face.instance_eval do eval wrapper, nil, file, line end + @face.instance_eval do eval wrapper, nil, file, line end # rubocop:disable Security/Eval @face.meta_def(internal_name, &block) @when_invoked = @face.method(name).unbind end diff --git a/lib/puppet/interface/action_builder.rb b/lib/puppet/interface/action_builder.rb index 9c8b8afcf5d..68ac69173a4 100644 --- a/lib/puppet/interface/action_builder.rb +++ b/lib/puppet/interface/action_builder.rb @@ -5,6 +5,8 @@ # within the context of a new instance of this class. # @api public class Puppet::Interface::ActionBuilder + extend Forwardable + # The action under construction # @return [Puppet::Interface::Action] # @api private @@ -142,15 +144,8 @@ def render_as(value = nil) property = setter.to_s.chomp('=') unless method_defined? property - # Using eval because the argument handling semantics are less awful than - # when we use the define_method/block version. The later warns on older - # Ruby versions if you pass the wrong number of arguments, but carries - # on, which is totally not what we want. --daniel 2011-04-18 - eval <<-METHOD - def #{property}(value) - @action.#{property} = value - end - METHOD + # ActionBuilder# delegates to Action# + def_delegator :@action, setter, property end end diff --git a/lib/puppet/pops/loader/ruby_data_type_instantiator.rb b/lib/puppet/pops/loader/ruby_data_type_instantiator.rb index 7eda1484e42..1e0014eba09 100644 --- a/lib/puppet/pops/loader/ruby_data_type_instantiator.rb +++ b/lib/puppet/pops/loader/ruby_data_type_instantiator.rb @@ -20,7 +20,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string) # make the private loader available in a binding to allow it to be passed on loader_for_type = loader.private_loader here = get_binding(loader_for_type) - created = eval(ruby_code_string, here, source_ref, 1) + created = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval unless created.is_a?(Puppet::Pops::Types::PAnyType) raise ArgumentError, _("The code loaded from %{source_ref} did not produce a data type when evaluated. Got '%{klass}'") % { source_ref: source_ref, klass: created.class } end diff --git a/lib/puppet/pops/loader/ruby_function_instantiator.rb b/lib/puppet/pops/loader/ruby_function_instantiator.rb index 14b305dbdfa..8742d9c48e9 100644 --- a/lib/puppet/pops/loader/ruby_function_instantiator.rb +++ b/lib/puppet/pops/loader/ruby_function_instantiator.rb @@ -20,7 +20,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string) # make the private loader available in a binding to allow it to be passed on loader_for_function = loader.private_loader here = get_binding(loader_for_function) - created = eval(ruby_code_string, here, source_ref, 1) + created = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval unless created.is_a?(Class) raise ArgumentError, _("The code loaded from %{source_ref} did not produce a Function class when evaluated. Got '%{klass}'") % { source_ref: source_ref, klass: created.class } end diff --git a/lib/puppet/pops/loader/ruby_legacy_function_instantiator.rb b/lib/puppet/pops/loader/ruby_legacy_function_instantiator.rb index 4c3e4c3c708..878ddec298e 100644 --- a/lib/puppet/pops/loader/ruby_legacy_function_instantiator.rb +++ b/lib/puppet/pops/loader/ruby_legacy_function_instantiator.rb @@ -38,7 +38,7 @@ def self.create(loader, typed_name, source_ref, ruby_code_string) # This will do the 3x loading and define the "function_" and "real_function_" methods # in the anonymous module used to hold function definitions. # - func_info = eval(ruby_code_string, here, source_ref, 1) + func_info = eval(ruby_code_string, here, source_ref, 1) # rubocop:disable Security/Eval # Validate what was loaded unless func_info.is_a?(Hash)