Merge pull request #29 from GitHubSecurityLab/clean

Alvaro Muñoz · web-flow · commit aa62603899dc · 2024-03-13T13:50:11.000+01:00
fix: clean debug lefovers
diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
@@ -121,7 +121,6 @@ class Outputs extends AstNode instanceof OutputsImpl {
 
   Expression getOutputExpr(string outputName) { result = super.getOutputExpr(outputName) }
 
-  // TODO: REMOVE
   override string toString() { result = "Job outputs node" }
 }
 
diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -252,11 +252,6 @@ class CompositeActionImpl extends AstNodeImpl, TCompositeAction {
 
   override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
 
-  // override AstNodeImpl getAChildNode() {
-  //   result = this.getInputs() or
-  //   result = this.getOutputs() or
-  //   result = this.getRuns()
-  // }
   override AstNodeImpl getParentNode() { none() }
 
   override string getAPrimaryQlClass() { result = "CompositeActionImpl" }
@@ -292,12 +287,6 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
 
   override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
 
-  // override AstNodeImpl getAChildNode() {
-  //   result = this.getAJob() or
-  //   result = this.getStrategy() or
-  //   result = this.getEnv() or
-  //   result = this.getPermissions()
-  // }
   override AstNodeImpl getParentNode() { none() }
 
   override string getAPrimaryQlClass() { result = "WorkflowImpl" }
@@ -306,8 +295,6 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode {
 
   override YamlMapping getNode() { result = n }
 
-  // /** Gets the `jobs` mapping from job IDs to job definitions in this workflow. */
-  // YamlMapping getJobs() { result = this.asYamlMapping().lookup("jobs") }
   /** Gets the 'global' `env` mapping in this workflow. */
   EnvImpl getEnv() { result.getNode() = n.lookup("env") }
 
@@ -346,11 +333,6 @@ class ReusableWorkflowImpl extends AstNodeImpl, WorkflowImpl {
 
   override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
 
-  // override AstNodeImpl getAChildNode() {
-  //   result = super.getAChildNode() or
-  //   result = this.getInputs() or
-  //   result = this.getOutputs()
-  // }
   OutputsImpl getOutputs() { result.getNode() = workflow_call.(YamlMapping).lookup("outputs") }
 
   ExpressionImpl getAnOutputExpr() { result = this.getOutputs().getAnOutputExpr() }
@@ -378,7 +360,6 @@ class RunsImpl extends AstNodeImpl, TRunsNode {
 
   override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
 
-  //override AstNodeImpl getAChildNode() { result = this.getAStep() }
   override CompositeActionImpl getParentNode() { result.getAChildNode() = this }
 
   override string getAPrimaryQlClass() { result = "RunsImpl" }
@@ -450,7 +431,6 @@ class OutputsImpl extends AstNodeImpl, TOutputsNode {
 
   override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
 
-  //override AstNodeImpl getAChildNode() { result = this.getAnOutputExpr() }
   override AstNodeImpl getParentNode() { result.getAChildNode() = this }
 
   override string getAPrimaryQlClass() { result = "OutputsImpl" }
@@ -503,7 +483,6 @@ class StrategyImpl extends AstNodeImpl, TStrategyNode {
 
   override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
 
-  //override ExpressionImpl getAChildNode() { result = this.getAMatrixVarExpr() }
   override AstNodeImpl getParentNode() { result.getAChildNode() = this }
 
   override string getAPrimaryQlClass() { result = "StrategyImpl" }
@@ -557,10 +536,8 @@ class JobImpl extends AstNodeImpl, TJobNode {
     workflow.getNode().lookup("jobs").(YamlMapping).lookup(jobId) = n
   }
 
-  // TODO: REMOVE
   override string toString() { result = "Job: " + jobId }
 
-  //override string toString() { result = n.toString() }
   override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
 
   override WorkflowImpl getParentNode() { result.getAChildNode() = this }
@@ -739,7 +716,6 @@ class UsesStepImpl extends StepImpl, UsesImpl {
     result.getParentNode().getNode() = n.lookup("with").(YamlMapping).lookup(key)
   }
 
-  // TODO: REMOVE
   override string toString() {
     if exists(this.getId()) then result = "Uses Step: " + this.getId() else result = "Uses Step"
   }
@@ -760,7 +736,6 @@ class ExternalJobImpl extends JobImpl, UsesImpl {
 
   ExternalJobImpl() { n.lookup("uses") = u }
 
-  //override AstNodeImpl getAChildNode() { result.getNode() = n.getAChildNode*() }
   override string getCallee() {
     if u.getValue().matches("./%")
     then result = u.getValue().regexpCapture(pathUsesParser(), 1)
@@ -796,7 +771,6 @@ class RunImpl extends StepImpl {
 
   ExpressionImpl getAnScriptExpr() { result.getParentNode().getNode() = script }
 
-  // TODO: REMOVE
   override string toString() {
     if exists(this.getId()) then result = "Run Step: " + this.getId() else result = "Run Step"
   }
@@ -807,14 +781,6 @@ class RunImpl extends StepImpl {
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
  */
 abstract class ContextExpressionImpl extends ExpressionImpl {
-  // TODO: REMOVE
-  // ContextExpressionImpl() {
-  //   expression
-  //       .regexpMatch([
-  //           stepsCtxRegex(), needsCtxRegex(), jobsCtxRegex(), envCtxRegex(), inputsCtxRegex(),
-  //           matrixCtxRegex()
-  //         ])
-  // }
   abstract string getFieldName();
 
   abstract AstNodeImpl getTarget();
diff --git a/ql/lib/codeql/actions/dataflow/FlowSources.qll b/ql/lib/codeql/actions/dataflow/FlowSources.qll
@@ -96,6 +96,7 @@ private predicate isExternalUserControlledWorkflowRun(string context) {
   exists(string reg |
     reg =
       [
+        "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow\\s*\\.\\s*path\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_branch\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*display_title\\b",
         "\\bgithub\\s*\\.\\s*event\\s*\\.\\s*workflow_run\\s*\\.\\s*head_repository\\b\\s*\\.\\s*description\\b",
diff --git a/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll b/ql/lib/codeql/actions/dataflow/internal/DataFlowPrivate.qll
@@ -72,7 +72,6 @@ class DataFlowCall instanceof Cfg::Node {
   /** Gets a textual representation of this element. */
   string toString() { result = super.toString() }
 
-  //Location getLocation() { result = super.getLocation() }
   string getName() { result = super.getAstNode().(Uses).getCallee() }
 
   DataFlowCallable getEnclosingCallable() { result = super.getScope() }
@@ -84,7 +83,6 @@ class DataFlowCall instanceof Cfg::Node {
 class DataFlowCallable instanceof Cfg::CfgScope {
   string toString() { result = super.toString() }
 
-  //Location getLocation() { result = super.getLocation() }
   string getName() {
     if this instanceof ReusableWorkflow
     then result = this.(ReusableWorkflow).getLocation().getFile().getRelativePath()
diff --git a/ql/src/Security/CWE-094/CriticalExpressionInjection.ql b/ql/src/Security/CWE-094/CriticalExpressionInjection.ql
@@ -34,14 +34,14 @@ module MyFlow = TaintTracking::Global<MyConfig>;
 
 import MyFlow::PathGraph
 
-from MyFlow::PathNode source, MyFlow::PathNode sink
+from MyFlow::PathNode source, MyFlow::PathNode sink, Workflow w
 where
   MyFlow::flowPath(source, sink) and
-  source
-      .getNode()
-      .asExpr()
-      .getEnclosingWorkflow()
-      .hasTriggerEvent(source.getNode().(RemoteFlowSource).getATriggerEvent())
+  w = source.getNode().asExpr().getEnclosingWorkflow() and
+  (
+    w instanceof ReusableWorkflow or
+    w.hasTriggerEvent(source.getNode().(RemoteFlowSource).getATriggerEvent())
+  )
 select sink.getNode(), source, sink,
   "Potential expression injection in $@, which may be controlled by an external user.", sink,
   sink.getNode().asExpr().(Expression).getExpression()
diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog.yml
@@ -0,0 +1,100 @@
+name: changelog
+
+on:
+  workflow_call:
+    inputs:
+      create:
+        description: Add a log to the changelog
+        type: boolean
+        required: false
+        default: false
+      update:
+        description: Update the existing changelog
+        type: boolean
+        required: false
+        default: false
+
+jobs:
+  changelog:
+    runs-on: ubuntu-latest
+    env:
+      file: CHANGELOG.md
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          fetch-depth: 0
+      - name: Check ${{ env.file }}
+        run: |
+          if [[ $(git diff --name-only origin/master HEAD -- ${{ env.file }} | grep '^${{ env.file }}$' -c) -eq 0 ]]; then
+            echo "Expected '${{ env.file }}' to be modified"
+            exit 1
+          fi
+  update:
+    runs-on: ubuntu-latest
+    needs: changelog
+    if: (inputs.create && failure()) || (inputs.update && success())
+    continue-on-error: true
+    env:
+      file: CHANGELOG.md
+      next_version: next
+      link: '[#${{ github.event.number }}](https://github.com/fabricjs/fabric.js/pull/${{ github.event.number }})'
+    steps:
+      - uses: actions/checkout@v3
+        with:
+          ref: ${{ github.event.pull_request.head.ref }}
+      - name: Update ${{ env.file }} from PR title
+        id: update
+        uses: actions/github-script@v6
+        env:
+          log: '- ${{ github.event.pull_request.title }} ${{ env.link }}\n'
+          prev_log: '- ${{ github.event.changes.title.from }} ${{ env.link }}\n'
+        with:
+          result-encoding: string
+          script: |
+            const fs = require('fs');
+            const file = './${{ env.file }}';
+            let content = fs.readFileSync(file).toString();
+            const title = '[${{ env.next_version }}]';
+            const log = '${{ env.log }}';
+            let exists = ${{ needs.changelog.result == 'success' }};
+
+            if (!content.includes(title)) {
+            const insertAt = content.indexOf('\n') + 1;
+            content =
+                content.slice(0, insertAt) +
+                `\n## ${title}\n\n\n` +
+                content.slice(insertAt);
+            }
+
+            const insertAt = content.indexOf('\n', content.indexOf(title) + title.length + 1) + 1;
+            if (exists && ${{ github.event.action == 'edited' }}) {
+                const prevLog = '${{ env.prev_log }}';
+                const index = content.indexOf(prevLog, insertAt);
+                if (index > -1) {
+                    content = content.slice(0, index) + content.slice(index + prevLog.length);
+                    exists = false;
+                }
+            }
+
+            if (!exists) {
+                content = content.slice(0, insertAt) + log + content.slice(insertAt);
+                fs.writeFileSync(file, content);
+                return true;
+            }
+
+            return false;
+      - name: Setup node
+        if: fromJson(steps.update.outputs.result)
+        uses: actions/setup-node@v3
+        with:
+          node-version: 18.x
+      - name: Commit & Push
+        if: fromJson(steps.update.outputs.result)
+        run: |
+          npm ci
+          npx prettier --write ${{ env.file }}
+          git config user.name github-actions[bot]
+          git config user.email github-actions[bot]@users.noreply.github.com
+          git add ${{ env.file }}
+          git commit -m "update ${{ env.file }}"
+          git push
diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog_required.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/changelog_required.yml
@@ -0,0 +1,9 @@
+name: '📋'
+
+on:
+  pull_request:
+    branches: [master]
+
+jobs:
+  changelog:
+    uses: ./.github/workflows/changelog.yml

Original file line number	Diff line number	Diff line change
`@@ -121,7 +121,6 @@ class Outputs extends AstNode instanceof OutputsImpl {`
`121`	`121`
`122`	`122`	`Expression getOutputExpr(string outputName) { result = super.getOutputExpr(outputName) }`
`123`	`123`
`124`		`- // TODO: REMOVE`
`125`	`124`	`override string toString() { result = "Job outputs node" }`
`126`	`125`	`}`
`127`	`126`
Original file line number	Diff line number	Diff line change
`@@ -96,6 +96,7 @@ private predicate isExternalUserControlledWorkflowRun(string context) {`
`96`	`96`	`exists(string reg \|`
`97`	`97`	`reg =`
`98`	`98`	`[`
	`99`	`+ "\\bgithub\\s\\.\\sevent\\s\\.\\sworkflow\\s\\.\\spath\\b",`
`99`	`100`	`"\\bgithub\\s\\.\\sevent\\s\\.\\sworkflow_run\\s\\.\\shead_branch\\b",`
`100`	`101`	`"\\bgithub\\s\\.\\sevent\\s\\.\\sworkflow_run\\s\\.\\sdisplay_title\\b",`
`101`	`102`	`"\\bgithub\\s\\.\\sevent\\s\\.\\sworkflow_run\\s\\.\\shead_repository\\b\\s\\.\\sdescription\\b",`