Merge branch 'main' into python-more-additional-taint-steps

python/ql/src/Security/CWE-020/IncompleteHostnameRegExp.qhelp

@@ -6,7 +6,7 @@
     <overview>
         <p>
 
-            Sanitizing untrusted URLs is an important technique for
+            Sanitizing untrusted URLs is a common technique for
             preventing attacks such as request forgeries and malicious
             redirections. Often, this is done by checking that the host of a URL
             is in a set of allowed hosts.

python/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.qhelp

@@ -6,7 +6,7 @@
     <overview>
         <p>
 
-            Sanitizing untrusted URLs is an important technique for
+            Sanitizing untrusted URLs is a common technique for
             preventing attacks such as request forgeries and malicious
             redirections. Usually, this is done by checking that the host of a URL
             is in a set of allowed hosts.

python/ql/src/experimental/dataflow/DataFlow.qll

@@ -15,7 +15,7 @@
  * `DataFlow::localFlowStep` with arguments of type `DataFlow::Node`.
  */
 
-import python
+private import python
 
 /**
  * Provides classes for performing local (intra-procedural) and

python/ql/src/experimental/dataflow/DataFlow2.qll

@@ -15,7 +15,7 @@
  * `DataFlow::localFlowStep` with arguments of type `DataFlow::Node`.
  */
 
-import python
+private import python
 
 /**
  * Provides classes for performing local (intra-procedural) and

python/ql/src/experimental/dataflow/TaintTracking.qll

@@ -8,7 +8,7 @@
  * `TaintTracking::localTaintStep` with arguments of type `DataFlow::Node`.
  */
 
-import python
+private import python
 
 /**
  * Provides classes for performing local (intra-procedural) and

python/ql/src/experimental/dataflow/internal/DataFlowImplConsistency.qll

@@ -123,8 +123,18 @@ module Consistency {
     n.getEnclosingCallable() != call.getEnclosingCallable()
   }
 
+  // This predicate helps the compiler forget that in some languages
+  // it is impossible for a result of `getPreUpdateNode` to be an
+  // instance of `PostUpdateNode`.
+  private Node getPre(PostUpdateNode n) {
+    result = n.getPreUpdateNode()
+    or
+    none()
+  }
+
   query predicate postIsNotPre(PostUpdateNode n, string msg) {
-    n.getPreUpdateNode() = n and msg = "PostUpdateNode should not equal its pre-update node."
+    getPre(n) = n and
+    msg = "PostUpdateNode should not equal its pre-update node."
   }
 
   query predicate postHasUniquePre(PostUpdateNode n, string msg) {
@@ -152,12 +162,6 @@ module Consistency {
     msg = "Origin of readStep is missing a PostUpdateNode."
   }
 
-  query predicate storeIsPostUpdate(Node n, string msg) {
-    storeStep(_, _, n) and
-    not n instanceof PostUpdateNode and
-    msg = "Store targets should be PostUpdateNodes."
-  }
-
   query predicate argHasPostUpdate(ArgumentNode n, string msg) {
     not hasPost(n) and
     not isImmutableOrUnobservable(n) and

python/ql/src/experimental/dataflow/internal/DataFlowPrivate.qll

@@ -1,5 +1,6 @@
 private import python
 private import DataFlowPublic
+import semmle.python.SpecialMethods
 
 //--------
 // Data flow graph
@@ -7,6 +8,39 @@ private import DataFlowPublic
 //--------
 // Nodes
 //--------
+predicate isExpressionNode(ControlFlowNode node) { node.getNode() instanceof Expr }
+
+/** A control flow node which is also a dataflow node */
+class DataFlowCfgNode extends ControlFlowNode {
+  DataFlowCfgNode() { isExpressionNode(this) }
+}
+
+/** A data flow node which should have an associated post-update node. */
+abstract class PreUpdateNode extends Node { }
+
+/** An argument might have its value changed as a result of a call. */
+class ArgumentPreUpdateNode extends PreUpdateNode, ArgumentNode { }
+
+/** An object might have its value changed after a store. */
+class StorePreUpdateNode extends PreUpdateNode, CfgNode {
+  StorePreUpdateNode() {
+    exists(Attribute a |
+      node = a.getObject().getAFlowNode() and
+      a.getCtx() instanceof Store
+    )
+  }
+}
+
+/** A node marking the state change of an object after a read */
+class ReadPreUpdateNode extends PreUpdateNode, CfgNode {
+  ReadPreUpdateNode() {
+    exists(Attribute a |
+      node = a.getObject().getAFlowNode() and
+      a.getCtx() instanceof Load
+    )
+  }
+}
+
 /**
  * A node associated with an object after an operation that might have
  * changed its state.
@@ -16,12 +50,21 @@ private import DataFlowPublic
  * an update to the field.
  *
  * Nodes corresponding to AST elements, for example `ExprNode`, usually refer
- * to the value before the update with the exception of `ObjectCreation`,
- * which represents the value after the constructor has run.
+ * to the value before the update.
  */
