FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Java: Regex injection · Pull Request #5704 · github/codeql · GitHub

/ codeql Public

Java: Regex injection - #5704

Merged
aschackmull merged 7 commits into
mainfrom
unknown repository
Jun 1, 2021
Merged

Java: Regex injection#5704
aschackmull merged 7 commits into
mainfrom
unknown repository

Conversation

ghost commented Apr 16, 2021

Copy link
Copy Markdown

No description provided.

ghost self-requested a review as a code owner April 16, 2021 20:33
ghost changed the title [Java]: Regex injection Java: Regex injection Apr 16, 2021

Copy link
Copy Markdown
Contributor

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 commented Apr 19, 2021

Copy link
Copy Markdown
Author

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

Good idea, will do that.

Marcono1234 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

}
}

abstract class Sanitizer extends DataFlow::ExprNode { }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You never know when pull request is finished :)

ghost commented Apr 22, 2021

Copy link
Copy Markdown
Author

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.

ghost mentioned this pull request Apr 26, 2021
1 task

Copy link
Copy Markdown
Contributor

I started an LGTM analysis of this query.

ghost commented May 24, 2021

Copy link
Copy Markdown
Author

Ping

smowton commented May 27, 2021

Copy link
Copy Markdown
Contributor

@tamasvajk

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

Looks ok to me.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants


Back | FazBrowse Home | New Git URL