Python: Add LDAP Injection query #5443

jorgectf · 2021-03-18T16:32:42Z

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

python/ql/src/experimental/Security/CWE-090/LDAPInjection.ql

jorgectf · 2021-03-23T17:54:35Z

python/ql/src/experimental/Security/CWE-090/LDAPInjection.ql

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


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?

python/ql/src/experimental/semmle/python/Concepts.qll

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

jorgectf · 2021-04-08T21:48:45Z

python/ql/test/experimental/query-tests/Security/CWE-090/LDAPInjection.expected

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


Refer to this discussion about the repeated result regarding unsafe_filter.

RasmusWL

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

RasmusWL

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.

python/ql/src/experimental/semmle/python/Concepts.qll

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

jorgectf · 2021-05-07T20:20:48Z

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

RasmusWL

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 💪

python/ql/src/experimental/semmle/python/frameworks/LDAP.qll

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

RasmusWL

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 💪

jorgectf · 2021-05-26T13:16:41Z

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

python/ql/src/experimental/Security/CWE-090/LDAPInjection.ql Outdated Show resolved Hide resolved

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

python/ql/src/experimental/semmle/python/Concepts.qll Outdated Show resolved Hide resolved

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

python/ql/src/experimental/semmle/python/frameworks/Stdlib.qll Outdated Show resolved Hide resolved

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

python/ql/src/experimental/Security/CWE-090/examples/example_good2.py Outdated Show resolved Hide resolved

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

python/ql/src/experimental/semmle/python/Concepts.qll Outdated Show resolved Hide resolved

python/ql/src/experimental/semmle/python/Concepts.qll Outdated Show resolved Hide resolved

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

Add documentation to main classes' functions.

c2b96b3

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

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

python/ql/src/experimental/semmle/python/frameworks/LDAP.qll Outdated Show resolved Hide resolved

python/ql/src/experimental/semmle/python/frameworks/LDAP.qll Outdated Show resolved Hide resolved

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

Apply documentation suggestions

9e9678b

Co-authored-by: Rasmus Wriedt Larsen <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python: Add LDAP Injection query #5443

Python: Add LDAP Injection query #5443

jorgectf commented Mar 18, 2021 •

edited

Loading

This comment has been minimized.

jorgectf Mar 23, 2021

mrthankyou Mar 26, 2021 •

edited

Loading

This comment has been minimized.

jorgectf Apr 8, 2021

RasmusWL left a comment •

edited

Loading

RasmusWL left a comment

jorgectf commented May 7, 2021

RasmusWL left a comment

RasmusWL left a comment

jorgectf commented May 26, 2021

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

Python: Add LDAP Injection query #5443

Conversation

jorgectf commented Mar 18, 2021 • edited Loading

This comment has been minimized.

jorgectf Mar 23, 2021

Choose a reason for hiding this comment

mrthankyou Mar 26, 2021 • edited Loading

Choose a reason for hiding this comment

This comment has been minimized.

jorgectf Apr 8, 2021

Choose a reason for hiding this comment

RasmusWL left a comment • edited Loading

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

jorgectf commented May 7, 2021

RasmusWL left a comment

Choose a reason for hiding this comment

RasmusWL left a comment

Choose a reason for hiding this comment

jorgectf commented May 26, 2021

jorgectf commented Mar 18, 2021 •

edited

Loading

mrthankyou Mar 26, 2021 •

edited

Loading

RasmusWL left a comment •

edited

Loading