Merge branch 'main' into henrymercer/actions-status

actions/extractor/codeql-extractor.yml

@@ -1,11 +1,12 @@
 name: "actions"
-aliases: []
 display_name: "GitHub Actions"
 version: 0.0.1
 column_kind: "utf16"
 unicode_newlines: true
 build_modes:
   - none
+default_queries:
+  - codeql/actions-queries
 # Actions workflows are not reported separately by the GitHub API, so we can't
 # associate them with a specific language.
 github_api_languages: []

actions/ql/lib/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.4.16
+
+No user-facing changes.
+
 ## 0.4.15
 
 No user-facing changes.

actions/ql/lib/change-notes/released/0.4.16.md

@@ -0,0 +1,3 @@
+## 0.4.16
+
+No user-facing changes.

actions/ql/lib/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.4.15
+lastReleaseVersion: 0.4.16

actions/ql/lib/codeql/Locations.qll

@@ -70,8 +70,8 @@ class Location extends TLocation, TBaseLocation {
 
   /**
    * Holds if this element is at the specified location.
-   * The location spans column `startcolumn` of line `startline` to
-   * column `endcolumn` of line `endline` in file `filepath`.
+   * The location spans column `sc` of line `sl` to
+   * column `ec` of line `el` in file `p`.
    * For more information, see
    * [Providing locations in CodeQL queries](https://codeql.github.com/docs/writing-codeql-queries/providing-locations-in-codeql-queries/).
    */

actions/ql/lib/codeql/actions/Ast.qll

@@ -261,7 +261,7 @@ class If extends AstNode instanceof IfImpl {
 }
 
 /**
- * An Environemnt node representing a deployment environment.
+ * An Environment node representing a deployment environment.
  */
 class Environment extends AstNode instanceof EnvironmentImpl {
   string getName() { result = super.getName() }

actions/ql/lib/codeql/actions/ast/internal/Ast.qll

@@ -125,12 +125,11 @@ abstract class AstNodeImpl extends TAstNode {
    * Gets the enclosing Step.
    */
   StepImpl getEnclosingStep() {
-    if this instanceof StepImpl
-    then result = this
-    else
-      if this instanceof ScalarValueImpl
-      then result.getAChildNode*() = this.getParentNode()
-      else none()
+    this instanceof StepImpl and
+    result = this
+    or
+    this instanceof ScalarValueImpl and
+    result.getAChildNode*() = this.getParentNode()
   }
 
   /**
@@ -1416,9 +1415,8 @@ class ExternalJobImpl extends JobImpl, UsesImpl {
   override string getVersion() {
     exists(YamlString name |
       n.lookup("uses") = name and
-      if not name.getValue().matches("\\.%")
-      then result = name.getValue().regexpCapture(repoUsesParser(), 4)
-      else none()
+      not name.getValue().matches("\\.%") and
+      result = name.getValue().regexpCapture(repoUsesParser(), 4)
     )
   }
 }

actions/ql/lib/codeql/actions/controlflow/BasicBlocks.qll

@@ -286,7 +286,7 @@ private module Cached {
   /**
    * Holds if `cfn` is the `i`th node in basic block `bb`.
    *
-   * In other words, `i` is the shortest distance from a node `bb`
+   * In other words, `i` is the shortest distance from a node `bbStart`
    * that starts a basic block to `cfn` along the `intraBBSucc` relation.
    */
   cached

actions/ql/lib/codeql/actions/controlflow/internal/Cfg.qll

@@ -3,6 +3,8 @@ private import codeql.controlflow.Cfg as CfgShared
 private import codeql.Locations
 
 module Completion {
+  import codeql.controlflow.SuccessorType
+
   private newtype TCompletion =
     TSimpleCompletion() or
     TBooleanCompletion(boolean b) { b in [false, true] } or
@@ -25,7 +27,7 @@ module Completion {
 
     override predicate isValidFor(AstNode e) { not any(Completion c).isValidForSpecific(e) }
 
-    override NormalSuccessor getAMatchingSuccessorType() { any() }
+    override DirectSuccessor getAMatchingSuccessorType() { any() }
   }
 
   class BooleanCompletion extends NormalCompletion, TBooleanCompletion {
@@ -49,34 +51,6 @@ module Completion {
 
     override ReturnSuccessor getAMatchingSuccessorType() { any() }
   }
-
-  cached
-  private newtype TSuccessorType =
-    TNormalSuccessor() or
-    TBooleanSuccessor(boolean b) { b in [false, true] } or
-    TReturnSuccessor()
-
-  class SuccessorType extends TSuccessorType {
-    string toString() { none() }
-  }
-
-  class NormalSuccessor extends SuccessorType, TNormalSuccessor {
-    override string toString() { result = "successor" }
-  }
-
-  class BooleanSuccessor extends SuccessorType, TBooleanSuccessor {
-    boolean value;
-
-    BooleanSuccessor() { this = TBooleanSuccessor(value) }
-
-    override string toString() { result = value.toString() }
-
-    boolean getValue() { result = value }
-  }
-
-  class ReturnSuccessor extends SuccessorType, TReturnSuccessor {
-    override string toString() { result = "return" }
-  }
 }
 
 module CfgScope {
@@ -127,14 +101,8 @@ private module Implementation implements CfgShared::InputSig<Location> {
     last(scope.(CompositeAction), e, c)
   }
 
-  predicate successorTypeIsSimple(SuccessorType t) { t instanceof NormalSuccessor }
-
-  predicate successorTypeIsCondition(SuccessorType t) { t instanceof BooleanSuccessor }
-
   SuccessorType getAMatchingSuccessorType(Completion c) { result = c.getAMatchingSuccessorType() }
 
-  predicate isAbnormalExitType(SuccessorType t) { none() }
-
   int idOfAstNode(AstNode node) { none() }
 
   int idOfCfgScope(CfgScope scope) { none() }

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

@@ -63,10 +63,10 @@ predicate madSource(DataFlow::Node source, string kind, string fieldName) {
     (
       if fieldName.trim().matches("env.%")
       then source.asExpr() = uses.getInScopeEnvVarExpr(fieldName.trim().replaceAll("env.", ""))
-      else
-        if fieldName.trim().matches("output.%")
-        then source.asExpr() = uses
-        else none()
+      else (
+        fieldName.trim().matches("output.%") and
+        source.asExpr() = uses
+      )
     )
   )
 }

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

@@ -31,14 +31,14 @@ abstract class RemoteFlowSource extends SourceNode {
 class GitHubCtxSource extends RemoteFlowSource {
   string flag;
   string event;
-  GitHubExpression e;
 
   GitHubCtxSource() {
-    this.asExpr() = e and
-    // github.head_ref
-    e.getFieldName() = "head_ref" and
-    flag = "branch" and
-    (
+    exists(GitHubExpression e |
+      this.asExpr() = e and
+      // github.head_ref
+      e.getFieldName() = "head_ref" and
+      flag = "branch"
+    |
       event = e.getATriggerEvent().getName() and
       event = "pull_request_target"
       or
@@ -148,7 +148,6 @@ class GhCLICommandSource extends RemoteFlowSource, CommandSource {
 class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
   string cmd;
   string flag;
-  string access_path;
   Run run;
 
   // Examples
@@ -163,7 +162,7 @@ class GitHubEventPathSource extends RemoteFlowSource, CommandSource {
     run.getScript().getACommand() = cmd and
     cmd.matches("jq%") and
     cmd.matches("%GITHUB_EVENT_PATH%") and
-    exists(string regexp |
+    exists(string regexp, string access_path |
       untrustedEventPropertiesDataModel(regexp, flag) and
       not flag = "json" and
       access_path = "github.event" + cmd.regexpCapture(".*\\s+([^\\s]+)\\s+.*", 1) and

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

@@ -19,7 +19,6 @@ abstract class ArgumentInjectionSink extends DataFlow::Node {
  */
 class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
   string command;
-  string argument;
 
   ArgumentInjectionFromEnvVarSink() {
     exists(Run run, string var |
@@ -28,7 +27,7 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
         exists(run.getInScopeEnvVarExpr(var)) or
         var = "GITHUB_HEAD_REF"
       ) and
-      run.getScript().getAnEnvReachingArgumentInjectionSink(var, command, argument)
+      run.getScript().getAnEnvReachingArgumentInjectionSink(var, command, _)
     )
   }
 
@@ -44,13 +43,12 @@ class ArgumentInjectionFromEnvVarSink extends ArgumentInjectionSink {
  */
 class ArgumentInjectionFromCommandSink extends ArgumentInjectionSink {
   string command;
-  string argument;
 
   ArgumentInjectionFromCommandSink() {
     exists(CommandSource source, Run run |
       run = source.getEnclosingRun() and
       this.asExpr() = run.getScript() and
-      run.getScript().getACmdReachingArgumentInjectionSink(source.getCommand(), command, argument)
+      run.getScript().getACmdReachingArgumentInjectionSink(source.getCommand(), command, _)
     )
   }
 

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

@@ -125,8 +125,6 @@ class LegitLabsDownloadArtifactActionStep extends UntrustedArtifactDownloadStep,
 }
 
 class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, UsesStep {
-  string script;
-
   ActionsGitHubScriptDownloadStep() {
     // eg:
     // - uses: actions/github-script@v6
@@ -149,12 +147,14 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use
     //      var fs = require('fs');
     //      fs.writeFileSync('${{github.workspace}}/test-results.zip', Buffer.from(download.data));
     this.getCallee() = "actions/github-script" and
-    this.getArgument("script") = script and
-    script.matches("%listWorkflowRunArtifacts(%") and
-    script.matches("%downloadArtifact(%") and
-    script.matches("%writeFileSync(%") and
-    // Filter out artifacts that were created by pull-request.
-    not script.matches("%exclude_pull_requests: true%")
+    exists(string script |
+      this.getArgument("script") = script and
+      script.matches("%listWorkflowRunArtifacts(%") and
+      script.matches("%downloadArtifact(%") and
+      script.matches("%writeFileSync(%") and
+      // Filter out artifacts that were created by pull-request.
+      not script.matches("%exclude_pull_requests: true%")
+    )
   }
 
   override string getPath() {
@@ -171,10 +171,10 @@ class ActionsGitHubScriptDownloadStep extends UntrustedArtifactDownloadStep, Use
                 .getScript()
                 .getACommand()
                 .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3)))
-    else
-      if this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp())
-      then result = "GITHUB_WORKSPACE/"
-      else none()
+    else (
+      this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp()) and
+      result = "GITHUB_WORKSPACE/"
+    )
   }
 }
 
@@ -207,12 +207,13 @@ class GHRunArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
                 .getScript()
                 .getACommand()
                 .regexpCapture(unzipRegexp() + unzipDirArgRegexp(), 3)))
-    else
-      if
+    else (
+      (
         this.getAFollowingStep().(Run).getScript().getACommand().regexpMatch(unzipRegexp()) or
         this.getScript().getACommand().regexpMatch(unzipRegexp())
-      then result = "GITHUB_WORKSPACE/"
-      else none()
+      ) and
+      result = "GITHUB_WORKSPACE/"
+    )
   }
 }
 
@@ -259,15 +260,15 @@ class DirectArtifactDownloadStep extends UntrustedArtifactDownloadStep, Run {
 
 class ArtifactPoisoningSink extends DataFlow::Node {
   UntrustedArtifactDownloadStep download;
-  PoisonableStep poisonable;
 
   ArtifactPoisoningSink() {
-    download.getAFollowingStep() = poisonable and
-    // excluding artifacts downloaded to the temporary directory
-    not download.getPath().regexpMatch("^/tmp.*") and
-    not download.getPath().regexpMatch("^\\$\\{\\{\\s*runner\\.temp\\s*}}.*") and
-    not download.getPath().regexpMatch("^\\$RUNNER_TEMP.*") and
-    (
+    exists(PoisonableStep poisonable |
+      download.getAFollowingStep() = poisonable and
+      // excluding artifacts downloaded to the temporary directory
+      not download.getPath().regexpMatch("^/tmp.*") and
+      not download.getPath().regexpMatch("^\\$\\{\\{\\s*runner\\.temp\\s*}}.*") and
+      not download.getPath().regexpMatch("^\\$RUNNER_TEMP.*")
+    |
       poisonable.(Run).getScript() = this.asExpr() and
       (
         // Check if the poisonable step is a local script execution step

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

@@ -159,11 +159,8 @@ abstract class CommentVsHeadDateCheck extends ControlCheck {
 
 /* Specific implementations of control checks */
 class LabelIfCheck extends LabelCheck instanceof If {
-  string condition;
-
   LabelIfCheck() {
-    condition = normalizeExpr(this.getCondition()) and
-    (
+    exists(string condition | condition = normalizeExpr(this.getCondition()) |
       // eg: contains(github.event.pull_request.labels.*.name, 'safe to test')
       condition.regexpMatch(".*(^|[^!])contains\\(\\s*github\\.event\\.pull_request\\.labels\\b.*")
       or

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

@@ -55,12 +55,8 @@ class EnvVarInjectionFromFileReadSink extends EnvVarInjectionSink {
  *          echo "COMMIT_MESSAGE=${COMMIT_MESSAGE}" >> $GITHUB_ENV
  */
 class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink {
-  CommandSource inCommand;
-  string injectedVar;
-  string command;
-
   EnvVarInjectionFromCommandSink() {
-    exists(Run run |
+    exists(Run run, CommandSource inCommand, string injectedVar, string command |
       this.asExpr() = inCommand.getEnclosingRun().getScript() and
       run = inCommand.getEnclosingRun() and
       run.getScript().getACmdReachingGitHubEnvWrite(inCommand.getCommand(), injectedVar) and
@@ -86,12 +82,8 @@ class EnvVarInjectionFromCommandSink extends EnvVarInjectionSink {
  *      echo "FOO=$BODY" >> $GITHUB_ENV
  */
 class EnvVarInjectionFromEnvVarSink extends EnvVarInjectionSink {
-  string inVar;
-  string injectedVar;
-  string command;
-
   EnvVarInjectionFromEnvVarSink() {
-    exists(Run run |
+    exists(Run run, string inVar, string injectedVar, string command |
       run.getScript() = this.asExpr() and
       exists(run.getInScopeEnvVarExpr(inVar)) and
       run.getScript().getAnEnvReachingGitHubEnvWrite(inVar, injectedVar) and

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

@@ -99,18 +99,14 @@ class OutputClobberingFromEnvVarSink extends OutputClobberingSink {
  *          echo $BODY
  */
 class WorkflowCommandClobberingFromEnvVarSink extends OutputClobberingSink {
-  string clobbering_var;
-  string clobbered_value;
-
   WorkflowCommandClobberingFromEnvVarSink() {
-    exists(Run run, string workflow_cmd_stmt, string clobbering_stmt |
+    exists(Run run, string workflow_cmd_stmt, string clobbering_stmt, string clobbering_var |
       run.getScript() = this.asExpr() and
       run.getScript().getAStmt() = clobbering_stmt and
       clobbering_stmt.regexpMatch("echo\\s+(-e\\s+)?(\"|')?\\$(\\{)?" + clobbering_var + ".*") and
       exists(run.getInScopeEnvVarExpr(clobbering_var)) and
       run.getScript().getAStmt() = workflow_cmd_stmt and
-      clobbered_value =
-        trimQuotes(workflow_cmd_stmt.regexpCapture(".*::set-output\\s+name=.*::(.*)", 1))
+      exists(trimQuotes(workflow_cmd_stmt.regexpCapture(".*::set-output\\s+name=.*::(.*)", 1)))
     )
   }
 }

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

@@ -1,10 +1,8 @@
 import actions
 
 class UnversionedImmutableAction extends UsesStep {
-  string immutable_action;
-
   UnversionedImmutableAction() {
-    isImmutableAction(this, immutable_action) and
+    isImmutableAction(this, _) and
     not isSemVer(this.getVersion())
   }
 }

actions/ql/lib/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/actions-all
-version: 0.4.16-dev
+version: 0.4.17-dev
 library: true
 warnOnImplicitThis: true
 dependencies:

actions/ql/src/CHANGELOG.md

@@ -1,3 +1,7 @@
+## 0.6.8
+
+No user-facing changes.
+
 ## 0.6.7
 
 No user-facing changes.

actions/ql/src/Security/CWE-829/UntrustedCheckoutCritical.md

@@ -32,7 +32,7 @@ jobs:
 
       - uses: actions/setup-node@v1
       - run: |
-          npm install # scripts in package.json from PR would be executed here
+          npm install # scripts in package.json from PR would be executed here
           npm build
 
       - uses: completely/fakeaction@v2

actions/ql/src/Security/CWE-829/UntrustedCheckoutHigh.md

@@ -32,7 +32,7 @@ jobs:
 
       - uses: actions/setup-node@v1
       - run: |
-          npm install # scripts in package.json from PR would be executed here
+          npm install # scripts in package.json from PR would be executed here
           npm build
 
       - uses: completely/fakeaction@v2

actions/ql/src/Security/CWE-829/UntrustedCheckoutMedium.md

@@ -32,7 +32,7 @@ jobs:
 
       - uses: actions/setup-node@v1
       - run: |
-          npm install # scripts in package.json from PR would be executed here
+          npm install # scripts in package.json from PR would be executed here
           npm build
 
       - uses: completely/fakeaction@v2

actions/ql/src/change-notes/released/0.6.8.md

@@ -0,0 +1,3 @@
+## 0.6.8
+
+No user-facing changes.

actions/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.6.7
+lastReleaseVersion: 0.6.8

actions/ql/src/experimental/Security/CWE-829/ArtifactPoisoningPathTraversal.ql

@@ -37,8 +37,6 @@ where
     )
     or
     // upload artifact is not used in the same workflow
-    not exists(UsesStep upload |
-      download.getEnclosingWorkflow().getAJob().(LocalJob).getAStep() = upload
-    )
+    not download.getEnclosingWorkflow().getAJob().(LocalJob).getAStep() instanceof UsesStep
   )
 select download, "Potential artifact poisoning"

actions/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/actions-queries
-version: 0.6.8-dev
+version: 0.6.9-dev
 library: false
 warnOnImplicitThis: true
 groups: [actions, queries]