feat(queries): Add Output Clobbering query

ql/lib/codeql/actions/dataflow/FlowSteps.qll

@@ -113,7 +113,7 @@ predicate envToRunStep(DataFlow::Node pred, DataFlow::Node succ) {
     run.getInScopeEnvVarExpr(var_name) = pred.asExpr() and
     succ.asExpr() = run.getScriptScalar() and
     (
-      envToSpecialFile(["GITHUB_ENV", "GITHUB_PATH"], var_name, run, _) or
+      envToSpecialFile(["GITHUB_ENV", "GITHUB_OUTPUT", "GITHUB_PATH"], var_name, run, _) or
       envToArgInjSink(var_name, run, _)
     )
   )

ql/lib/codeql/actions/security/OutputClobberingQuery.qll

@@ -1,43 +1,98 @@
 private import actions
 private import codeql.actions.TaintTracking
 private import codeql.actions.dataflow.ExternalFlow
-private import codeql.actions.security.CodeInjectionQuery
 private import codeql.actions.security.ArtifactPoisoningQuery
-import codeql.actions.dataflow.FlowSources
+private import codeql.actions.dataflow.FlowSteps
 import codeql.actions.DataFlow
+import codeql.actions.dataflow.FlowSources
 
-abstract class OutputClobberingSource extends Step { }
+abstract class OutputClobberingSink extends DataFlow::Node { }
 
