From 2a82609dc589e22064cfc9e8aa50b582a717bbdc Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Wed, 17 Feb 2021 21:41:03 -0800 Subject: [PATCH 01/12] Very simplest return values --- lib/brakeman/processors/alias_processor.rb | 9 +++++ .../processors/lib/call_conversion_helper.rb | 6 --- lib/brakeman/tracker/collection.rb | 40 +++++++++++++++++++ lib/brakeman/util.rb | 8 +++- test/apps/rails6/app/models/group.rb | 8 ++++ 5 files changed, 64 insertions(+), 7 deletions(-) diff --git a/lib/brakeman/processors/alias_processor.rb b/lib/brakeman/processors/alias_processor.rb index b3305bb5b0..b5d12ecbde 100644 --- a/lib/brakeman/processors/alias_processor.rb +++ b/lib/brakeman/processors/alias_processor.rb @@ -208,6 +208,15 @@ def process_call exp return Sexp.new(:false).line(exp.line) end + # For the simplest case of `Foo.thing` + if node_type? target, :const and first_arg.nil? + if tracker and (klass = tracker.find_class(class_name(target.value))) + if return_value = klass.get_simple_method_return_value(:class, method) + return return_value.deep_clone(exp.line) + end + end + end + #See if it is possible to simplify some basic cases #of addition/concatenation. case method diff --git a/lib/brakeman/processors/lib/call_conversion_helper.rb b/lib/brakeman/processors/lib/call_conversion_helper.rb index efadb43226..59215976fd 100644 --- a/lib/brakeman/processors/lib/call_conversion_helper.rb +++ b/lib/brakeman/processors/lib/call_conversion_helper.rb @@ -1,11 +1,5 @@ module Brakeman module CallConversionHelper - def all_literals? exp, expected_type = :array - node_type? exp, expected_type and - exp.length > 1 and - exp.all? { |e| e.is_a? Symbol or node_type? e, :lit, :str } - end - # Join two array literals into one. def join_arrays lhs, rhs, original_exp = nil if array? lhs and array? rhs diff --git a/lib/brakeman/tracker/collection.rb b/lib/brakeman/tracker/collection.rb index 903f98187d..41ab19606a 100644 --- a/lib/brakeman/tracker/collection.rb +++ b/lib/brakeman/tracker/collection.rb @@ -15,6 +15,7 @@ def initialize name, parent, file_name, src, tracker @includes = [] @methods = { :public => {}, :private => {}, :protected => {} } @class_methods = {} + @simple_methods = { :class => {}, instance: {} } @options = {} @tracker = tracker @@ -49,6 +50,7 @@ def add_option name, exp def add_method visibility, name, src, file_name meth_info = Brakeman::MethodInfo.new(name, src, self, file_name) + add_simple_method_maybe meth_info if src.node_type == :defs @class_methods[name] = meth_info @@ -112,5 +114,43 @@ def top_line def methods_public @methods[:public] end + + def get_simple_method_return_value type, name + @simple_methods[type][name] + end + + private + + def add_simple_method_maybe meth_info + src = meth_info.src + + # Simple methods have one (simple) expression in the body and + # no arguments + if src.formal_args.length == 1 # no args + body = src.body + if body.length == 1 # single expression in body + value = body.first + + if simple_literal? value or + (array? value and all_literals? value) + + add_simple_method meth_info, value + end + end + end + end + + def add_simple_method meth_info, value + name = meth_info.name + + case meth_info.src.node_type + when :defn + @simple_methods[:instance][name] = value + when :defs + @simple_methods[:class][name] = value + else + raise "Expected sexp type: #{src.node_type}" + end + end end end diff --git a/lib/brakeman/util.rb b/lib/brakeman/util.rb index 9507003e52..137f9d23a3 100644 --- a/lib/brakeman/util.rb +++ b/lib/brakeman/util.rb @@ -293,7 +293,13 @@ def node_type? exp, *types exp.is_a? Sexp and types.include? exp.node_type end - LITERALS = [:lit, :false, :str, :true, :array, :hash] + SIMPLE_LITERALS = [:lit, :false, :str, :true] + + def simple_literal? exp + exp.is_a? Sexp and SIMPLE_LITERALS.include? exp.node_type + end + + LITERALS = [*SIMPLE_LITERALS, :array, :hash] def literal? exp exp.is_a? Sexp and LITERALS.include? exp.node_type diff --git a/test/apps/rails6/app/models/group.rb b/test/apps/rails6/app/models/group.rb index a2fc777764..4f5daed606 100644 --- a/test/apps/rails6/app/models/group.rb +++ b/test/apps/rails6/app/models/group.rb @@ -18,4 +18,12 @@ def fetch_constant_hash_value(role_name) role = roles.fetch(role_name) Arel.sql("role = '#{role}'") end + + def use_simple_method + self.where("thing = #{Group.simple_method}") + end + + def self.simple_method + "Hello" + end end From f1378ae01030629e84c97707b55f5f515cd16b26 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Mon, 22 Feb 2021 23:10:52 -0800 Subject: [PATCH 02/12] Slightly better pluralize --- lib/brakeman/util.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/lib/brakeman/util.rb b/lib/brakeman/util.rb index 137f9d23a3..265e2e5c22 100644 --- a/lib/brakeman/util.rb +++ b/lib/brakeman/util.rb @@ -50,7 +50,11 @@ def underscore camel_cased_word # stupid simple, used to delegate to ActiveSupport def pluralize word - word + "s" + if word.end_with? 's' + word + 'es' + else + word + 's' + end end #Returns a class name as a Symbol. From 217eecd6bc28084fbbf18954580e1177b19fae46 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Mon, 22 Feb 2021 23:24:13 -0800 Subject: [PATCH 03/12] Add Sexp#method_length for efficiency --- lib/ruby_parser/bm_sexp.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/lib/ruby_parser/bm_sexp.rb b/lib/ruby_parser/bm_sexp.rb index abc6f6f007..3cfe7dd30e 100644 --- a/lib/ruby_parser/bm_sexp.rb +++ b/lib/ruby_parser/bm_sexp.rb @@ -543,6 +543,20 @@ def body_list self.body.unshift :rlist end + # Number of "statements" in a method. + # This is more effecient than `Sexp#body.length` + # because `Sexp#body` creates a new Sexp. + def method_length + expect :defn, :defs + + case self.node_type + when :defn + self.length - 3 + when :defs + self.length - 4 + end + end + def render_type expect :render self[1] From 02b9bd1a8679679dc117f6171cf74192be4c8fb5 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Tue, 23 Feb 2021 10:07:59 -0800 Subject: [PATCH 04/12] Better faster simple method check --- lib/brakeman/tracker/method_info.rb | 39 +++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/lib/brakeman/tracker/method_info.rb b/lib/brakeman/tracker/method_info.rb index 6df344b8d5..e423f40497 100644 --- a/lib/brakeman/tracker/method_info.rb +++ b/lib/brakeman/tracker/method_info.rb @@ -25,5 +25,44 @@ def initialize name, src, owner, file def [] attr self.send(attr) end + + def very_simple_method? + return @simple_method == :very unless @simple_method.nil? + + # Very simple methods have one (simple) expression in the body and + # no arguments + if src.formal_args.length == 1 # no args + if src.method_length == 1 # Single expression in body + value = first_body # First expression in body + + if simple_literal? value or + (array? value and all_literals? value) or + (hash? value and all_literals? value, :hash) + + @return_value = value + @simple_method = :very + end + end + end + + @simple_method ||= false + end + + def return_value env = nil + if very_simple_method? + return @return_value + else + nil + end + end + + def first_body + case @type + when :class + src[4] + when :instance + src[3] + end + end end end From 304534da6952d3cd5f1a73543fabf643a8a031a8 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Tue, 23 Feb 2021 10:23:21 -0800 Subject: [PATCH 05/12] Support ActiveRecord enums --- lib/brakeman/processors/model_processor.rb | 29 ++++++++++++++++++++++ test/apps/rails6/app/models/group.rb | 6 +++++ 2 files changed, 35 insertions(+) diff --git a/lib/brakeman/processors/model_processor.rb b/lib/brakeman/processors/model_processor.rb index 4fa517a2da..495d9d0c32 100644 --- a/lib/brakeman/processors/model_processor.rb +++ b/lib/brakeman/processors/model_processor.rb @@ -73,6 +73,8 @@ def process_call exp @current_class.set_attr_accessible exp when :attr_protected @current_class.set_attr_protected exp + when :enum + add_enum_method exp else if @current_class @current_class.add_option method, exp @@ -87,4 +89,31 @@ def process_call exp call end end + + def add_enum_method call + arg = call.first_arg + return unless hash? arg + + enum_name = arg[1].value # first key + enums = arg[2] # first value + enums_name = pluralize(enum_name.to_s).to_sym + + if hash? enums + enum_values = enums + elsif array? enums + # Build hash for enum values like Rails does + enum_values = s(:hash).line(call.line) + + enums.each_sexp.with_index do |v, index| + enum_values << v + enum_values << s(:lit, index) + end + end + + enum_method = s(:defs, s(:self), enum_name, s(:args), safe_literal(call.line)) + enums_method = s(:defs, s(:self), enums_name, s(:args), enum_values) + + @current_class.add_method :public, enum_name, enum_method, @current_file + @current_class.add_method :public, enums_name, enums_method, @current_file + end end diff --git a/test/apps/rails6/app/models/group.rb b/test/apps/rails6/app/models/group.rb index 4f5daed606..21be6ab739 100644 --- a/test/apps/rails6/app/models/group.rb +++ b/test/apps/rails6/app/models/group.rb @@ -1,4 +1,6 @@ class Group < ApplicationRecord + enum status: { start: 0, stop: 2, in_process: 3 } + def uuid_in_sql ActiveRecord::Base.connection.exec_query("select * where x = #{User.uuid}") end @@ -26,4 +28,8 @@ def use_simple_method def self.simple_method "Hello" end + + def use_enum + self.where("thing IN #{Group.statuses.values_at(*[:start, :stop]).join(',')}") + end end From be0cfb4e3ebb415bc8e478e966b89ed639fc4aa7 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Wed, 17 Feb 2021 21:10:58 -0800 Subject: [PATCH 06/12] Move all_literals? to Brakeman::Util --- lib/brakeman/util.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/brakeman/util.rb b/lib/brakeman/util.rb index 265e2e5c22..78dfb1029b 100644 --- a/lib/brakeman/util.rb +++ b/lib/brakeman/util.rb @@ -309,6 +309,12 @@ def literal? exp exp.is_a? Sexp and LITERALS.include? exp.node_type end + def all_literals? exp, expected_type = :array + node_type? exp, expected_type and + exp.length > 1 and + exp.all? { |e| e.is_a? Symbol or node_type? e, :lit, :str } + end + DIR_CONST = s(:const, :Dir) # Dir.glob(...).whatever From 964c37f0edd402cee213d648b226a4aa73ea5c5f Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Sun, 13 Jun 2021 11:02:46 -0700 Subject: [PATCH 07/12] Check for Hash literals too for simple methods --- lib/brakeman/tracker/collection.rb | 3 ++- test/apps/rails6/app/models/group.rb | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/brakeman/tracker/collection.rb b/lib/brakeman/tracker/collection.rb index 41ab19606a..d6153181e6 100644 --- a/lib/brakeman/tracker/collection.rb +++ b/lib/brakeman/tracker/collection.rb @@ -132,7 +132,8 @@ def add_simple_method_maybe meth_info value = body.first if simple_literal? value or - (array? value and all_literals? value) + all_literals? value, :array or + all_literals? value, :hash add_simple_method meth_info, value end diff --git a/test/apps/rails6/app/models/group.rb b/test/apps/rails6/app/models/group.rb index 21be6ab739..cca3b2fdb9 100644 --- a/test/apps/rails6/app/models/group.rb +++ b/test/apps/rails6/app/models/group.rb @@ -22,6 +22,7 @@ def fetch_constant_hash_value(role_name) end def use_simple_method + # No warning self.where("thing = #{Group.simple_method}") end @@ -30,6 +31,7 @@ def self.simple_method end def use_enum + # No warning self.where("thing IN #{Group.statuses.values_at(*[:start, :stop]).join(',')}") end end From 3554a2c7f259b3a96dc69877c9596c81190eed8d Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Thu, 17 Jun 2021 17:01:00 -0700 Subject: [PATCH 08/12] Support Hash#include? like Array#include? --- lib/brakeman/processors/alias_processor.rb | 26 +++++++++++++++++-- .../app/controllers/groups_controller.rb | 11 ++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/lib/brakeman/processors/alias_processor.rb b/lib/brakeman/processors/alias_processor.rb index b5d12ecbde..6207208b14 100644 --- a/lib/brakeman/processors/alias_processor.rb +++ b/lib/brakeman/processors/alias_processor.rb @@ -803,6 +803,18 @@ def process_cdecl exp exp end + def hash_or_array_include_all_literals? exp + return unless call? exp and sexp? exp.target + target = exp.target + + case target.node_type + when :hash + hash_include_all_literals? exp + else + array_include_all_literals? exp + end + end + # Check if exp is a call to Array#include? on an array literal # that contains all literal values. For example: # @@ -821,6 +833,16 @@ def array_detect_all_literals? exp (all_literals? exp.target or dir_glob? exp.target) end + # Check if exp is a call to Hash#include? on a hash literal + # that contains all literal values. For example: + # + # {x: 1}.include? x + def hash_include_all_literals? exp + call? exp and + exp.method == :include? and + all_literals? exp.target, :hash + end + #Sets @inside_if = true def process_if exp if @ignore_ifs.nil? @@ -861,7 +883,7 @@ def process_if exp scope do @branch_env = env.current branch_index = 2 + i # s(:if, condition, then_branch, else_branch) - if i == 0 and array_include_all_literals? condition + if i == 0 and hash_or_array_include_all_literals? condition # If the condition is ["a", "b"].include? x # set x to "a" inside the true branch var = condition.first_arg @@ -869,7 +891,7 @@ def process_if exp env.current[var] = safe_literal(var.line) exp[branch_index] = process_if_branch branch env.current[var] = previous_value - elsif i == 1 and array_include_all_literals? condition and early_return? branch + elsif i == 1 and hash_or_array_include_all_literals? condition and early_return? branch var = condition.first_arg env.current[var] = safe_literal(var.line) exp[branch_index] = process_if_branch branch diff --git a/test/apps/rails6/app/controllers/groups_controller.rb b/test/apps/rails6/app/controllers/groups_controller.rb index 2510140123..4468113ea8 100644 --- a/test/apps/rails6/app/controllers/groups_controller.rb +++ b/test/apps/rails6/app/controllers/groups_controller.rb @@ -69,4 +69,15 @@ def test_rails6_sqli User.order("name #{params[:direction]}") # Warn in 6.0, not in 6.1 User.order(:name).reorder(params[:column]) # Warn in 6.0, not in 6.1 end + + # From https://github.com/presidentbeef/brakeman/issues/1492 + def enum_include_check + status = "#{params[:status]}" + if Group.statuses.include? status + @status = status.to_sym + @countries = Group.send(@status) # Should not warn + else + redirect_to root_path, notice: 'Invalid status' + end + end end From 5342cbef6e46ce2dda6945c7745fbd6eec65cf8a Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Thu, 8 Jul 2021 16:22:19 -0700 Subject: [PATCH 09/12] Enum name is an instance method, not class method --- lib/brakeman/processors/model_processor.rb | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/brakeman/processors/model_processor.rb b/lib/brakeman/processors/model_processor.rb index 495d9d0c32..8a92187e00 100644 --- a/lib/brakeman/processors/model_processor.rb +++ b/lib/brakeman/processors/model_processor.rb @@ -98,19 +98,21 @@ def add_enum_method call enums = arg[2] # first value enums_name = pluralize(enum_name.to_s).to_sym + call_line = call.line + if hash? enums enum_values = enums elsif array? enums # Build hash for enum values like Rails does - enum_values = s(:hash).line(call.line) + enum_values = s(:hash).line(call_line) enums.each_sexp.with_index do |v, index| enum_values << v - enum_values << s(:lit, index) + enum_values << s(:lit, index).line(call_line) end end - enum_method = s(:defs, s(:self), enum_name, s(:args), safe_literal(call.line)) + enum_method = s(:defn, enum_name, s(:args), safe_literal(call_line)) enums_method = s(:defs, s(:self), enums_name, s(:args), enum_values) @current_class.add_method :public, enum_name, enum_method, @current_file From 73b87107193f0a3a5cd4ec772840891dc05df168 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Mon, 12 Jul 2021 17:02:43 -0700 Subject: [PATCH 10/12] Test the array form of `enum` --- test/apps/rails6/app/models/user.rb | 6 ++++++ test/tests/rails6.rb | 14 ++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/test/apps/rails6/app/models/user.rb b/test/apps/rails6/app/models/user.rb index f339f4f2b0..24c890c333 100644 --- a/test/apps/rails6/app/models/user.rb +++ b/test/apps/rails6/app/models/user.rb @@ -24,4 +24,10 @@ def self.more_heredocs def recent_stuff where("date > #{Date.today - 1}") end + + enum state: ["pending", "active", "archived"] + + def check_enum + where("state = #{User.states["pending"]}") + end end diff --git a/test/tests/rails6.rb b/test/tests/rails6.rb index 3323cd16e0..fd75f22508 100644 --- a/test/tests/rails6.rb +++ b/test/tests/rails6.rb @@ -186,6 +186,7 @@ def test_sql_injection_with_date :user_input => s(:call, s(:call, s(:const, :Date), :today), :-, s(:lit, 1)) end +<<<<<<< HEAD def test_sql_injection_rewhere assert_warning :type => :warning, :warning_code => 0, @@ -254,6 +255,19 @@ def test_sql_injection_reorder :user_input => s(:call, s(:params), :[], s(:lit, :column)) end + def test_sql_injection_enum + assert_no_warning :type => :warning, + :warning_code => 0, + :fingerprint => "b2071137eba7ef6ecbcc1c6381a428e5c576a5fadf73dc04b2e155c41043e1d2", + :warning_type => "SQL Injection", + :line => 31, + :message => /^Possible\ SQL\ injection/, + :confidence => 0, + :relative_path => "app/models/user.rb", + :code => s(:call, nil, :where, s(:dstr, "state = ", s(:evstr, s(:call, s(:call, s(:const, :User), :states), :[], s(:str, "pending"))))), + :user_input => s(:call, s(:call, s(:const, :User), :states), :[], s(:str, "pending")) + end + def test_cross_site_scripting_sanity assert_warning :type => :template, :warning_code => 2, From e44ae9ac3e17bc8550774e11b8bece2e9f6a2db0 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Tue, 13 Jul 2021 12:17:13 -0700 Subject: [PATCH 11/12] Use simple method checks from MethInfo --- lib/brakeman/tracker/collection.rb | 27 +++++++-------------------- lib/brakeman/tracker/method_info.rb | 2 ++ 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/lib/brakeman/tracker/collection.rb b/lib/brakeman/tracker/collection.rb index d6153181e6..0255b1aee1 100644 --- a/lib/brakeman/tracker/collection.rb +++ b/lib/brakeman/tracker/collection.rb @@ -50,7 +50,7 @@ def add_option name, exp def add_method visibility, name, src, file_name meth_info = Brakeman::MethodInfo.new(name, src, self, file_name) - add_simple_method_maybe meth_info + add_simple_method_maybe meth_info if src.node_type == :defs @class_methods[name] = meth_info @@ -121,28 +121,15 @@ def get_simple_method_return_value type, name private - def add_simple_method_maybe meth_info - src = meth_info.src - - # Simple methods have one (simple) expression in the body and - # no arguments - if src.formal_args.length == 1 # no args - body = src.body - if body.length == 1 # single expression in body - value = body.first - - if simple_literal? value or - all_literals? value, :array or - all_literals? value, :hash - - add_simple_method meth_info, value - end - end + def add_simple_method_maybe meth_info + if meth_info.very_simple_method? + add_simple_method meth_info end end - def add_simple_method meth_info, value - name = meth_info.name + def add_simple_method meth_info + name = meth_info.name + value = meth_info.return_value case meth_info.src.node_type when :defn diff --git a/lib/brakeman/tracker/method_info.rb b/lib/brakeman/tracker/method_info.rb index e423f40497..31a2b6b4f4 100644 --- a/lib/brakeman/tracker/method_info.rb +++ b/lib/brakeman/tracker/method_info.rb @@ -19,6 +19,8 @@ def initialize name, src, owner, file else raise "Expected sexp type: #{src.node_type}" end + + @simple_method = nil end # To support legacy code that expected a Hash From 1920a237de03ed9b01929afbbdda800becff0879 Mon Sep 17 00:00:00 2001 From: Justin Collins Date: Tue, 13 Jul 2021 15:23:11 -0700 Subject: [PATCH 12/12] Additional test for enums --- test/apps/rails6/app/models/group.rb | 4 ++-- test/tests/rails6.rb | 14 +++++++++++++- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/test/apps/rails6/app/models/group.rb b/test/apps/rails6/app/models/group.rb index cca3b2fdb9..0ceba8ec68 100644 --- a/test/apps/rails6/app/models/group.rb +++ b/test/apps/rails6/app/models/group.rb @@ -1,6 +1,4 @@ class Group < ApplicationRecord - enum status: { start: 0, stop: 2, in_process: 3 } - def uuid_in_sql ActiveRecord::Base.connection.exec_query("select * where x = #{User.uuid}") end @@ -30,6 +28,8 @@ def self.simple_method "Hello" end + enum status: { start: 0, stop: 2, in_process: 3 } + def use_enum # No warning self.where("thing IN #{Group.statuses.values_at(*[:start, :stop]).join(',')}") diff --git a/test/tests/rails6.rb b/test/tests/rails6.rb index fd75f22508..d9fa71d263 100644 --- a/test/tests/rails6.rb +++ b/test/tests/rails6.rb @@ -186,7 +186,6 @@ def test_sql_injection_with_date :user_input => s(:call, s(:call, s(:const, :Date), :today), :-, s(:lit, 1)) end -<<<<<<< HEAD def test_sql_injection_rewhere assert_warning :type => :warning, :warning_code => 0, @@ -268,6 +267,19 @@ def test_sql_injection_enum :user_input => s(:call, s(:call, s(:const, :User), :states), :[], s(:str, "pending")) end + def test_dangerous_send_enum + assert_no_warning :type => :warning, + :warning_code => 23, + :fingerprint => "483fa36e41f5791e86f345a19b517a61859886d685ce40ef852871bb7a935f2d", + :warning_type => "Dangerous Send", + :line => 78, + :message => /^User\ controlled\ method\ execution/, + :confidence => 0, + :relative_path => "app/controllers/groups_controller.rb", + :code => s(:call, s(:const, :Group), :send, s(:call, s(:dstr, "", s(:evstr, s(:call, s(:params), :[], s(:lit, :status)))), :to_sym)), + :user_input => s(:call, s(:params), :[], s(:lit, :status)) + end + def test_cross_site_scripting_sanity assert_warning :type => :template, :warning_code => 2,