Expand Regex DoS check to include String#match and #match? coercion #1715
Conversation
bensheldon
changed the title
|
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.
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) }
.sortI'd love advice for better ways to identify |
Closes #1714.
I added additional behavior to the existing
check_regex_dos.rbfile. 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 onStringrather 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_inputwill be a string, but I'm not confident about that.