diff --git a/.github/action/src/codeql.ts b/.github/action/src/codeql.ts index 0fcdd81ee3f..e845ec9fd4f 100644 --- a/.github/action/src/codeql.ts +++ b/.github/action/src/codeql.ts @@ -147,6 +147,13 @@ export async function codeqlDatabaseAnalyze( codeql_output, ]; + const extPackPath = process.env["EXTPACK_PATH"]; + const extPackName = process.env["EXTPACK_NAME"]; + if (extPackPath !== undefined && extPackName !== undefined) { + cmd.push("--additional-packs", extPackPath); + cmd.push("--extension-packs", extPackName); + } + // remote pack or local pack if (codeql.pack.startsWith("githubsecuritylab/")) { var suite = codeql.pack + ":" + codeql.suite; diff --git a/action.yml b/action.yml index 9281212ea24..42141a1dd74 100644 --- a/action.yml +++ b/action.yml @@ -18,10 +18,41 @@ inputs: description: "CodeQL Suite to run" default: "actions-code-scanning" + workflow-models: + description: "Workflow models" + required: false + runs: using: 'composite' steps: - - name: Do something with context + - name: Process workflow models + shell: bash + if: inputs.workflow-models + run: | + // Create QLPack directory + mkdir workflow-extpack + cd workflow-extpack + + // Store the extension pack file + cat > models.yml << 'EOF' + ${{ inputs.workflow-models }} + EOF + + // Create QLPack + cat > qlpack.yml << 'EOF' + name: local/workflow-models + library: true + extensionTargets: + githubsecuritylab/actions-all: '*' + dataExtensions: + - models.yml + EOF + + // Set env vars + echo "EXTPACK_PATH=./workflow-extpack" >> $GITHUB_ENV + echo "EXTPACK_NAME=local/workflow-models" >> $GITHUB_ENV + + - name: Scan workflows shell: bash env: GITHUB_TOKEN: ${{ inputs.token }} @@ -29,5 +60,7 @@ runs: INPUT_SOURCE-ROOT: ${{ inputs.source-root }} INPUT_SARIF-OUTPUT: ${{ inputs.sarif-output }} INPUT_SUITE: ${{ inputs.suite }} + EXTPACK_PATH: ${{ inputs.extpack-path }} + EXTPACK_NAME: ${{ inputs.extpack-name }} run: | node ${{ github.action_path }}/.github/action/dist/index.js diff --git a/ql/lib/codeql/actions/Ast.qll b/ql/lib/codeql/actions/Ast.qll index ac222741c02..7c4bf9aa8af 100644 --- a/ql/lib/codeql/actions/Ast.qll +++ b/ql/lib/codeql/actions/Ast.qll @@ -228,36 +228,7 @@ class Workflow extends AstNode instanceof WorkflowImpl { Strategy getStrategy() { result = super.getStrategy() } - predicate hasSingleTrigger(string trigger) { - this.getATriggerEvent() = trigger and - count(this.getATriggerEvent()) = 1 - } - - predicate isPrivileged() { - // The Workflow has a permission to write to some scope - this.getPermissions().getAPermission() = "write" - or - // 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` - count(this.getATriggerEvent()) = 1 and - not this.getATriggerEvent() = ["pull_request", "workflow_call"] - or - // The Workflow is only triggered by `workflow_call` and there is - // a caller workflow triggered by an event other than `pull_request` - this.hasSingleTrigger("workflow_call") and - exists(ExternalJob call, Workflow caller | - call.getCallee() = this.getLocation().getFile().getRelativePath() and - caller = call.getWorkflow() and - caller.isPrivileged() - ) - or - // The Workflow has multiple triggers so at least one is ont "pull_request" - count(this.getATriggerEvent()) > 1 - } + predicate isPrivileged() { super.isPrivileged() } } class ReusableWorkflow extends Workflow instanceof ReusableWorkflowImpl { @@ -325,6 +296,8 @@ abstract class Job extends AstNode instanceof JobImpl { Permissions getPermissions() { result = super.getPermissions() } Strategy getStrategy() { result = super.getStrategy() } + + predicate isPrivileged() { super.isPrivileged() } } class LocalJob extends Job instanceof LocalJobImpl { diff --git a/ql/lib/codeql/actions/ast/internal/Ast.qll b/ql/lib/codeql/actions/ast/internal/Ast.qll index dff5f351a69..7cc70c86d20 100644 --- a/ql/lib/codeql/actions/ast/internal/Ast.qll +++ b/ql/lib/codeql/actions/ast/internal/Ast.qll @@ -1,6 +1,7 @@ private import codeql.actions.ast.internal.Yaml private import codeql.Locations private import codeql.actions.Ast::Utils as Utils +private import codeql.actions.dataflow.ExternalFlow /** * Gets the length of each line in the StringValue . @@ -332,8 +333,40 @@ class WorkflowImpl extends AstNodeImpl, TWorkflowNode { /** Gets the permissions granted to this workflow. */ PermissionsImpl getPermissions() { result.getNode() = n.lookup("permissions") } + private predicate hasSingleTrigger(string trigger) { + this.getATriggerEvent() = trigger and + count(this.getATriggerEvent()) = 1 + } + /** Gets the strategy for this workflow. */ StrategyImpl getStrategy() { result.getNode() = n.lookup("strategy") } + + /** Holds if the workflow is privileged. */ + predicate isPrivileged() { + // The Workflow has a permission to write to some scope + this.getPermissions().getAPermission() = "write" + or + // The Workflow accesses a secret + exists(SecretsExpressionImpl expr | + expr.getEnclosingWorkflow() = this and not expr.getFieldName() = "GITHUB_TOKEN" + ) + or + // The Workflow is triggered by an event other than `pull_request` + count(this.getATriggerEvent()) = 1 and + not this.getATriggerEvent() = ["pull_request", "workflow_call"] + or + // The Workflow is only triggered by `workflow_call` and there is + // a caller workflow triggered by an event other than `pull_request` + this.hasSingleTrigger("workflow_call") and + exists(ExternalJobImpl call, WorkflowImpl caller | + call.getCallee() = this.getLocation().getFile().getRelativePath() and + caller = call.getWorkflow() and + caller.isPrivileged() + ) + or + // The Workflow has multiple triggers so at least one is not "pull_request" + count(this.getATriggerEvent()) > 1 + } } class ReusableWorkflowImpl extends AstNodeImpl, WorkflowImpl { @@ -597,6 +630,36 @@ class JobImpl extends AstNodeImpl, TJobNode { /** Gets the strategy for this job. */ StrategyImpl getStrategy() { result.getNode() = n.lookup("strategy") } + + /** Holds if the workflow is privileged. */ + predicate isPrivileged() { + // The job has a permission to write to some scope + this.getPermissions().getAPermission() = "write" + or + // The job accesses a secret + exists(SecretsExpressionImpl expr | + expr.getEnclosingJob() = this and not expr.getFieldName() = "GITHUB_TOKEN" + ) + or + // The effective permissions have write access + exists(string path, string name, string secrets_source, string perms | + workflowDataModel(path, _, name, secrets_source, perms, _) and + path.trim() = this.getLocation().getFile().getRelativePath() and + name.trim().matches(this.getId() + "%") and + ( + secrets_source.trim().toLowerCase() = "actions" or + perms.toLowerCase().matches("%write%") + ) + ) + or + // The job has no expliclit permission, but the enclosing workflow is privileged + not exists(this.getPermissions()) and + not exists(SecretsExpressionImpl expr | + expr.getEnclosingJob() = this and not expr.getFieldName() = "GITHUB_TOKEN" + ) and + // The enclosing workflow is privileged + this.getEnclosingWorkflow().isPrivileged() + } } class LocalJobImpl extends JobImpl { diff --git a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll index cc7e4c633e3..5db10e7823e 100644 --- a/ql/lib/codeql/actions/dataflow/ExternalFlow.qll +++ b/ql/lib/codeql/actions/dataflow/ExternalFlow.qll @@ -2,6 +2,13 @@ private import internal.ExternalFlowExtensions as Extensions private import codeql.actions.DataFlow private import actions +predicate workflowDataModel( + string path, string visibility, string job, string secrets_source, string permissions, + string runner +) { + Extensions::workflowDataModel(path, visibility, job, secrets_source, permissions, runner) +} + /** * MaD sources * Fields: diff --git a/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll b/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll index 8e8ce10bba9..529f7721e71 100644 --- a/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll +++ b/ql/lib/codeql/actions/dataflow/internal/ExternalFlowExtensions.qll @@ -22,3 +22,8 @@ extensible predicate summaryModel( extensible predicate sinkModel( string action, string version, string input, string kind, string provenance ); + +extensible predicate workflowDataModel( + string path, string visibility, string job, string secrets_source, string permissions, + string runner +); diff --git a/ql/lib/ext/workflow-models/workflow-models.yml b/ql/lib/ext/workflow-models/workflow-models.yml new file mode 100644 index 00000000000..f9f983be693 --- /dev/null +++ b/ql/lib/ext/workflow-models/workflow-models.yml @@ -0,0 +1,5 @@ +extensions: + - addsTo: + pack: githubsecuritylab/actions-all + extensible: workflowDataModel + data: [] diff --git a/ql/lib/qlpack.yml b/ql/lib/qlpack.yml index 94df84766b5..00b31b33bf5 100644 --- a/ql/lib/qlpack.yml +++ b/ql/lib/qlpack.yml @@ -2,7 +2,7 @@ library: true warnOnImplicitThis: true name: githubsecuritylab/actions-all -version: 0.0.15 +version: 0.0.16 dependencies: codeql/util: ^0.2.0 codeql/yaml: ^0.1.2 @@ -15,3 +15,4 @@ groups: dataExtensions: - ext/*.model.yml - ext/**/*.model.yml + - ext/workflow-models/workflow-models.yml diff --git a/ql/src/Security/CWE-077/EnvPathInjection.ql b/ql/src/Security/CWE-077/EnvPathInjection.ql index 19b4cf6c01b..720b7aed8cc 100644 --- a/ql/src/Security/CWE-077/EnvPathInjection.ql +++ b/ql/src/Security/CWE-077/EnvPathInjection.ql @@ -20,11 +20,11 @@ from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink where EnvPathInjectionFlow::flowPath(source, sink) and ( - exists(source.getNode().asExpr().getEnclosingCompositeAction()) + exists(sink.getNode().asExpr().getEnclosingCompositeAction()) or - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - not w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + not j.isPrivileged() ) ) select sink.getNode(), source, sink, diff --git a/ql/src/Security/CWE-077/EnvVarInjection.ql b/ql/src/Security/CWE-077/EnvVarInjection.ql index 2fca3b32494..af3f2998cc9 100644 --- a/ql/src/Security/CWE-077/EnvVarInjection.ql +++ b/ql/src/Security/CWE-077/EnvVarInjection.ql @@ -20,11 +20,11 @@ from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink where EnvVarInjectionFlow::flowPath(source, sink) and ( - exists(source.getNode().asExpr().getEnclosingCompositeAction()) + exists(sink.getNode().asExpr().getEnclosingCompositeAction()) or - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - not w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + not j.isPrivileged() ) ) select sink.getNode(), source, sink, diff --git a/ql/src/Security/CWE-077/PrivilegedEnvPathInjection.ql b/ql/src/Security/CWE-077/PrivilegedEnvPathInjection.ql index e9f55d1cbb2..3e7c74ab895 100644 --- a/ql/src/Security/CWE-077/PrivilegedEnvPathInjection.ql +++ b/ql/src/Security/CWE-077/PrivilegedEnvPathInjection.ql @@ -19,9 +19,9 @@ import EnvPathInjectionFlow::PathGraph from EnvPathInjectionFlow::PathNode source, EnvPathInjectionFlow::PathNode sink where EnvPathInjectionFlow::flowPath(source, sink) and - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + j.isPrivileged() ) select sink.getNode(), source, sink, "Potential privileged PATH environment variable injection in $@, which may be controlled by an external user.", diff --git a/ql/src/Security/CWE-077/PrivilegedEnvVarInjection.ql b/ql/src/Security/CWE-077/PrivilegedEnvVarInjection.ql index 1a32183bfb2..aac7568e654 100644 --- a/ql/src/Security/CWE-077/PrivilegedEnvVarInjection.ql +++ b/ql/src/Security/CWE-077/PrivilegedEnvVarInjection.ql @@ -19,9 +19,9 @@ import EnvVarInjectionFlow::PathGraph from EnvVarInjectionFlow::PathNode source, EnvVarInjectionFlow::PathNode sink where EnvVarInjectionFlow::flowPath(source, sink) and - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + j.isPrivileged() ) select sink.getNode(), source, sink, "Potential privileged environment variable injection in $@, which may be controlled by an external user.", diff --git a/ql/src/Security/CWE-078/CommandInjection.ql b/ql/src/Security/CWE-078/CommandInjection.ql index de60141bb40..6ac15f83207 100644 --- a/ql/src/Security/CWE-078/CommandInjection.ql +++ b/ql/src/Security/CWE-078/CommandInjection.ql @@ -20,11 +20,11 @@ from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink where CommandInjectionFlow::flowPath(source, sink) and ( - exists(source.getNode().asExpr().getEnclosingCompositeAction()) + exists(sink.getNode().asExpr().getEnclosingCompositeAction()) or - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - not w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + not j.isPrivileged() ) ) select sink.getNode(), source, sink, diff --git a/ql/src/Security/CWE-078/PrivilegedCommandInjection.ql b/ql/src/Security/CWE-078/PrivilegedCommandInjection.ql index bbfb226ecd1..adb8f25f077 100644 --- a/ql/src/Security/CWE-078/PrivilegedCommandInjection.ql +++ b/ql/src/Security/CWE-078/PrivilegedCommandInjection.ql @@ -19,9 +19,9 @@ import CommandInjectionFlow::PathGraph from CommandInjectionFlow::PathNode source, CommandInjectionFlow::PathNode sink where CommandInjectionFlow::flowPath(source, sink) and - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + j.isPrivileged() ) select sink.getNode(), source, sink, "Potential privileged command injection in $@, which may be controlled by an external user.", diff --git a/ql/src/Security/CWE-094/CodeInjection.ql b/ql/src/Security/CWE-094/CodeInjection.ql index dc28cc2569f..aa5bbfdf75a 100644 --- a/ql/src/Security/CWE-094/CodeInjection.ql +++ b/ql/src/Security/CWE-094/CodeInjection.ql @@ -22,11 +22,11 @@ from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink where CodeInjectionFlow::flowPath(source, sink) and ( - exists(source.getNode().asExpr().getEnclosingCompositeAction()) + exists(sink.getNode().asExpr().getEnclosingCompositeAction()) or - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - not w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + not j.isPrivileged() ) ) select sink.getNode(), source, sink, diff --git a/ql/src/Security/CWE-094/PrivilegedCodeInjection.ql b/ql/src/Security/CWE-094/PrivilegedCodeInjection.ql index 9814df091dd..d043bd930b6 100644 --- a/ql/src/Security/CWE-094/PrivilegedCodeInjection.ql +++ b/ql/src/Security/CWE-094/PrivilegedCodeInjection.ql @@ -21,9 +21,9 @@ import CodeInjectionFlow::PathGraph from CodeInjectionFlow::PathNode source, CodeInjectionFlow::PathNode sink where CodeInjectionFlow::flowPath(source, sink) and - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + j.isPrivileged() ) select sink.getNode(), source, sink, "Potential privileged code injection in $@, which may be controlled by an external user.", sink, diff --git a/ql/src/Security/CWE-829/ArtifactPoisoning.ql b/ql/src/Security/CWE-829/ArtifactPoisoning.ql index 19b007902bd..c26862960d1 100644 --- a/ql/src/Security/CWE-829/ArtifactPoisoning.ql +++ b/ql/src/Security/CWE-829/ArtifactPoisoning.ql @@ -19,11 +19,11 @@ from ArtifactPoisoningFlow::PathNode source, ArtifactPoisoningFlow::PathNode sin where ArtifactPoisoningFlow::flowPath(source, sink) and ( - exists(source.getNode().asExpr().getEnclosingCompositeAction()) + exists(sink.getNode().asExpr().getEnclosingCompositeAction()) or - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - not w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + not j.isPrivileged() ) ) select sink.getNode(), source, sink, diff --git a/ql/src/Security/CWE-829/PrivilegedArtifactPoisoning.ql b/ql/src/Security/CWE-829/PrivilegedArtifactPoisoning.ql index cd6d5eeb108..379babf35f8 100644 --- a/ql/src/Security/CWE-829/PrivilegedArtifactPoisoning.ql +++ b/ql/src/Security/CWE-829/PrivilegedArtifactPoisoning.ql @@ -18,9 +18,9 @@ import ArtifactPoisoningFlow::PathGraph from ArtifactPoisoningFlow::PathNode source, ArtifactPoisoningFlow::PathNode sink where ArtifactPoisoningFlow::flowPath(source, sink) and - exists(Workflow w | - w = source.getNode().asExpr().getEnclosingWorkflow() and - w.isPrivileged() + exists(Job j | + j = sink.getNode().asExpr().getEnclosingJob() and + j.isPrivileged() ) select sink.getNode(), source, sink, "Potential privileged artifact poisoning in $@, which may be controlled by an external user.", diff --git a/ql/src/qlpack.yml b/ql/src/qlpack.yml index 60e21004b84..dc9c140e60f 100644 --- a/ql/src/qlpack.yml +++ b/ql/src/qlpack.yml @@ -1,7 +1,7 @@ --- library: false name: githubsecuritylab/actions-queries -version: 0.0.15 +version: 0.0.16 groups: - actions - queries diff --git a/ql/test/library-tests/workflowenum.expected b/ql/test/library-tests/workflowenum.expected new file mode 100644 index 00000000000..e69de29bb2d diff --git a/ql/test/library-tests/workflowenum.ql b/ql/test/library-tests/workflowenum.ql new file mode 100644 index 00000000000..692d1eb706b --- /dev/null +++ b/ql/test/library-tests/workflowenum.ql @@ -0,0 +1,8 @@ +import actions +import codeql.actions.dataflow.internal.ExternalFlowExtensions as Extensions + +from + string path, string visibility, string job, string secrets_source, string permissions, + string runner +where Extensions::workflowDataModel(path, visibility, job, secrets_source, permissions, runner) +select visibility, path, job, secrets_source, permissions, runner diff --git a/ql/test/query-tests/Security/CWE-077/EnvPathInjection.actual b/ql/test/query-tests/Security/CWE-077/EnvPathInjection.expected similarity index 100% rename from ql/test/query-tests/Security/CWE-077/EnvPathInjection.actual rename to ql/test/query-tests/Security/CWE-077/EnvPathInjection.expected diff --git a/ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning61.yml b/ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning61.yml new file mode 100644 index 00000000000..dcc80c6e74f --- /dev/null +++ b/ql/test/query-tests/Security/CWE-829/.github/workflows/artifactpoisoning61.yml @@ -0,0 +1,53 @@ +name: Dependency Tree Reporter +on: + workflow_run: + workflows: [ "Dependency Tree Input Builder" ] + types: + - completed + +permissions: {} + +jobs: + compare: + permissions: + actions: read + pull-requests: write + runs-on: ubuntu-latest + if: > + ${{ github.event.workflow_run.event == 'pull_request' && + github.event.workflow_run.conclusion == 'success' }} + steps: + - name: Download artifacts + uses: actions/github-script@v7.0.1 + with: + script: | + var artifacts = await github.rest.actions.listWorkflowRunArtifacts({ + owner: context.repo.owner, + repo: context.repo.repo, + run_id: ${{github.event.workflow_run.id }}, + }); + console.log(artifacts); + var matchArtifact = artifacts.data.artifacts.filter((artifact) => { + return artifact.name == "input-artifacts" + })[0]; + var download = await github.rest.actions.downloadArtifact({ + owner: context.repo.owner, + repo: context.repo.repo, + artifact_id: matchArtifact.id, + archive_format: 'zip', + }); + var fs = require('fs'); + fs.writeFileSync('${{github.workspace}}/input.zip', Buffer.from(download.data)); + - name: Set needed env vars in outputs + id: prepare + run: | + unzip input.zip + echo current directory contents + ls -al + + echo Reading PR number + tmp=$(> $GITHUB_OUTPUT + + - run: echo ${{ steps.prepare.outputs.pr }}