Go: Rewrite InlineFlowTest as a parameterized module

Author: jketema

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

@@ -0,0 +1,4 @@
+---
+category: deprecated
+---
+* The `LogInjection::Configuration` taint flow configuration class has been deprecated. Use the `LogInjection::Flow` module instead.

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

@@ -17,7 +17,7 @@ module LogInjection {
   /**
    * A taint-tracking configuration for reasoning about log injection vulnerabilities.
    */
-  class Configuration extends TaintTracking::Configuration {
+  deprecated class Configuration extends TaintTracking::Configuration {
     Configuration() { this = "LogInjection" }
 
     override predicate isSource(DataFlow::Node source) { source instanceof Source }
@@ -30,4 +30,17 @@ module LogInjection {
       guard instanceof SanitizerGuard
     }
   }
+
+  /**
+   * A taint-tracking configuration for reasoning about log injection vulnerabilities.
+   */
+  module Config implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    predicate isBarrier(DataFlow::Node sanitizer) { sanitizer instanceof Sanitizer }
+  }
+
+  module Flow = TaintTracking::Global<Config>;
 }

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

@@ -13,9 +13,9 @@
 
 import go
 import semmle.go.security.LogInjection
-import DataFlow::PathGraph
+import LogInjection::Flow::PathGraph
 
-from LogInjection::Configuration c, DataFlow::PathNode source, DataFlow::PathNode sink
-where c.hasFlowPath(source, sink)
+from LogInjection::Flow::PathNode source, LogInjection::Flow::PathNode sink
+where LogInjection::Flow::flowPath(source, sink)
 select sink.getNode(), source, sink, "This log entry depends on a $@.", source.getNode(),
   "user-provided value"

go/ql/test/TestUtilities/InlineFlowTest.qll

@@ -3,37 +3,30 @@
  *
  * Example for a test.ql:
  * ```ql
- * import java
+ * import go
  * import TestUtilities.InlineFlowTest
+ * import DefaultFlowTest
  * ```
  *
  * To declare expectations, you can use the $hasTaintFlow or $hasValueFlow comments within the test source files.
- * Example of the corresponding test file, e.g. Test.java
+ * Example of the corresponding test file, e.g. Test.go
  * ```go
- * public class Test {
- *
- * 	Object source() { return null; }
- * 	String taint() { return null; }
- * 	void sink(Object o) { }
- *
- * 	public void test() {
- * 		Object s = source();
- * 		sink(s); //$hasValueFlow
- * 		String t = "foo" + taint();
- * 		sink(t); //$hasTaintFlow
- * 	}
+ * func source() string { return ""; }
+ * func taint() string { return ""; }
+ * func sink(s string) { }
  *
+ * func test() {
+ *   s := source()
+ *   sink(s) // $ hasValueFlow="s"
+ *   t := "foo" + taint()
+ *   sink(t) // $ hasTaintFlow="t"
  * }
  * ```
  *
- * If you're not interested in a specific flow type, you can disable either value or taint flow expectations as follows:
- * ```ql
- * class HasFlowTest extends InlineFlowTest {
- *   override DataFlow::Configuration getTaintFlowConfig() { none() }
- *
- *   override DataFlow::Configuration getValueFlowConfig() { none() }
- * }
- * ```
+ * If you are only interested in value flow, then instead of importing `DefaultFlowTest`, you can import
+ * `ValueFlowTest<DefaultFlowConfig>`. Similarly, if you are only interested in taint flow, then instead of
+ * importing `DefaultFlowTest`, you can import `TaintFlowTest<DefaultFlowConfig>`. In both cases
+ * `DefaultFlowConfig` can be replaced by another implementation of `DataFlow::ConfigSig`.
  *
  * If you need more fine-grained tuning, consider implementing a test using `InlineExpectationsTest`.
  */
@@ -47,57 +40,62 @@ private predicate defaultSource(DataFlow::Node source) {
   )
 }
 
-class DefaultValueFlowConf extends DataFlow::Configuration {
-  DefaultValueFlowConf() { this = "qltest:defaultValueFlowConf" }
+private predicate defaultSink(DataFlow::Node sink) {
+  exists(Function fn | fn.hasQualifiedName(_, "sink") | sink = fn.getACall().getAnArgument())
+}
+
+module DefaultFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { defaultSource(source) }
 
-  override predicate isSource(DataFlow::Node source) { defaultSource(source) }
+  predicate isSink(DataFlow::Node sink) { defaultSink(sink) }
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(Function fn | fn.hasQualifiedName(_, "sink") | sink = fn.getACall().getAnArgument())
-  }
+  int fieldFlowBranchLimit() { result = 1000 }
+}
+
+private module NoFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { none() }
 
-  override int fieldFlowBranchLimit() { result = 1000 }
+  predicate isSink(DataFlow::Node sink) { none() }
 }
 
