Merge pull request #1899 from presidentbeef/update_eval_check

Update `eval` check to be a little noisier

lib/brakeman/checks/check_evaluation.rb

@@ -23,13 +23,31 @@ def process_result result
     return unless original? result
 
     if input = include_user_input?(result[:call].arglist)
+      confidence = :high
+      message = msg(msg_input(input), " evaluated as code")
+    elsif string_evaluation? result[:call].first_arg
+      confidence = :low
+      message = "Dynamic string evaluated as code"
+    elsif safe_literal? result[:call].first_arg
+      # don't warn
+    elsif result[:call].method == :eval
+      confidence = :low
+      message = "Dynamic code evaluation"
+    end
+
+    if confidence
       warn :result => result,
         :warning_type => "Dangerous Eval",
         :warning_code => :code_eval,
-        :message => "User input in eval",
+        :message => message,
         :user_input => input,
-        :confidence => :high,
+        :confidence => confidence,
         :cwe_id => [913, 95]
     end
   end
+
+  def string_evaluation? exp
+    string_interp? exp or
+      (call? exp and string? exp.target)
+  end
 end

test/apps/rails8/lib/evals.rb

@@ -0,0 +1,17 @@
+class Evals
+  def evals
+    instance_eval "plain string - no warning" 
+    instance_eval "interpolated #{string} - warning"
+    instance_eval anything_else # no warning
+
+    eval anything # warning
+
+    self.class.class_eval do
+      # no warning
+    end
+
+    if [1, 2, 3].include? code
+      eval code # no warning
+    end
+  end
+end

test/tests/rails2.rb

@@ -32,7 +32,7 @@ def test_config_sanity
   def test_eval
     assert_warning :warning_type => "Dangerous Eval",
       :line => 40,
-      :message => /^User input in eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :format_code => /eval\(params\[:dangerous_input\]\)/,
       :file => /home_controller.rb/,
       :relative_path => "app/controllers/home_controller.rb"

test/tests/rails3.rb

@@ -38,7 +38,7 @@ def test_eval_params
       :fingerprint => "4efdd73fb759135f5980b5da1d9804aa4eb5c7475eabfd0f0cf41299d1d7ec42",
       :warning_type => "Dangerous Eval",
       :line => 40,
