Java: Regex injection#5704
Conversation
|
Maybe it would be good to cover Apache Commons Lang's 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. |
Good idea, will do that. |
Marcono1234
left a comment
There was a problem hiding this comment.
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")
> 12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890Depending 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)
java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql
Outdated
Show resolved
Hide resolved
java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| abstract class Sanitizer extends DataFlow::ExprNode { } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
You never know when pull request is finished :)
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. |
java/ql/src/experimental/Security/CWE/CWE-730/RegexInjection.ql
Outdated
Show resolved
Hide resolved
|
I started an LGTM analysis of this query. |
|
Ping |
|
@smowton I asked @aschackmull to have a final look. He'll check the PR next week. |
|
Looks ok to me. |
No description provided.