Python: Add Regular Expression Injection query by jorgectf · Pull Request #5442 · github/codeql

jorgectf · 2021-03-18T16:26:45Z

Any suggestions/tricks to improve the query or workarounds (to learn), no matter how minimal they are, are massively appreciated.

erik-krogh · 2021-03-18T17:54:21Z

Just a small comment on naming.

There are currently ReDoS queries implemented for C# and JavaScript.
Both of these look for expensive regular expressions running on user input.

Your query looks for regular expressions being constructed from user input.
There is a similar query implemented for JavaScript, which is called Regular expression injection, and I think that name better describes what your query does.
And it would make the naming consistent across languages.

jorgectf · 2021-03-18T18:46:08Z

Thanks for the detail @erik-krogh! I've updated everything according to Regular Expression Injection instead of Regular Expression Denial of Service.

yoff · 2021-03-18T21:51:30Z

Regarding the structure we now try to follow:

RegexInjectionSink might be added to Concepts.qll (and called something like RegexExecution).
EscapeSanitizer might be moved to Concepts.qll (and called something like RegexEscape)
The configuration should be moved to its own file (see e.g. StackTraceExposure.ql and StackTraceExposure.qll)

yoff · 2021-03-18T21:55:11Z

If the sanitiser is moved to concepts, it might have to be made a bit more precise, but I am not sure. Is there an awful lot of ways to escape?

jorgectf · 2021-03-18T22:08:49Z

If the sanitiser is moved to concepts, it might have to be made a bit more precise, but I am not sure. Is there an awful lot of ways to escape?

As far as I know, re.escape is the only already-built function that escapes the regular expression, but the actual Sanitizer may also work for custom-made escape functions (I guess it'd very strange but could happen). Should it be reduced only to re.escape?

jorgectf · 2021-03-18T22:12:48Z

RegexInjectionSink might be added to Concepts.qll (and called something like RegexExecution).

Shall I include DirectRegex and CompiledRegex into Concepts.qll too?

yoff · 2021-03-18T22:52:28Z

As far as I know, re.escape is the only already-built function that escapes the regular expression, but the actual Sanitizer may also work for custom-made escape functions (I guess it'd very strange but could happen). Should it be reduced only to re.escape?

In that case, we should probably limit it for now. We can then expand the concept if we see false positives due to custom escape mechanisms.

yoff · 2021-03-18T22:59:53Z

RegexInjectionSink might be added to Concepts.qll (and called something like RegexExecution).

Shall I include DirectRegex and CompiledRegex into Concepts.qll too?

We would actually have those predicates that populate the concepts scattered around in the models of the libraries where the concepts occur. So in this case, you identify some regex execution in the re module in the standard library. So it should go into a module named re inside Stdlib.qll. We can then later add other regex executions e.g. from the regex library in Regex.qll next to Stdlib.qll.

yoff

As mentioned we will have to put it all into experimental. I will add the required files and folders to main, then you can just pull and move things around.
We also still need qldocs on public methods, but that is a smaller thing.

yoff

My apologies, I did not explain the idea behind getRegexNode clearly. I thought I had described how it would change the definition of sinks, but clearly those thoughts did not make it out of my mind. I have given the relevant modifications; this should also make the query compile again :-)

jorgectf · 2021-03-24T09:08:36Z

Absolutely no problem @yoff! I made some tests yesterday and had to unset the override in RegexExecution's Range since DataFlow's Node has no Range class, but didn't manage to understand what to do with regexNode. Thank you so much for your awesome explanations! :D

yoff · 2021-03-24T14:03:26Z

#5493 has now been merged, so you have somewhere safe to put all the files :-)

Update `RegexExecution` docs and use `flowsTo()` instead of `getALocalSource()`. Co-authored-by: yoff <lerchedahl@gmail.com>

Signed-off-by: jorgectf <jorgectf@protonmail.com>

yoff

Final consistency polish :-)

Co-authored-by: yoff <lerchedahl@gmail.com>

jorgectf · 2021-04-29T14:27:19Z

Final consistency polish :-)

Thanks 👍

yoff

LGTM

yoff

Just needed to make the auto format check happy..

jorgectf · 2021-05-10T14:53:30Z

Just needed to make the auto format check happy..

😆

jorgectf · 2021-05-11T10:13:10Z

🎉

jorgectf requested a review from a team as a code owner March 18, 2021 16:26

github-actions Bot added documentation Python labels Mar 18, 2021

jorgectf mentioned this pull request Mar 18, 2021

[Python] CWE-400: Regular Expression Injection github/securitylab#318

Closed

1 task

intrigus-lgtm reviewed Mar 18, 2021

View reviewed changes

Comment thread python/ql/src/Security/CWE-730/RegexDoS.qhelp Outdated

yoff reviewed Mar 18, 2021

View reviewed changes

Comment thread python/ql/src/experimental/Security/CWE-730/RegexDoS.ql Outdated

yoff reviewed Mar 18, 2021

View reviewed changes

Comment thread python/ql/src/experimental/Security/CWE-730/RegexDoS.ql Outdated

yoff reviewed Mar 18, 2021

View reviewed changes

