Skip to content

Commit eb62df6

Browse files
jketemajketema
committed
Go: Rewrite InlineFlowTest as a parameterized module
1 parent ad2b020 commit eb62df6

File tree

10 files changed

+92
-84
lines changed

10 files changed

+92
-84
lines changed

‎go/ql/lib/change-notes/2023-06-14-log-injection-deprecation.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: deprecated
3+
---
4+
* The `LogInjection::Configuration` taint flow configuration class has been deprecated. Use the `LogInjection::Flow` module instead.

‎go/ql/lib/semmle/go/security/LogInjection.qll

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ module LogInjection {
1717
/**
1818
* A taint-tracking configuration for reasoning about log injection vulnerabilities.
1919
*/
20-
class Configuration extends TaintTracking::Configuration {
20+
deprecated class Configuration extends TaintTracking::Configuration {
2121
Configuration() { this = "LogInjection" }
2222

2323
override predicate isSource(DataFlow::Node source) { source instanceof Source }
@@ -30,4 +30,17 @@ module LogInjection {
3030
guard instanceof SanitizerGuard
3131
}
3232
}
33+
34+
/**
35+
* A taint-tracking configuration for reasoning about log injection vulnerabilities.
36+
*/
37+
module Config implements DataFlow::ConfigSig {
38+
predicate isSource(DataFlow::Node source) { source instanceof Source }
39+
40+
predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
41+
42+
predicate isBarrier(DataFlow::Node sanitizer) { sanitizer instanceof Sanitizer }
43+
}
44+
45+
module Flow = TaintTracking::Global<Config>;
3346
}

‎go/ql/src/Security/CWE-117/LogInjection.ql

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,9 @@
1313

1414
import go
1515
import semmle.go.security.LogInjection
16-
import DataFlow::PathGraph
16+
import LogInjection::Flow::PathGraph
1717

18-
from LogInjection::Configuration c, DataFlow::PathNode source, DataFlow::PathNode sink
19-
where c.hasFlowPath(source, sink)
18+
from LogInjection::Flow::PathNode source, LogInjection::Flow::PathNode sink
19+
where LogInjection::Flow::flowPath(source, sink)
2020
select sink.getNode(), source, sink, "This log entry depends on a $@.", source.getNode(),
2121
"user-provided value"

‎go/ql/test/TestUtilities/InlineFlowTest.qll

Lines changed: 60 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -3,37 +3,30 @@
33
*
44
* Example for a test.ql:
55
* ```ql
6-
* import java
6+
* import go
77
* import TestUtilities.InlineFlowTest
8+
* import DefaultFlowTest
89
* ```
910
*
1011
* To declare expectations, you can use the $hasTaintFlow or $hasValueFlow comments within the test source files.
11-
* Example of the corresponding test file, e.g. Test.java
12+
* Example of the corresponding test file, e.g. Test.go
1213
* ```go
13-
* public class Test {
14-
*
15-
* Object source() { return null; }
16-
* String taint() { return null; }
17-
* void sink(Object o) { }
18-
*
19-
* public void test() {
20-
* Object s = source();
21-
* sink(s); //$hasValueFlow
22-
* String t = "foo" + taint();
23-
* sink(t); //$hasTaintFlow
24-
* }
14+
* func source() string { return ""; }
15+
* func taint() string { return ""; }
16+
* func sink(s string) { }
2517
*
18+
* func test() {
19+
* s := source()
20+
* sink(s) // $ hasValueFlow="s"
21+
* t := "foo" + taint()
22+
* sink(t) // $ hasTaintFlow="t"
2623
* }
2724
* ```
2825
*
29-
* If you're not interested in a specific flow type, you can disable either value or taint flow expectations as follows:
30-
* ```ql
31-
* class HasFlowTest extends InlineFlowTest {
32-
* override DataFlow::Configuration getTaintFlowConfig() { none() }
33-
*
34-
* override DataFlow::Configuration getValueFlowConfig() { none() }
35-
* }
36-
* ```
26+
* If you are only interested in value flow, then instead of importing `DefaultFlowTest`, you can import
27+
* `ValueFlowTest<DefaultFlowConfig>`. Similarly, if you are only interested in taint flow, then instead of
28+
* importing `DefaultFlowTest`, you can import `TaintFlowTest<DefaultFlowConfig>`. In both cases
29+
* `DefaultFlowConfig` can be replaced by another implementation of `DataFlow::ConfigSig`.
3730
*
3831
* If you need more fine-grained tuning, consider implementing a test using `InlineExpectationsTest`.
3932
*/
@@ -47,57 +40,62 @@ private predicate defaultSource(DataFlow::Node source) {
4740
)
4841
}
4942

