From cac261858c9871848cca0cb32899a1ae6142a28d Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Mon, 18 Nov 2019 16:18:44 +0100 Subject: [PATCH 1/2] Python: Don't report mutable parameters that are in fact immutable. Fixes #1832. In the taint sink, we add an additional check that the given control-flow node can indeed point to a value that is mutable. This takes care of the guard on the type. If and when we get around to adding configurations for all of the taint analyses, we may want to implement this as a barrier instead, pruning any steps that go through a type test where the type is not mutable. --- .../ModificationOfParameterWithDefault.ql | 8 +++++- ...odificationOfParameterWithDefault.expected | 10 ++++++++ .../Functions/general/functions_test.py | 25 +++++++++++++++++++ 3 files changed, 42 insertions(+), 1 deletion(-) diff --git a/python/ql/src/Functions/ModificationOfParameterWithDefault.ql b/python/ql/src/Functions/ModificationOfParameterWithDefault.ql index ab94a9735763..0df85d7383ce 100644 --- a/python/ql/src/Functions/ModificationOfParameterWithDefault.ql +++ b/python/ql/src/Functions/ModificationOfParameterWithDefault.ql @@ -73,6 +73,11 @@ 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) @@ -80,7 +85,8 @@ class Mutation extends TaintSink { 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() ) } diff --git a/python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected b/python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected index 9b7af6df8b72..08586d02c100 100644 --- a/python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected +++ b/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 | diff --git a/python/ql/test/query-tests/Functions/general/functions_test.py b/python/ql/test/query-tests/Functions/general/functions_test.py index 271539792e72..367a5e54e660 100644 --- a/python/ql/test/query-tests/Functions/general/functions_test.py +++ b/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) From 3c47394b7a32ef0d40a8bc9c0384c0ea91b0aa32 Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Mon, 18 Nov 2019 16:28:54 +0100 Subject: [PATCH 2/2] Python: Apply auto-format. --- .../ModificationOfParameterWithDefault.ql | 48 ++++++++----------- 1 file changed, 21 insertions(+), 27 deletions(-) diff --git a/python/ql/src/Functions/ModificationOfParameterWithDefault.ql b/python/ql/src/Functions/ModificationOfParameterWithDefault.ql index 0df85d7383ce..04cd3e798328 100644 --- a/python/ql/src/Functions/ModificationOfParameterWithDefault.ql +++ b/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 @@ -82,8 +76,7 @@ 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()) and this.(ControlFlowNode).pointsTo().getClass() = mutable_class() @@ -99,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"