Python: Add LDAP Injection query by jorgectf · Pull Request #5443 · github/codeql · GitHub

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

Pending refactor. (Pending decision -> covering the specific methods of the libs or matching %sanitize% like this)

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?

Refer to this discussion about the repeated result regarding unsafe_filter.

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 👍

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-experimental frameworks folder, you can see that our normal way of doing this is to create one .qll file for each package modeled (and then all of those .qll files 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>

If you take a look at the non-experimental frameworks folder, you can see that our normal way of doing this is to create one .qll file for each package modeled

34b8af3

A few of the elements were missing QLDoc (we strive to add QLDoc to all publicly exposed elements). I added suggestions for adding these.

c2b96b3

you don't need to suffix everything with Node

6159fbe

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>

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 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 :)

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

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

jorgectf mentioned this pull request Mar 18, 2021

[Python] CWE-090: LDAP Injection github/securitylab#319

Closed

1 task

This comment has been minimized.

Sign in to view

jorgectf commented Mar 23, 2021

View reviewed changes

Comment thread python/ql/src/experimental/Security/CWE-090/LDAPInjection.ql Outdated

jorgectf commented Mar 23, 2021

View reviewed changes

jorgectf added 4 commits March 28, 2021 18:33

Upload LDAP Injection query, qhelp and tests

799d509

Move to experimental folder

719b48c

Precision warn and Remove CWE reference

95a1dae

Refactor in progress

85ec82a

jorgectf force-pushed the jorgectf/python/ldapInjection branch from 9902679 to 85ec82a Compare March 28, 2021 19:07

This comment has been minimized.

Sign in to view

Refactor LDAP3 stuff (untested)

ad36bea

mrthankyou reviewed Mar 29, 2021

View reviewed changes

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

jorgectf added 4 commits March 29, 2021 23:28

Add a test without attributes

8223539

Polish up ldap3 tests

3cda2e5

Update Sink

8faafb6

Remove attrs feature

4328ff3

jorgectf commented Mar 31, 2021

View reviewed changes

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

jorgectf added 10 commits March 31, 2021 23:19

Improve Sanitizer calls

9b43031

Simplify query

1bcb9cd

Optimize calls

33423ea

Change LDAP config (qll) filename

a1850dd

Polish LDAP3Query

8661cb0

Polish tests

7296879

Improve qhelp

3c1ca72

Create qhelp examples

1554f4f

Move tests to /test

95bfdc4

Add qlref

4f85de8

Generate .expected

7819d1a

jorgectf commented Apr 8, 2021

View reviewed changes

jorgectf added 6 commits April 8, 2021 23:49

Add qhelp last newline

b405c67

Polish metadata

82f47f8

Fix qhelp examples extension

cd75433

Write documentation

a2e8d88

Polish documentation

b020ea6

Fix documentation typo

1c34230

RasmusWL reviewed May 5, 2021

View reviewed changes

Comment thread python/ql/src/experimental/Security/CWE-090/examples/example_good2.py Outdated

RasmusWL self-assigned this May 5, 2021

RasmusWL mentioned this pull request May 5, 2021

Python: Add LDAP Improper Authentication query #5444

Merged

RasmusWL reviewed May 7, 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

jorgectf and others added 4 commits May 7, 2021 21:51

Add documentation to main classes' functions.

c2b96b3

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Move structure to LDAP.qll

34b8af3

Update functions naming

6159fbe

Add LDAP framework entry in Frameworks.qll

2ad72ad

Update sink and sanitizer to match new naming

8665747

jorgectf force-pushed the jorgectf/python/ldapInjection branch from 08be907 to 8665747 Compare May 8, 2021 16:09

RasmusWL reviewed May 10, 2021

View reviewed changes

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

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

jorgectf and others added 2 commits May 21, 2021 16:17

Apply documentation suggestions

9e9678b

Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>

Update tests and .expected

37d6ff7

jorgectf force-pushed the jorgectf/python/ldapInjection branch from ac55c76 to 37d6ff7 Compare May 21, 2021 16:41

RasmusWL added 2 commits May 26, 2021 11:01

Merge branch 'main' into jorgectf/python/ldapInjection

d5f2846

Python: autoformat

f807c2f

RasmusWL approved these changes May 26, 2021

View reviewed changes

RasmusWL merged commit 795a1c7 into github:main May 26, 2021

jorgectf mentioned this pull request Jun 16, 2021

Python: CWE-943 - Add NoSQL injection query #5612

Merged

+              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
+                  )
+                }
+              }

		\| 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 \|

Python: Add LDAP Injection query by jorgectf · Pull Request #5443 · github/codeql · GitHub

Uh oh!

Python: Add LDAP Injection query - #5443

Python: Add LDAP Injection query#5443RasmusWL merged 35 commits intogithub:maingithub/codeql:mainfrom jorgectf:jorgectf/python/ldapInjectionjorgectf/codeql:jorgectf/python/ldapInjectionCopy head branch name to clipboard

Conversation

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

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

jorgectf Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

jorgectf Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

RasmusWL left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jorgectf commented May 7, 2021

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

jorgectf commented May 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Python: Add LDAP Injection query#5443
RasmusWL merged 35 commits into
github:mainfrom
jorgectf:jorgectf/python/ldapInjection

jorgectf commented Mar 18, 2021 •
edited
Loading

mrthankyou Mar 26, 2021 •
edited
Loading

RasmusWL left a comment •
edited
Loading