Merge pull request #2375 from tausbn/python-fix-mutable-value-type-coercion-fp

Python: Don't report mutable parameters that are in fact immutable.

Author: RasmusWL

python/ql/src/Functions/ModificationOfParameterWithDefault.ql

@@ -15,56 +15,50 @@ 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
-    name = "items" or name = "keys" or name = "values" or name = "iteritems" or name = "iterkeys" or name = "itervalues"
+    name = "count" or
+    name = "index" or
+    name = "copy" or
+    name = "get" or
+    name = "has_key" or
+    name = "items" or
+    name = "keys" or
+    name = "values" or
+    name = "iteritems" or
+    name = "iterkeys" or
+    name = "itervalues"
 }
 
 /** Gets the truthiness (non emptyness) of the default of `p` if that value is mutable */
 private boolean mutableDefaultValue(Parameter p) {
-    exists(Dict d |
-        p.getDefault() = d |
+    exists(Dict d | p.getDefault() = d |
         exists(d.getAKey()) and result = true
         or
         not exists(d.getAKey()) and result = false
     )
     or
-    exists(List l |
-        p.getDefault() = l |
+    exists(List l | p.getDefault() = l |
         exists(l.getAnElt()) and result = true
         or
         not exists(l.getAnElt()) and result = false
     )
 }
 
-
 class NonEmptyMutableValue extends TaintKind {
-    NonEmptyMutableValue() {
-        this = "non-empty mutable value"
-    }
+    NonEmptyMutableValue() { this = "non-empty mutable value" }
 }
 
 class EmptyMutableValue extends TaintKind {
-    EmptyMutableValue() {
-        this = "empty mutable value"
-    }
-
-    override boolean booleanValue() {
-        result = false
-    }
+    EmptyMutableValue() { this = "empty mutable value" }
 
+    override boolean booleanValue() { result = false }
 }
 
 class MutableDefaultValue extends TaintSource {
-
     boolean nonEmpty;
 
-    MutableDefaultValue() {
-        nonEmpty = mutableDefaultValue(this.(NameNode).getNode())
-    }
+    MutableDefaultValue() { nonEmpty = mutableDefaultValue(this.(NameNode).getNode()) }
 
-    override string toString() {
-        result = "mutable default value"
-    }
+    override string toString() { result = "mutable default value" }
 
     override predicate isSourceOf(TaintKind kind) {
         nonEmpty = false and kind instanceof EmptyMutableValue
@@ -73,14 +67,19 @@ class MutableDefaultValue extends TaintSource {
     }
 }
 
+private ClassValue mutable_class() {
+    result = Value::named("list") or
+    result = Value::named("dict")
+}
+
 class Mutation extends TaintSink {
     Mutation() {
         exists(AugAssign a | a.getTarget().getAFlowNode() = this)
         or
-        exists(Call c, Attribute a |
-            c.getFunc() = a |
+        exists(Call c, Attribute a | c.getFunc() = a |
             a.getObject().getAFlowNode() = this and
-            not safe_method(a.getName())
+            not safe_method(a.getName()) and
+            this.(ControlFlowNode).pointsTo().getClass() = mutable_class()
         )
     }
 
@@ -93,4 +92,5 @@ class Mutation extends TaintSink {
 
 from TaintedPathSource src, TaintedPathSink sink
 where src.flowsTo(sink)
-select sink.getSink(), src, sink, "$@ flows to here and is mutated.", src.getSource(), "Default value"
+select sink.getSink(), src, sink, "$@ flows to here and is mutated.", src.getSource(),
+    "Default value"

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

@@ -7,8 +7,18 @@ edges
 | functions_test.py:157:27:157:27 | empty mutable value | functions_test.py:159:21:159:21 | empty mutable value |
 | functions_test.py:158:25:158:25 | empty mutable value | functions_test.py:151:25:151:25 | empty mutable value |
 | functions_test.py:159:21:159:21 | empty mutable value | functions_test.py:154:21:154:21 | empty mutable value |
+| functions_test.py:175:28:175:28 | non-empty mutable value | functions_test.py:179:9:179:9 | non-empty mutable value |
+| functions_test.py:175:28:175:28 | non-empty mutable value | functions_test.py:181:9:181:9 | non-empty mutable value |
+| functions_test.py:188:18:188:18 | non-empty mutable value | functions_test.py:189:28:189:28 | non-empty mutable value |
+| functions_test.py:189:28:189:28 | non-empty mutable value | functions_test.py:175:28:175:28 | non-empty mutable value |
+| functions_test.py:191:18:191:18 | non-empty mutable value | functions_test.py:192:28:192:28 | non-empty mutable value |
+| functions_test.py:192:28:192:28 | non-empty mutable value | functions_test.py:175:28:175:28 | non-empty mutable value |
 #select
 | functions_test.py:40:5:40:5 | x | functions_test.py:39:9:39:9 | empty mutable value | functions_test.py:40:5:40:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:39:9:39:9 | x | Default value |
 | functions_test.py:134:5:134:5 | x | functions_test.py:133:15:133:15 | empty mutable value | functions_test.py:134:5:134:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:133:15:133:15 | x | Default value |
 | functions_test.py:152:5:152:5 | x | functions_test.py:157:27:157:27 | empty mutable value | functions_test.py:152:5:152:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:157:27:157:27 | y | Default value |
 | functions_test.py:155:5:155:5 | x | functions_test.py:157:27:157:27 | empty mutable value | functions_test.py:155:5:155:5 | empty mutable value | $@ flows to here and is mutated. | functions_test.py:157:27:157:27 | y | Default value |
+| functions_test.py:179:9:179:9 | x | functions_test.py:188:18:188:18 | non-empty mutable value | functions_test.py:179:9:179:9 | non-empty mutable value | $@ flows to here and is mutated. | functions_test.py:188:18:188:18 | x | Default value |
+| functions_test.py:179:9:179:9 | x | functions_test.py:191:18:191:18 | non-empty mutable value | functions_test.py:179:9:179:9 | non-empty mutable value | $@ flows to here and is mutated. | functions_test.py:191:18:191:18 | x | Default value |
+| functions_test.py:181:9:181:9 | x | functions_test.py:188:18:188:18 | non-empty mutable value | functions_test.py:181:9:181:9 | non-empty mutable value | $@ flows to here and is mutated. | functions_test.py:188:18:188:18 | x | Default value |
+| functions_test.py:181:9:181:9 | x | functions_test.py:191:18:191:18 | non-empty mutable value | functions_test.py:181:9:181:9 | non-empty mutable value | $@ flows to here and is mutated. | functions_test.py:191:18:191:18 | x | Default value |

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

@@ -168,3 +168,28 @@ def issue1143(expr, param=[]):
         return result
     for i in param:
         param.remove(i) # Mutation here
+
+
+# Type guarding of modification of parameter with default:
+
+def do_stuff_based_on_type(x):
+    if isinstance(x, str):
+        x = x.split()
+    elif isinstance(x, dict):
+        x.setdefault('foo', 'bar')
+    elif isinstance(x, list):
+        x.append(5)
+    elif isinstance(x, tuple):
+        x = x.unknown_method()
+
+def str_default(x="hello world"):
+    do_stuff_based_on_type(x)
+
+def dict_default(x={'baz':'quux'}):
+    do_stuff_based_on_type(x)
+
+def list_default(x=[1,2,3,4]):
+    do_stuff_based_on_type(x)
+
+def tuple_default(x=(1,2)):
+    do_stuff_based_on_type(x)