50-
class DefaultValueFlowConf extends DataFlow::Configuration {
51-
DefaultValueFlowConf() { this = "qltest:defaultValueFlowConf" }
43+
private predicate defaultSink(DataFlow::Node sink) {
44+
exists(Function fn | fn.hasQualifiedName(_, "sink") | sink = fn.getACall().getAnArgument())
45+
}
46+
47+
module DefaultFlowConfig implements DataFlow::ConfigSig {
48+
predicate isSource(DataFlow::Node source) { defaultSource(source) }
5249

53-
override predicate isSource(DataFlow::Node source) { defaultSource(source) }
50+
predicate isSink(DataFlow::Node sink) { defaultSink(sink) }
5451

55-
override predicate isSink(DataFlow::Node sink) {
56-
exists(Function fn | fn.hasQualifiedName(_, "sink") | sink = fn.getACall().getAnArgument())
57-
}
52+
int fieldFlowBranchLimit() { result = 1000 }
53+
}
54+
55+
private module NoFlowConfig implements DataFlow::ConfigSig {
56+
predicate isSource(DataFlow::Node source) { none() }
5857

59-
override int fieldFlowBranchLimit() { result = 1000 }
58+
predicate isSink(DataFlow::Node sink) { none() }
6059
}
6160

62-
class DefaultTaintFlowConf extends TaintTracking::Configuration {
63-
DefaultTaintFlowConf() { this = "qltest:defaultTaintFlowConf" }
61+
module FlowTest<DataFlow::ConfigSig ValueFlowConfig, DataFlow::ConfigSig TaintFlowConfig> {
62+
module ValueFlow = DataFlow::Global<ValueFlowConfig>;
6463

65-
override predicate isSource(DataFlow::Node source) { defaultSource(source) }
64+
module TaintFlow = TaintTracking::Global<TaintFlowConfig>;
6665

67-
override predicate isSink(DataFlow::Node sink) {
68-
exists(Function fn | fn.hasQualifiedName(_, "sink") | sink = fn.getACall().getAnArgument())
66+
private module InlineTest implements TestSig {
67+
string getARelevantTag() { result = ["hasValueFlow", "hasTaintFlow"] }
68+
69+
predicate hasActualResult(Location location, string element, string tag, string value) {
70+
tag = "hasValueFlow" and
71+
exists(DataFlow::Node sink | ValueFlow::flowTo(sink) |
72+
sink.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
73+
location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
74+
element = sink.toString() and
75+
value = "\"" + sink.toString() + "\""
76+
)
77+
or
78+
tag = "hasTaintFlow" and
79+
exists(DataFlow::Node src, DataFlow::Node sink |
80+
TaintFlow::flow(src, sink) and not ValueFlow::flow(src, sink)
81+
|
82+
sink.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
83+
location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
84+
element = sink.toString() and
85+
value = "\"" + sink.toString() + "\""
86+
)
87+
}
6988
}
7089

71-
override int fieldFlowBranchLimit() { result = 1000 }
90+
import MakeTest<InlineTest>
7291
}
7392

74-
class InlineFlowTest extends InlineExpectationsTest {
75-
InlineFlowTest() { this = "HasFlowTest" }
76-
77-
override string getARelevantTag() { result = ["hasValueFlow", "hasTaintFlow"] }
78-
79-
override predicate hasActualResult(Location location, string element, string tag, string value) {
80-
tag = "hasValueFlow" and
81-
exists(DataFlow::Node sink | this.getValueFlowConfig().hasFlowTo(sink) |
82-
sink.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
83-
location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
84-
element = sink.toString() and
85-
value = "\"" + sink.toString() + "\""
86-
)
87-
or
88-
tag = "hasTaintFlow" and
89-
exists(DataFlow::Node src, DataFlow::Node sink |
90-
this.getTaintFlowConfig().hasFlow(src, sink) and
91-
not this.getValueFlowConfig().hasFlow(src, sink)
92-
|
93-
sink.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
94-
location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
95-
element = sink.toString() and
96-
value = "\"" + sink.toString() + "\""
97-
)
98-
}
93+
module DefaultFlowTest = FlowTest<DefaultFlowConfig, DefaultFlowConfig>;
9994

100-
DataFlow::Configuration getValueFlowConfig() { result = any(DefaultValueFlowConf config) }
95+
module ValueFlowTest<DataFlow::ConfigSig ValueFlowConfig> {
96+
import FlowTest<ValueFlowConfig, NoFlowConfig>
97+
}
10198

102-
DataFlow::Configuration getTaintFlowConfig() { result = any(DefaultTaintFlowConf config) }
99+
module TaintFlowTest<DataFlow::ConfigSig TaintFlowConfig> {
100+
import FlowTest<NoFlowConfig, TaintFlowConfig>
103101
}

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlow/completetest.expected

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
failures
22
invalidModelRow
3+
testFailures

‎go/ql/test/library-tests/semmle/go/dataflow/ExternalFlow/completetest.ql

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,10 @@ import ModelValidation
88
import semmle.go.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
99
import TestUtilities.InlineFlowTest
1010

11-
class Config extends TaintTracking::Configuration {
12-
Config() { this = "external-flow-test" }
11+
module Config implements DataFlow::ConfigSig {
12+
predicate isSource(DataFlow::Node src) { sourceNode(src, "qltest") }
1313

14-
override predicate isSource(DataFlow::Node src) { sourceNode(src, "qltest") }
15-
16-
override predicate isSink(DataFlow::Node src) { sinkNode(src, "qltest") }
14+
predicate isSink(DataFlow::Node src) { sinkNode(src, "qltest") }
1715
}
1816

19-
class ExternalFlowTest extends InlineFlowTest {
20-
override DataFlow::Configuration getValueFlowConfig() { none() }
21-
22-
override DataFlow::Configuration getTaintFlowConfig() { result = any(Config config) }
23-
}
17+
import TaintFlowTest<Config>

‎go/ql/test/library-tests/semmle/go/dataflow/GenericFunctionsAndTypes/Flows.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
failures
2+
testFailures

‎go/ql/test/library-tests/semmle/go/dataflow/GenericFunctionsAndTypes/Flows.ql

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,2 +1,3 @@
11
import go
22
import TestUtilities.InlineFlowTest
3+
import DefaultFlowTest

‎go/ql/test/query-tests/Security/CWE-117/LogInjectionTest.expected

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
failures
2+
testFailures

‎go/ql/test/query-tests/Security/CWE-117/LogInjectionTest.ql

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,4 @@
11
import go
22
import TestUtilities.InlineFlowTest
33
import semmle.go.security.LogInjection
4-
5-
class LogInjectionTest extends InlineFlowTest {
6-
override DataFlow::Configuration getTaintFlowConfig() {
7-
result = any(LogInjection::Configuration config)
8-
}
9-
10-
override DataFlow::Configuration getValueFlowConfig() { none() }
11-
}
4+
import TaintFlowTest<LogInjection::Config>

0 commit comments

Comments
 (0)