brakeman icon indicating copy to clipboard operation
brakeman copied to clipboard

Expand Regex DoS check to include String#match and #match? coercion

Open bensheldon opened this issue 2 years ago • 4 comments

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.

bensheldon avatar Jun 08 '22 23:06 bensheldon

oops, sorry this got submitted in a half-written state. Cat on keyboard problems 😸

bensheldon avatar Jun 08 '22 23:06 bensheldon

@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.

bensheldon avatar Jun 22 '22 04:06 bensheldon

In real code, would we expect to see string literals like the test cases? :thinking:

presidentbeef avatar Jul 24 '22 18:07 presidentbeef

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]).

bensheldon avatar Jul 25 '22 23:07 bensheldon