Comment thread python/ql/src/experimental/Security/CWE-730/RegexDoS.ql Outdated

intrigus-lgtm reviewed Mar 18, 2021

View reviewed changes

Comment thread python/ql/src/experimental/Security/CWE-730/RegexInjection.ql Outdated

jorgectf changed the title ~~Python: Add ReDoS query~~ Python: Add Regular Expression Injection query Mar 18, 2021

mrthankyou reviewed Mar 19, 2021

View reviewed changes

Comment thread python/ql/src/Security/CWE-730/RegexInjection.qhelp Outdated

yoff reviewed Mar 19, 2021

View reviewed changes

Comment thread python/ql/src/semmle/python/Concepts.qll Outdated

yoff reviewed Mar 19, 2021

View reviewed changes

Comment thread python/ql/src/semmle/python/Concepts.qll Outdated

yoff reviewed Mar 19, 2021

View reviewed changes

Comment thread python/ql/src/semmle/python/frameworks/Stdlib.qll Outdated

jorgectf commented Mar 21, 2021

View reviewed changes

Comment thread python/ql/src/semmle/python/Concepts.qll Outdated

RasmusWL reviewed Mar 22, 2021

View reviewed changes

Comment thread python/ql/src/Security/CWE-730/RegexInjection.ql Outdated

yoff requested changes Mar 22, 2021

View reviewed changes

Comment thread python/ql/src/semmle/python/Concepts.qll Outdated

Comment thread python/ql/src/semmle/python/frameworks/Stdlib.qll Outdated

Comment thread python/ql/src/semmle/python/frameworks/Stdlib.qll Outdated

Comment thread python/ql/src/semmle/python/frameworks/Stdlib.qll Outdated

yoff requested changes Mar 23, 2021

View reviewed changes

jorgectf commented Mar 24, 2021

View reviewed changes

Comment thread python/ql/src/semmle/python/frameworks/Stdlib.qll Outdated

jorgectf force-pushed the jorgectf/python/redos branch from 6b24752 to f4fe354 Compare March 24, 2021 17:00

jorgectf and others added 14 commits April 27, 2021 19:54

Add comment to Sink's predicates

03825a6

Sink's predicate typo

ec85ee4

Add .expected and qlref

d401d18

Move tests and qlref from /src to /test

81d23c0

Move expected to /test

d968eea

Add newline to qhelp

6a20a4d

Take ApiGraphs out of Concepts.qll

3fae3fd

Remove wrong comment

05ee853

Update .expected

12ccd7e

Polish qhelp

c432284

Apply suggestions from code review

c0c71c5

Update `RegexExecution` docs and use `flowsTo()` instead of `getALocalSource()`. Co-authored-by: yoff <lerchedahl@gmail.com>

Update to-cast sink's naming

20b532e

Signed-off-by: jorgectf <jorgectf@protonmail.com>

Remove unused class variables

8a80098

Signed-off-by: jorgectf <jorgectf@protonmail.com>

Add code example in CompiledRegex

21e01b8

Signed-off-by: jorgectf <jorgectf@protonmail.com>

jorgectf force-pushed the jorgectf/python/redos branch from 9e7e05b to 21e01b8 Compare April 27, 2021 17:55

Edit code example in CompiledRegex

213d011

Signed-off-by: jorgectf <jorgectf@protonmail.com>

yoff self-requested a review April 29, 2021 13:23

yoff requested changes Apr 29, 2021

View reviewed changes

Comment thread python/ql/src/experimental/semmle/python/Concepts.qll Outdated

Comment thread python/ql/src/experimental/semmle/python/Concepts.qll Outdated

Comment thread python/ql/src/experimental/semmle/python/Concepts.qll Outdated

Polish documentation consistency

bd4b189

Co-authored-by: yoff <lerchedahl@gmail.com>

yoff previously approved these changes Apr 29, 2021

View reviewed changes

yoff reviewed May 10, 2021

View reviewed changes

Comment thread python/ql/src/experimental/semmle/python/frameworks/Stdlib.qll Outdated

Update python/ql/src/experimental/semmle/python/frameworks/Stdlib.qll

78370cf

yoff dismissed stale reviews from themself via 78370cf May 10, 2021 12:53

yoff approved these changes May 10, 2021

View reviewed changes

yoff merged commit 0e5a2c4 into github:main May 11, 2021

calumgrant assigned yoff Aug 23, 2021

Conversation

jorgectf commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erik-krogh commented Mar 18, 2021

Uh oh!

jorgectf commented Mar 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yoff commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yoff commented Mar 18, 2021

Uh oh!

jorgectf commented Mar 18, 2021

Uh oh!

jorgectf commented Mar 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yoff commented Mar 18, 2021

Uh oh!

yoff commented Mar 18, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorgectf commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yoff commented Mar 24, 2021

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jorgectf commented Apr 29, 2021

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

jorgectf commented May 10, 2021

Uh oh!

jorgectf commented May 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

jorgectf commented Mar 18, 2021 •

edited

Loading

yoff commented Mar 18, 2021 •

edited

Loading

jorgectf commented Mar 18, 2021 •

edited

Loading

jorgectf commented Mar 24, 2021 •

edited

Loading