Python: Add LDAP Injection query#5443
Conversation
This comment has been minimized.
This comment has been minimized.
| class EscapeSanitizer extends DataFlow::Node { | ||
| EscapeSanitizer() { | ||
| exists(Call c | | ||
| ( | ||
| // avoid flow through any %escape% function | ||
| c.getFunc().(Attribute).getName().matches("%escape%") or // something.%escape%() | ||
| c.getFunc().(Name).getId().matches("%escape%") // %escape%() | ||
| ) and | ||
| this.asExpr() = c | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Pending refactor. (Pending decision -> covering the specific methods of the libs or matching %sanitize% like this)
There was a problem hiding this comment.
Not sure how the CodeQL team feels about it but if there are cases where a CodeQL query developer thinks that data is going to be sanitized by a sanitize-type function, should the FilterOrDNSanitizationCall predicate be extracted out so that it can be used in other queries and not just the LDAP Injection vulnerabilities?
9902679 to
85ec82a
Compare
This comment has been minimized.
This comment has been minimized.
| | ldap3_bad.py:18:28:18:40 | ControlFlowNode for unsafe_filter | ldap3_bad.py:13:27:13:33 | ControlFlowNode for request | ldap3_bad.py:18:28:18:40 | ControlFlowNode for unsafe_filter | $@ LDAP query parameter comes from $@. | ldap3_bad.py:18:28:18:40 | ControlFlowNode for unsafe_filter | This | ldap3_bad.py:13:27:13:33 | ControlFlowNode for request | a user-provided value | | ||
| | ldap3_bad.py:18:28:18:40 | ControlFlowNode for unsafe_filter | ldap3_bad.py:14:35:14:41 | ControlFlowNode for request | ldap3_bad.py:18:28:18:40 | ControlFlowNode for unsafe_filter | $@ LDAP query parameter comes from $@. | ldap3_bad.py:18:28:18:40 | ControlFlowNode for unsafe_filter | This | ldap3_bad.py:14:35:14:41 | ControlFlowNode for request | a user-provided value | |
There was a problem hiding this comment.
Refer to this discussion about the repeated result regarding unsafe_filter.
There was a problem hiding this comment.
Overall this looks like fine port of the query from Java/C#.
There are some minor things I would clean up before promoting this from experimental (like the examples for how proper escaping should look like being functional), but overall the code looks ok.
I'll get in touch privately to see how to we should proceed form here, since I know you're busy at the moment 👍
python/ql/src/experimental/Security/CWE-090/examples/example_good2.py
Outdated
Show resolved
Hide resolved
RasmusWL
left a comment
There was a problem hiding this comment.
The other minor things you could look at before we merge this PR is:
- Currently the modeling of these external LDAP packages are done in
python/ql/src/experimental/semmle/python/frameworks/Stdlib.qll. If you take a look at the non-experimentalframeworksfolder, you can see that our normal way of doing this is to create one.qllfile for each package modeled (and then all of those.qllfiles are mentioned in https://github.com/github/codeql/blob/main/python/ql/src/semmle/python/Frameworks.qll) - A few of the elements were missing QLDoc (we strive to add QLDoc to all publicly exposed elements). I added suggestions for adding these.
Just FYI, as discussed in #5680 (comment), you don't need to suffix everything with Node. For example, I would probably have called LDAPQuery.getLDAPNode for LDAPQuery.getQuery() (since just calling it LDAPQuery.getLDAP seems wrong), and LDAPEscape.getEscapeNode for LDAPEscape.getAnInput (again, since LDAPEscape.getEscape sounds strange)... but this is clearly just nitpicks, and something that will be easy to do if we promote this query out of experimental.
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
|
08be907 to
8665747
Compare
RasmusWL
left a comment
There was a problem hiding this comment.
Thanks for making those last changes 👍 Besides fixing up the example of how to use escaping, and a few QLDocs that could be slightly improved, I think this PR looks good 💪
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
ac55c76 to
37d6ff7
Compare
RasmusWL
left a comment
There was a problem hiding this comment.
Thanks for working through the last few things, I've resolved the merge conflict and made the autoformatter happy, so now we can merge this query 💪
Thanks for the fix and the merge :) |
Any suggestions/tricks to improve the query or workarounds (to learn), no matter how minimal they are, are massively appreciated.