Python: support `pathlib` by yoff · Pull Request #5681 · github/codeql

yoff · 2021-04-15T07:02:41Z

Adds support for pathlib.Paths by tracking these objects with a type tracker and adding appropriate taint steps.
This enables a new extension of FileSystemAccess base on pathlib.Path objects.

I included most of the API since the QLDoc for FileSystemAccess is quite broad, but I am open to dropping calls such as resolve or is_fifo which may be harder to exploit.

Currently missing:

class methods cwd and home
methods returning lists

This because `resolve` accesses the file system, I am open to not include that fact in the modelling.

that were only listed as file sytem accesses.

RasmusWL · 2021-04-15T09:40:42Z

+        // Special handling of the `/` operator
+        exists(BinaryExprNode slash, DataFlow::Node left |
+          slash.getOp() instanceof Div and
+          left.asCfgNode() = slash.getLeft() and
+          left.getALocalSource() = pathlibPath()
+        |
+          nodeTo.asCfgNode() = slash and
+          (
+            // type-preserving call
+            nodeFrom = left
+            or
+            // data injection
+            nodeFrom.asCfgNode() = slash.getRight()
+          )
+        )


Can we extend this to also handle if the path is the right operator?

A demonstration:

In [1]: from pathlib import Path In [2]: "bar" / Path("foo") Out[2]: PosixPath('bar/foo')

Aha, thanks for catching this!

Will also support both operands being paths

tausbn

A few suggestions for improvements. Apart from that I think this looks good. 👍

tausbn · 2021-04-15T14:50:03Z

+        // Type-preserving call
+        exists(DataFlow::AttrRead returnsPath |
+          returnsPath.getAttributeName() = pathlibPathMethod()
+        |
+          nodeTo.(DataFlow::CallCfgNode).getFunction() = returnsPath and
+          nodeFrom = returnsPath.getObject()
+        )
+        or
+        // Type-preserving attribute
+        exists(DataFlow::AttrRead isPath | isPath.getAttributeName() = pathlibPathAttribute() |
+          nodeTo = isPath and
+          nodeFrom = isPath.getObject()
+        )


It feels like there's a lot of overlap between this and the type tracker. Could we perhaps factor out these steps appropriately, and avoid repeating ourselves?

I tried this, see if you like...it could potentially also be done for the injection part.

tausbn · 2021-04-15T14:52:39Z

+        )
+      )
+      or
+      // Data injection


I must admit, it's not immediately obvious to me what "Data injection" means here. 🤔

It means that we should (taint-)track data from a non-Path-object (in)to a Path-object.

also standardise on camelCase.

tausbn

I think the refactoring helped a lot, but I still think it could be improved further. I have added a few suggestions.

tausbn · 2021-04-19T11:03:53Z

