Merge pull request #28 from github/feat/matrix_expressions

Resolve Matrix expression to their possible values
2026-05-01 19:55:15 +02:00 · 2024-05-13 16:25:52 +02:00
parent b2d7c823b3 cee0389d6e
commit 54d103ffe4
5 changed files with 197 additions and 51 deletions
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -544,9 +544,15 @@ class StrategyImpl extends AstNodeImpl, TStrategyNode {

  override YamlMapping getNode() { result = n }

-  /** Gets a specific matric expression (YamlMapping) by name. */
-  ExpressionImpl getMatrixVarExpr(string name) {
-    n.lookup("matrix").(YamlMapping).lookup(name) = result.getNode()
+  YamlMapping getMatrix() { result = n.lookup("matrix") }
+
+  /** Gets a specific matrix expression (YamlMapping) by name. */
+  ExpressionImpl getMatrixVarExpr(string accessPath) {
+    exists(MatrixAccessPathImpl p, ScalarValueImpl v |
+      p.toString() = accessPath and
+      resolveMatrixAccessPath(n.lookup("matrix"), p).getNode(_) = v.getNode() and
+      result.getParentNode() = v
+    )
  }

  /** Gets a specific matric expression (YamlMapping) by name. */
@@ -777,14 +783,27 @@ class JobImpl extends AstNodeImpl, TJobNode {

  /** Gets the runs-on field of the job. */
  string getARunsOnLabel() {
-    exists(string lbl, YamlNode r |
+    exists(ScalarValueImpl lbl |
      (
-        r = runson.getNode(lbl) and
-        not lbl = ["group", "labels"]
+        lbl.getNode() = runson.getNode(_) and
+        not lbl.getNode() = runson.getNode("group")
        or
-        r = runson.getNode("labels").(YamlMappingLikeNode).getNode(lbl)
+        lbl.getNode() = runson.getNode("labels").(YamlMappingLikeNode).getNode(_)
      ) and
-      result = lbl.trim().regexpReplaceAll("^('|\")", "").regexpReplaceAll("('|\")$", "").trim()
+      (
+        not exists(MatrixExpressionImpl e | e.getParentNode() = lbl) and
+        result =
+          lbl.getValue()
+              .trim()
+              .regexpReplaceAll("^('|\")", "")
+              .regexpReplaceAll("('|\")$", "")
+              .trim()
+        or
+        exists(MatrixExpressionImpl e |
+          e.getParentNode() = lbl and
+          result = e.getLiteralValues()
+        )
+      )
    )
  }
 }
@@ -1050,7 +1069,7 @@ private string jobsCtxRegex() {

 private string envCtxRegex() { result = Utils::wrapRegexp("env\\.([A-Za-z0-9_-]+)") }

-private string matrixCtxRegex() { result = Utils::wrapRegexp("matrix\\.([A-Za-z0-9_-]+)") }
+private string matrixCtxRegex() { result = Utils::wrapRegexp("matrix\\.(.+)") }

 private string inputsCtxRegex() {
  result =
@@ -1224,24 +1243,65 @@ class EnvExpressionImpl extends SimpleReferenceExpressionImpl {
 * e.g. `${{ matrix.foo }}`
 */
 class MatrixExpressionImpl extends SimpleReferenceExpressionImpl {
-  string fieldName;
+  string fieldAccess;

  MatrixExpressionImpl() {
    Utils::normalizeExpr(expression).regexpMatch(matrixCtxRegex()) and
-    fieldName = Utils::normalizeExpr(expression).regexpCapture(matrixCtxRegex(), 1)
+    fieldAccess = Utils::normalizeExpr(expression).regexpCapture(matrixCtxRegex(), 1)
  }

-  override string getFieldName() { result = fieldName }
+  override string getFieldName() { result = fieldAccess }

  override AstNodeImpl getTarget() {
-    exists(WorkflowImpl w |
-      w.getStrategy().getMatrixVarExpr(fieldName) = result and
-      w.getAChildNode*() = this
-    )
-    or
-    exists(JobImpl j |
-      j.getStrategy().getMatrixVarExpr(fieldName) = result and
-      j.getAChildNode*() = this
+    result = this.getEnclosingWorkflow().getStrategy().getMatrixVarExpr(fieldAccess) or
+    result = this.getEnclosingJob().getStrategy().getMatrixVarExpr(fieldAccess)
+  }
+
+  string getLiteralValues() {
+    exists(StrategyImpl s, MatrixAccessPathImpl p, ScalarValueImpl v |
+      (s = this.getEnclosingJob().getStrategy() or s = this.getEnclosingWorkflow().getStrategy()) and
+      p.toString() = fieldAccess and
+      resolveMatrixAccessPath(s.getMatrix(), p).getNode(_) = v.getNode() and
+      // Exclude values containing matrix expressions to avoid recursion
+      not exists(MatrixExpressionImpl e | e.getParentNode() = v) and
+      result = v.getValue()
    )
  }
 }
+
+bindingset[accessPath]
+string explodeAccessPath(string accessPath) {
+  result = accessPath or
+  result = accessPath.suffix(accessPath.indexOf(".") + 1) or
+  result = accessPath.prefix(accessPath.indexOf("."))
+}
+
+private newtype TAccessPath =
+  TMatrixAccessPathNode(string accessPath) {
+    exists(MatrixExpressionImpl e | accessPath = explodeAccessPath(e.getFieldName()))
+  }
+
+class MatrixAccessPathImpl extends TMatrixAccessPathNode {
+  string accessPath;
+
+  MatrixAccessPathImpl() { this = TMatrixAccessPathNode(accessPath) }
+
+  string toString() { result = accessPath }
+}
+
+private YamlMappingLikeNode resolveMatrixAccessPath(
+  YamlMappingLikeNode root, MatrixAccessPathImpl accessPath
+) {
+  // access path contains no dots. eg: "os"
+  result = root.getNode(accessPath.toString())
+  or
+  // access path contains dots. eg: "plaform.os"
+  exists(MatrixAccessPathImpl first, MatrixAccessPathImpl rest, YamlMappingLikeNode newRoot |
+    first.toString() = accessPath.toString().splitAt(".", 0) and
+    rest.toString() = accessPath.toString().suffix(first.toString().length() + 1) and
+    newRoot = root.getNode(first.toString()) and
+    if newRoot instanceof YamlSequence
+    then result = resolveMatrixAccessPath(newRoot.(YamlSequence).getElementNode(_), rest)
+    else result = resolveMatrixAccessPath(newRoot, rest)
+  )
+}
--- a/ql/lib/codeql/actions/security/SelfHostedQuery.qll
+++ b/ql/lib/codeql/actions/security/SelfHostedQuery.qll
@@ -0,0 +1,45 @@
+import actions
+import codeql.actions.dataflow.ExternalFlow
+
+bindingset[runner]
+predicate isGithubHostedRunner(string runner) {
+  // list of github hosted repos: https://github.com/actions/runner-images/blob/main/README.md#available-images
+  runner
+      .toLowerCase()
+      .regexpMatch("^(ubuntu-([0-9.]+|latest)|macos-([0-9]+|latest)(-x?large)?|windows-([0-9.]+|latest))$")
+}
+
+bindingset[runner]
+predicate is3rdPartyHostedRunner(string runner) {
+  runner.toLowerCase().regexpMatch("^(buildjet|warp)-[a-z0-9-]+$")
+}
+
+/**
+ * This predicate uses data available in the workflow file to identify self-hosted runners.
+ * It does not know if the repository is public or private.
+ * It is a best-effort approach to identify self-hosted runners.
+ */
+predicate staticallyIdentifiedSelfHostedRunner(Job job) {
+  exists(string label |
+    job.getATriggerEvent().getName() =
+      [
+        "issue_comment", "pull_request", "pull_request_review", "pull_request_review_comment",
+        "pull_request_target", "workflow_run"
+      ] and
+    label = job.getARunsOnLabel() and
+    not isGithubHostedRunner(label) and
+    not is3rdPartyHostedRunner(label)
+  )
+}
+
+/**
+ * This predicate uses data available in the job log files to identify self-hosted runners.
+ * It is a best-effort approach to identify self-hosted runners.
+ */
+predicate dynamicallyIdentifiedSelfHostedRunner(Job job) {
+  exists(string runner_info |
+    workflowDataModel(job.getEnclosingWorkflow().getLocation().getFile().getRelativePath(),
+      "public", job.getId(), _, _, runner_info) and
+    runner_info.indexOf("self-hosted:true") > 0
+  )
+}
--- a/ql/src/Security/CWE-284/CodeExecutionOnSelfHostedRunner.ql
+++ b/ql/src/Security/CWE-284/CodeExecutionOnSelfHostedRunner.ql
@@ -11,36 +11,7 @@
 *       external/cwe/cwe-284
 */

-import actions
-import codeql.actions.dataflow.ExternalFlow
-
-/**
- * This predicate uses data available in the workflow file to identify self-hosted runners.
- * It does not know if the repository is public or private.
- * It is a best-effort approach to identify self-hosted runners.
- */
-predicate staticallyIdentifiedSelfHostedRunner(Job job) {
-  exists(string label |
-    job.getEnclosingWorkflow().getATriggerEvent().getName() =
-      ["pull_request", "pull_request_review", "pull_request_review_comment", "pull_request_target"] and
-    label = job.getARunsOnLabel() and
-    // source: https://github.com/boostsecurityio/poutine/blob/main/opa/rego/poutine/utils.rego#L49C3-L49C136
-    not label
-        .regexpMatch("(?i)^((ubuntu-(([0-9]{2})\\.04|latest)|macos-([0-9]{2}|latest)(-x?large)?|windows-(20[0-9]{2}|latest)|(buildjet|warp)-[a-z0-9-]+))$")
-  )
-}
-
-/**
- * This predicate uses data available in the job log files to identify self-hosted runners.
- * It is a best-effort approach to identify self-hosted runners.
- */
-predicate dynamicallyIdentifiedSelfHostedRunner(Job job) {
-  exists(string runner_info |
-    workflowDataModel(job.getEnclosingWorkflow().getLocation().getFile().getRelativePath(),
-      "public", job.getId(), _, _, runner_info) and
-    runner_info.matches("self-hosted:true")
-  )
-}
+import codeql.actions.security.SelfHostedQuery

 from Job job
 where staticallyIdentifiedSelfHostedRunner(job) or dynamicallyIdentifiedSelfHostedRunner(job)
--- a/ql/test/query-tests/Security/CWE-284/.github/workflows/test1.yml
+++ b/ql/test/query-tests/Security/CWE-284/.github/workflows/test1.yml
@@ -26,3 +26,69 @@ jobs:
    runs-on: self-hosted-azure
    steps:
    - run: cmd
+  test5:
+    strategy:
+      fail-fast: false
+      matrix:
+        platform:
+          - name: Linux
+            os: ubuntu-latest
+            shell: bash
+          - name: macOS
+            os: macos-latest
+            shell: bash
+          - name: Windows
+            os: windows-latest
+            shell: cmd
+        node-version:
+          - 16.14.0
+          - 16.x
+          - 18.0.0
+          - 18.x
+          - 20.x
+    runs-on: ${{ matrix.platform.os }}
+    steps:
+    - run: cmd
+  test6:
+    strategy:
+      matrix:
+        os: [ubuntu-latest, macos-latest]
+    runs-on: ${{ matrix.os }}
+    steps:
+    - run: cmd
+  test7:
+    strategy:
+      matrix:
+        os: [self-hosted, ubuntu-latest]
+    runs-on: ${{ matrix.os }}
+    steps:
+    - run: cmd
+  test8:
+    strategy:
+      matrix:
+        settings:
+          - host:
+              - 'self-hosted'
+              - 'macos'
+              - 'arm64'
+            target: 'x86_64-apple-darwin'
+    runs-on: ${{ matrix.settings.host }}
+    steps:
+    - run: cmd
+  test9:
+    strategy:
+      matrix:
+        os: ${{ github.repository }}
+    runs-on: ${{ matrix.os }}
+    steps:
+    - run: cmd
+  test10:
+    strategy:
+      matrix:
+        os: ${{ github.repository }}
+        foo:
+          - bar: ${{ github.repository }}
+            baz: "asdf"
+    runs-on: ${{ matrix.foo.bar }}
+    steps:
+    - run: cmd
--- a/ql/test/query-tests/Security/CWE-284/CodeExecutionOnSelfHostedRunner.expected
+++ b/ql/test/query-tests/Security/CWE-284/CodeExecutionOnSelfHostedRunner.expected
@@ -1,4 +1,8 @@
 | .github/workflows/test1.yml:8:5:11:2 | Job: test1 | Job runs on self-hosted runner |
 | .github/workflows/test1.yml:12:5:17:2 | Job: test2 | Job runs on self-hosted runner |
 | .github/workflows/test1.yml:18:5:25:2 | Job: test3 | Job runs on self-hosted runner |
-| .github/workflows/test1.yml:26:5:28:15 | Job: test4 | Job runs on self-hosted runner |
+| .github/workflows/test1.yml:26:5:29:2 | Job: test4 | Job runs on self-hosted runner |
+| .github/workflows/test1.yml:60:5:66:2 | Job: test7 | Job runs on self-hosted runner |
+| .github/workflows/test1.yml:67:5:78:2 | Job: test8 | Job runs on self-hosted runner |
+| .github/workflows/test1.yml:79:5:85:2 | Job: test9 | Job runs on self-hosted runner |
+| .github/workflows/test1.yml:86:5:94:15 | Job: test10 | Job runs on self-hosted runner |