From 895b237e3cca9114c235e1e960426e040006f021 Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Mon, 4 Feb 2019 19:03:43 +0100 Subject: [PATCH 1/3] Python: Make "Modification of parameter with default" flow-sensitive. --- change-notes/1.20/analysis-python.md | 1 + .../ModificationOfParameterWithDefault.ql | 64 ++++++++++++------- ...odificationOfParameterWithDefault.expected | 22 ++++++- .../Functions/general/functions_test.py | 10 +++ 4 files changed, 71 insertions(+), 26 deletions(-) diff --git a/change-notes/1.20/analysis-python.md b/change-notes/1.20/analysis-python.md index 442697b61762..c7ba35a66c0e 100644 --- a/change-notes/1.20/analysis-python.md +++ b/change-notes/1.20/analysis-python.md @@ -24,6 +24,7 @@ Removes false positives seen when using Python 3.6, but not when using earlier v | **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 reported correctly. | | 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. | diff --git a/python/ql/src/Functions/ModificationOfParameterWithDefault.ql b/python/ql/src/Functions/ModificationOfParameterWithDefault.ql index 03e76477dea2..2610870ee341 100644 --- a/python/ql/src/Functions/ModificationOfParameterWithDefault.ql +++ b/python/ql/src/Functions/ModificationOfParameterWithDefault.ql @@ -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" diff --git a/python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected b/python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected index 520b55ea3c27..fa8feaf186ba 100644 --- a/python/ql/test/query-tests/Functions/general/ModificationOfParameterWithDefault.expected +++ b/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 | 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 71fea62a737a..280af2bc2279 100644 --- a/python/ql/test/query-tests/Functions/general/functions_test.py +++ b/python/ql/test/query-tests/Functions/general/functions_test.py @@ -286,3 +286,13 @@ def meth(arg): 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) From b550da2b45f61e96816b305098b1cdac955eb05f Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Tue, 5 Feb 2019 16:01:45 +0100 Subject: [PATCH 2/3] Improve change note. --- change-notes/1.20/analysis-python.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/change-notes/1.20/analysis-python.md b/change-notes/1.20/analysis-python.md index c7ba35a66c0e..5204b2041443 100644 --- a/change-notes/1.20/analysis-python.md +++ b/change-notes/1.20/analysis-python.md @@ -24,7 +24,7 @@ Removes false positives seen when using Python 3.6, but not when using earlier v | **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 reported correctly. | +| 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. | | 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. | From 91cfc9bd4c547d90adfd01e594a8073fe7b5a1d4 Mon Sep 17 00:00:00 2001 From: Taus Brock-Nannestad Date: Fri, 1 Mar 2019 11:06:48 +0100 Subject: [PATCH 3/3] Change kind to `path-problem`. --- python/ql/src/Functions/ModificationOfParameterWithDefault.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/ql/src/Functions/ModificationOfParameterWithDefault.ql b/python/ql/src/Functions/ModificationOfParameterWithDefault.ql index 2610870ee341..206174e02b6a 100644 --- a/python/ql/src/Functions/ModificationOfParameterWithDefault.ql +++ b/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