Merge pull request #1 from hvitved/nodeselector-actions-lockfile-aware-pinning

Code review suggestions

Author: nodeselector

actions/ql/lib/codeql/actions/Lock.qll

@@ -2,35 +2,21 @@
  * Provides classes for working with GitHub Actions lockfiles.
  */
 
+private import actions
 private import codeql.actions.ast.internal.Yaml
 
-/** An `actions.lock` file. */
-class ActionsLock extends YamlDocument {
-  ActionsLock() { this.getFile().getBaseName() = "actions.lock" }
+/** A `.github/workflows/actions.lock` file. */
+class ActionsLock extends YamlDocument, YamlMapping {
+  ActionsLock() { this.getFile().getRelativePath() = ".github/workflows/actions.lock" }
 
-  /**
-   * Holds if the lockfile records `nwo` at `ref` for `workflowPath` with a full commit digest.
-   * Repository pins also cover sub-actions such as `actions/cache/save`.
-   */
-  bindingset[nwo]
-  predicate pins(string workflowPath, string nwo, string ref) {
-    this.getFile().getRelativePath() = ".github/workflows/actions.lock" and
-    exists(
-      YamlMapping root, YamlSequence workflowPins, YamlScalar pinNode, YamlMapping dependency,
-      string pin, string pinnedNwo
-    |
-      root = this and
-      root.lookup("workflows").(YamlMapping).lookup(workflowPath) = workflowPins and
+  private predicate pins0(string workflowPath, string pinnedNwo, string ref) {
+    exists(YamlSequence workflowPins, YamlScalar pinNode, YamlMapping dependency, string pin |
+      this.lookup("workflows").(YamlMapping).lookup(workflowPath) = workflowPins and
       workflowPins.getElement(_) = pinNode and
       pin = pinNode.getValue() and
       pinnedNwo = pin.regexpCapture("^([^/@:]+/[^/@:]+)@([^:]+)$", 1) and
       ref = pin.regexpCapture("^([^/@:]+/[^/@:]+)@([^:]+)$", 2) and
-      (
-        nwo.toLowerCase() = pinnedNwo.toLowerCase()
-        or
-        nwo.toLowerCase().prefix(pinnedNwo.length() + 1) = pinnedNwo.toLowerCase() + "/"
-      ) and
-      root.lookup("dependencies").(YamlMapping).lookup(pin) = dependency and
+      this.lookup("dependencies").(YamlMapping).lookup(pin) = dependency and
       dependency.lookup("ref").(YamlScalar).getValue() = ref and
       dependency
           .lookup("commit")
@@ -39,4 +25,20 @@ class ActionsLock extends YamlDocument {
           .regexpMatch("^(sha1-[A-Fa-f0-9]{40}|sha256-[A-Fa-f0-9]{64})$")
     )
   }
+
+  /**
+   * Holds if this lockfile pins the use at `uses` to `ref` with a full commit digest.
+   * Repository pins also cover sub-actions such as `actions/cache/save`.
+   */
+  predicate pins(UsesStep uses, string ref) {
+    exists(string workflowPath, string pinnedNwo, string nwo |
+      this.pins0(workflowPath, pinnedNwo, ref) and
+      workflowPath = uses.getLocation().getFile().getRelativePath() and
+      nwo = uses.getCallee()
+    |
+      nwo.toLowerCase() = pinnedNwo.toLowerCase()
+      or
+      nwo.toLowerCase().prefix(pinnedNwo.length() + 1) = pinnedNwo.toLowerCase() + "/"
+    )
+  }
 }

actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql

@@ -62,12 +62,7 @@ where
   uses.getVersion() = version and
   not isTrustedOwner(nwo) and
   not isSelfRepository(nwo) and
-  not exists(UsesStep step |
-    uses = step and
-    exists(ActionsLock lock |
-      lock.pins(step.getLocation().getFile().getRelativePath(), nwo, version)
-    )
-  ) and
+  not any(ActionsLock lock).pins(uses, version) and
   not (
     if uses instanceof UsesStep and isContainerImage(nwo)
     then isPinnedContainer(version)