Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python: Add additional sanitizers to SSRF #16657

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

joefarebrother
Copy link
Contributor

Closes https://github.com/github/codeql-team/issues/3024

Adds checks against the contents of a string (such as .isalnum; and regex matches) as barrier guards for SSRF.

@github-actions github-actions bot added the Python label Jun 3, 2024
@joefarebrother joefarebrother changed the title [Draft] Python: Add additional sanitizers to SSRF Python: Add additional sanitizers to SSRF Jun 4, 2024
@joefarebrother joefarebrother marked this pull request as ready for review June 4, 2024 08:54
@joefarebrother joefarebrother requested a review from a team as a code owner June 4, 2024 08:54
@tausbn tausbn self-requested a review June 10, 2024 11:27
Copy link
Contributor

@tausbn tausbn left a comment

Choose a reason for hiding this comment

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

This looks good to me, with just one small improvement suggestion. (And there's a redundant import you can get rid of.) 👍

Comment on lines +158 to +159
call = API::moduleImport("re").getMember(["match", "fullmatch"]).getACall() and
strNode = [call.getArg(1), call.getArgByName("string")]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is great, but I think we should also support the case where the regular expression is compiled in advance. That is

import re
sanitizer_re = re.compile("...")

if sanitizer_re.match(input):
   # do stuff

Unfortunately, I don't think we expose such a notion in the libraries already (there's compiledRegex in Stdlib.qll, but it's in the internal StdlibPrivate module). However, I think in this case adding a separate case for API::moduleImport("re").getMember("compile").getReturn() (and then the methods you include above) should suffice. (Note that the argument position changes slightly for compiled regexes, so it's not simply a case of augmenting the API::moduleImport("re") bit above.)

@joefarebrother joefarebrother merged commit f441c68 into github:main Jun 11, 2024
13 checks passed
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.

2 participants