From 841499eeb0e39f597431a9643e82d4dbd8172e5b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= <pwntester@github.com>
Date: Thu, 11 Apr 2024 16:23:51 +0200
Subject: [PATCH] Improve privleged workflow detection

---
 ql/lib/codeql/actions/Ast.qll                 | 17 ++++++++++++--
 ql/lib/codeql/actions/ast/internal/Ast.qll    | 23 +++++++++++++++++++
 ql/src/Security/CWE-829/ArtifactPoisoning.ql  |  2 +-
 .../CWE-094/.github/workflows/simple3.yml     |  2 +-
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll
index cf5b63399f0..91ee95a90ab 100644
--- a/ql/lib/codeql/actions/Ast.qll
+++ b/ql/lib/codeql/actions/Ast.qll
@@ -20,7 +20,7 @@ module Utils {
   }
 
   bindingset[str]
-  string trimQuotes(string str) {
+  private string trimQuotes(string str) {
     result = str.trim().regexpReplaceAll("^(\"|')", "").regexpReplaceAll("(\"|')$", "")
   }
 
@@ -212,6 +212,13 @@ class Workflow extends AstNode instanceof WorkflowImpl {
   }
 
   predicate isPrivileged() {
+    // The Workflow has a permission to write to some scope
+    this.getPermissions().getAPermission() = "write" and
+    // The Workflow accesses a secret
+    exists(SecretsExpression expr |
+      expr.getEnclosingWorkflow() = this and not expr.getFieldName() = "GITHUB_TOKEN"
+    )
+    or
     // The Workflow is triggered by an event other than `pull_request`
     not this.hasSingleTrigger("pull_request")
     or
@@ -248,7 +255,11 @@ class Outputs extends AstNode instanceof OutputsImpl {
   override string toString() { result = "Job outputs node" }
 }
 
-class Permissions extends AstNode instanceof PermissionsImpl { }
+class Permissions extends AstNode instanceof PermissionsImpl {
+  string getPermission(string perm) { result = super.getPermission(perm) }
+
+  string getAPermission() { result = super.getAPermission() }
+}
 
 class Strategy extends AstNode instanceof StrategyImpl {
   Expression getMatrixVarExpr(string varName) { result = super.getMatrixVarExpr(varName) }
@@ -348,6 +359,8 @@ abstract class SimpleReferenceExpression extends AstNode instanceof SimpleRefere
   AstNode getTarget() { result = super.getTarget() }
 }
 
+class SecretsExpression extends SimpleReferenceExpression instanceof SecretsExpressionImpl { }
+
 class StepsExpression extends SimpleReferenceExpression instanceof StepsExpressionImpl {
   string getStepId() { result = super.getStepId() }
 }
diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll
index bba5c1a47d3..300377536d6 100644
--- a/ql/lib/codeql/actions/ast/internal/Ast.qll
+++ b/ql/lib/codeql/actions/ast/internal/Ast.qll
@@ -484,6 +484,10 @@ class PermissionsImpl extends AstNodeImpl, TPermissionsNode {
   override Location getLocation() { result = n.getLocation() }
 
   override YamlMapping getNode() { result = n }
+
+  string getPermission(string perm) { result = n.lookup(perm).(YamlScalar).getValue() }
+
+  string getAPermission() { result = this.getPermission(_) }
 }
 
 class StrategyImpl extends AstNodeImpl, TStrategyNode {
@@ -851,6 +855,25 @@ private string inputsCtxRegex() {
     Utils::wrapRegexp(["inputs\\.([A-Za-z0-9_-]+)", "github\\.event\\.inputs\\.([A-Za-z0-9_-]+)"])
 }
 
+private string secretsCtxRegex() { result = Utils::wrapRegexp("secrets\\.([A-Za-z0-9_-]+)") }
+
+/**
+ * Holds for an expression accesing the `secrets` context.
+ * e.g. `${{ secrets.FOO }}`
+ */
+class SecretsExpressionImpl extends SimpleReferenceExpressionImpl {
+  string fieldName;
+
+  SecretsExpressionImpl() {
+    Utils::normalizeExpr(expression).regexpMatch(secretsCtxRegex()) and
+    fieldName = Utils::normalizeExpr(expression).regexpCapture(secretsCtxRegex(), 1)
+  }
+
+  override string getFieldName() { result = fieldName }
+
+  override AstNodeImpl getTarget() { none() }
+}
+
 /**
  * Holds for an expression accesing the `steps` context.
  * https://docs.github.com/en/actions/learn-github-actions/contexts#context-availability
diff --git a/ql/src/Security/CWE-829/ArtifactPoisoning.ql b/ql/src/Security/CWE-829/ArtifactPoisoning.ql
index 6d3910e2ca5..19b007902bd 100644
--- a/ql/src/Security/CWE-829/ArtifactPoisoning.ql
+++ b/ql/src/Security/CWE-829/ArtifactPoisoning.ql
@@ -4,7 +4,7 @@
  * @kind path-problem
  * @problem.severity warning
  * @precision high
- * @security-severity 9.3
+ * @security-severity 5.0
  * @id actions/artifact-poisoning
  * @tags actions
  *       security
diff --git a/ql/test/query-tests/Security/CWE-094/.github/workflows/simple3.yml b/ql/test/query-tests/Security/CWE-094/.github/workflows/simple3.yml
index be1559d4711..3128aacc93c 100644
--- a/ql/test/query-tests/Security/CWE-094/.github/workflows/simple3.yml
+++ b/ql/test/query-tests/Security/CWE-094/.github/workflows/simple3.yml
@@ -8,7 +8,7 @@ on:
 permissions:
   actions: read
   checks: read
-  contents: read
+  contents: write
 
 jobs:
   echo_trigger: