better identification of Composite Actions input and output nodes

Author: Alvaro Muñoz

ql/lib/codeql/actions/Ast.qll

@@ -92,7 +92,8 @@ class OutputsStmt extends Statement instanceof YamlMapping {
    * Gets a specific output expression (YamlMapping) by name.
    */
   OutputExpr getOutputExpr(string name) {
-    this.(YamlMapping).lookup(name).(YamlMapping).lookup("value") = result
+    this.(YamlMapping).lookup(name).(YamlMapping).lookup("value") = result or
+    this.(YamlMapping).lookup(name) = result
   }
 }
 
@@ -101,7 +102,12 @@ class InputExpr extends Expression instanceof YamlString {
 }
 
 class OutputExpr extends Expression instanceof YamlString {
-  OutputExpr() { exists(OutputsStmt outputs | outputs.(YamlMapping).maps(_, this)) }
+  OutputExpr() {
+    exists(OutputsStmt outputs |
+      outputs.(YamlMapping).lookup(_).(YamlMapping).lookup("value") = this or
+      outputs.(YamlMapping).lookup(_) = this
+    )
+  }
 }
 
 /**

ql/lib/codeql/actions/dataflow/internal/DataFlowPublic.qll

@@ -48,22 +48,22 @@ class ExprNode extends Node, TExprNode {
  * Reusable workflow input nodes
  */
 class ParameterNode extends ExprNode {
-  private InputExpr parameter;
+  private InputExpr input;
 
   ParameterNode() {
-    this.asExpr() = parameter and
-    parameter = any(ReusableWorkflowStmt w).getInputsStmt().getInputExpr(_)
+    this.asExpr() = input and
+    input = any(InputsStmt s).getInputExpr(_)
   }
 
   predicate isParameterOf(DataFlowCallable c, ParameterPosition pos) {
-    parameter = c.(ReusableWorkflowStmt).getInputsStmt().getInputExpr(pos)
+    input = c.(ReusableWorkflowStmt).getInputsStmt().getInputExpr(pos)
   }
 
-  override string toString() { result = parameter.toString() }
+  override string toString() { result = "input " + input.toString() }
 
-  override Location getLocation() { result = parameter.getLocation() }
+  override Location getLocation() { result = input.getLocation() }
 
-  InputExpr getInputExpr() { result = parameter } 
+  InputExpr getInputExpr() { result = input }
 }
 
 /**
@@ -83,19 +83,18 @@ class ArgumentNode extends ExprNode {
  * Reusable workflow output nodes
  */
 class ReturnNode extends ExprNode {
-  private Cfg::Node node;
+  private OutputExpr output;
 
   ReturnNode() {
-    this.getCfgNode() = node and
-    (node.getAstNode() = any(ReusableWorkflowStmt w).getOutputsStmt().getOutputExpr(_) or
-    node.getAstNode() = any(CompositeActionStmt a).getOutputsStmt().getOutputExpr(_))
+    this.asExpr() = output and
+    output = any(OutputsStmt s).getOutputExpr(_)
   }
 
   ReturnKind getKind() { result = TNormalReturn() }
 
-  override string toString() { result = "return " + node.toString() }
+  override string toString() { result = "output " + output.toString() }
 
-  override Location getLocation() { result = node.getLocation() }
+  override Location getLocation() { result = output.getLocation() }
 }
 
 /** Gets the node corresponding to `e`. */

ql/src/Security/CWE-020/CompositeActionSummaries.ql

@@ -15,16 +15,16 @@ import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
 
-private class OutputVariableSink extends DataFlow::Node {
-  OutputVariableSink() { exists(OutputsStmt s | s.getOutputExpr(_) = this.asExpr()) }
-}
-
 private module MyConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
+    source instanceof DataFlow::ParameterNode and
     exists(CompositeActionStmt c | c.getInputsStmt().getInputExpr(_) = source.asExpr())
   }
 
-  predicate isSink(DataFlow::Node sink) { sink instanceof OutputVariableSink }
+  predicate isSink(DataFlow::Node sink) {
+    sink instanceof DataFlow::ReturnNode and
+    exists(CompositeActionStmt c | c.getOutputsStmt().getOutputExpr(_) = sink.asExpr())
+  }
 }
 
 module MyFlow = TaintTracking::Global<MyConfig>;

ql/src/Security/CWE-020/CompositeActionsSources.ql

@@ -15,18 +15,17 @@ import codeql.actions.TaintTracking
 import codeql.actions.dataflow.FlowSources
 import codeql.actions.dataflow.ExternalFlow
 
-private class OutputVariableSink extends DataFlow::Node {
-  OutputVariableSink() { exists(OutputsStmt s | s.getOutputExpr(_) = this.asExpr()) }
-}
-
 private module MyConfig implements DataFlow::ConfigSig {
   predicate isSource(DataFlow::Node source) {
     source instanceof RemoteFlowSource and
-    exists(CompositeActionStmt c | c.getAChildNode*() = source.asExpr()) and
-    not exists(CompositeActionStmt c | c.getInputsStmt().getInputExpr(_) = source.asExpr())
+    not source instanceof DataFlow::ParameterNode and
+    exists(CompositeActionStmt c | c.getAChildNode*() = source.asExpr())
   }
 
-  predicate isSink(DataFlow::Node sink) { sink instanceof OutputVariableSink }
+  predicate isSink(DataFlow::Node sink) {
+    sink instanceof DataFlow::ReturnNode and
+    exists(CompositeActionStmt c | c.getOutputsStmt().getOutputExpr(_) = sink.asExpr())
+  }
 }
 
 module MyFlow = TaintTracking::Global<MyConfig>;