-
Notifications
You must be signed in to change notification settings - Fork 732
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
Expand Regex DoS check to include String#match and #match? coercion #1715
base: main
Are you sure you want to change the base?
Conversation
oops, sorry this got submitted in a half-written state. Cat on keyboard problems 😸 |
|
||
#Process calls | ||
def run_check | ||
Brakeman.debug "Finding dynamic regexes" | ||
calls = tracker.find_call :method => [:brakeman_regex_interp] | ||
COERCES_STRING_TO_REGEX.each do |coercion_method| | ||
calls.concat tracker.find_call(:method => [coercion_method]).select { |call| string?(call[:target]) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could simplify this to
tracker.find_call(:methods => CEORCES_STRING_TO_REGEX).select { ... }
@presidentbeef I updated the code based on your feedback. Thank you! 🙏🏻 I also made one behavior change: I added a method called |
In real code, would we expect to see string literals like the test cases? 🤔 |
This was the code we discovered (lightly anonymized): result = @active_record_resource.a_hash_value
.keys
.select { |key| key.downcase.match?(unsafe_string_query) }
.sort I'd love advice for better ways to identify |
Closes #1714.
I added additional behavior to the existing
check_regex_dos.rb
file. I thought it seemed like the correct location because it is checking for ReDoS, though it's arguably a bit different because it's checking methods onString
rather than regex literals.I also am not sure whether I should also validate that the object being passed to
match
/match?
is a String; it seems like the expectation is that anyuser_input
will be a string, but I'm not confident about that.