Python: promote LDAP injection query by yoff · Pull Request #7783 · github/codeql · GitHub

Some questions:

Is the use of flow states acceptable or would you rather see two configurations?
Should we rename the query to simply "LDAP injection"?

The original configuration did not match sinks with sanitizers. Here it is resolved using flow state, it could also be done by using two configurations.

The original configuration did not match sinks with sanitizers.
Here it is resolved using flow state, it could also be done by using two configurations.

Good catch. I remember this bit, and thought it was called out in the original PR, but I couldn't see it anywhere.

I think it would be a simpler setup with two configurations (if there were 3 different kinds, the state-transitions would become overly complex).

Other things

This is no LdapInjectionCustomizations.qll file, which should be added.

Please also update docs/codeql/support/reusables/frameworks.rst

Should we rename the query to simply "LDAP injection"?

I don't understand what you're suggesting. That use that for the @name field? (I'm not against it, but it would also be nice to match what it's called in Java/C#)

I'm thinking we should create an Ldap module that will contain all LDAP related concepts, like we've done for HTTP related concepts. I know that we have multiple experimental queries, so I think it could be worth the effort 👍

I am happy to do that, it was like that in the external contribution, actually :-)

Oh, I misunderstood. Yes, that is a good idea.

Done

If we do add such a Ldap module, I'm really at odds with whether we should move those ldap escaping classes in there (since they're clearly only ldap related), or whether we should keep them here close to the other escaping classes 🤔 I guess that since we have HtmlEscaping defined right here, we should not move them.

(keeping this note as a way to highlight this, but nothing to do for this code)

is there a reason you're not using the typical nodeFrom -> nodeTo naming?

That comes from the data flow API. I would change it (but I will do a two-config version instead first).

This is no LdapInjectionCustomizations.qll file, which should be added.

I wondered about this, but forgot to write about it. Do we always add a customisations file, even if the reasonable customisations are covered by concepts?

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

source nodes get duplicated, so perhaps flow states are actually better for performance?

Just a few minor things, then we should be good to go 👍

nitpick: our style guide suggests that because this acronym is longer than 2 characters, it should be written as Ldap. I guess there is some value in having consistency between the existing HTTP module. I'm not really sure whether this is something we need to change...

Yeah, I modelled it after the HTTP module. I guess we can change both in a clean-up PR? Or are we creating deprecation work for ourselves that way?

I think it's OK

Found a really minor detail, otherwise LGTM 👍

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

Thanks 👍

yoff added 7 commits January 26, 2022 19:05

python: fix typo in qhelp

e6b5833

python: port concepts and implementations

163c888

Python: Update list of frameworks

d539920

Python: Move configuration over and refine it

a026120

The original configuration did not match sinks with sanitizers. Here it is resolved using flow state, it could also be done by using two configurations.

python: Move over query and tests

4c3c4de

python: rename files

ab43f04

python: add change note

68d18ea

yoff requested a review from a team as a code owner January 28, 2022 13:03

github-actions Bot added documentation Python labels Jan 28, 2022

RasmusWL requested changes Jan 31, 2022

View reviewed changes

yoff and others added 5 commits February 1, 2022 10:06

Apply suggestions from code review

45f0bfd

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

Update python/ql/src/Security/CWE-090/LdapInjection.ql

b120721

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

python: update frameworks.rst

7df5c70

python: rewrite to separate configurations

c2cd58e

source nodes get duplicated, so perhaps flow states are actually better for performance?

python: create LDAP module in Concepts

1e2428c

yoff requested a review from RasmusWL February 1, 2022 13:40

python: add missing QlDoc

4ad99d9

RasmusWL requested changes Feb 8, 2022

View reviewed changes

jorgectf mentioned this pull request Feb 8, 2022

Python: Port and extend XXE modeling #6112

Merged

python: more detailed alert message

84447e4

yoff requested a review from RasmusWL February 14, 2022 10:56

python: fix change-note formatting

d1200d0

RasmusWL reviewed Feb 14, 2022

View reviewed changes

Comment thread docs/codeql/support/reusables/frameworks.rst Outdated

Update docs/codeql/support/reusables/frameworks.rst

86786d3

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

yoff requested a review from RasmusWL February 14, 2022 15:06

RasmusWL approved these changes Feb 15, 2022

View reviewed changes

RasmusWL merged commit 5a90214 into github:main Feb 15, 2022

+              /**
+               * An escape of a string so it can be safely used as a distinguished name (DN)
+               * in an LDAP search.
+               */
+              class LdapDnEscaping extends Escaping {
+                LdapDnEscaping() { range.getKind() = Escaping::getLdapDnKind() }
+              }
+              /**
+               * An escape of a string so it can be safely used as a filter in an LDAP search.
+               */
+              class LdapFilterEscaping extends Escaping {
+                LdapFilterEscaping() { range.getKind() = Escaping::getLdapFilterKind() }
+              }

-              /** Provides classes for modeling new LDAP query execution-related APIs. */
-              module LdapExecution {
+              /** Provides classes for modeling LDAP-related APIs. */
+              module LDAP {

		DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
		DataFlow::FlowState state2

Python: promote LDAP injection query by yoff · Pull Request #7783 · github/codeql · GitHub

Python: promote LDAP injection query - #7783

Python: promote LDAP injection query#7783RasmusWL merged 16 commits intogithub:maingithub/codeql:mainfrom yoff:python/promote-ldap-injectionyoff/codeql:python/promote-ldap-injectionCopy head branch name to clipboard

Conversation

yoff commented Jan 28, 2022

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Other things

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yoff commented Feb 1, 2022

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

RasmusWL left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Python: promote LDAP injection query#7783
RasmusWL merged 16 commits into
github:mainfrom
yoff:python/promote-ldap-injection