+  /**
+   * Flow for type presering mehtods.
+   */
+  private predicate typePreservingCall(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {


This is a very generic-sounding name (and QLDoc comment) for something that's specific to pathlib. Perhaps something with pathlib in the name would be better?
(Also, two typos: preserving methods)

Yeah, I think I had the generalisation of this in my head...

I created an issue for this now.

Alas, the typo survived. (And the name, given that it lives immediately inside the Stdlib module could really use some disambiguation.)

tausbn · 2021-04-19T11:04:11Z

+  /**
+   * Flow for type presering attributes.
+   */
+  private predicate typePreservingAttribute(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {


Same as my other comment (and also another typo).

tausbn · 2021-04-19T11:11:29Z

+          slash.getOp() instanceof Div and
+          (
+            pathOperand.asCfgNode() = slash.getLeft() and
+            dataOperand.asCfgNode() = slash.getRight()
+            or
+            pathOperand.asCfgNode() = slash.getRight() and
+            dataOperand.asCfgNode() = slash.getLeft()
+          ) and
+          pathOperand.getALocalSource() = pathlibPath()
+        |
+          nodeTo.asCfgNode() = slash and
+          nodeFrom in [
+              // type-preserving call
+              pathOperand,
+              // data injection
+              dataOperand
+            ]


Does this not just boil down to the following?

slash.getOp() instanceof Div and nodeFrom.asCfgNode() = slash.getAnOperand() and nodeFrom.getALocalSource() = pathlibPath() and nodeTo.asCfgNode() = slash

(In which case, more sharing with the pathlibPath typetracker might be possible.)

Almost, except the argument of type path lib.Path does not have to be nodeFrom. It still does become much shorter this way, though 👍

Sharing is a bit tricky for data injection, because the taint step would prefer to refer to the type tracker to limit the set of nodes.

Co-authored-by: Taus <tausbn@github.com>

…athlib

tausbn

Went through the PR again. There are still some points I would like to see addressed, but I don't want this to turn into an eternal PR, so feel free to ignore my suggestions if you disagree with them. We can always clean this up later.

I believe the code is functionally correct.

With that in mind, I've marked this as approved.

tausbn · 2021-04-22T15:06:40Z

+  /**
+   * Flow for type presering mehtods.
+   */
+  private predicate typePreservingCall(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {


Alas, the typo survived. (And the name, given that it lives immediately inside the Stdlib module could really use some disambiguation.)

Co-authored-by: Taus <tausbn@github.com>

tausbn

Alright, let's see if it floats. 🚢

github-actions Bot added the Python label Apr 15, 2021

yoff added 3 commits April 15, 2021 09:40

Python: Add some tests for pathlib

8489403

Python: Attempts at modelling pathlib-Paths

2387dc6

Python update tests

52a9040

yoff force-pushed the python-support-pathlib branch from 6cc8a5b to 52a9040 Compare April 15, 2021 07:47

yoff marked this pull request as ready for review April 15, 2021 07:51

yoff requested a review from a team as a code owner April 15, 2021 07:51

Python: add changenote

c9b2c78

github-actions Bot added the documentation label Apr 15, 2021

yoff added 3 commits April 15, 2021 10:47

Python: update test expectations

3eb1813

Python: update annotations

02e41d8

This because `resolve` accesses the file system, I am open to not include that fact in the modelling.

Python: add some path returning functions

d361d99

that were only listed as file sytem accesses.

RasmusWL reviewed Apr 15, 2021

View reviewed changes

Python: support / from the right

42ae5f4

Will also support both operands being paths

yoff requested a review from RasmusWL April 15, 2021 14:08

tausbn requested changes Apr 15, 2021

View reviewed changes

yoff added 4 commits April 16, 2021 07:36

Python: use standard tracking construction

8aa6b1a

Python: simplify code following review suggestion

341dbce

also standardise on camelCase.

Python: refactor based on review suggestion

0678745

Python: update test expectation

a8280f9

yoff requested a review from tausbn April 16, 2021 15:02

tausbn requested changes Apr 19, 2021

View reviewed changes

yoff and others added 2 commits April 20, 2021 22:15

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

0c41811

Co-authored-by: Taus <tausbn@github.com>

Python: Improvements from review suggestions

b724e51

yoff requested a review from tausbn April 22, 2021 09:12

Merge branch 'main' of github.com:github/codeql into python-support-p…

5a4e661

…athlib

tausbn previously approved these changes Apr 22, 2021

View reviewed changes

Apply suggestions from code review

1954c0b

Co-authored-by: Taus <tausbn@github.com>

yoff dismissed stale reviews from tausbn via 1954c0b April 23, 2021 08:20

Python: More cleanup from reviewer suggestions

7cc9783

yoff requested a review from tausbn April 23, 2021 18:27

Python: add flow from methods to calls

16bde27

RasmusWL removed their request for review April 29, 2021 12:42

tausbn approved these changes May 4, 2021

View reviewed changes

codeql-ci merged commit 95f26aa into github:main May 4, 2021

Conversation

yoff commented Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

tausbn 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tausbn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yoff commented Apr 15, 2021 •

edited

Loading