-      :message => /^User input in eval near line 40: eval\(pa/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :file => /home_controller\.rb/
   end

test/tests/rails31.rb

@@ -1370,7 +1370,7 @@ def test_eval_from_lambda_filter
       :fingerprint => "58e5c4088dc57057a112ab5c472633752f787b8f6b0437bbd19d82fa06afbddb",
       :warning_type => "Dangerous Eval",
       :line => 53,
-      :message => /^User\ input\ in\ eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :relative_path => "app/controllers/admin_controller.rb",
       :user_input => s(:call, s(:params), :[], s(:lit, :t))

test/tests/rails4.rb

@@ -1632,7 +1632,7 @@ def test_before_filter_block
       :fingerprint => "f8081023e9a6026264eaee41a4a1f520fc98ee5dbcba2129245e6a3873cb6409",
       :warning_type => "Dangerous Eval",
       :line => 7,
-      :message => /^User\ input\ in\ eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :relative_path => "app/controllers/another_controller.rb",
       :method => :before_filter,
@@ -1645,7 +1645,7 @@ def test_eval_duplicates
       :fingerprint => "33067304aaa21c6a874fed3b9bb0084cb66b607cc620065cb8ab06a640d3ab14",
       :warning_type => "Dangerous Eval",
       :line => 88,
-      :message => /^User\ input\ in\ eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :relative_path => "app/controllers/users_controller.rb",
       :user_input => s(:call, s(:params), :[], s(:lit, :x))
@@ -1655,7 +1655,7 @@ def test_eval_duplicates
       :fingerprint => "dc94bedbdf82991d7a356de94650325c256c5876227480b3b98e24aadaab1fd5",
       :warning_type => "Dangerous Eval",
       :line => 1,
-      :message => /^User\ input\ in\ eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :relative_path => "app/views/users/eval_it.html.erb",
       :user_input => s(:call, s(:params), :[], s(:lit, :x))
@@ -1667,7 +1667,7 @@ def test_private_call
       :fingerprint => "f0463ae920dc6ebbed7f66d0bdf1cc41b7c7257f7f724107377d7c59c5ee8707",
       :warning_type => "Dangerous Eval",
       :line => 76,
-      :message => /^User\ input\ in\ eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :relative_path => "app/controllers/friendly_controller.rb",
       :code => s(:call, nil, :eval, s(:call, s(:call, nil, :params), :[], s(:lit, :what_is_this_java?))),

test/tests/rails5.rb

@@ -422,7 +422,7 @@ def test_warning_in_helper_method
       :fingerprint => "e90f8e364e35ed2f6a56b4597e7de8945c836c75ef673006d960a380ecdf47e8",
       :warning_type => "Dangerous Eval",
       :line => 3,
-      :message => /^User\ input\ in\ eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :relative_path => "app/helpers/users_helper.rb",
       :code => s(:call, nil, :eval, s(:call, s(:params), :[], s(:lit, :x))),
@@ -723,7 +723,7 @@ def test_dangerous_eval_in_prior_class_method_with_same_name
       :fingerprint => "7fe3142d1d11b7118463e45a82b4b7a2b5b5bac95cf8904050c101fae16b8168",
       :warning_type => "Dangerous Eval",
       :line => 7,
-      :message => /User input in eval near line 7/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :method => :"User.evaluate_user_input",
       :confidence => 0,
       :relative_path => "app/models/user.rb",

test/tests/rails6.rb

@@ -806,7 +806,7 @@ def test_skip_dev_environment
       :fingerprint => "a7759c4ad34056fffc847aff31c9b40d90803cd5637a7189b0edfd7615132f37",
       :warning_type => "Dangerous Eval",
       :line => 54,
-      :message => /^User\ input\ in\ eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :relative_path => "app/controllers/groups_controller.rb",
       :code => s(:call, nil, :eval, s(:call, s(:params), :[], s(:lit, :x))),
@@ -819,7 +819,7 @@ def test_dangerous_eval_as_method_target
       :fingerprint => "3c4b94f3fc4ff4cfb005299349eb4f9a89832f35fc33ed9edc8481b98a047edb",
       :warning_type => "Dangerous Eval",
       :line => 27,
-      :message => /^User\ input\ in\ eval/,
+      :message => /^Parameter\ value\ evaluated\ as\ code/,
       :confidence => 0,
       :relative_path => "app/controllers/accounts_controller.rb",
       :code => s(:call, nil, :eval, s(:call, s(:params), :[], s(:lit, :x))),

test/tests/rails8.rb

@@ -16,7 +16,35 @@ def expected
       controller: 0,
       model:      0,
       template:   0,
-      warning:    0
+      warning:    2
     }
   end
+
+  def test_dangerous_eval_1
+    assert_warning check_name: "Evaluation",
+      type: :warning,
+      warning_code: 13,
+      fingerprint: "00a0720a22562960da1d793140ccae5987da210d45fb8478daabad13a5901130",
+      warning_type: "Dangerous Eval",
+      line: 7,
+      message: /^Dynamic\ code\ evaluation/,
+      confidence: 2,
+      relative_path: "lib/evals.rb",
+      code: s(:call, nil, :eval, s(:call, nil, :anything)),
+      user_input: nil
+  end
+
+  def test_dangerous_eval_2
+    assert_warning check_name: "Evaluation",
+      type: :warning,
+      warning_code: 13,
+      fingerprint: "c9b3bb7b8898b84a0d82bf59a110b8259d4a7796a092aecda9910c8a02d7ae5e",
+      warning_type: "Dangerous Eval",
+      line: 4,
+      message: /^Dynamic\ string\ evaluated\ as\ code/,
+      confidence: 2,
+      relative_path: "lib/evals.rb",
+      code: s(:call, nil, :instance_eval, s(:dstr, "interpolated ", s(:evstr, s(:call, nil, :string)), s(:str, " - warning"))),
+      user_input: nil
+  end
 end