Refactor to use flow state instead of 3 flow configs (copilot)

Author: owen-mc

go/ql/src/Security/CWE-079/HTMLTemplateEscapingPassthrough.ql

@@ -13,21 +13,6 @@
 
 import go
 
-/**
- * Holds if the provided `untrusted` node flows into a conversion to a PassthroughType.
- * The `targetType` parameter gets populated with the name of the PassthroughType,
- * and `conversionSink` gets populated with the node where the conversion happens.
- */
-predicate flowsFromUntrustedToConversion(
-  DataFlow::Node untrusted, PassthroughTypeName targetType, DataFlow::Node conversionSink
-) {
-  exists(DataFlow::Node source |
-    UntrustedToPassthroughTypeConversionFlow::flow(source, conversionSink) and
-    source = untrusted and
-    UntrustedToPassthroughTypeConversionConfig::isSinkToPassthroughType(conversionSink, targetType)
-  )
-}
-
 /**
  * A name of a type that will not be escaped when passed to
  * a `html/template` template.
@@ -36,65 +21,6 @@ class PassthroughTypeName extends string {
   PassthroughTypeName() { this = ["HTML", "HTMLAttr", "JS", "JSStr", "CSS", "Srcset", "URL"] }
 }
 
-module UntrustedToPassthroughTypeConversionConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource }
-
-  additional predicate isSinkToPassthroughType(DataFlow::TypeCastNode sink, PassthroughTypeName name) {
-    exists(Type typ |
-      typ = sink.getResultType() and
-      typ.getUnderlyingType*().hasQualifiedName("html/template", name)
-    )
-  }
-
-  predicate isSink(DataFlow::Node sink) { isSinkToPassthroughType(sink, _) }
-
-  predicate isBarrier(DataFlow::Node node) {
-    node instanceof SharedXss::Sanitizer or node.getType() instanceof NumericType
-  }
-}
-
-/**
- * Tracks taint flow for reasoning about when a `ActiveThreatModelSource` is
- * converted into a special "passthrough" type which will not be escaped by the
- * template generator; this allows the injection of arbitrary content (html,
- * css, js) into the generated output of the templates.
- */
-module UntrustedToPassthroughTypeConversionFlow =
-  TaintTracking::Global<UntrustedToPassthroughTypeConversionConfig>;
-
-/**
- * Holds if the provided `conversion` node flows into the provided `execSink`.
- */
-predicate flowsFromConversionToExec(
-  DataFlow::Node conversion, PassthroughTypeName targetType, DataFlow::Node execSink
-) {
-  PassthroughTypeConversionToTemplateExecutionCallFlow::flow(conversion, execSink) and
-  PassthroughTypeConversionToTemplateExecutionCallConfig::isSourceConversionToPassthroughType(conversion,
-    targetType)
-}
-
-module PassthroughTypeConversionToTemplateExecutionCallConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { isSourceConversionToPassthroughType(source, _) }
-
-  additional predicate isSourceConversionToPassthroughType(
-    DataFlow::TypeCastNode source, PassthroughTypeName name
-  ) {
-    exists(Type typ |
-      typ = source.getResultType() and
-      typ.getUnderlyingType*().hasQualifiedName("html/template", name)
-    )
-  }
-
-  predicate isSink(DataFlow::Node sink) { isSinkToTemplateExec(sink, _) }
-}
-
-/**
- * Tracks taint flow for reasoning about when the result of a conversion to a
- * PassthroughType flows to a template execution call.
- */
-module PassthroughTypeConversionToTemplateExecutionCallFlow =
-  TaintTracking::Global<PassthroughTypeConversionToTemplateExecutionCallConfig>;
-
 /**
  * Holds if the sink is a data value argument of a template execution call.
  */
@@ -109,46 +35,70 @@ predicate isSinkToTemplateExec(DataFlow::Node sink, DataFlow::CallNode call) {
   )
 }
 
-module FromUntrustedToTemplateExecutionCallConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { source instanceof ActiveThreatModelSource }
+/**
+ * Flow state for tracking whether a conversion to a passthrough type has occurred.
+ */
+class FlowState extends int {
+  FlowState() { this = 0 or this = 1 }
+
+  predicate isBeforeConversion() { this = 0 }
 
-  predicate isSink(DataFlow::Node sink) { isSinkToTemplateExec(sink, _) }
+  predicate isAfterConversion() { this = 1 }
 }
 
 /**
- * Tracks taint flow from a `ActiveThreatModelSource` into a template executor
- * call.
+ * Data flow configuration that tracks flows from untrusted sources (A) to template execution calls (C),
+ * and tracks whether a conversion to a passthrough type (B) has occurred.
  */
-module FromUntrustedToTemplateExecutionCallFlow =
-  TaintTracking::Global<FromUntrustedToTemplateExecutionCallConfig>;
+module UntrustedToTemplateExecWithConversionConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source, FlowState state) {
+    state.isBeforeConversion() and source instanceof ActiveThreatModelSource
+  }
+
+  predicate isSink(DataFlow::Node sink, FlowState state) {
+    state.isAfterConversion() and isSinkToTemplateExec(sink, _)
+  }
 
-import FromUntrustedToTemplateExecutionCallFlow::PathGraph
+  predicate isBarrier(DataFlow::Node node, FlowState state) {
+    node instanceof SharedXss::Sanitizer or node.getType() instanceof NumericType
+  }
 
-/**
- * Holds if the provided `untrusted` node flows into the provided `execSink`.
- */
-predicate flowsFromUntrustedToExec(
-  FromUntrustedToTemplateExecutionCallFlow::PathNode untrusted,
-  FromUntrustedToTemplateExecutionCallFlow::PathNode execSink
-) {
-  FromUntrustedToTemplateExecutionCallFlow::flowPath(untrusted, execSink)
+  /**
+   * When a conversion to a passthrough type is encountered, transition the flow state.
+   */
+  predicate step(DataFlow::Node pred, FlowState predState, DataFlow::Node succ, FlowState succState) {
+    // If not yet converted, look for a conversion to a passthrough type
+    predState.isBeforeConversion() and
+    succState.isAfterConversion() and
+    pred = succ and
+    exists(Type typ, PassthroughTypeName name |
+      typ = pred.getType() and
+      typ.getUnderlyingType*().hasQualifiedName("html/template", name)
+    )
+    or
+    // Otherwise, normal flow with unchanged state
+    predState = succState
+  }
 }
 
+module UntrustedToTemplateExecWithConversionFlow =
+  DataFlow::PathGraph<UntrustedToTemplateExecWithConversionConfig>;
+
 from
-  FromUntrustedToTemplateExecutionCallFlow::PathNode untrustedSource,
-  FromUntrustedToTemplateExecutionCallFlow::PathNode templateExecCall,
-  PassthroughTypeName targetTypeName, DataFlow::Node conversion
+  UntrustedToTemplateExecWithConversionFlow::PathNode untrustedSource,
+  UntrustedToTemplateExecWithConversionFlow::PathNode templateExecCall, DataFlow::Node conversion,
+  PassthroughTypeName targetTypeName
 where
-  // A = untrusted remote flow source
-  // B = conversion to PassthroughType
-  // C = template execution call
-  // Flows:
-  // A -> B
-  flowsFromUntrustedToConversion(untrustedSource.getNode(), targetTypeName, conversion) and
-  // B -> C
-  flowsFromConversionToExec(conversion, targetTypeName, templateExecCall.getNode()) and
-  // A -> C
-  flowsFromUntrustedToExec(untrustedSource, templateExecCall)
+  UntrustedToTemplateExecWithConversionFlow::flowPath(untrustedSource, templateExecCall) and
+  // Find the conversion node in the path
+  exists(int i |
+    i = templateExecCall.getPathIndex() - 1 and
+    conversion = templateExecCall.getPathNode(i).getNode() and
+    exists(Type typ |
+      typ = conversion.getType() and
+      typ.getUnderlyingType*().hasQualifiedName("html/template", targetTypeName)
+    )
+  )
 select templateExecCall.getNode(), untrustedSource, templateExecCall,
   "Data from an $@ will not be auto-escaped because it was $@ to template." + targetTypeName,
   untrustedSource.getNode(), "untrusted source", conversion, "converted"