Merge pull request github#4720 from RasmusWL/python-better-open-models

Python: better models of `open` function

Author: yoff

python/change-notes/2020-11-25-better-open-models.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* Modeling of file system access has been improved to recognize `io.open` and `builtins.open`.

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

@@ -651,7 +651,7 @@ private module Stdlib {
    * WARNING: Only holds for a few predefined attributes.
    */
   private DataFlow::Node builtins_attr(DataFlow::TypeTracker t, string attr_name) {
-    attr_name in ["exec", "eval", "compile"] and
+    attr_name in ["exec", "eval", "compile", "open"] and
     (
       t.start() and
       result = DataFlow::importNode(["builtins", "__builtin__"] + "." + attr_name)
@@ -728,165 +728,222 @@ private module Stdlib {
       )
     }
   }
-}
 
-/**
- * An exec statement (only Python 2).
- * Se ehttps://docs.python.org/2/reference/simple_stmts.html#the-exec-statement.
- */
-private class ExecStatement extends CodeExecution::Range {
-  ExecStatement() {
-    // since there are no DataFlow::Nodes for a Statement, we can't do anything like
-    // `this = any(Exec exec)`
-    this.asExpr() = any(Exec exec).getBody()
-  }
+  /**
+   * A call to the builtin `open` function.
+   * See https://docs.python.org/3/library/functions.html#open
+   */
+  private class OpenCall extends FileSystemAccess::Range, DataFlow::CfgNode {
+    override CallNode node;
 
-  override DataFlow::Node getCode() { result = this }
-}
+    OpenCall() {
+      node.getFunction() = builtins_attr("open").asCfgNode()
+      or
+      node.getFunction() = io_attr("open").asCfgNode()
+    }
 
-/**
- * A call to the builtin `open` function.
- * See https://docs.python.org/3/library/functions.html#open
- */
-private class OpenCall extends FileSystemAccess::Range, DataFlow::CfgNode {
-  override CallNode node;
+    override DataFlow::Node getAPathArgument() {
+      result.asCfgNode() in [node.getArg(0), node.getArgByName("file")]
+    }
+  }
 
-  OpenCall() { node.getFunction().(NameNode).getId() = "open" }
+  /**
+   * An exec statement (only Python 2).
+   * Se ehttps://docs.python.org/2/reference/simple_stmts.html#the-exec-statement.
+   */
+  private class ExecStatement extends CodeExecution::Range {
+    ExecStatement() {
+      // since there are no DataFlow::Nodes for a Statement, we can't do anything like
+      // `this = any(Exec exec)`
+      this.asExpr() = any(Exec exec).getBody()
+    }
 
-  override DataFlow::Node getAPathArgument() {
-    result.asCfgNode() in [node.getArg(0), node.getArgByName("file")]
+    override DataFlow::Node getCode() { result = this }
   }
-}
 
-// ---------------------------------------------------------------------------
-// base64
-// ---------------------------------------------------------------------------
-/** Gets a reference to the `base64` module. */
-private DataFlow::Node base64(DataFlow::TypeTracker t) {
-  t.start() and
-  result = DataFlow::importNode("base64")
-  or
-  exists(DataFlow::TypeTracker t2 | result = base64(t2).track(t2, t))
-}
+  // ---------------------------------------------------------------------------
+  // base64
+  // ---------------------------------------------------------------------------
+  /** Gets a reference to the `base64` module. */
+  private DataFlow::Node base64(DataFlow::TypeTracker t) {
+    t.start() and
+    result = DataFlow::importNode("base64")
+    or
+    exists(DataFlow::TypeTracker t2 | result = base64(t2).track(t2, t))
+  }
 
-/** Gets a reference to the `base64` module. */
-DataFlow::Node base64() { result = base64(DataFlow::TypeTracker::end()) }
+  /** Gets a reference to the `base64` module. */
+  DataFlow::Node base64() { result = base64(DataFlow::TypeTracker::end()) }
 
-/**
- * Gets a reference to the attribute `attr_name` of the `base64` module.
- * WARNING: Only holds for a few predefined attributes.
- */
-private DataFlow::Node base64_attr(DataFlow::TypeTracker t, string attr_name) {
-  attr_name in [
-      "b64encode", "b64decode", "standard_b64encode", "standard_b64decode", "urlsafe_b64encode",
-      "urlsafe_b64decode", "b32encode", "b32decode", "b16encode", "b16decode", "encodestring",
-      "decodestring", "a85encode", "a85decode", "b85encode", "b85decode", "encodebytes",
-      "decodebytes"
-    ] and
-  (
-    t.start() and
-    result = DataFlow::importNode("base64" + "." + attr_name)
+  /**
+   * Gets a reference to the attribute `attr_name` of the `base64` module.
+   * WARNING: Only holds for a few predefined attributes.
+   */
+  private DataFlow::Node base64_attr(DataFlow::TypeTracker t, string attr_name) {
+    attr_name in [
+        "b64encode", "b64decode", "standard_b64encode", "standard_b64decode", "urlsafe_b64encode",
+        "urlsafe_b64decode", "b32encode", "b32decode", "b16encode", "b16decode", "encodestring",
+        "decodestring", "a85encode", "a85decode", "b85encode", "b85decode", "encodebytes",
+        "decodebytes"
+      ] and
+    (
+      t.start() and
+      result = DataFlow::importNode("base64" + "." + attr_name)
+      or
+      t.startInAttr(attr_name) and
+      result = base64()
+    )
     or
-    t.startInAttr(attr_name) and
-    result = base64()
-  )
-  or
-  // Due to bad performance when using normal setup with `base64_attr(t2, attr_name).track(t2, t)`
-  // we have inlined that code and forced a join
-  exists(DataFlow::TypeTracker t2 |
-    exists(DataFlow::StepSummary summary |
-      base64_attr_first_join(t2, attr_name, result, summary) and
-      t = t2.append(summary)
+    // Due to bad performance when using normal setup with `base64_attr(t2, attr_name).track(t2, t)`
+    // we have inlined that code and forced a join
+    exists(DataFlow::TypeTracker t2 |
+      exists(DataFlow::StepSummary summary |
+        base64_attr_first_join(t2, attr_name, result, summary) and
+        t = t2.append(summary)
+      )
     )
-  )
-}
-
-pragma[nomagic]
-private predicate base64_attr_first_join(
-  DataFlow::TypeTracker t2, string attr_name, DataFlow::Node res, DataFlow::StepSummary summary
-) {
-  DataFlow::StepSummary::step(base64_attr(t2, attr_name), res, summary)
-}
+  }
 
-/**
- * Gets a reference to the attribute `attr_name` of the `base64` module.
- * WARNING: Only holds for a few predefined attributes.
- */
-private DataFlow::Node base64_attr(string attr_name) {
-  result = base64_attr(DataFlow::TypeTracker::end(), attr_name)
-}
+  pragma[nomagic]
+  private predicate base64_attr_first_join(
+    DataFlow::TypeTracker t2, string attr_name, DataFlow::Node res, DataFlow::StepSummary summary
+  ) {
+    DataFlow::StepSummary::step(base64_attr(t2, attr_name), res, summary)
+  }
 
-/** A call to any of the encode functions in the `base64` module. */
-private class Base64EncodeCall extends Encoding::Range, DataFlow::CfgNode {
-  override CallNode node;
-
-  Base64EncodeCall() {
-    exists(string name |
-      name in [
-          "b64encode", "standard_b64encode", "urlsafe_b64encode", "b32encode", "b16encode",
-          "encodestring", "a85encode", "b85encode", "encodebytes"
-        ] and
-      node.getFunction() = base64_attr(name).asCfgNode()
-    )
+  /**
+   * Gets a reference to the attribute `attr_name` of the `base64` module.
+   * WARNING: Only holds for a few predefined attributes.
+   */
+  private DataFlow::Node base64_attr(string attr_name) {
+    result = base64_attr(DataFlow::TypeTracker::end(), attr_name)
   }
 
-  override DataFlow::Node getAnInput() { result.asCfgNode() = node.getArg(0) }
+  /** A call to any of the encode functions in the `base64` module. */
+  private class Base64EncodeCall extends Encoding::Range, DataFlow::CfgNode {
+    override CallNode node;
 
-  override DataFlow::Node getOutput() { result = this }
+    Base64EncodeCall() {
+      exists(string name |
+        name in [
+            "b64encode", "standard_b64encode", "urlsafe_b64encode", "b32encode", "b16encode",
+            "encodestring", "a85encode", "b85encode", "encodebytes"
+          ] and
+        node.getFunction() = base64_attr(name).asCfgNode()
+      )
+    }
 
-  override string getFormat() {
-    exists(string name | node.getFunction() = base64_attr(name).asCfgNode() |
-      name in [
-          "b64encode", "standard_b64encode", "urlsafe_b64encode", "encodestring", "encodebytes"
-        ] and
-      result = "Base64"
-      or
-      name = "b32encode" and result = "Base32"
-      or
-      name = "b16encode" and result = "Base16"
-      or
-      name = "a85encode" and result = "Ascii85"
-      or
-      name = "b85encode" and result = "Base85"
-    )
-  }
-}
+    override DataFlow::Node getAnInput() { result.asCfgNode() = node.getArg(0) }
 
-/** A call to any of the decode functions in the `base64` module. */
-private class Base64DecodeCall extends Decoding::Range, DataFlow::CfgNode {
-  override CallNode node;
-
-  Base64DecodeCall() {
-    exists(string name |
-      name in [
-          "b64decode", "standard_b64decode", "urlsafe_b64decode", "b32decode", "b16decode",
-          "decodestring", "a85decode", "b85decode", "decodebytes"
-        ] and
-      node.getFunction() = base64_attr(name).asCfgNode()
-    )
+    override DataFlow::Node getOutput() { result = this }
+
+    override string getFormat() {
+      exists(string name | node.getFunction() = base64_attr(name).asCfgNode() |
+        name in [
+            "b64encode", "standard_b64encode", "urlsafe_b64encode", "encodestring", "encodebytes"
+          ] and
+        result = "Base64"
+        or
+        name = "b32encode" and result = "Base32"
+        or
+        name = "b16encode" and result = "Base16"
+        or
+        name = "a85encode" and result = "Ascii85"
+        or
+        name = "b85encode" and result = "Base85"
+      )
+    }
   }
 
-  override predicate mayExecuteInput() { none() }
+  /** A call to any of the decode functions in the `base64` module. */
+  private class Base64DecodeCall extends Decoding::Range, DataFlow::CfgNode {
+    override CallNode node;
+
+    Base64DecodeCall() {
+      exists(string name |
+        name in [
+            "b64decode", "standard_b64decode", "urlsafe_b64decode", "b32decode", "b16decode",
+            "decodestring", "a85decode", "b85decode", "decodebytes"
+          ] and
+        node.getFunction() = base64_attr(name).asCfgNode()
+      )
+    }
 
-  override DataFlow::Node getAnInput() { result.asCfgNode() = node.getArg(0) }
+    override predicate mayExecuteInput() { none() }
 
-  override DataFlow::Node getOutput() { result = this }
+    override DataFlow::Node getAnInput() { result.asCfgNode() = node.getArg(0) }
 
-  override string getFormat() {
-    exists(string name | node.getFunction() = base64_attr(name).asCfgNode() |
-      name in [
-          "b64decode", "standard_b64decode", "urlsafe_b64decode", "decodestring", "decodebytes"
-        ] and
-      result = "Base64"
-      or
-      name = "b32decode" and result = "Base32"
-      or
-      name = "b16decode" and result = "Base16"
-      or
-      name = "a85decode" and result = "Ascii85"
+    override DataFlow::Node getOutput() { result = this }
+
+    override string getFormat() {
+      exists(string name | node.getFunction() = base64_attr(name).asCfgNode() |
+        name in [
+            "b64decode", "standard_b64decode", "urlsafe_b64decode", "decodestring", "decodebytes"
+          ] and
+        result = "Base64"
+        or
+        name = "b32decode" and result = "Base32"
+        or
+        name = "b16decode" and result = "Base16"
+        or
+        name = "a85decode" and result = "Ascii85"
+        or
+        name = "b85decode" and result = "Base85"
+      )
+    }
+  }
+
+  // ---------------------------------------------------------------------------
+  // io
+  // ---------------------------------------------------------------------------
+  /** Gets a reference to the `io` module. */
+  private DataFlow::Node io(DataFlow::TypeTracker t) {
+    t.start() and
+    result = DataFlow::importNode("io")
+    or
+    exists(DataFlow::TypeTracker t2 | result = io(t2).track(t2, t))
+  }
+
+  /** Gets a reference to the `io` module. */
+  DataFlow::Node io() { result = io(DataFlow::TypeTracker::end()) }
+
+  /**
+   * Gets a reference to the attribute `attr_name` of the `io` module.
+   * WARNING: Only holds for a few predefined attributes.
+   */
+  private DataFlow::Node io_attr(DataFlow::TypeTracker t, string attr_name) {
+    attr_name in ["open"] and
+    (
+      t.start() and
+      result = DataFlow::importNode("io" + "." + attr_name)
       or
-      name = "b85decode" and result = "Base85"
+      t.startInAttr(attr_name) and
+      result = io()
     )
+    or
+    // Due to bad performance when using normal setup with `io_attr(t2, attr_name).track(t2, t)`
+    // we have inlined that code and forced a join
+    exists(DataFlow::TypeTracker t2 |
+      exists(DataFlow::StepSummary summary |
+        io_attr_first_join(t2, attr_name, result, summary) and
+        t = t2.append(summary)
+      )
+    )
+  }
+
+  pragma[nomagic]
+  private predicate io_attr_first_join(
+    DataFlow::TypeTracker t2, string attr_name, DataFlow::Node res, DataFlow::StepSummary summary
+  ) {
+    DataFlow::StepSummary::step(io_attr(t2, attr_name), res, summary)
+  }
+
+  /**
+   * Gets a reference to the attribute `attr_name` of the `io` module.
+   * WARNING: Only holds for a few predefined attributes.
+   */
+  private DataFlow::Node io_attr(string attr_name) {
+    result = io_attr(DataFlow::TypeTracker::end(), attr_name)
   }
 }
 

python/ql/test/experimental/library-tests/frameworks/stdlib/FileSystemAccess.py

@@ -1,7 +1,18 @@
+import builtins
+import io
+
 open("filepath")  # $getAPathArgument="filepath"
 open(file="filepath")  # $getAPathArgument="filepath"
 
 o = open
 
-o("filepath")  # $ MISSING: getAPathArgument="filepath"
-o(file="filepath")  # $ MISSING: getAPathArgument="filepath"
+o("filepath")  # $getAPathArgument="filepath"
+o(file="filepath")  # $getAPathArgument="filepath"
+
+
+builtins.open("filepath")  # $getAPathArgument="filepath"
+builtins.open(file="filepath")  # $getAPathArgument="filepath"
+
+
+io.open("filepath")  # $getAPathArgument="filepath"
+io.open(file="filepath")  # $getAPathArgument="filepath"