Skip to content

Commit a5bc537

Browse files
yoffyoff
committed
python: Rewrite path injection to use flow state
This removes the FP cause by chaining This PR also removes `ChainedConfigs12.qll`, as we hope to solve future problems via flow states.
1 parent c09b669 commit a5bc537

File tree

6 files changed

+88
-260
lines changed

6 files changed

+88
-260
lines changed

‎python/ql/lib/semmle/python/Concepts.qll

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,8 @@ module Path {
115115
PathNormalization::Range range;
116116

117117
PathNormalization() { this = range }
118+
119+
DataFlow::Node getPathArg() { result = range.getPathArg() }
118120
}
119121

120122
/** Provides a class for modeling new path normalization APIs. */
@@ -123,7 +125,10 @@ module Path {
123125
* A data-flow node that performs path normalization. This is often needed in order
124126
* to safely access paths.
125127
*/
126-
abstract class Range extends DataFlow::Node { }
128+
abstract class Range extends DataFlow::Node {
129+
/** Gets an argument to this path normalization that is interpreted as a path. */
130+
abstract DataFlow::Node getPathArg();
131+
}
127132
}
128133

129134
/** A data-flow node that checks that a path is safe to access. */

‎python/ql/lib/semmle/python/frameworks/Stdlib.qll

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -970,7 +970,7 @@ private module StdlibPrivate {
970970
private class OsPathNormpathCall extends Path::PathNormalization::Range, DataFlow::CallCfgNode {
971971
OsPathNormpathCall() { this = os::path().getMember("normpath").getACall() }
972972

973-
DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
973+
override DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
974974
}
975975

976976
/**
@@ -980,7 +980,7 @@ private module StdlibPrivate {
980980
private class OsPathAbspathCall extends Path::PathNormalization::Range, DataFlow::CallCfgNode {
981981
OsPathAbspathCall() { this = os::path().getMember("abspath").getACall() }
982982

983-
DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
983+
override DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
984984
}
985985

986986
/**
@@ -990,7 +990,7 @@ private module StdlibPrivate {
990990
private class OsPathRealpathCall extends Path::PathNormalization::Range, DataFlow::CallCfgNode {
991991
OsPathRealpathCall() { this = os::path().getMember("realpath").getACall() }
992992

993-
DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
993+
override DataFlow::Node getPathArg() { result in [this.getArg(0), this.getArgByName("path")] }
994994
}
995995

996996
/**

‎python/ql/lib/semmle/python/security/dataflow/ChainedConfigs12.qll

Lines changed: 0 additions & 83 deletions
This file was deleted.

‎python/ql/lib/semmle/python/security/dataflow/PathInjection.qll

Lines changed: 65 additions & 91 deletions
Original file line numberDiff line numberDiff line change
@@ -2,107 +2,81 @@
22
* Provides taint-tracking configurations for detecting "path injection" vulnerabilities.
33
*
44
* Note, for performance reasons: only import this file if
5-
* the Configurations or the `pathInjection` predicate are needed, otherwise
5+
* `PathInjection::Configuration` is needed, otherwise
66
* `PathInjectionCustomizations` should be imported instead.
77
*/
88

99
private import python
1010
private import semmle.python.Concepts
11-
private import semmle.python.dataflow.new.DataFlow
12-
private import semmle.python.dataflow.new.DataFlow2
13-
private import semmle.python.dataflow.new.TaintTracking
14-
private import semmle.python.dataflow.new.TaintTracking2
15-
import ChainedConfigs12
16-
import PathInjectionCustomizations::PathInjection
17-
18-
// ---------------------------------------------------------------------------
19-
// Case 1. The path is never normalized.
20-
// ---------------------------------------------------------------------------
21-
/** Configuration to find paths from sources to sinks that contain no normalization. */
22-
class PathNotNormalizedConfiguration extends TaintTracking::Configuration {
23-
PathNotNormalizedConfiguration() { this = "PathNotNormalizedConfiguration" }
24-
25-
override predicate isSource(DataFlow::Node source) { source instanceof Source }
26-
27-
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
28-
29-
override predicate isSanitizer(DataFlow::Node node) {
30-
node instanceof Sanitizer
31-
or
32-
node instanceof Path::PathNormalization
33-
}
34-
35-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
36-
guard instanceof SanitizerGuard
37-
}
38-
}
11+
import semmle.python.dataflow.new.DataFlow
12+
import semmle.python.dataflow.new.TaintTracking
3913

