提交 837e215b 编写于 作者: J Justin

Merge pull request #85 from presidentbeef/include_detected_value_in_warning

Include detected user input value in warnings and highlight in reports
......@@ -23,6 +23,7 @@ module Brakeman
# * :config_file - configuration file
# * :escape_html - escape HTML by default (automatic)
# * :exit_on_warn - return false if warnings found, true otherwise. Not recommended for library use (default: false)
# * :highlight_user_input - highlight user input in reported warnings (default: true)
# * :html_style - path to CSS file
# * :ignore_model_output - consider models safe (default: false)
# * :message_limit - limit length of messages
......@@ -113,6 +114,7 @@ module Brakeman
:min_confidence => 2,
:combine_locations => true,
:collapse_mass_assignment => true,
:highlight_user_input => true,
:ignore_redirect_to_model => true,
:ignore_model_output => false,
:message_limit => 100,
......
......@@ -12,6 +12,8 @@ class Brakeman::BaseCheck < SexpProcessor
CONFIDENCE = { :high => 0, :med => 1, :low => 2 }
Match = Struct.new(:type, :match)
#Initialize Check with Checks.
def initialize tracker
super()
......@@ -66,13 +68,13 @@ class Brakeman::BaseCheck < SexpProcessor
process exp[3]
if params? exp[1]
@has_user_input = :params
@has_user_input = Match.new(:params, exp)
elsif cookies? exp[1]
@has_user_input = :cookies
@has_user_input = Match.new(:cookies, exp)
elsif request_env? exp[1]
@has_user_input = :request
@has_user_input = Match.new(:request, exp)
elsif sexp? exp[1] and model_name? exp[1][1]
@has_user_input = :model
@has_user_input = Match.new(:model, exp)
end
exp
......@@ -92,13 +94,13 @@ class Brakeman::BaseCheck < SexpProcessor
#Note that params are included in current expression
def process_params exp
@has_user_input = :params
@has_user_input = Match.new(:params, exp)
exp
end
#Note that cookies are included in current expression
def process_cookies exp
@has_user_input = :cookies
@has_user_input = Match.new(:cookies, exp)
exp
end
......@@ -206,19 +208,26 @@ class Brakeman::BaseCheck < SexpProcessor
#Does not actually process string interpolation, but notes that it occurred.
def process_string_interp exp
@string_interp = true
@string_interp = Match.new(:interp, exp)
exp
end
#Checks if an expression contains string interpolation.
#
#Returns Match with :interp type if found.
def include_interp? exp
@string_interp = false
process exp
@string_interp
end
#Checks if _exp_ includes parameters or cookies, but this only works
#with the base process_default.
#Checks if _exp_ includes user input in the form of cookies, parameters,
#request environment, or model attributes.
#
#If found, returns a struct containing a type (:cookies, :params, :request, :model) and
#the matching expression (Match#type and Match#match).
#
#Returns false otherwise.
def include_user_input? exp
@has_user_input = false
process exp
......@@ -227,24 +236,24 @@ class Brakeman::BaseCheck < SexpProcessor
#This is used to check for user input being used directly.
#
#Returns false if none is found, otherwise it returns an array
#where the first element is the type of user input
#(either :params or :cookies) and the second element is the matching
#expression
##If found, returns a struct containing a type (:cookies, :params, :request) and
#the matching expression (Match#type and Match#match).
#
#Returns false otherwise.
def has_immediate_user_input? exp
if exp.nil?
false
elsif params? exp
return :params, exp
return Match.new(:params, exp)
elsif cookies? exp
return :cookies, exp
return Match.new(:cookies, exp)
elsif call? exp
if params? exp[1]
return :params, exp
return Match.new(:params, exp)
elsif cookies? exp[1]
return :cookies, exp
return Match.new(:cookies, exp)
elsif request_env? exp[1]
return :request, exp
return Match.new(:request, exp)
else
false
end
......@@ -253,10 +262,8 @@ class Brakeman::BaseCheck < SexpProcessor
when :string_interp
exp.each do |e|
if sexp? e
type, match = has_immediate_user_input?(e)
if type
return type, match
end
match = has_immediate_user_input?(e)
return match if match
end
end
false
......@@ -265,10 +272,8 @@ class Brakeman::BaseCheck < SexpProcessor
if exp[1].node_type == :rlist
exp[1].each do |e|
if sexp? e
type, match = has_immediate_user_input?(e)
if type
return type, match
end
match = has_immediate_user_input?(e)
return match if match
end
end
false
......
......@@ -83,11 +83,10 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
out = exp[1][3][1]
end
type, match = has_immediate_user_input? out
if type
if input = has_immediate_user_input?(out)
add_result exp
case type
case input.type
when :params
message = "Unescaped parameter value"
when :cookies
......@@ -101,8 +100,8 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
warn :template => @current_template,
:warning_type => "Cross Site Scripting",
:message => message,
:line => match.line,
:code => match,
:line => input.match.line,
:code => input.match,
:confidence => CONFIDENCE[:high]
elsif not tracker.options[:ignore_model_output] and match = has_immediate_model?(out)
......@@ -161,29 +160,35 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
actually_process_call exp
message = nil
if @matched == :model and not tracker.options[:ignore_model_output]
message = "Unescaped model attribute"
elsif @matched == :params
message = "Unescaped parameter value"
elsif @matched == :cookies
message = "Unescaped cookie value"
end
if message and not duplicate? exp
add_result exp
if exp[1].nil? and @known_dangerous.include? exp[2]
confidence = CONFIDENCE[:high]
else
confidence = CONFIDENCE[:low]
if @matched
case @matched.type
when :model
unless tracker.options[:ignore_model_output]
message = "Unescaped model attribute"
end
when :params
message = "Unescaped parameter value"
when :cookies
message = "Unescaped cookie value"
end
warn :template => @current_template,
:warning_type => "Cross Site Scripting",
:message => message,
:line => exp.line,
:code => exp,
:confidence => confidence
if message and not duplicate? exp
add_result exp
if exp[1].nil? and @known_dangerous.include? exp[2]
confidence = CONFIDENCE[:high]
else
confidence = CONFIDENCE[:low]
end
warn :template => @current_template,
:warning_type => "Cross Site Scripting",
:message => message,
:line => exp.line,
:code => exp,
:user_input => @matched.match,
:confidence => confidence
end
end
@mark = @matched = false
......@@ -204,7 +209,7 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
#Ignore safe items
if (target.nil? and (@ignore_methods.include? method or method.to_s =~ IGNORE_LIKE)) or
(@matched == :model and IGNORE_MODEL_METHODS.include? method) or
(@matched and @matched.type == :model and IGNORE_MODEL_METHODS.include? method) or
(target == HAML_HELPERS and method == :html_escape) or
((target == URI or target == CGI) and method == :escape) or
(target == XML_HELPER and method == :escape_xml) or
......@@ -214,11 +219,11 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
#exp[0] = :ignore #should not be necessary
@matched = false
elsif sexp? exp[1] and model_name? exp[1][1]
@matched = :model
@matched = Match.new(:model, exp)
elsif cookies? exp
@matched = :cookies
@matched = Match.new(:cookies, exp)
elsif @inspect_arguments and params? exp
@matched = :params
@matched = Match.new(:params, exp)
elsif @inspect_arguments
process args
end
......@@ -226,13 +231,13 @@ class Brakeman::CheckCrossSiteScripting < Brakeman::BaseCheck
#Note that params have been found
def process_params exp
@matched = :params
@matched = Match.new(:params, exp)
exp
end
#Note that cookies have been found
def process_cookies exp
@matched = :cookies
@matched = Match.new(:cookies, exp)
exp
end
......
......@@ -20,11 +20,12 @@ class Brakeman::CheckEvaluation < Brakeman::BaseCheck
#Warns if eval includes user input
def process_result result
if include_user_input? result[:call][-1]
if input = include_user_input?(result[:call][-1])
warn :result => result,
:warning_type => "Dangerous Eval",
:message => "User input in eval",
:code => result[:call],
:user_input => input.match,
:confidence => CONFIDENCE[:high]
end
end
......
......@@ -54,6 +54,7 @@ class Brakeman::CheckExecute < Brakeman::BaseCheck
:message => "Possible command injection",
:line => call.line,
:code => call,
:user_input => failure.match,
:confidence => confidence
end
end
......@@ -75,16 +76,19 @@ class Brakeman::CheckExecute < Brakeman::BaseCheck
exp = result[:call]
if include_user_input? exp
if input = include_user_input?(exp)
confidence = CONFIDENCE[:high]
user_input = input.match
else
confidence = CONFIDENCE[:med]
user_input = nil
end
warning = { :warning_type => "Command Injection",
:message => "Possible command injection",
:line => exp.line,
:code => exp,
:user_input => user_input,
:confidence => confidence }
if result[:location][0] == :template
......
......@@ -28,13 +28,14 @@ class Brakeman::CheckFileAccess < Brakeman::BaseCheck
file_name = call[3][1]
if check = include_user_input?(file_name)
if input = include_user_input?(file_name)
unless duplicate? result
add_result result
if check == :params
case input.type
when :params
message = "Parameter"
elsif check == :cookies
when :cookies
message = "Cookie"
else
message = "User input"
......@@ -47,7 +48,8 @@ class Brakeman::CheckFileAccess < Brakeman::BaseCheck
:message => message,
:confidence => CONFIDENCE[:high],
:line => call.line,
:code => call
:code => call,
:user_input => input.match
end
end
end
......
......@@ -60,10 +60,9 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
def check_argument result, exp
arg = process exp
type, match = has_immediate_user_input? arg
if type
case type
if input = has_immediate_user_input?(arg)
case input.type
when :params
message = "Unescaped parameter value in link_to"
when :cookies
......@@ -76,7 +75,9 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
warn :result => result,
:warning_type => "Cross Site Scripting",
:message => message,
:user_input => input.match,
:confidence => CONFIDENCE[:high]
elsif not tracker.options[:ignore_model_output] and match = has_immediate_model?(arg)
method = match[2]
......@@ -92,13 +93,14 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
warn :result => result,
:warning_type => "Cross Site Scripting",
:message => "Unescaped model attribute in link_to",
:user_input => match,
:confidence => confidence
end
elsif @matched
if @matched == :model and not tracker.options[:ignore_model_output]
if @matched.type == :model and not tracker.options[:ignore_model_output]
message = "Unescaped model attribute in link_to"
elsif @matched == :params
elsif @matched.type == :params
message = "Unescaped parameter value in link_to"
end
......@@ -108,6 +110,7 @@ class Brakeman::CheckLinkTo < Brakeman::CheckCrossSiteScripting
warn :result => result,
:warning_type => "Cross Site Scripting",
:message => message,
:user_input => @matched.match,
:confidence => CONFIDENCE[:med]
end
end
......
......@@ -40,10 +40,9 @@ class Brakeman::CheckLinkToHref < Brakeman::CheckLinkTo
#with something before the user input
return if node_type?(url_arg, :string_interp) && !url_arg[1].chomp.empty?
type, match = has_immediate_user_input? url_arg
if type
case type
if input = has_immediate_user_input?(url_arg)
case input.type
when :params
message = "Unsafe parameter value in link_to href"
when :cookies
......@@ -57,6 +56,7 @@ class Brakeman::CheckLinkToHref < Brakeman::CheckLinkTo
warn :result => result,
:warning_type => "Cross Site Scripting",
:message => message,
:user_input => input.match,
:confidence => CONFIDENCE[:high]
end
elsif has_immediate_model? url_arg
......@@ -72,9 +72,9 @@ class Brakeman::CheckLinkToHref < Brakeman::CheckLinkTo
# attack.
elsif @matched
if @matched == :model and not tracker.options[:ignore_model_output]
if @matched.type == :model and not tracker.options[:ignore_model_output]
message = "Unsafe model attribute in link_to href"
elsif @matched == :params
elsif @matched.type == :params
message = "Unsafe parameter value in link_to href"
end
......@@ -83,6 +83,7 @@ class Brakeman::CheckLinkToHref < Brakeman::CheckLinkTo
warn :result => result,
:warning_type => "Cross Site Scripting",
:message => message,
:user_input => @matched.match,
:confidence => CONFIDENCE[:med]
end
end
......
......@@ -52,10 +52,17 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
if attr_protected and tracker.options[:ignore_attr_protected]
return
elsif include_user_input? call[3] and not hash? call[3][1] and not attr_protected
confidence = CONFIDENCE[:high]
elsif input = include_user_input?(call[3])
if not hash? call[3][1] and not attr_protected
confidence = CONFIDENCE[:high]
user_input = input.match
else
confidence = CONFIDENCE[:low]
user_input = input.match
end
else
confidence = CONFIDENCE[:low]
user_input = nil
end
warn :result => res,
......@@ -63,6 +70,7 @@ class Brakeman::CheckMassAssignment < Brakeman::BaseCheck
:message => "Unprotected mass assignment",
:line => call.line,
:code => call,
:user_input => user_input,
:confidence => confidence
end
......
......@@ -28,7 +28,7 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck
if method == :redirect_to and not only_path?(call) and res = include_user_input?(call)
add_result result
if res == :immediate
if res.type == :immediate
confidence = CONFIDENCE[:high]
else
confidence = CONFIDENCE[:low]
......@@ -39,6 +39,7 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck
:message => "Possible unprotected redirect",
:line => call.line,
:code => call,
:user_input => res.match,
:confidence => confidence
end
end
......@@ -64,16 +65,18 @@ class Brakeman::CheckRedirect < Brakeman::BaseCheck
call[3].each do |arg|
if call? arg
if request_value? arg or request_value? arg[1]
return :immediate
if request_value? arg
return Match.new(:immediate, arg)
elsif request_value? arg[1]
return Match.new(:immediate, arg[1])
elsif arg[2] == :url_for and include_user_input? arg
return :immediate
return Match.new(:immediate, arg)
#Ignore helpers like some_model_url?
elsif arg[2].to_s =~ /_(url|path)$/
return false
end
elsif request_value? arg
return :immediate
return Match.new(:immediate, arg)
end
end
......
......@@ -32,11 +32,10 @@ class Brakeman::CheckRender < Brakeman::BaseCheck
if sexp? view and not duplicate? result
add_result result
type, match = has_immediate_user_input? view
if type
if input = has_immediate_user_input?(view)
confidence = CONFIDENCE[:high]
elsif type = include_user_input?(view)
elsif input = include_user_input?(view)
if node_type? view, :string_interp, :dstr
confidence = CONFIDENCE[:med]
else
......@@ -48,7 +47,7 @@ class Brakeman::CheckRender < Brakeman::BaseCheck
message = "Render path contains "
case type
case input.type
when :params
message << "parameter value"
when :cookies
......@@ -66,6 +65,7 @@ class Brakeman::CheckRender < Brakeman::BaseCheck
warn :result => result,
:warning_type => "Dynamic Render Path",
:message => message,
:user_input => input.match,
:confidence => confidence
end
end
......
......@@ -19,19 +19,21 @@ class Brakeman::CheckSend < Brakeman::BaseCheck
args = process result[:call][3]
target = process result[:call][1]
if has_immediate_user_input? args[1]
if input = has_immediate_user_input?(args[1])
warn :result => result,
:warning_type => "Dangerous Send",
:message => "User controlled method execution",
:code => result[:call],
:user_input => input.match,
:confidence => CONFIDENCE[:high]
end
if has_immediate_user_input?(target)
if input = has_immediate_user_input?(target)
warn :result => result,
:warning_type => "Dangerous Send",
:message => "User defined target of method invocation",
:code => result[:call],
:user_input => input.match,
:confidence => CONFIDENCE[:med]
end
end
......
......@@ -122,15 +122,18 @@ class Brakeman::CheckSQL < Brakeman::BaseCheck
if failed and not call.original_line and not duplicate? result
add_result result
if include_user_input? args[-1]
if input = include_user_input?(args[-1])
confidence = CONFIDENCE[:high]
user_input = input.match
else
confidence = CONFIDENCE[:med]
user_input = nil
end
warn :result => result,
:warning_type => "SQL Injection",
:message => "Possible SQL injection",
:user_input => user_input,
:confidence => confidence
end
......
......@@ -48,10 +48,12 @@ class Brakeman::CheckWithoutProtection < Brakeman::BaseCheck
if true? value
add_result res
if include_user_input? call[3]
if input = include_user_input?(call[3])
confidence = CONFIDENCE[:high]
user_input = input.match
else
confidence = CONFIDENCE[:med]
user_input = nil
end
warn :result => res,
......@@ -59,6 +61,7 @@ class Brakeman::CheckWithoutProtection < Brakeman::BaseCheck
:message => "Unprotected mass assignment",
:line => call.line,
:code => call,
:user_input => user_input,
:confidence => confidence
end
......
......@@ -108,3 +108,7 @@ p {
pre.context {
margin-bottom: 1px;
}
.user_input {
background-color: #fcecab;
}
......@@ -144,6 +144,10 @@ module Brakeman::Options
options[:combine_locations] = combine
end
opts.on "--[no-]highlights", "Highlight user input in report" do |highlight|
options[:highlight_user_input] = highlight
end
opts.on "-m", "--routes", "Report controller information" do
options[:report_routes] = true
end
......
......@@ -34,6 +34,7 @@ class Brakeman::Report
@checks = tracker.checks
@element_id = 0 #Used for HTML ids
@warnings_summary = nil
@highlight_user_input = tracker.options[:highlight_user_input]
end
#Generate summary table of what was parsed
......@@ -97,6 +98,7 @@ class Brakeman::Report
w["Message"] = with_context warning, w["Message"]
else
w["Confidence"] = TEXT_CONFIDENCE[w["Confidence"]]
w["Message"] = text_message warning, w["Message"]
end
warning_messages << w
......@@ -134,6 +136,7 @@ class Brakeman::Report
w["Message"] = with_context warning, w["Message"]
else
w["Confidence"] = TEXT_CONFIDENCE[w["Confidence"]]
w["Message"] = text_message warning, w["Message"]
end
warnings << w
......@@ -169,6 +172,7 @@ class Brakeman::Report
w["Message"] = with_context warning, w["Message"]
else
w["Confidence"] = TEXT_CONFIDENCE[w["Confidence"]]
w["Message"] = text_message warning, w["Message"]
end
warnings << w
......@@ -204,6 +208,7 @@ class Brakeman::Report
w["Message"] = with_context warning, w["Message"]
else
w["Confidence"] = TEXT_CONFIDENCE[w["Confidence"]]
w["Message"] = text_message warning, w["Message"]
end
warnings << w
......@@ -485,6 +490,28 @@ class Brakeman::Report
@warnings_summary = summary
end
#Escape warning message and highlight user input in text output
def text_message warning, message
if @highlight_user_input and warning.user_input
user_input = warning.format_user_input
message.gsub(user_input, "+#{user_input}+")
else
message
end
end
#Escape warning message and highlight user input in HTML output
def html_message warning, message
message = CGI.escapeHTML(message)
if @highlight_user_input and warning.user_input
user_input = warning.format_user_input
message.gsub!(user_input, "<span class=\"user_input\">#{user_input}</span>")
end
message
end
#Generate HTML for warnings, including context show/hidden via Javascript
def with_context warning, message
......@@ -495,12 +522,14 @@ class Brakeman::Report
tracker.options[:message_limit] > 0 and
message.length > tracker.options[:message_limit]
full_message = message
full_message = html_message(warning, message)
message = message[0..tracker.options[:message_limit]] << "..."
end
message = html_message(warning, message)
if context.empty? and not full_message
return CGI.escapeHTML(message)
return message
end
@element_id += 1
......@@ -510,10 +539,10 @@ class Brakeman::Report
alt = false
output = "<div class='warning_message' onClick=\"toggle('#{code_id}');toggle('#{message_id}');toggle('#{full_message_id}')\" >" <<
if full_message
"<span id='#{message_id}' style='display:block' >#{CGI.escapeHTML(message)}</span>" <<
"<span id='#{full_message_id}' style='display:none'>#{CGI.escapeHTML(full_message)}</span>"
"<span id='#{message_id}' style='display:block' >#{message}</span>" <<
"<span id='#{full_message_id}' style='display:none'>#{full_message}</span>"
else
CGI.escapeHTML(message)
message
end <<
"<table id='#{code_id}' class='context' style='display:none'>" <<
"<caption>#{(warning.file || '').gsub(tracker.options[:app_path], "")}</caption>"
......
#The Warning class stores information about warnings
class Brakeman::Warning
attr_reader :called_from, :check, :class, :confidence, :controller,
:line, :method, :model, :template, :warning_set, :warning_type
:line, :method, :model, :template, :user_input, :warning_set, :warning_type
attr_accessor :code, :context, :file, :message
......@@ -12,7 +12,7 @@ class Brakeman::Warning
@view_name = nil
[:called_from, :check, :class, :code, :confidence, :controller, :file, :line,
:message, :method, :model, :template, :warning_set, :warning_type].each do |option|
:message, :method, :model, :template, :user_input, :warning_set, :warning_type].each do |option|
self.instance_variable_set("@#{option}", options[option])
end
......@@ -74,6 +74,12 @@ class Brakeman::Warning
Brakeman::OutputProcessor.new.format(self.code).gsub(/(\t|\r|\n)+/, " ")
end
#Return String of the user input formatted and
#stripped of newlines and tabs.
def format_user_input
Brakeman::OutputProcessor.new.format(self.user_input).gsub(/(\t|\r|\n)+/, " ")
end
#Return formatted warning message
def format_message
return @format_message if @format_message
......@@ -143,6 +149,7 @@ class Brakeman::Warning
:line => self.line,
:code => (@code && self.format_code),
:location => location,
:user_input => (@user_input && self.format_user_input),
:confidence => TEXT_CONFIDENCE[self.confidence]
}
end
......
Markdown is supported
0% .
You are about to add 0 people to the discussion. Proceed with caution.
先完成此消息的编辑!
想要评论请 注册