Python: Use new dataflow API #14068

RasmusWL · 2023-08-28T09:48:00Z

Highly recommend reviewing commit by commit 😅

The .expected file will have changes if:

The configuration uses isAdditionalFlowStep, since such steps will now always be part of the path-graph (lines added to .expected file)
There were previously multiple configurations in scope, which could cause some unrelated PathNodes/edges from other configurations to have been part of the .expected file before. (lines removed from .expected file)

(no change-note like in Go: #13820)

This PR covers all usage I could find in src/ and lib/. I did not focus on test/ yet. We should fix those too, but I don't think it's strictly required right now 😊

github-actions · 2023-08-28T09:49:18Z

QHelp previews:

python/ql/src/experimental/Security/CWE-522/LdapInsecureAuth.qhelp

Python Insecure LDAP Authentication

Failing to ensure the utilization of SSL in an LDAP connection can cause the entire communication to be sent in cleartext making it easier for an attacker to intercept it.

Recommendation

Always set use_SSL to True, call start_tls_s() or set a proper option flag (ldap.OPT_X_TLS_XXXXXX).

Example

This example shows both good and bad ways to deal with this issue under Python 3.

The first one sets use_SSL to true as a keyword argument whereas the second one fails to provide a value for it, so the default one is used (False).

from ldap3 import Server, Connection, ALL
from flask import request, Flask

app = Flask(__name__)


@app.route("/good")
def good():
    srv = Server(host, port, use_ssl=True)
    conn = Connection(srv, dn, password)
    conn.search(dn, search_filter)
    return conn.response


@app.route("/bad")
def bad():
    srv = Server(host, port)
    conn = Connection(srv, dn, password)
    conn.search(dn, search_filter)
    return conn.response

python/ql/src/experimental/Security/CWE-340/TokenBuiltFromUUID.ql

python/ql/src/experimental/semmle/python/security/LDAPInsecureAuth.qll

...src/experimental/Security/CWE-208/TimingAttackAgainstHash/PossibleTimingAttackAgainstHash.ql

python/ql/src/experimental/semmle/python/security/InsecureRandomness.qll

We could have switched to a stateful config, but I tried to keep changes as straight forward as possible.

I adopted helper predicates to do the "heavy" lifting of .asPathNode1(), maybe I like this approach better... let me know what you think 😊

…uestForgery` to new dataflow API

…w dataflow API

…use` a bit To use consistent naming

Since we want to know the _sinks_ and not just the flow, we need to expose the config as well :|

Split it into multiple commits to make it easier to review.

(and any higher number as well)

calumgrant · 2023-08-29T10:38:51Z

python/ql/src/experimental/semmle/python/security/InsecureRandomness.qll

  }
+
+  /** Global taint-tracking for detecting "random values that are not cryptographically secure" vulnerabilities. */
+  module Flow = TaintTracking::Global<Config>;


Is there a good reason why this uses the alternative naming convention (option 1)?

I tried to keep changes to a minimal. Since it was already inside a properly named module, I let things stay that way.

So no better reason than I tried to get things done 😅

yoff

LGTM, just minor comments.
Also, the QL4QL-alerts are interesting, we should probably follow up on those.

yoff · 2023-09-04T10:32:21Z

python/ql/lib/semmle/python/security/dataflow/WeakSensitiveDataHashingQuery.qll

+
+    predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }


What was previously covered by super.isSanitizer(node)?

Nothing 😂 I found that in a few cases I had copied this pattern from JS

yoff · 2023-09-04T10:33:14Z

python/ql/lib/semmle/python/security/dataflow/WeakSensitiveDataHashingQuery.qll

+/** Holds if data can flow from `source` to `sink` with `NormalHashFunction::Flow`. */
+predicate normalHashFunctionFlowPath(
+  WeakSensitiveDataHashingFlow::PathNode source, WeakSensitiveDataHashingFlow::PathNode sink
+) {
+  NormalHashFunction::Flow::flowPath(source.asPathNode1(), sink.asPathNode1())
+}
+
+/** Holds if data can flow from `source` to `sink` with `ComputationallyExpensiveHashFunction::Flow`. */
+predicate computationallyExpensiveHashFunctionFlowPath(
+  WeakSensitiveDataHashingFlow::PathNode source, WeakSensitiveDataHashingFlow::PathNode sink
+) {
+  ComputationallyExpensiveHashFunction::Flow::flowPath(source.asPathNode2(), sink.asPathNode2())
 }


yoff · 2023-09-04T10:54:13Z

python/ql/src/experimental/semmle/python/security/TimingAttack.qll

-    exists(UserInputInComparisonConfig config |
-      config.hasFlowTo(DataFlow2::exprNode(anotherParameter))
-    )
+    UserInputInComparisonFlow::flowTo(DataFlow2::exprNode(anotherParameter))


I think we should not need DataFlow2 any more?

I see you solved this in a later commit 👍

RasmusWL · 2023-09-04T19:03:46Z

Also, the QL4QL-alerts are interesting, we should probably follow up on those.

If you're talking about the annotations on https://github.com/github/codeql/pull/14068/checks?check_run_id=16274578369, I don't think I want to do anything about those right now 😊

RasmusWL added the no-change-note-required This PR does not need a change note label Aug 28, 2023

github-actions bot added documentation Python labels Aug 28, 2023

github-advanced-security bot found potential problems Aug 28, 2023

View reviewed changes

RasmusWL force-pushed the dataflow-config-refactor branch 2 times, most recently from 66be4f4 to 18d16b4 Compare August 28, 2023 11:02

RasmusWL added 23 commits August 28, 2023 15:27

Python: Move SqlInjection to new dataflow API

245c240

Python: Move PathInjection to new dataflow API