-abstract class PostUpdateNode extends Node {
+class PostUpdateNode extends Node, TPostUpdateNode {
+  PreUpdateNode pre;
+
+  PostUpdateNode() { this = TPostUpdateNode(pre) }
+
   /** Gets the node before the state update. */
-  abstract Node getPreUpdateNode();
+  Node getPreUpdateNode() { result = pre }
+
+  override string toString() { result = "[post] " + pre.toString() }
+
+  override Scope getScope() { result = pre.getScope() }
+
+  override Location getLocation() { result = pre.getLocation() }
 }
 
 class DataFlowExpr = Expr;
@@ -90,7 +133,17 @@ module EssaFlow {
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   not nodeFrom.(EssaNode).getVar() instanceof GlobalSsaVariable and
   not nodeTo.(EssaNode).getVar() instanceof GlobalSsaVariable and
-  EssaFlow::essaFlowStep(nodeFrom, nodeTo)
+  EssaFlow::essaFlowStep(update(nodeFrom), nodeTo)
+}
+
+private Node update(Node node) {
+  exists(PostUpdateNode pun |
+    node = pun.getPreUpdateNode() and
+    result = pun
+  )
+  or
+  not exists(PostUpdateNode pun | node = pun.getPreUpdateNode()) and
+  result = node
 }
 
 // TODO: Make modules for these headings
@@ -157,17 +210,67 @@ class DataFlowClassValue extends DataFlowCallable, TClassValue {
   override string getName() { result = c.getName() }
 }
 
-/** Represents a call to a callable */
-class DataFlowCall extends CallNode {
-  DataFlowCallable callable;
+newtype TDataFlowCall =
+  TCallNode(CallNode call) or
+  TSpecialCall(SpecialMethodCallNode special)
 
-  DataFlowCall() { this = callable.getACall() }
+abstract class DataFlowCall extends TDataFlowCall {
+  /** Gets a textual representation of this element. */
+  abstract string toString();
 
   /** Get the callable to which this call goes. */
-  DataFlowCallable getCallable() { result = callable }
+  abstract DataFlowCallable getCallable();
+
+  /** Get the specified argument to this call. */
+  abstract ControlFlowNode getArg(int n);
+
+  /** Get the control flow node representing this call. */
+  abstract ControlFlowNode getNode();
 
   /** Gets the enclosing callable of this call. */
-  DataFlowCallable getEnclosingCallable() { result.getScope() = this.getNode().getScope() }
+  abstract DataFlowCallable getEnclosingCallable();
+}
+
+/** Represents a call to a callable. */
+class CallNodeCall extends DataFlowCall, TCallNode {
+  CallNode call;
+  DataFlowCallable callable;
+
+  CallNodeCall() {
+    this = TCallNode(call) and
+    call = callable.getACall()
+  }
+
+  override string toString() { result = call.toString() }
+
+  override ControlFlowNode getArg(int n) { result = call.getArg(n) }
+
+  override ControlFlowNode getNode() { result = call }
+
+  override DataFlowCallable getCallable() { result = callable }
+
+  override DataFlowCallable getEnclosingCallable() { result.getScope() = call.getNode().getScope() }
+}
+
+/** Represents a call to a special method. */
+class SpecialCall extends DataFlowCall, TSpecialCall {
+  SpecialMethodCallNode special;
+
+  SpecialCall() { this = TSpecialCall(special) }
+
+  override string toString() { result = special.toString() }
+
+  override ControlFlowNode getArg(int n) { result = special.(SpecialMethod::Potential).getArg(n) }
+
+  override ControlFlowNode getNode() { result = special }
+
+  override DataFlowCallable getCallable() {
+    result = TCallableValue(special.getResolvedSpecialMethod())
+  }
+
+  override DataFlowCallable getEnclosingCallable() {
+    result.getScope() = special.getNode().getScope()
+  }
 }
 
 /** A data flow node that represents a call argument. */
@@ -220,7 +323,7 @@ class OutNode extends CfgNode {
  * `kind`.
  */
 OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) {
-  call = result.getNode() and
+  call.getNode() = result.getNode() and
   kind = TNormalReturnKind()
 }
 

python/ql/src/experimental/dataflow/internal/DataFlowPublic.qll

@@ -2,7 +2,7 @@
  * Provides Python-specific definitions for use in the data flow library.
  */
 
-import python
+private import python
 private import DataFlowPrivate
 
 /**
@@ -20,7 +20,9 @@ newtype TNode =
   /** A node corresponding to an SSA variable. */
   TEssaNode(EssaVariable var) or
   /** A node corresponding to a control flow node. */
-  TCfgNode(ControlFlowNode node)
+  TCfgNode(DataFlowCfgNode node) or
+  /** A node representing the value of an object after a state change */
+  TPostUpdateNode(PreUpdateNode pre)
 
 /**
  * An element, viewed as a node in a data flow graph. Either an SSA variable
@@ -58,6 +60,15 @@ class Node extends TNode {
   ) {
     this.getLocation().hasLocationInfo(filepath, startline, startcolumn, endline, endcolumn)
   }
+
+  /** Convenience method for casting to EssaNode and calling getVar. */
+  EssaVariable asVar() { none() }
+
+  /** Convenience method for casting to CfgNode and calling getNode. */
+  ControlFlowNode asCfgNode() { none() }
+
+  /** Convenience method for casting to ExprNode and calling getNode and getNode again. */
+  Expr asExpr() { none() }
 }
 
 class EssaNode extends Node, TEssaNode {
@@ -67,6 +78,8 @@ class EssaNode extends Node, TEssaNode {
 
   EssaVariable getVar() { result = var }
 
+  override EssaVariable asVar() { result = var }
+
   /** Gets a textual representation of this element. */
   override string toString() { result = var.toString() }
 
@@ -76,12 +89,14 @@ class EssaNode extends Node, TEssaNode {
 }
 
 class CfgNode extends Node, TCfgNode {
-  ControlFlowNode node;
+  DataFlowCfgNode node;
 
   CfgNode() { this = TCfgNode(node) }
 
   ControlFlowNode getNode() { result = node }
 
+  override ControlFlowNode asCfgNode() { result = node }
+
   /** Gets a textual representation of this element. */
   override string toString() { result = node.toString() }
 
@@ -97,10 +112,14 @@ class CfgNode extends Node, TCfgNode {
  * to multiple `ExprNode`s, just like it may correspond to multiple
  * `ControlFlow::Node`s.
  */
-class ExprNode extends Node { }
+class ExprNode extends CfgNode {
+  ExprNode() { isExpressionNode(node) }
+
+  override Expr asExpr() { result = node.getNode() }
+}
 
 /** Gets a node corresponding to expression `e`. */
-ExprNode exprNode(DataFlowExpr e) { none() }
+ExprNode exprNode(DataFlowExpr e) { result.getNode().getNode() = e }
 
 /**
  * The value of a parameter at function entry, viewed as a node in a data

python/ql/src/experimental/dataflow/internal/TaintTrackingPrivate.qll

@@ -46,8 +46,8 @@ predicate localAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeT
  * Holds if taint can flow from `nodeFrom` to `nodeTo` with a step related to concatenation.
  *
  * Note that since we cannot easily distinguish interesting types (like string, list, tuple),
- * we consider any `+` operation to propagate taint. After consulting with the JS team, this
- * doesn't sound like it is a big problem in practice.
+ * we consider any `+` operation to propagate taint. This is what is done in the JS libraries,
+ * and isn't a big problem in practice.
  */
 predicate concatStep(DataFlow::CfgNode nodeFrom, DataFlow::CfgNode nodeTo) {
   exists(BinaryExprNode add | add = nodeTo.getNode() |

python/ql/src/semmle/python/SpecialMethods.qll

@@ -0,0 +1,117 @@
+/**
+ * Provides support for special methods.
+ * This is done in two steps:
+ *   - A subset of `ControlFlowNode`s are labelled as potentially corresponding to
+ *     a special method call (by being an instance of `SpecialMethod::Potential`).
+ *   - A subset of the potential special method calls are labelled as being actual
+ *     special method calls (`SpecialMethodCallNode`) if the appropriate method is defined.
+ * Extend `SpecialMethod::Potential` to capture more cases.
+ */
+
+import python
+
+/** A control flow node which might correspond to a special method call. */
+class PotentialSpecialMethodCallNode extends ControlFlowNode {
+  PotentialSpecialMethodCallNode() { this instanceof SpecialMethod::Potential }
+}
+
+/**
+ * Machinery for detecting special method calls.
+ * Extend `SpecialMethod::Potential` to capture more cases.
+ */
+module SpecialMethod {
+  /** A control flow node which might correspond to a special method call. */
+  abstract class Potential extends ControlFlowNode {
+    /** Gets the name of the method that would be called. */
+    abstract string getSpecialMethodName();
+
+    /** Gets the control flow node that would be passed as the specified argument. */
+    abstract ControlFlowNode getArg(int n);
+
+    /**
+     * Gets the control flow node corresponding to the instance
+     * that would define the special method.
+     */
+    ControlFlowNode getSelf() { result = this.getArg(0) }
+  }
+
+  /** A binary expression node that might correspond to a special method call. */
+  class SpecialBinOp extends Potential, BinaryExprNode {
+    Operator operator;
+
+    SpecialBinOp() { this.getOp() = operator }
+
+    override string getSpecialMethodName() { result = operator.getSpecialMethodName() }
+
+    override ControlFlowNode getArg(int n) {
+      n = 0 and result = this.getLeft()
+      or
+      n = 1 and result = this.getRight()
+    }
+  }
+
+  /** A subscript expression node that might correspond to a special method call. */
+  abstract class SpecialSubscript extends Potential, SubscriptNode {
+    override ControlFlowNode getArg(int n) {
+      n = 0 and result = this.getObject()
+      or
+      n = 1 and result = this.getIndex()
+    }
+  }
+
+  /** A subscript expression node that might correspond to a call to __getitem__. */
+  class SpecialGetItem extends SpecialSubscript {
+    SpecialGetItem() { this.isLoad() }
+
+    override string getSpecialMethodName() { result = "__getitem__" }
+  }
+
+  /** A subscript expression node that might correspond to a call to __setitem__. */
+  class SpecialSetItem extends SpecialSubscript {
+    SpecialSetItem() { this.isStore() }
+
+    override string getSpecialMethodName() { result = "__setitem__" }
+
+    override ControlFlowNode getArg(int n) {
+      n = 0 and result = this.getObject()
+      or
+      n = 1 and result = this.getIndex()
+      or
+      n = 2 and result = this.getValueNode()
+    }
+
+    private ControlFlowNode getValueNode() {
+      exists(AssignStmt a |
+        a.getATarget() = this.getNode() and
+        result.getNode() = a.getValue()
+      )
+      or
+      exists(AugAssign a |
+        a.getTarget() = this.getNode() and
+        result.getNode() = a.getValue()
+      )
+    }
+  }
+
+  /** A subscript expression node that might correspond to a call to __delitem__. */
+  class SpecialDelItem extends SpecialSubscript {
+    SpecialDelItem() { this.isDelete() }
+
+    override string getSpecialMethodName() { result = "__delitem__" }
+  }
+}
+
+/** A control flow node corresponding to a special method call. */
+class SpecialMethodCallNode extends PotentialSpecialMethodCallNode {
+  Value resolvedSpecialMethod;
+
+  SpecialMethodCallNode() {
+    exists(SpecialMethod::Potential pot |
+      this.(SpecialMethod::Potential) = pot and
+      pot.getSelf().pointsTo().getClass().lookup(pot.getSpecialMethodName()) = resolvedSpecialMethod
+    )
+  }
+
+  /** The method that is called. */
+  Value getResolvedSpecialMethod() { result = resolvedSpecialMethod }
+}