Java: Regex injection

[ghost]

No description provided.

[Marcono1234]

Maybe it would be good to cover Apache Commons Lang's RegExUtils as well since there is already some support for that class provided by the Java CodeQL library, see #5339.

It might be useful to model all methods accepting regex arguments in some way so future queries (e.g. DoS from hardcoded regex patterns, or erroneous regex patterns) can reuse it. Though I am not a member of this project, so I can't tell if and how that should be implemented.

[ghost]

Maybe it would be good to cover Apache Commons Lang's RegExUtils

Good idea, will do that.

[Marcono1234]

The following review comments are mostly cosmetic.

An interesting aspect might also be the replacement string of the replace calls. That string supports referencing captured groups (see documentation), including $0 to reference the complete match. This might be abusable:

"1234567890".replaceFirst("\\d+", "$0$0$0$0$0$0$0$0$0$0$0")
> 12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890

Depending on how large the match is and whether there are any length restrictions on the replacement string this could allow DoS attacks through high memory usage.
What do you think? (Though the projects on which I tried a query checking for this had no results)

[Marcono1234]

Since only RegExpSanitizationCall is extending this class it might be best to remove it and have RegExpSanitizationCall directly extend DataFlow::ExprNode (unless this pull request is not finished yet and you are planning to add more sanitizers).

[ghost]

You never know when pull request is finished :)

[ghost]

The following review comments are mostly cosmetic.

An interesting aspect might also be the replacement string of the replace calls. That string supports referencing captured groups (see documentation), including $0 to reference the complete match. This might be abusable:

"1234567890".replaceFirst("\\d+", "$0$0$0$0$0$0$0$0$0$0$0")
> 12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890

Depending on how large the match is and whether there are any length restrictions on the replacement string this could allow DoS attacks through high memory usage.
What do you think? (Though the projects on which I tried a query checking for this had no results)

User input in any of the regex arguments could alter the program logic. In this case I think it can only throw out of memory exception, that is far away from CPU intensive regex DoS.

[tamasvajk]

I started an LGTM analysis of this query.

[ghost]

Ping

[smowton]

@tamasvajk

[tamasvajk]

@smowton I asked @aschackmull to have a final look. He'll check the PR next week.

[aschackmull]

Looks ok to me.