4014
/**
41-
* Holds if there is a path injection from source to sink, where the (python) path is
42-
* not normalized.
15+
* Provides a taint-tracking configuration for detecting "path injection" vulnerabilities.
4316
*/
44-
predicate pathNotNormalized(CustomPathNode source, CustomPathNode sink) {
45-
any(PathNotNormalizedConfiguration config).hasFlowPath(source.asNode1(), sink.asNode1())
46-
}
47-
48-
// ---------------------------------------------------------------------------
49-
// Case 2. The path is normalized at least once, but never checked afterwards.
50-
// ---------------------------------------------------------------------------
51-
/** Configuration to find paths from sources to normalizations that contain no prior normalizations. */
52-
class FirstNormalizationConfiguration extends TaintTracking::Configuration {
53-
FirstNormalizationConfiguration() { this = "FirstNormalizationConfiguration" }
54-
55-
override predicate isSource(DataFlow::Node source) { source instanceof Source }
56-
57-
override predicate isSink(DataFlow::Node sink) { sink instanceof Path::PathNormalization }
58-
59-
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
60-
61-
override predicate isSanitizerOut(DataFlow::Node node) { node instanceof Path::PathNormalization }
62-
63-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
64-
guard instanceof SanitizerGuard
17+
module PathInjection {
18+
import PathInjectionCustomizations::PathInjection
19+
20+
/**
21+
* A taint-tracking configuration for detecting "path injection" vulnerabilities.
22+
*
23+
* This configuration uses two flow states, `NotNormalized` and `NormalizedUnchecked`,
24+
* to track the requirement that a file path must be first normalized and then checked
25+
* before it is safe to use.
26+
*
27+
* At sources, paths are assumed not normalized. At normalization points, they change
28+
* state to `NormalizedUnchecked` after which they can be made safe by an appropriate
29+
* check of the prefix.
30+
*
31+
* Such checks are ineffective in the `NotNormalized` state.
32+
*/
33+
class Configuration extends TaintTracking::Configuration {
34+
Configuration() { this = "PathInjection" }
35+
36+
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
37+
source instanceof Source and state instanceof NotNormalized
38+
}
39+
40+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
41+
sink instanceof Sink and
42+
(
43+
state instanceof NotNormalized or
44+
state instanceof NormalizedUnchecked
45+
)
46+
}
47+
48+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
49+
50+
override predicate isBarrier(DataFlow::Node node, DataFlow::FlowState state) {
51+
// Block `NotNormalized` paths here, since they change state to `NormalizedUnchecked`
52+
node instanceof Path::PathNormalization and
53+
state instanceof NotNormalized
54+
or
55+
node = any(Path::SafeAccessCheck c).getAGuardedNode() and
56+
state instanceof NormalizedUnchecked
57+
}
58+
59+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
60+
guard instanceof SanitizerGuard
61+
}
62+
63+
override predicate isAdditionalFlowStep(
64+
DataFlow::Node nodeFrom, DataFlow::FlowState stateFrom, DataFlow::Node nodeTo,
65+
DataFlow::FlowState stateTo
66+
) {
67+
nodeFrom = nodeTo.(Path::PathNormalization).getPathArg() and
68+
stateFrom instanceof NotNormalized and
69+
stateTo instanceof NormalizedUnchecked
70+
}
6571
}
66-
}
67-
68-
/** Configuration to find paths from normalizations to sinks that do not go through a check. */
69-
class NormalizedPathNotCheckedConfiguration extends TaintTracking2::Configuration {
70-
NormalizedPathNotCheckedConfiguration() { this = "NormalizedPathNotCheckedConfiguration" }
71-
72-
override predicate isSource(DataFlow::Node source) { source instanceof Path::PathNormalization }
73-
74-
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
75-
76-
override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
7772

78-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
79-
guard instanceof Path::SafeAccessCheck
80-
or
81-
guard instanceof SanitizerGuard
73+
/** A state signifying that the file path has not been normalized. */
74+
class NotNormalized extends DataFlow::FlowState {
75+
NotNormalized() { this = "NotNormalized" }
8276
}
83-
}
84-
85-
/**
86-
* Holds if there is a path injection from source to sink, where the (python) path is
87-
* normalized at least once, but never checked afterwards.
88-
*/
89-
predicate pathNotCheckedAfterNormalization(CustomPathNode source, CustomPathNode sink) {
90-
exists(
91-
FirstNormalizationConfiguration config, DataFlow::PathNode mid1, DataFlow2::PathNode mid2,
92-
NormalizedPathNotCheckedConfiguration config2
93-
|
94-
config.hasFlowPath(source.asNode1(), mid1) and
95-
config2.hasFlowPath(mid2, sink.asNode2()) and
96-
mid1.getNode().asCfgNode() = mid2.getNode().asCfgNode()
97-
)
98-
}
9977

100-
// ---------------------------------------------------------------------------
101-
// Query: Either case 1 or case 2.
102-
// ---------------------------------------------------------------------------
103-
/** Holds if there is a path injection from source to sink */
104-
predicate pathInjection(CustomPathNode source, CustomPathNode sink) {
105-
pathNotNormalized(source, sink)
106-
or
107-
pathNotCheckedAfterNormalization(source, sink)
78+
/** A state signifying that the file path has been normalized, but not checked. */
79+
class NormalizedUnchecked extends DataFlow::FlowState {
80+
NormalizedUnchecked() { this = "NormalizedUnchecked" }
81+
}
10882
}

‎python/ql/src/Security/CWE-022/PathInjection.ql

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,9 @@
1818

1919
import python
2020
import semmle.python.security.dataflow.PathInjection
21+
import DataFlow::PathGraph
2122

22-
from CustomPathNode source, CustomPathNode sink
23-
where pathInjection(source, sink)
24-
select sink, source, sink, "This path depends on $@.", source, "a user-provided value"
23+
from PathInjection::Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
24+
where config.hasFlowPath(source, sink)
25+
select sink.getNode(), source, sink, "This path depends on $@.", source.getNode(),
26+
"a user-provided value"

0 commit comments

Comments
 (0)