-class DefaultTaintFlowConf extends TaintTracking::Configuration {
-  DefaultTaintFlowConf() { this = "qltest:defaultTaintFlowConf" }
+module FlowTest<DataFlow::ConfigSig ValueFlowConfig, DataFlow::ConfigSig TaintFlowConfig> {
+  module ValueFlow = DataFlow::Global<ValueFlowConfig>;
 
-  override predicate isSource(DataFlow::Node source) { defaultSource(source) }
+  module TaintFlow = TaintTracking::Global<TaintFlowConfig>;
 
-  override predicate isSink(DataFlow::Node sink) {
-    exists(Function fn | fn.hasQualifiedName(_, "sink") | sink = fn.getACall().getAnArgument())
+  private module InlineTest implements TestSig {
+    string getARelevantTag() { result = ["hasValueFlow", "hasTaintFlow"] }
+
+    predicate hasActualResult(Location location, string element, string tag, string value) {
+      tag = "hasValueFlow" and
+      exists(DataFlow::Node sink | ValueFlow::flowTo(sink) |
+        sink.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
+          location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
+        element = sink.toString() and
+        value = "\"" + sink.toString() + "\""
+      )
+      or
+      tag = "hasTaintFlow" and
+      exists(DataFlow::Node src, DataFlow::Node sink |
+        TaintFlow::flow(src, sink) and not ValueFlow::flow(src, sink)
+      |
+        sink.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
+          location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
+        element = sink.toString() and
+        value = "\"" + sink.toString() + "\""
+      )
+    }
   }
 
-  override int fieldFlowBranchLimit() { result = 1000 }
+  import MakeTest<InlineTest>
 }
 
-class InlineFlowTest extends InlineExpectationsTest {
-  InlineFlowTest() { this = "HasFlowTest" }
-
-  override string getARelevantTag() { result = ["hasValueFlow", "hasTaintFlow"] }
-
-  override predicate hasActualResult(Location location, string element, string tag, string value) {
-    tag = "hasValueFlow" and
-    exists(DataFlow::Node sink | this.getValueFlowConfig().hasFlowTo(sink) |
-      sink.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
-        location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
-      element = sink.toString() and
-      value = "\"" + sink.toString() + "\""
-    )
-    or
-    tag = "hasTaintFlow" and
-    exists(DataFlow::Node src, DataFlow::Node sink |
-      this.getTaintFlowConfig().hasFlow(src, sink) and
-      not this.getValueFlowConfig().hasFlow(src, sink)
-    |
-      sink.hasLocationInfo(location.getFile().getAbsolutePath(), location.getStartLine(),
-        location.getStartColumn(), location.getEndLine(), location.getEndColumn()) and
-      element = sink.toString() and
-      value = "\"" + sink.toString() + "\""
-    )
-  }
+module DefaultFlowTest = FlowTest<DefaultFlowConfig, DefaultFlowConfig>;
 
-  DataFlow::Configuration getValueFlowConfig() { result = any(DefaultValueFlowConf config) }
+module ValueFlowTest<DataFlow::ConfigSig ValueFlowConfig> {
+  import FlowTest<ValueFlowConfig, NoFlowConfig>
+}
 
-  DataFlow::Configuration getTaintFlowConfig() { result = any(DefaultTaintFlowConf config) }
+module TaintFlowTest<DataFlow::ConfigSig TaintFlowConfig> {
+  import FlowTest<NoFlowConfig, TaintFlowConfig>
 }

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

@@ -1,2 +1,3 @@
 failures
 invalidModelRow
+testFailures

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

@@ -8,16 +8,10 @@ import ModelValidation
 import semmle.go.dataflow.internal.FlowSummaryImpl as FlowSummaryImpl
 import TestUtilities.InlineFlowTest
 
-class Config extends TaintTracking::Configuration {
-  Config() { this = "external-flow-test" }
+module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node src) { sourceNode(src, "qltest") }
 
-  override predicate isSource(DataFlow::Node src) { sourceNode(src, "qltest") }
-
-  override predicate isSink(DataFlow::Node src) { sinkNode(src, "qltest") }
+  predicate isSink(DataFlow::Node src) { sinkNode(src, "qltest") }
 }
 
-class ExternalFlowTest extends InlineFlowTest {
-  override DataFlow::Configuration getValueFlowConfig() { none() }
-
-  override DataFlow::Configuration getTaintFlowConfig() { result = any(Config config) }
-}
+import TaintFlowTest<Config>

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

@@ -0,0 +1,2 @@
+failures
+testFailures

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

@@ -1,2 +1,3 @@
 import go
 import TestUtilities.InlineFlowTest
+import DefaultFlowTest

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

@@ -0,0 +1,2 @@
+failures
+testFailures

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

@@ -1,11 +1,4 @@
 import go
 import TestUtilities.InlineFlowTest
 import semmle.go.security.LogInjection
-
-class LogInjectionTest extends InlineFlowTest {
-  override DataFlow::Configuration getTaintFlowConfig() {
-    result = any(LogInjection::Configuration config)
-  }
-
-  override DataFlow::Configuration getValueFlowConfig() { none() }
-}
+import TaintFlowTest<LogInjection::Config>