Ruby: add library input as a source for rb/polynomial-redos
#10782
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from | ||
| PolynomialReDoS::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink, | ||
| PolynomialReDoS::Sink sinkNode, PolynomialBackTrackingTerm regexp | ||
| where | ||
| config.hasFlowPath(source, sink) and | ||
| sinkNode = sink.getNode() and | ||
| regexp = sinkNode.getRegExp() | ||
| select sinkNode.getHighlight(), source, sink, | ||
| "This $@ that depends on a $@ may run slow on strings " + regexp.getPrefixMessage() + | ||
| "with many repetitions of '" + regexp.getPumpString() + "'.", regexp, "regular expression", | ||
| source.getNode(), "user-provided value" | ||
| source.getNode(), source.getNode().(PolynomialReDoS::Source).describe() |
Check warning
Code scanning / CodeQL
Consistent alert message
erik-krogh
force-pushed
the
rbPoly
branch
2 times, most recently
from
8bb972c
to
45aee2b
Compare
|
Rebased to include #11358 in a clean way. |
|
I've looked through a bunch of the results in the LGTM run and ~90% of them look quite legitimate, so I don't feel so concerned about the noise potential of this change. I think we should consider changing the precision for this query and take some care when deploying this change, but overall I think it is probably fine 👍. I've not looked at the code change itself (yet). |
| @@ -53,6 +59,15 @@ module PolynomialReDoS { | |||
| */ | |||
| class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { } | |||
|
|
|||
| import codeql.ruby.frameworks.core.Gem::Gem as Gem | |||
There was a problem hiding this comment.
Is this not equivalent to import codeql.ruby.frameworks.core.Gem? Also, should this be a private import?
There was a problem hiding this comment.
Yes that would be equivalent, and yes that should be private.
I want to keep the more verbose version though. I just like that style sometimes.
| s.name = 'poly-redos' | ||
| s.require_path = "lib" | ||
| end | ||
|
|
There was a problem hiding this comment.
(nit)
Something is a bit off with the formatting here. end should align with Gem and ideally there should be a newline at the end of the file, rather than whatever whitespace I guess is on L5.
There was a problem hiding this comment.
Yes, that happens when I copy-paste the .gemspec from one test to the other.
So this formatting error will probably also appear in other PRs.
But yeah, I'm not going to fix it.
|
Merge with |
CVE-2022-24836: TP/TN
CVE-2021-22880: TP/TN
CVE-2021-4250: TP/TN
CVE-2023-22796: TP/TN
CVE-2022-4891: TP
CVE-2023-22799: TP (source is
protectedmethod. TP with #11879).Evaluation looks as expected.
No performance regression, and a bunch of results.
The results are a bit noisy, but there are a lot of CVEs from this kind of issue, so people care about it.
Another larger evaluation.