Skip to content

Commit

Permalink
(MCO-765) don't use eval un unchecked arguments
Browse files Browse the repository at this point in the history
This patch drops the use of `eval` to evaluate comparison expressions in
discovery filters. Instead the corresponding comparison method is called
on the arguments directly while coercing the arguments to appropriate data
types.
While this approach might produce different results than the original
implementation for expressions which relied on the `eval` to perform more
than just the comparison, it closes a whole lot of vulnerabilities stemming
from insufficient checking of the comparison arguments.
  • Loading branch information
mruzicka committed Jun 27, 2016
1 parent 9030ad1 commit 4de959d
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 8 deletions.
41 changes: 33 additions & 8 deletions lib/mcollective/matcher.rb
Expand Up @@ -157,11 +157,6 @@ def self.eval_compound_fstatement(function_hash)
return false
end

# Escape strings for evaluation
if (l_compare.is_a?(String) && !(function_hash["operator"] =~ /=~|!=~/))
r_compare = "\"#{r_compare}\""
end

# Do a regex comparison if right compare string is a regex
if operator=~ /(=~|!=~)/
# Fail if left compare value isn't a string
Expand All @@ -177,10 +172,40 @@ def self.eval_compound_fstatement(function_hash)
return !!result
end
end
# Otherwise evaluate the logical comparison
# Otherwise do a normal comparison while taking the type into account
else
l_compare = "\"#{l_compare}\"" if l_compare.is_a?(String)
result = eval("#{l_compare} #{operator} #{r_compare}")
if l_compare.is_a? String
r_compare = r_compare.to_s
elsif r_compare.is_a? String
if l_compare.is_a? Numeric
r_compare = r_compare.strip
begin
r_compare = Integer(r_compare)
rescue ArgumentError
begin
r_compare = Float(r_compare)
rescue ArgumentError
raise ArgumentError, "invalid numeric value: #{r_compare}"
end
end
elsif l_compare.is_a? TrueClass or l_compare.is_a? FalseClass
r_compare = r_compare.strip
if r_compare == true.to_s
r_compare = true
elsif r_compare == false.to_s
r_compare = false
else
raise ArgumentError, "invalid boolean value: #{r_compare}"
end
end
end
operator = operator.strip
if operator =~ /(?:(!=|<=|>=|<|>)|==?)/
operator = $1 ? $1.to_sym : :==
else
raise ArgumentError, "invalid operator: #{operator}"
end
result = l_compare.send(operator, r_compare)
return result
end
end
Expand Down
16 changes: 16 additions & 0 deletions spec/unit/mcollective/matcher_spec.rb
Expand Up @@ -262,6 +262,22 @@ module MCollective
result.should == true
end

it "should handle integer literals" do
Matcher.expects(:execute_function).returns(10)
@function_hash["r_compare"] = "0xa"
@function_hash["operator"] = "="
result = Matcher.eval_compound_fstatement(@function_hash)
result.should == true
end

it "should handle float literals" do
Matcher.expects(:execute_function).returns(50)
@function_hash["r_compare"] = "0.5e2"
@function_hash["operator"] = "="
result = Matcher.eval_compound_fstatement(@function_hash)
result.should == true
end

it "should return true if we do a false=false comparison" do
Matcher.expects(:execute_function).returns(false)
@function_hash["r_compare"] = false
Expand Down

0 comments on commit 4de959d

Please sign in to comment.