Skip to content

Commit 5680b2d

Browse files
jbjjbj
committed
Merge remote-tracking branch 'upstream/main' into better-syntax-for-false-positives-and-negatives-inline-expectation
Required fixing up semantic conflicts in tests. Conflicts: python/ql/test/experimental/library-tests/frameworks/stdlib/Decoding.py
2 parents 6d0783a + 82f37e9 commit 5680b2d

File tree

180 files changed

+3493
-3247
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

180 files changed

+3493
-3247
lines changed

‎config/identical-files.json‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,17 +19,17 @@
1919
"csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImpl3.qll",
2020
"csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImpl4.qll",
2121
"csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImpl5.qll",
22-
"python/ql/src/experimental/dataflow/internal/DataFlowImpl.qll",
23-
"python/ql/src/experimental/dataflow/internal/DataFlowImpl2.qll",
24-
"python/ql/src/experimental/dataflow/internal/DataFlowImpl3.qll",
25-
"python/ql/src/experimental/dataflow/internal/DataFlowImpl4.qll"
22+
"python/ql/src/semmle/python/dataflow/new/internal/DataFlowImpl.qll",
23+
"python/ql/src/semmle/python/dataflow/new/internal/DataFlowImpl2.qll",
24+
"python/ql/src/semmle/python/dataflow/new/internal/DataFlowImpl3.qll",
25+
"python/ql/src/semmle/python/dataflow/new/internal/DataFlowImpl4.qll"
2626
],
2727
"DataFlow Java/C++/C#/Python Common": [
2828
"java/ql/src/semmle/code/java/dataflow/internal/DataFlowImplCommon.qll",
2929
"cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImplCommon.qll",
3030
"cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImplCommon.qll",
3131
"csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImplCommon.qll",
32-
"python/ql/src/experimental/dataflow/internal/DataFlowImplCommon.qll"
32+
"python/ql/src/semmle/python/dataflow/new/internal/DataFlowImplCommon.qll"
3333
],
3434
"TaintTracking::Configuration Java/C++/C#/Python": [
3535
"cpp/ql/src/semmle/code/cpp/dataflow/internal/tainttracking1/TaintTrackingImpl.qll",
@@ -43,17 +43,17 @@
4343
"csharp/ql/src/semmle/code/csharp/dataflow/internal/tainttracking5/TaintTrackingImpl.qll",
4444
"java/ql/src/semmle/code/java/dataflow/internal/tainttracking1/TaintTrackingImpl.qll",
4545
"java/ql/src/semmle/code/java/dataflow/internal/tainttracking2/TaintTrackingImpl.qll",
46-
"python/ql/src/experimental/dataflow/internal/tainttracking1/TaintTrackingImpl.qll",
47-
"python/ql/src/experimental/dataflow/internal/tainttracking2/TaintTrackingImpl.qll",
48-
"python/ql/src/experimental/dataflow/internal/tainttracking3/TaintTrackingImpl.qll",
49-
"python/ql/src/experimental/dataflow/internal/tainttracking4/TaintTrackingImpl.qll"
46+
"python/ql/src/semmle/python/dataflow/new/internal/tainttracking1/TaintTrackingImpl.qll",
47+
"python/ql/src/semmle/python/dataflow/new/internal/tainttracking2/TaintTrackingImpl.qll",
48+
"python/ql/src/semmle/python/dataflow/new/internal/tainttracking3/TaintTrackingImpl.qll",
49+
"python/ql/src/semmle/python/dataflow/new/internal/tainttracking4/TaintTrackingImpl.qll"
5050
],
5151
"DataFlow Java/C++/C#/Python Consistency checks": [
5252
"java/ql/src/semmle/code/java/dataflow/internal/DataFlowImplConsistency.qll",
5353
"cpp/ql/src/semmle/code/cpp/dataflow/internal/DataFlowImplConsistency.qll",
5454
"cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowImplConsistency.qll",
5555
"csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowImplConsistency.qll",
56-
"python/ql/src/experimental/dataflow/internal/DataFlowImplConsistency.qll"
56+
"python/ql/src/semmle/python/dataflow/new/internal/DataFlowImplConsistency.qll"
5757
],
5858
"SsaReadPosition Java/C#": [
5959
"java/ql/src/semmle/code/java/dataflow/internal/rangeanalysis/SsaReadPositionCommon.qll",

‎csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -533,6 +533,11 @@ private TypeParameter getATypeParameterSubType(DataFlowTypeOrUnifiable t) {
533533
exists(Type t0 | t = Gvn::getGlobalValueNumber(t0) | implicitConversionRestricted(result, t0))
534534
}
535535

536+
pragma[noinline]
537+
private TypeParameter getATypeParameterSubTypeRestricted(DataFlowType t) {
538+
result = getATypeParameterSubType(t)
539+
}
540+
536541
pragma[noinline]
537542
private Gvn::GvnType getANonTypeParameterSubType(DataFlowTypeOrUnifiable t) {
538543
not t instanceof Gvn::TypeParameterGvnType and
@@ -544,6 +549,11 @@ private Gvn::GvnType getANonTypeParameterSubType(DataFlowTypeOrUnifiable t) {
544549
)
545550
}
546551

552+
pragma[noinline]
553+
private Gvn::GvnType getANonTypeParameterSubTypeRestricted(DataFlowType t) {
554+
result = getANonTypeParameterSubType(t)
555+
}
556+
547557
/** A collection of cached types and predicates to be evaluated in the same stage. */
548558
cached
549559
private module Cached {
@@ -793,9 +803,9 @@ private module Cached {
793803
not t1 instanceof Gvn::TypeParameterGvnType and
794804
t1 = t2
795805
or
796-
getATypeParameterSubType(t1) = getATypeParameterSubType(t2)
806+
getATypeParameterSubType(t1) = getATypeParameterSubTypeRestricted(t2)
797807
or
798-
getANonTypeParameterSubType(t1) = getANonTypeParameterSubType(t2)
808+
getANonTypeParameterSubType(t1) = getANonTypeParameterSubTypeRestricted(t2)
799809
}
800810

801811
/**

python/ql/src/experimental/Security-new-dataflow/CWE-022/ChainedConfigs12.qll renamed to python/ql/src/Security/CWE-022/ChainedConfigs12.qll

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,10 +6,10 @@
66
*/
77

88
import python
9-
import experimental.dataflow.DataFlow
10-
import experimental.dataflow.DataFlow2
11-
import experimental.dataflow.TaintTracking
12-
import experimental.dataflow.TaintTracking2
9+
import semmle.python.dataflow.new.DataFlow
10+
import semmle.python.dataflow.new.DataFlow2
11+
import semmle.python.dataflow.new.TaintTracking
12+
import semmle.python.dataflow.new.TaintTracking2
1313

1414
/**
1515
* A `DataFlow::Node` that appears as a sink in Config1 and a source in Config2.

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

Lines changed: 87 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -14,35 +14,103 @@
1414
* external/cwe/cwe-036
1515
* external/cwe/cwe-073
1616
* external/cwe/cwe-099
17+
*
18+
* The query detects cases where a user-controlled path is used in an unsafe manner,
19+
* meaning it is not both normalized and _afterwards_ checked.
20+
*
21+
* It does so by dividing the problematic situation into two cases:
22+
* 1. The file path is never normalized.
23+
* This is easily detected by using normalization as a sanitizer.
24+
*
25+
* 2. The file path is normalized at least once, but never checked afterwards.
26+
* This is detected by finding the earliest normalization and then ensuring that
27+
* no checks happen later. Since we start from the earliest normalization,
28+
* we know that the absence of checks means that no normalization has a
29+
* check after it. (No checks after a second normalization would be ok if
30+
* there was a check between the first and the second.)
31+
*
32+
* Note that one could make the dual split on whether the file path is ever checked. This does
33+
* not work as nicely, however, since checking is modelled as a `BarrierGuard` rather than
34+
* as a `Sanitizer`. That means that only some dataflow paths out of a check will be removed,
35+
* and so identifying the last check is not possible simply by finding a dataflow path from it
36+
* to a sink.
1737
*/
1838

1939
import python
20-
import semmle.python.security.Paths
21-
/* Sources */
22-
import semmle.python.web.HttpRequest
23-
/* Sinks */
24-
import semmle.python.security.injection.Path
40+
import semmle.python.dataflow.new.DataFlow
41+
import semmle.python.dataflow.new.DataFlow2
42+
import semmle.python.dataflow.new.TaintTracking
43+
import semmle.python.dataflow.new.TaintTracking2
44+
import semmle.python.Concepts
45+
import semmle.python.dataflow.new.RemoteFlowSources
46+
import ChainedConfigs12
2547

26-
class PathInjectionConfiguration extends TaintTracking::Configuration {
27-
PathInjectionConfiguration() { this = "Path injection configuration" }
48+
// ---------------------------------------------------------------------------
49+
// Case 1. The path is never normalized.
50+
// ---------------------------------------------------------------------------
51+
/** Configuration to find paths from sources to sinks that contain no normalization. */
52+
class PathNotNormalizedConfiguration extends TaintTracking::Configuration {
53+
PathNotNormalizedConfiguration() { this = "PathNotNormalizedConfiguration" }
2854

29-
override predicate isSource(TaintTracking::Source source) {
30-
source instanceof HttpRequestTaintSource
55+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
56+
57+
override predicate isSink(DataFlow::Node sink) {
58+
sink = any(FileSystemAccess e).getAPathArgument()
3159
}
3260

33-
override predicate isSink(TaintTracking::Sink sink) { sink instanceof OpenNode }
61+
override predicate isSanitizer(DataFlow::Node node) { node instanceof Path::PathNormalization }
62+
}
63+
64+
predicate pathNotNormalized(CustomPathNode source, CustomPathNode sink) {
65+
any(PathNotNormalizedConfiguration config).hasFlowPath(source.asNode1(), sink.asNode1())
66+
}
67+
68+
// ---------------------------------------------------------------------------
69+
// Case 2. The path is normalized at least once, but never checked afterwards.
70+
// ---------------------------------------------------------------------------
71+
/** Configuration to find paths from sources to normalizations that contain no prior normalizations. */
72+
class FirstNormalizationConfiguration extends TaintTracking::Configuration {
73+
FirstNormalizationConfiguration() { this = "FirstNormalizationConfiguration" }
74+
75+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
76+
77+
override predicate isSink(DataFlow::Node sink) { sink instanceof Path::PathNormalization }
3478

35-
override predicate isSanitizer(Sanitizer sanitizer) {
36-
sanitizer instanceof PathSanitizer or
37-
sanitizer instanceof NormalizedPathSanitizer
79+
override predicate isSanitizerOut(DataFlow::Node node) { node instanceof Path::PathNormalization }
80+
}
81+
82+
/** Configuration to find paths from normalizations to sinks that do not go through a check. */
83+
class NormalizedPathNotCheckedConfiguration extends TaintTracking2::Configuration {
84+
NormalizedPathNotCheckedConfiguration() { this = "NormalizedPathNotCheckedConfiguration" }
85+
86+
override predicate isSource(DataFlow::Node source) { source instanceof Path::PathNormalization }
87+
88+
override predicate isSink(DataFlow::Node sink) {
89+
sink = any(FileSystemAccess e).getAPathArgument()
3890
}
3991

40-
override predicate isExtension(TaintTracking::Extension extension) {
41-
extension instanceof AbsPath
92+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
93+
guard instanceof Path::SafeAccessCheck
4294
}
4395
}
4496

45-
from PathInjectionConfiguration config, TaintedPathSource src, TaintedPathSink sink
46-
where config.hasFlowPath(src, sink)
47-
select sink.getSink(), src, sink, "This path depends on $@.", src.getSource(),
48-
"a user-provided value"
97+
predicate pathNotCheckedAfterNormalization(CustomPathNode source, CustomPathNode sink) {
98+
exists(
99+
FirstNormalizationConfiguration config, DataFlow::PathNode mid1, DataFlow2::PathNode mid2,
100+
NormalizedPathNotCheckedConfiguration config2
101+
|
102+
config.hasFlowPath(source.asNode1(), mid1) and
103+
config2.hasFlowPath(mid2, sink.asNode2()) and
104+
mid1.getNode().asCfgNode() = mid2.getNode().asCfgNode()
105+
)
106+
}
107+
108+
// ---------------------------------------------------------------------------
109+
// Query: Either case 1 or case 2.
110+
// ---------------------------------------------------------------------------
111+
from CustomPathNode source, CustomPathNode sink
112+
where
113+
pathNotNormalized(source, sink)
114+
or
115+
pathNotCheckedAfterNormalization(source, sink)
116+
select sink, source, sink, "This path depends on $@.", source, "a user-provided value"

‎python/ql/src/Security/CWE-078/CommandInjection.ql‎

Lines changed: 40 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -15,29 +15,51 @@
1515
*/
1616

1717
import python
18-
import semmle.python.security.Paths
19-
/* Sources */
20-
import semmle.python.web.HttpRequest
21-
/* Sinks */
22-
import semmle.python.security.injection.Command
18+
import semmle.python.dataflow.new.DataFlow
19+
import semmle.python.dataflow.new.TaintTracking
20+
import semmle.python.Concepts
21+
import semmle.python.dataflow.new.RemoteFlowSources
22+
import DataFlow::PathGraph
2323

2424
class CommandInjectionConfiguration extends TaintTracking::Configuration {
25-
CommandInjectionConfiguration() { this = "Command injection configuration" }
25+
CommandInjectionConfiguration() { this = "CommandInjectionConfiguration" }
2626

27-
override predicate isSource(TaintTracking::Source source) {
28-
source instanceof HttpRequestTaintSource
29-
}
30-
31-
override predicate isSink(TaintTracking::Sink sink) { sink instanceof CommandSink }
27+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
3228

33-
override predicate isExtension(TaintTracking::Extension extension) {
34-
extension instanceof FirstElementFlow
35-
or
36-
extension instanceof FabricExecuteExtension
29+
override predicate isSink(DataFlow::Node sink) {
30+
sink = any(SystemCommandExecution e).getCommand() and
31+
// Since the implementation of standard library functions such `os.popen` looks like
32+
// ```py
33+
// def popen(cmd, mode="r", buffering=-1):
34+
// ...
35+
// proc = subprocess.Popen(cmd, ...)
36+
// ```
37+
// any time we would report flow to the `os.popen` sink, we can ALSO report the flow
38+
// from the `cmd` parameter to the `subprocess.Popen` sink -- obviously we don't
39+
// want that.
40+
//
41+
// However, simply removing taint edges out of a sink is not a good enough solution,
42+
// since we would only flag one of the `os.system` calls in the following example
43+
// due to use-use flow
44+
// ```py
45+
// os.system(cmd)
46+
// os.system(cmd)
47+
// ```
48+
//
49+
// Best solution I could come up with is to exclude all sinks inside the modules of
50+
// known sinks. This does have a downside: If we have overlooked a function in any
51+
// of these, that internally runs a command, we no longer give an alert :| -- and we
52+
// need to keep them updated (which is hard to remember)
53+
//
54+
// This does not only affect `os.popen`, but also the helper functions in
55+
// `subprocess`. See:
56+
// https://github.com/python/cpython/blob/fa7ce080175f65d678a7d5756c94f82887fc9803/Lib/os.py#L974
57+
// https://github.com/python/cpython/blob/fa7ce080175f65d678a7d5756c94f82887fc9803/Lib/subprocess.py#L341
58+
not sink.getScope().getEnclosingModule().getName() in ["os", "subprocess", "platform", "popen2"]
3759
}
3860
}
3961

40-
from CommandInjectionConfiguration config, TaintedPathSource src, TaintedPathSink sink
41-
where config.hasFlowPath(src, sink)
42-
select sink.getSink(), src, sink, "This command depends on $@.", src.getSource(),
62+
from CommandInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
63+
where config.hasFlowPath(source, sink)
64+
select sink.getNode(), source, sink, "This command depends on $@.", source.getNode(),
4365
"a user-provided value"

‎python/ql/src/Security/CWE-079/ReflectedXss.ql‎

Lines changed: 16 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -13,30 +13,26 @@
1313
*/
1414

1515
import python
16-
import semmle.python.security.Paths
17-
/* Sources */
18-
import semmle.python.web.HttpRequest
19-
/* Sinks */
20-
import semmle.python.web.HttpResponse
21-
/* Flow */
22-
import semmle.python.security.strings.Untrusted
16+
import semmle.python.dataflow.new.DataFlow
17+
import semmle.python.dataflow.new.TaintTracking
18+
import semmle.python.Concepts
19+
import semmle.python.dataflow.new.RemoteFlowSources
20+
import DataFlow::PathGraph
2321

2422
class ReflectedXssConfiguration extends TaintTracking::Configuration {
25-
ReflectedXssConfiguration() { this = "Reflected XSS configuration" }
23+
ReflectedXssConfiguration() { this = "ReflectedXssConfiguration" }
2624

27-
override predicate isSource(TaintTracking::Source source) {
28-
source instanceof HttpRequestTaintSource
29-
}
25+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
3026

31-
override predicate isSink(TaintTracking::Sink sink) {
32-
sink instanceof HttpResponseTaintSink and
33-
not sink instanceof DjangoResponseContent
34-
or
35-
sink instanceof DjangoResponseContentXSSVulnerable
27+
override predicate isSink(DataFlow::Node sink) {
28+
exists(HTTP::Server::HttpResponse response |
29+
response.getMimetype().toLowerCase() = "text/html" and
30+
sink = response.getBody()
31+
)
3632
}
3733
}
3834

39-
from ReflectedXssConfiguration config, TaintedPathSource src, TaintedPathSink sink
40-
where config.hasFlowPath(src, sink)
41-
select sink.getSink(), src, sink, "Cross-site scripting vulnerability due to $@.", src.getSource(),
42-
"a user-provided value"
35+
from ReflectedXssConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
36+
where config.hasFlowPath(source, sink)
37+
select sink.getNode(), source, sink, "Cross-site scripting vulnerability due to $@.",
38+
source.getNode(), "a user-provided value"

‎python/ql/src/Security/CWE-089/SqlInjection.ql‎

100755100644
Lines changed: 11 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -12,40 +12,21 @@
1212
*/
1313

1414
import python
15-
import semmle.python.security.Paths
16-
/* Sources */
17-
import semmle.python.web.HttpRequest
18-
/* Sinks */
19-
import semmle.python.security.injection.Sql
20-
import semmle.python.web.django.Db
21-
import semmle.python.web.django.Model
15+
import semmle.python.dataflow.new.DataFlow
16+
import semmle.python.dataflow.new.TaintTracking
17+
import semmle.python.Concepts
18+
import semmle.python.dataflow.new.RemoteFlowSources
19+
import DataFlow::PathGraph
2220

2321
class SQLInjectionConfiguration extends TaintTracking::Configuration {
24-
SQLInjectionConfiguration() { this = "SQL injection configuration" }
22+
SQLInjectionConfiguration() { this = "SQLInjectionConfiguration" }
2523

26-
override predicate isSource(TaintTracking::Source source) {
27-
source instanceof HttpRequestTaintSource
28-
}
24+
override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
2925

30-
override predicate isSink(TaintTracking::Sink sink) { sink instanceof SqlInjectionSink }
26+
override predicate isSink(DataFlow::Node sink) { sink = any(SqlExecution e).getSql() }
3127
}
3228

33-
/*
34-
* Additional configuration to support tracking of DB objects. Connections, cursors, etc.
35-
* Without this configuration (or the LegacyConfiguration), the pattern of
36-
* `any(MyTaintKind k).taints(control_flow_node)` used in DbConnectionExecuteArgument would not work.
37-
*/
38-
39-
class DbConfiguration extends TaintTracking::Configuration {
40-
DbConfiguration() { this = "DB configuration" }
41-
42-
override predicate isSource(TaintTracking::Source source) {
43-
source instanceof DjangoModelObjects or
44-
source instanceof DbConnectionSource
45-
}
46-
}
47-
48-
from SQLInjectionConfiguration config, TaintedPathSource src, TaintedPathSink sink
49-
where config.hasFlowPath(src, sink)
50-
select sink.getSink(), src, sink, "This SQL query depends on $@.", src.getSource(),
29+
from SQLInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
30+
where config.hasFlowPath(source, sink)
31+
select sink.getNode(), source, sink, "This SQL query depends on $@.", source.getNode(),
5132
"a user-provided value"

0 commit comments

Comments
 (0)