-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Python: Add additional sanitizers to SSRF #16657
Conversation
There was a problem hiding this 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.) 👍
call = API::moduleImport("re").getMember(["match", "fullmatch"]).getACall() and | ||
strNode = [call.getArg(1), call.getArgByName("string")] |
There was a problem hiding this comment.
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.)
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.