Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add check for regex DoS #454

Merged
merged 6 commits into from Mar 29, 2014

Conversation

Projects
None yet
2 participants
@mastahyeti
Copy link
Contributor

mastahyeti commented Mar 2, 2014

Checking for user controlled input in regular expressions.


#Warns if regex includes user input
def process_result result
if input = include_user_input?(result[:call])

This comment has been minimized.

Copy link
@mastahyeti

mastahyeti Mar 2, 2014

Author Contributor

Not sure if this should be has_immediate_user_input? instead.

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

Yes, otherwise it will give high confidence warnings for things like /#{something_benign(params[:x])}/.

has_immediate_user_input? behaves differently, though. You will have to loop through the interpolated regex values. Sexp#each_sexp should be helpful.

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Mar 3, 2014

Hi Ben,

Thanks for the contribution. I will take a look and get back to you tomorrow!

require 'brakeman/checks/base_check'

#This check looks for regexes that include user input.
class Brakeman::CheckRegexDos < Brakeman::BaseCheck

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

Please rename as CheckRegexDoS for consistency with other checks.

end

#Warns if regex includes user input
def process_result result

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

Please check for duplicates. See any of the other checks for examples.

exp.each { |arg| process arg if sexp? arg }

@calls << { :target => nil,
:method => :regex_interp,

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

I don't trust that regex_interp is unique enough. Maybe brakeman_regex_interp or something equally ridiculous. And/or in the check double check that the result does not happen to be a real call.

@@ -64,6 +64,21 @@ def process_string_interp exp
out
end

def process_dregx exp

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

Why is this needed? Seems like Ruby2Ruby handles this fine.

This comment has been minimized.

Copy link
@mastahyeti

mastahyeti Mar 11, 2014

Author Contributor

Hmm. I was having trouble with it displaying things incorrectly before, but it seems to be working now.

@@ -123,7 +123,7 @@ def test_redirect

assert_warning :type => :warning,
:warning_type => "Redirect",
:line => 182,
:line => 186,

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

Preferable not to change unrelated tests when adding new tests, but not a huge deal.

This comment has been minimized.

Copy link
@mastahyeti

mastahyeti Mar 11, 2014

Author Contributor

Any better way to do this since the line changed for that test?

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 11, 2014

Owner

Try to avoid editing test apps in such a way that it affects unrelated tests. You could easily put the regex in any controller in any of the test apps, or add another controller.

@@ -1246,6 +1246,16 @@ def test_unsafe_symbol_creation_3
:relative_path => "app/controllers/application_controller.rb"
end

def test_regex_dos

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

Please include fingerprint and user_input values. The current to_test.rb should generate these for you.

warn :result => result,
:warning_type => "Denial of Service",
:warning_code => :regex_dos,
:message => "User input in regex",

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

It's nicer to use friendly_type_of and have a more specific warning message.

:warning_type => "Denial of Service",
:warning_code => :regex_dos,
:message => "User input in regex",
:code => result[:call],

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

This isn't necessary when using :result => result.

@@ -160,6 +160,10 @@ def test_more_send_methods
table.classify.constantize.try(:meth)
end

def test_regex_dos

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 4, 2014

Owner

More tests would be nice, maybe make sure it doesn't warn on stuff that isn't user input, still works if there is more to the regex (like /something#{params[x:]}something/), etc.

end
end

def process_call(exp)

This comment has been minimized.

Copy link
@mastahyeti

mastahyeti Mar 14, 2014

Author Contributor

Is this pattern okay for checking escapes? I based it roughly on the XSS check.

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 14, 2014

Owner

Yep, seems fine to me.

/#{params[:regex]}/
end

def test_escaped_regex

This comment has been minimized.

Copy link
@mastahyeti

mastahyeti Mar 14, 2014

Author Contributor

Is there a good way to test that something isn't reported on, or can I just rely on the tests of the expected vs. real findings?

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 14, 2014

Owner

You can use assert_no_warning. But I'm also okay with relying on expected number of findings.

@mastahyeti

This comment has been minimized.

Copy link
Contributor Author

mastahyeti commented Mar 14, 2014

Ok. I think this PR is ready for 👀 again. I've addressed all the concerns you raised.

@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Mar 20, 2014

I think this is the issue you saw with formatting the regexes: seattlerb/ruby_parser#154

Brakeman::Checks.add self

ESCAPES = {
s(:const, :Regex) => [

This comment has been minimized.

Copy link
@presidentbeef

presidentbeef Mar 29, 2014

Owner

Was seeing a really confusing result...this should be Regexp not Regex (I make the same mistake all the time).

mastahyeti added some commits Mar 29, 2014

presidentbeef added a commit that referenced this pull request Mar 29, 2014

Merge pull request #454 from mastahyeti/regex_dos
Add check for regex DoS

@presidentbeef presidentbeef merged commit a4ee53d into presidentbeef:master Mar 29, 2014

1 check passed

default The Travis CI build passed
Details
@presidentbeef

This comment has been minimized.

Copy link
Owner

presidentbeef commented Mar 29, 2014

Looks good, thank you for your patience!

@mastahyeti

This comment has been minimized.

Copy link
Contributor Author

mastahyeti commented Mar 30, 2014

🤘

Repository owner locked and limited conversation to collaborators Feb 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.