e970329

Python: Move TarSlip to new dataflow API

d4e4e2d

Python: Move UnsafeShellCommandConstruction to new dataflow API

700841e

Python: Move CommandInjection to new dataflow API

b30142c

Python: Move ReflectedXss to new dataflow API

c360346

Python: Move LdapInjection to new dataflow API

0557390

We could have switched to a stateful config, but I tried to keep changes as straight forward as possible.

Python: Move CodeInjection to new dataflow API

88cf9c9

Python: Move LogInjection to new dataflow API

f75e65c

Python: Move StackTraceExposure to new dataflow API

dcd9608

Python: Move PamAuthorization to new dataflow API

cca78f3

Python: Move CleartextLogging to new dataflow API

7009544

Python: Move CleartextStorage to new dataflow API

9d6b96d

Python: Move WeakSensitiveDataHashing to new dataflow API

dd07417

I adopted helper predicates to do the "heavy" lifting of .asPathNode1(), maybe I like this approach better... let me know what you think 😊

Python: Move UnsafeDeserialization to new dataflow API

6f08e73

Python: Move UrlRedirect to new dataflow API

4c76ca6

Python: Move Xxe to new dataflow API

60e4533

Python: Move XpathInjection to new dataflow API

4c33699

Python: Move PolynomialReDoS to new dataflow API

c6caf83

Python: Move RegexInjection to new dataflow API

add1077

Python: Move XmlBomb to new dataflow API

46322b7

Python: Move HardcodedCredentials to new dataflow API

dbfe517

Python: Move FullServerSideRequestForgery and `PartialServerSideReq…

657b199

…uestForgery` to new dataflow API

RasmusWL added 18 commits August 28, 2023 15:31

Python: Move experimental HeaderInjection to new dataflow API

d948e10

Python: Move experimental ClientSuppliedIpUsedInSecurityCheck to ne…

ace1e23

…w dataflow API

Python: Move experimental CsvInjection to new dataflow API

2c41270

Python: Move experimental CookieInjection to new dataflow API

2c06394

Python: Move experimental UnicodeBypassValidation to new dataflow API

c6911c2

Python: Move experimental LDAPInsecureAuth to new dataflow API

acd0f2a

Python: Move experimental paramiko to new dataflow API

bfcc194

Python: Modernize `py/azure-storage/unsafe-client-side-encryption-in-…

d5e2a30

…use` a bit To use consistent naming

Python: Move SmtpMessageConfig to new dataflow API

852b01c

Python: Modernize WebAppConstantSecretKey

6d4491e

Python: Accept missing DataflowQueryTest implementation for now

ed0e441

Python: Rename to EmailXss

6961ca5

Python: Adopt tests to new DataflowQueryTest

5ba8e10

Since we want to know the _sinks_ and not just the flow, we need to expose the config as well :|

Python: Autoformat

98538d2

Python: Add missing qldocs

efec4e7

Python: Fix naming style guide violations

996364d

Python: More style-guide renaming

c665c21

Split it into multiple commits to make it easier to review.

Python: Remove all usage of DataFlow2+TaintTracking2

e8e8d97

(and any higher number as well)

RasmusWL force-pushed the dataflow-config-refactor branch from 18d16b4 to dadefa0 Compare August 28, 2023 13:44

Python: Move ModificationOfParameterWithDefault to new dataflow API

ce63358

RasmusWL force-pushed the dataflow-config-refactor branch from dadefa0 to ce63358 Compare August 28, 2023 14:20

RasmusWL marked this pull request as ready for review August 28, 2023 15:11

RasmusWL requested a review from a team as a code owner August 28, 2023 15:11

calumgrant reviewed Aug 29, 2023

View reviewed changes

calumgrant requested a review from yoff August 29, 2023 11:25

yoff approved these changes Sep 4, 2023

View reviewed changes

RasmusWL merged commit 49f5d38 into github:main Sep 4, 2023

RasmusWL deleted the dataflow-config-refactor branch September 4, 2023 19:04

RasmusWL mentioned this pull request Sep 5, 2023

Revert "Python: Use new dataflow API" #14139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Python: Use new dataflow API #14068

Python: Use new dataflow API #14068

Uh oh!

RasmusWL commented Aug 28, 2023 •

edited

Loading

Uh oh!

github-actions bot commented Aug 28, 2023 •

edited

Loading

Python Insecure LDAP Authentication

Recommendation

Example

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calumgrant Aug 29, 2023

Uh oh!

RasmusWL Aug 29, 2023

Uh oh!

yoff left a comment

Uh oh!

yoff Sep 4, 2023

Uh oh!

RasmusWL Sep 4, 2023

Uh oh!

yoff Sep 4, 2023

Uh oh!

yoff Sep 4, 2023

Uh oh!

yoff Sep 4, 2023

Uh oh!

RasmusWL commented Sep 4, 2023

Uh oh!

Uh oh!


		predicate isSink(DataFlow::Node sink) { sink instanceof Sink }

		predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }

Python: Use new dataflow API #14068

Python: Use new dataflow API #14068

Uh oh!

Conversation

RasmusWL commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Python Insecure LDAP Authentication

Recommendation

Example

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

calumgrant Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

RasmusWL Aug 29, 2023

Choose a reason for hiding this comment

Uh oh!

yoff left a comment

Choose a reason for hiding this comment

Uh oh!

yoff Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

RasmusWL Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

yoff Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

yoff Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

yoff Sep 4, 2023

Choose a reason for hiding this comment

Uh oh!

RasmusWL commented Sep 4, 2023

Uh oh!

Uh oh!

RasmusWL commented Aug 28, 2023 •

edited

Loading

github-actions bot commented Aug 28, 2023 •

edited

Loading