Skip to content

Commit 4a16be2

Browse files
authored
Merge pull request #6557 from yoff/python/port-modification-of-default-value
Python: port modification of default value
2 parents eaf0530 + 8ea7a28 commit 4a16be2

12 files changed

+581
-111
lines changed
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* Updated _Modification of parameter with default_ (`py/modification-of-default-value`) query to use the new data flow library instead of the old taint tracking library and to remove the use of points-to analysis. You may see differences in the results found by the query, but overall this change should result in a more robust and accurate analysis.

python/ql/src/Functions/ModificationOfParameterWithDefault.ql

Lines changed: 8 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -12,88 +12,12 @@
1212
*/
1313

1414
import python
15-
import semmle.python.security.Paths
16-
17-
predicate safe_method(string name) {
18-
name = "count" or
19-
name = "index" or
20-
name = "copy" or
21-
name = "get" or
22-
name = "has_key" or
23-
name = "items" or
24-
name = "keys" or
25-
name = "values" or
26-
name = "iteritems" or
27-
name = "iterkeys" or
28-
name = "itervalues" or
29-
name = "__contains__" or
30-
name = "__getitem__" or
31-
name = "__getattribute__"
32-
}
33-
34-
/** Gets the truthiness (non emptyness) of the default of `p` if that value is mutable */
35-
private boolean mutableDefaultValue(Parameter p) {
36-
exists(Dict d | p.getDefault() = d |
37-
exists(d.getAKey()) and result = true
38-
or
39-
not exists(d.getAKey()) and result = false
40-
)
41-
or
42-
exists(List l | p.getDefault() = l |
43-
exists(l.getAnElt()) and result = true
44-
or
45-
not exists(l.getAnElt()) and result = false
46-
)
47-
}
48-
49-
class NonEmptyMutableValue extends TaintKind {
50-
NonEmptyMutableValue() { this = "non-empty mutable value" }
51-
}
52-
53-
class EmptyMutableValue extends TaintKind {
54-
EmptyMutableValue() { this = "empty mutable value" }
55-
56-
override boolean booleanValue() { result = false }
57-
}
58-
59-
class MutableDefaultValue extends TaintSource {
60-
boolean nonEmpty;
61-
62-
MutableDefaultValue() { nonEmpty = mutableDefaultValue(this.(NameNode).getNode()) }
63-
64-
override string toString() { result = "mutable default value" }
65-
66-
override predicate isSourceOf(TaintKind kind) {
67-
nonEmpty = false and kind instanceof EmptyMutableValue
68-
or
69-
nonEmpty = true and kind instanceof NonEmptyMutableValue
70-
}
71-
}
72-
73-
private ClassValue mutable_class() {
74-
result = Value::named("list") or
75-
result = Value::named("dict")
76-
}
77-
78-
class Mutation extends TaintSink {
79-
Mutation() {
80-
exists(AugAssign a | a.getTarget().getAFlowNode() = this)
81-
or
82-
exists(Call c, Attribute a | c.getFunc() = a |
83-
a.getObject().getAFlowNode() = this and
84-
not safe_method(a.getName()) and
85-
this.(ControlFlowNode).pointsTo().getClass() = mutable_class()
86-
)
87-
}
88-
89-
override predicate sinks(TaintKind kind) {
90-
kind instanceof EmptyMutableValue
91-
or
92-
kind instanceof NonEmptyMutableValue
93-
}
94-
}
95-
96-
from TaintedPathSource src, TaintedPathSink sink
97-
where src.flowsTo(sink)
98-
select sink.getSink(), src, sink, "$@ flows to here and is mutated.", src.getSource(),
15+
import semmle.python.functions.ModificationOfParameterWithDefault
16+
import DataFlow::PathGraph
17+
18+
from
19+
ModificationOfParameterWithDefault::Configuration config, DataFlow::PathNode source,
20+
DataFlow::PathNode sink
21+
where config.hasFlowPath(source, sink)
22+
select sink.getNode(), source, sink, "$@ flows to here and is mutated.", source.getNode(),
9923
"Default value"
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
/**
2+
* Provides a data-flow configuration for detecting modifications of a parameters default value.
3+
*
4+
* Note, for performance reasons: only import this file if
5+
* `ModificationOfParameterWithDefault::Configuration` is needed, otherwise
6+
* `ModificationOfParameterWithDefaultCustomizations` should be imported instead.
7+
*/
8+
9+
private import python
10+
import semmle.python.dataflow.new.DataFlow
11+
12+
/**
13+
* Provides a data-flow configuration for detecting modifications of a parameters default value.
14+
*/
15+
module ModificationOfParameterWithDefault {
16+
import ModificationOfParameterWithDefaultCustomizations::ModificationOfParameterWithDefault
17+
18+
/**
19+
* A data-flow configuration for detecting modifications of a parameters default value.
20+
*/
21+
class Configuration extends DataFlow::Configuration {
22+
/** Record whether the default value being tracked is non-empty. */
23+
boolean nonEmptyDefault;
24+
25+
Configuration() {
26+
nonEmptyDefault in [true, false] and
27+
this = "ModificationOfParameterWithDefault:" + nonEmptyDefault.toString()
28+
}
29+
30+
override predicate isSource(DataFlow::Node source) {
31+
source.(Source).isNonEmpty() = nonEmptyDefault
32+
}
33+
34+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
35+
36+
override predicate isBarrier(DataFlow::Node node) {
37+
// if we are tracking a non-empty default, then it is ok to modify empty values,
38+
// so our tracking ends at those.
39+
nonEmptyDefault = true and node instanceof MustBeEmpty
40+
or
41+
// if we are tracking a empty default, then it is ok to modify non-empty values,
42+
// so our tracking ends at those.
43+
nonEmptyDefault = false and node instanceof MustBeNonEmpty
44+
}
45+
}
46+
}
Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,190 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for detecting
3+
* modifications of a parameters default value, as well as extension points for adding your own.
4+
*/
5+
6+
private import python
7+
private import semmle.python.dataflow.new.DataFlow
8+
private import semmle.python.dataflow.new.BarrierGuards
9+
10+
/**
11+
* Provides default sources, sinks and sanitizers for detecting
12+
* "command injection"
13+
* vulnerabilities, as well as extension points for adding your own.
14+
*/
15+
module ModificationOfParameterWithDefault {
16+
/**
17+
* A data flow source for detecting modifications of a parameters default value,
18+
* that is a default value for some parameter.
19+
*/
20+
abstract class Source extends DataFlow::Node {
21+
/** Result is true if the default value is non-empty for this source and false if not. */
22+
abstract boolean isNonEmpty();
23+
}
24+
25+
/**
26+
* A data flow sink for detecting modifications of a parameters default value,
27+
* that is a node representing a modification.
28+
*/
29+
abstract class Sink extends DataFlow::Node { }
30+
31+
/**
32+
* A sanitizer for detecting modifications of a parameters default value
33+
* should determine if the node (which is perhaps about to be modified)
34+
* can be the default value or not.
35+
*
36+
* In this query we do not track the default value exactly, but rather wheter
37+
* it is empty or not (see `Source`).
38+
*
39+
* This is the extension point for determining that a node must be empty and
40+
* therefor is allowed to be modified if the tracked default value is non-empty.
41+
*/
42+
abstract class MustBeEmpty extends DataFlow::Node { }
43+
44+
/**
45+
* A sanitizer for detecting modifications of a parameters default value
46+
* should determine if the node (which is perhaps about to be modified)
47+
* can be the default value or not.
48+
*
49+
* In this query we do not track the default value exactly, but rather wheter
50+
* it is empty or not (see `Source`).
51+
*
52+
* This is the extension point for determining that a node must be non-empty
53+
* and therefor is allowed to be modified if the tracked default value is empty.
54+
*/
55+
abstract class MustBeNonEmpty extends DataFlow::Node { }
56+
57+
/** Gets the truthiness (non emptyness) of the default of `p` if that value is mutable */
58+
private boolean mutableDefaultValue(Parameter p) {
59+
exists(Dict d | p.getDefault() = d |
60+
exists(d.getAKey()) and result = true
61+
or
62+
not exists(d.getAKey()) and result = false
63+
)
64+
or
65+
exists(List l | p.getDefault() = l |
66+
exists(l.getAnElt()) and result = true
67+
or
68+
not exists(l.getAnElt()) and result = false
69+
)
70+
}
71+
72+
/**
73+
* A mutable default value for a parameter, considered as a flow source.
74+
*/
75+
class MutableDefaultValue extends Source {
76+
boolean nonEmpty;
77+
78+
MutableDefaultValue() { nonEmpty = mutableDefaultValue(this.asCfgNode().(NameNode).getNode()) }
79+
80+
override boolean isNonEmpty() { result = nonEmpty }
81+
}
82+
83+
/**
84+
* A name of a list function that modifies the list.
85+
* See https://docs.python.org/3/tutorial/datastructures.html#more-on-lists
86+
*/
87+
string list_modifying_method() {
88+
result in ["append", "extend", "insert", "remove", "pop", "clear", "sort", "reverse"]
89+
}
90+
91+
/**
92+
* A name of a dict function that modifies the dict.
93+
* See https://docs.python.org/3/library/stdtypes.html#dict
94+
*/
95+
string dict_modifying_method() { result in ["clear", "pop", "popitem", "setdefault", "update"] }
96+
97+
/**
98+
* A mutation of the default value is a flow sink.
99+
*
100+
* Syntactic constructs that modify a list are:
101+
* - s[i] = x
102+
* - s[i:j] = t
103+
* - del s[i:j]
104+
* - s[i:j:k] = t
105+
* - del s[i:j:k]
106+
* - s += t
107+
* - s *= n
108+
* See https://docs.python.org/3/library/stdtypes.html#mutable-sequence-types
109+
*
110+
* Syntactic constructs that modify a dictionary are:
111+
* - d[key] = value
112+
* - del d[key]
113+
* - d |= other
114+
* See https://docs.python.org/3/library/stdtypes.html#dict
115+
*
116+
* These are all covered by:
117+
* - assignment to a subscript (includes slices)
118+
* - deletion of a subscript
119+
* - augmented assignment to the value
120+
*/
121+
class Mutation extends Sink {
122+
Mutation() {
123+
// assignment to a subscript (includes slices)
124+
exists(DefinitionNode d | d.(SubscriptNode).getObject() = this.asCfgNode())
125+
or
126+
// deletion of a subscript
127+
exists(DeletionNode d | d.getTarget().(SubscriptNode).getObject() = this.asCfgNode())
128+
or
129+
// augmented assignment to the value
130+
exists(AugAssign a | a.getTarget().getAFlowNode() = this.asCfgNode())
131+
or
132+
// modifying function call
133+
exists(DataFlow::CallCfgNode c, DataFlow::AttrRead a | c.getFunction() = a |
134+
a.getObject() = this and
135+
a.getAttributeName() in [list_modifying_method(), dict_modifying_method()]
136+
)
137+
}
138+
}
139+
140+
// This to reimplement some of the functionality of the DataFlow::BarrierGuard
141+
private import semmle.python.essa.SsaCompute
142+
143+
/**
144+
* A data-flow node that is known to be either truthy or falsey.
145+
*
146+
* It handles the cases `if x` and `if not x`.
147+
*
148+
* For example, in the following code, `this` will be the `x` that is printed,
149+
* which we will know is truthy:
150+
*
151+
* ```py
152+
* if x:
153+
* print(x)
154+
* ```
155+
*/
156+
private class MustBe extends DataFlow::Node {
157+
boolean truthy;
158+
159+
MustBe() {
160+
exists(DataFlow::GuardNode guard, NameNode guarded, boolean branch |
161+
// case: if x
162+
guard = guarded and
163+
branch = truthy
164+
or
165+
// case: if not x
166+
guard.(UnaryExprNode).getNode().getOp() instanceof Not and
167+
guarded = guard.(UnaryExprNode).getOperand() and
168+
branch = truthy.booleanNot()
169+
|
170+
// guard controls this
171+
guard.controlsBlock(this.asCfgNode().getBasicBlock(), branch) and
172+
// there is a definition tying the guarded value to this
173+
exists(EssaDefinition def |
174+
AdjacentUses::useOfDef(def, this.asCfgNode()) and
175+
AdjacentUses::useOfDef(def, guarded)
176+
)
177+
)
178+
}
179+
}
180+
181+
/** Simple guard detecting truthy values. */
182+
private class MustBeTruthy extends MustBe, MustBeNonEmpty {
183+
MustBeTruthy() { truthy = true }
184+
}
185+
186+
/** Simple guard detecting falsey values. */
187+
private class MustBeFalsey extends MustBe, MustBeEmpty {
188+
MustBeFalsey() { truthy = false }
189+
}
190+
}

0 commit comments

Comments
 (0)