brakeman
brakeman copied to clipboard
Expand Regex DoS check to include String#match and #match? coercion
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 on String
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 any user_input
will be a string, but I'm not confident about that.
oops, sorry this got submitted in a half-written state. Cat on keyboard problems 😸
@presidentbeef I updated the code based on your feedback. Thank you! 🙏🏻
I also made one behavior change: I added a method called string_or_modified_string?
that recurses down the target to see if the initial call was a String. I added this because I found making a trivial modification like downcase
would not trigger the check (e.g. "haystack".downcase.match(params[:unsafe])
). I couldn't find an existing recursive method, but maybe I overlooked one.
In real code, would we expect to see string literals like the test cases? :thinking:
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 [String]#match?([String])
.