-class RunOutputClobbering extends OutputClobberingSource, Run {
-  RunOutputClobbering() {
-    exists(UntrustedArtifactDownloadStep download, string script |
-      download.getAFollowingStep() = this and
-      this.getScript() = script and
-      exists(int i |
-        script.splitAt("\n", i).matches(["%GITHUB_OUTPUT%", "%::set-output name%"]) and
-        i < count(string line | line = script.splitAt("\n") | line) - 1
+/**
+ * Holds if a Run step declares an environment variable with contents from a local file.
+ * e.g.
+ *    run: |
+ *      echo "sha=$(cat test-results/sha-number)" >> $GITHUB_OUTPUT
+ *      echo "sha=$(<test-results/sha-number)" >> $GITHUB_OUTPUT
+ */
+class OutputClobberingFromFileReadSink extends OutputClobberingSink {
+  OutputClobberingFromFileReadSink() {
+    exists(Run run, UntrustedArtifactDownloadStep step, string content, string key, string value |
+      this.asExpr() = run.getScriptScalar() and
+      step.getAFollowingStep() = run and
+      writeToGitHubOutput(run, content) and
+      extractVariableAndValue(content, key, value) and
+      // there is a different output variable in the same script
+      // TODO: key2/value2 should be declared before key/value
+      exists(string content2, string key2 |
+        writeToGitHubOutput(run, content2) and
+        extractVariableAndValue(content2, key2, _) and
+        not key2 = key
+      ) and
+      (
+        outputsPartialFileContent(value)
+        or
+        // e.g.
+        // FOO=$(cat test-results/sha-number)
+        // echo "FOO=$FOO" >> $GITHUB_OUTPUT
+        exists(string line, string var_name, string var_value |
+          run.getScript().splitAt("\n") = line
+        |
+          var_name = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 1) and
+          var_value = line.regexpCapture("([a-zA-Z0-9\\-_]+)=(.*)", 2) and
+          outputsPartialFileContent(var_value) and
+          (
+            value.matches("%$" + ["", "{", "ENV{"] + var_name + "%")
+            or
+            value.matches("$(echo %") and value.indexOf(var_name) > 0
+          )
+        )
       )
     )
   }
 }
 
 /**
- * A taint-tracking configuration for unsafe user input
- * that is used to construct and evaluate a code script.
+ * Holds if a Run step declares an environment variable, uses it to declare env var.
+ * e.g.
+ *    env:
+ *      BODY: ${{ github.event.comment.body }}
+ *    run: |
+ *      echo "FOO=$BODY" >> $GITHUB_OUTPUT
  */
-private module OutputClobberingConfig implements DataFlow::ConfigSig {
-  predicate isSource(DataFlow::Node source) { source.asExpr() instanceof OutputClobberingSource }
-
-  predicate isSink(DataFlow::Node sink) { sink instanceof CodeInjectionSink }
-
-  predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) {
-    exists(StepsExpression e |
-      e.getTarget() = prev.asExpr() and
-      prev.asExpr() instanceof OutputClobberingSource and
-      succ.asExpr() = e
+class OutputClobberingFromEnvVarSink extends OutputClobberingSink {
+  OutputClobberingFromEnvVarSink() {
+    exists(Run run, string var_name, string key |
+      envToSpecialFile("GITHUB_OUTPUT", var_name, run, key) and
+      // there is a different output variable in the same script
+      // TODO: key2/value2 should be declared before key/value
+      exists(string content2, string key2 |
+        writeToGitHubOutput(run, content2) and
+        extractVariableAndValue(content2, key2, _) and
+        not key2 = key
+      ) and
+      exists(run.getInScopeEnvVarExpr(var_name)) and
+      run.getScriptScalar() = this.asExpr()
     )
   }
 }
 
-/** Tracks flow of unsafe user input that is used to construct and evaluate a code script. */
+class OutputClobberingFromMaDSink extends OutputClobberingSink {
+  OutputClobberingFromMaDSink() { madSink(this, "output-clobbering") }
+}
+
+/**
+ * A taint-tracking configuration for unsafe user input
+ * that is used to construct and evaluate an environment variable.
+ */
+private module OutputClobberingConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source instanceof RemoteFlowSource and
+    not source.(RemoteFlowSource).getSourceType() = "branch"
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof OutputClobberingSink }
+}
+
+/** Tracks flow of unsafe user input that is used to construct and evaluate an environment variable. */
 module OutputClobberingFlow = TaintTracking::Global<OutputClobberingConfig>;

ql/src/Security/CWE-077/OutputClobberingHigh.ql

@@ -0,0 +1,37 @@
+/**
+ * @name Output Clobbering
+ * @description A Step output can be clobbered which may allow an attacker to manipulate the expected and trusted values of a variable.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 7.3
+ * @precision high
+ * @id actions/output-clobbering/high
+ * @tags actions
+ *       security
+ *       experimental
+ *       external/cwe/cwe-094
+ *       external/cwe/cwe-095
+ *       external/cwe/cwe-116
+ */
+
+import actions
+import codeql.actions.security.OutputClobberingQuery
+import codeql.actions.dataflow.ExternalFlow
+import OutputClobberingFlow::PathGraph
+
+from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink
+where
+  OutputClobberingFlow::flowPath(source, sink) and
+  inPrivilegedContext(sink.getNode().asExpr()) and
+  // exclude paths to file read sinks from non-artifact sources
+  (
+    not source.getNode().(RemoteFlowSource).getSourceType() = "artifact"
+    or
+    source.getNode().(RemoteFlowSource).getSourceType() = "artifact" and
+    (
+      sink.getNode() instanceof OutputClobberingFromFileReadSink or
+      madSink(sink.getNode(), "output-clobbering")
+    )
+  )
+select sink.getNode(), source, sink, "Potential clobbering of a step output in $@.", sink,
+  sink.getNode().toString()

ql/src/Security/CWE-094/OutputClobberingMedium.ql

@@ -1,31 +0,0 @@
-/**
- * @name Output Clobbering
- * @description A Step output can be clobbered which may allow an attacker to manipulate the expected and trusted values of a variable.
- * @kind path-problem
- * @problem.severity warning
- * @security-severity 5.0
- * @precision medium
- * @id actions/output-clobbering/medium
- * @tags actions
- *       security
- *       external/cwe/cwe-094
- *       external/cwe/cwe-095
- *       external/cwe/cwe-116
- */
-
-import actions
-import codeql.actions.security.OutputClobberingQuery
-import OutputClobberingFlow::PathGraph
-
-from OutputClobberingFlow::PathNode source, OutputClobberingFlow::PathNode sink
-where
-  OutputClobberingFlow::flowPath(source, sink) and
-  inPrivilegedContext(sink.getNode().asExpr()) and
-  // exclude cases where the sink is a JS script and the expression uses toJson
-  not exists(UsesStep script |
-    script.getCallee() = "actions/github-script" and
-    script.getArgumentExpr("script") = sink.getNode().asExpr() and
-    exists(getAToJsonReferenceExpression(sink.getNode().asExpr().(Expression).getExpression(), _))
-  )
-select sink.getNode(), source, sink, "Potential output clobbering leading to code injection in $@.",
-  sink, sink.getNode().asExpr().(Expression).getRawExpression()

ql/test/query-tests/Security/CWE-077/.github/workflows/output1.yml

@@ -0,0 +1,38 @@
+on:
+  issue_comment:
+jobs:
+  test1:
+    runs-on: ubuntu-latest
+    steps:
+      - id: clob1
+        env:
+          BODY: ${{ github.event.comment.body }}
+        run: |
+          # VULNERABLE
+          echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT
+          echo "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT
+      - id: clob2
+        run: |
+          echo ${{ steps.clob1.outputs.OUTPUT_1 }}
+          echo ${{ steps.clob1.outputs.OUTPUT_2 }}
+  test2:
+    runs-on: ubuntu-latest
+    steps:
+      - id: clob1
+        env:
+          BODY: ${{ github.event.comment.body }}
+        run: |
+          # NOT VULNERABLE
+          echo "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT
+  test3:
+    runs-on: ubuntu-latest
+    steps:
+      - name: Download artifact
+        uses: dawidd6/action-download-artifact@v6
+        with:
+          run_id: ${{ github.event.workflow_run.id }}
+          name: pr_number 
+      - id: clob1
+        run: |
+          echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT
+          echo "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT

ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.expected

@@ -0,0 +1,12 @@
+edges
+| .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | provenance |  |
+| .github/workflows/output1.yml:30:9:35:6 | Uses Step | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | provenance |  |
+nodes
+| .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | semmle.label | github.event.comment.body |
+| .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | semmle.label | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n |
+| .github/workflows/output1.yml:30:9:35:6 | Uses Step | semmle.label | Uses Step |
+| .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | semmle.label | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n |
+subpaths
+#select
+| .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | .github/workflows/output1.yml:9:18:9:49 | github.event.comment.body | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | Potential clobbering of a step output in $@. | .github/workflows/output1.yml:10:14:13:50 | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n | # VULNERABLE\necho "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$BODY" >> $GITHUB_OUTPUT\n |
+| .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | .github/workflows/output1.yml:30:9:35:6 | Uses Step | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | Potential clobbering of a step output in $@. | .github/workflows/output1.yml:36:14:38:58 | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n | echo "OUTPUT_1=HARDCODED" >> $GITHUB_OUTPUT\necho "OUTPUT_2=$(<pr-number)" >> $GITHUB_OUTPUT\n |

ql/test/query-tests/Security/CWE-077/OutputClobberingHigh.qlref

@@ -0,0 +1 @@
+Security/CWE-077/OutputClobberingHigh.ql