Merge pull request #878 from taus-semmle/python-mutable-default-with-flow

Python: Make "Modification of parameter with default" flow-sensitive.

change-notes/1.20/analysis-python.md

@@ -28,6 +28,7 @@ The API has been improved to declutter the global namespace and improve discover
  | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
 | Comparison using is when operands support \_\_eq\_\_ (`py/comparison-using-is`) | Fewer false positive results | Results where one of the objects being compared is an enum member are no longer reported. |
+| Modification of parameter with default (`py/modification-of-default-value`) | More true positive results | Instances where the mutable default value is mutated inside other functions are now also reported. |
 | Mutation of descriptor in \_\_get\_\_ or \_\_set\_\_ method (`py/mutable-descriptor`) | Fewer false positive results | Results where the mutation does not occur when calling one of the  `__get__`, `__set__` or `__delete__` methods are no longer reported. |
 | Unused import (`py/unused-import`) | Fewer false positive results | Results where the imported module is used in a `doctest` string are no longer reported. |
 | Unused import (`py/unused-import`) | Fewer false positive results | Results where the imported module is used in a type-hint comment are no longer reported. |

python/ql/src/Functions/ModificationOfParameterWithDefault.ql

@@ -2,7 +2,7 @@
  * @name Modification of parameter with default
  * @description Modifying the default value of a parameter can lead to unexpected
  *              results.
- * @kind problem
+ * @kind path-problem
  * @tags reliability
  *       maintainability
  * @problem.severity error
@@ -12,6 +12,7 @@
  */
 
 import python
+import semmle.python.security.Paths
 
 predicate safe_method(string name) {
     name = "count" or name = "index" or name = "copy" or name =  "get"  or name = "has_key" or
@@ -23,27 +24,6 @@ predicate maybe_parameter(SsaVariable var, Function f, Parameter p) {
     f.getAnArg() = p
 }
 
-Name use_of_parameter(Parameter p) {
-    exists(SsaVariable var |
-        p = var.getAnUltimateDefinition().getDefinition().getNode() and
-        var.getAUse().getNode() = result
-    )
-}
-
-predicate modifying_call(Call c, Parameter p) {
-    exists(Attribute a |
-        c.getFunc() = a |
-        a.getObject() = use_of_parameter(p) and
-        not safe_method(a.getName())
-    )
-}
-
-predicate is_modification(AstNode a, Parameter p) {
-    modifying_call(a, p)
-    or
-    a.(AugAssign).getTarget() = use_of_parameter(p)
-}
-
 predicate has_mutable_default(Parameter p) {
     exists(SsaVariable v, FunctionExpr f | maybe_parameter(v, f.getInnerScope(), p) and
         exists(int i, int def_cnt, int arg_cnt |
@@ -56,6 +36,42 @@ predicate has_mutable_default(Parameter p) {
     )
 }
 
-from AstNode a, Parameter p
-where has_mutable_default(p) and is_modification(a, p)
-select a, "Modification of parameter $@, which has mutable default value.", p, p.asName().getId()
+class MutableValue extends TaintKind {
+    MutableValue() {
+        this = "mutable value"
+    }
+}
+
+class MutableDefaultValue extends TaintSource {
+    MutableDefaultValue() {
+        has_mutable_default(this.(NameNode).getNode())
+    }
+
+    override string toString() {
+        result = "mutable default value"
+    }
+
+    override predicate isSourceOf(TaintKind kind) {
+        kind instanceof MutableValue
+    }
+}
+
+class Mutation extends TaintSink {
+    Mutation() {
+        exists(AugAssign a | a.getTarget().getAFlowNode() = this)
+        or
+        exists(Call c, Attribute a |
+            c.getFunc() = a |
+            a.getObject().getAFlowNode() = this and
+            not safe_method(a.getName())
+        )
+    }
+
+    override predicate sinks(TaintKind kind) {
+        kind instanceof MutableValue
+    }
+}
+
+from TaintedPathSource src, TaintedPathSink sink
+where src.flowsTo(sink)
+select sink.getSink(), src, sink, "$@ flows to here and is mutated.", src.getSource(), "Default value"

python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected

@@ -1,2 +1,20 @@
-| functions_test.py:40:5:40:17 | Attribute() | Modification of parameter $@, which has mutable default value. | functions_test.py:39:9:39:9 | Parameter | x |
-| functions_test.py:239:5:239:14 | AugAssign | Modification of parameter $@, which has mutable default value. | functions_test.py:238:15:238:15 | Parameter | x |
+edges
+| functions_test.py:36:9:36:9 | mutable value | functions_test.py:37:16:37:16 | mutable value |
+| functions_test.py:39:9:39:9 | mutable value | functions_test.py:40:5:40:5 | mutable value |
+| functions_test.py:238:15:238:15 | mutable value | functions_test.py:239:5:239:5 | mutable value |
+| functions_test.py:290:25:290:25 | mutable value | functions_test.py:291:5:291:5 | mutable value |
+| functions_test.py:293:21:293:21 | mutable value | functions_test.py:294:5:294:5 | mutable value |
+| functions_test.py:296:27:296:27 | mutable value | functions_test.py:297:25:297:25 | mutable value |
+| functions_test.py:296:27:296:27 | mutable value | functions_test.py:298:21:298:21 | mutable value |
+| functions_test.py:297:25:297:25 | mutable value | functions_test.py:290:25:290:25 | mutable value |
+| functions_test.py:298:21:298:21 | mutable value | functions_test.py:293:21:293:21 | mutable value |
+parents
+| functions_test.py:290:25:290:25 | mutable value | functions_test.py:297:25:297:25 | mutable value |
+| functions_test.py:291:5:291:5 | mutable value | functions_test.py:297:25:297:25 | mutable value |
+| functions_test.py:293:21:293:21 | mutable value | functions_test.py:298:21:298:21 | mutable value |
+| functions_test.py:294:5:294:5 | mutable value | functions_test.py:298:21:298:21 | mutable value |
+#select
+| functions_test.py:40:5:40:5 | Taint sink | functions_test.py:39:9:39:9 | mutable value | functions_test.py:40:5:40:5 | mutable value | $@ flows to here and is mutated. | functions_test.py:39:9:39:9 | mutable default value | Default value |
+| functions_test.py:239:5:239:5 | Taint sink | functions_test.py:238:15:238:15 | mutable value | functions_test.py:239:5:239:5 | mutable value | $@ flows to here and is mutated. | functions_test.py:238:15:238:15 | mutable default value | Default value |
+| functions_test.py:291:5:291:5 | Taint sink | functions_test.py:296:27:296:27 | mutable value | functions_test.py:291:5:291:5 | mutable value | $@ flows to here and is mutated. | functions_test.py:296:27:296:27 | mutable default value | Default value |
+| functions_test.py:294:5:294:5 | Taint sink | functions_test.py:296:27:296:27 | mutable value | functions_test.py:294:5:294:5 | mutable value | $@ flows to here and is mutated. | functions_test.py:296:27:296:27 | mutable default value | Default value |

python/ql/test/query-tests/Functions/general/functions_test.py

@@ -286,3 +286,13 @@ class Z(zope.interface.Interface):
 
 Z().meth(0)
 
+# indirect modification of parameter with default
+def aug_assign_argument(x):
+    x += ['x']
+
+def mutate_argument(x):
+    x.append('x')
+
+def indirect_modification(y = []):
+    aug_assign_argument(y)
+    mutate_argument(y)