From e7bb47f335b29661e43ee22e54d66f78c385cb1c Mon Sep 17 00:00:00 2001 From: yoff Date: Fri, 28 Mar 2025 12:57:58 +0100 Subject: [PATCH 01/10] ruby: add MaD model for permissions needed by actions Use this to suggest minimal set of nedded permissions --- actions/ql/lib/codeql/actions/Ast.qll | 2 + .../lib/codeql/actions/ast/internal/Ast.qll | 2 + .../ql/lib/codeql/actions/config/Config.qll | 10 +++++ .../actions/config/ConfigExtensions.qll | 5 +++ .../ql/lib/ext/config/actions_permissions.yml | 37 +++++++++++++++++++ .../CWE-275/MissingActionsPermissions.ql | 29 +++++++++++++-- .../CWE-275/.github/workflows/perms6.yml | 13 +++++++ .../MissingActionsPermissions.expected | 7 ++-- 8 files changed, 99 insertions(+), 6 deletions(-) create mode 100644 actions/ql/lib/ext/config/actions_permissions.yml create mode 100644 actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms6.yml diff --git a/actions/ql/lib/codeql/actions/Ast.qll b/actions/ql/lib/codeql/actions/Ast.qll index 8c1925f3288c..a50febdea973 100644 --- a/actions/ql/lib/codeql/actions/Ast.qll +++ b/actions/ql/lib/codeql/actions/Ast.qll @@ -242,6 +242,8 @@ class Step extends AstNode instanceof StepImpl { If getIf() { result = super.getIf() } + AstNode getUses() { result = super.getUses() } + StepsContainer getContainer() { result = super.getContainer() } Step getNextStep() { result = super.getNextStep() } diff --git a/actions/ql/lib/codeql/actions/ast/internal/Ast.qll b/actions/ql/lib/codeql/actions/ast/internal/Ast.qll index b0cbb8a1d79e..44ae082a34db 100644 --- a/actions/ql/lib/codeql/actions/ast/internal/Ast.qll +++ b/actions/ql/lib/codeql/actions/ast/internal/Ast.qll @@ -1194,6 +1194,8 @@ class StepImpl extends AstNodeImpl, TStepNode { /** Gets the value of the `if` field in this step, if any. */ IfImpl getIf() { result.getNode() = n.lookup("if") } + AstNodeImpl getUses() { result.getNode() = n.lookup("uses") } + /** Gets the Runs or LocalJob that this step is in. */ StepsContainerImpl getContainer() { result = this.getParentNode().(RunsImpl) or diff --git a/actions/ql/lib/codeql/actions/config/Config.qll b/actions/ql/lib/codeql/actions/config/Config.qll index 08bc7e860c67..e55d36f1bab8 100644 --- a/actions/ql/lib/codeql/actions/config/Config.qll +++ b/actions/ql/lib/codeql/actions/config/Config.qll @@ -154,3 +154,13 @@ predicate untrustedGitCommandDataModel(string cmd_regex, string flag) { predicate untrustedGhCommandDataModel(string cmd_regex, string flag) { Extensions::untrustedGhCommandDataModel(cmd_regex, flag) } + +/** + * MaD models for permissions needed by actions + * Fields: + * - action: action name + * - permission: permission name + */ +predicate actionsPermissionsDataModel(string action, string permission) { + Extensions::actionsPermissionsDataModel(action, permission) +} diff --git a/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll b/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll index 68685f5874bb..b86ce68a5fdd 100644 --- a/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll +++ b/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll @@ -77,3 +77,8 @@ extensible predicate untrustedGitCommandDataModel(string cmd_regex, string flag) * Holds for gh commands that may introduce untrusted data */ extensible predicate untrustedGhCommandDataModel(string cmd_regex, string flag); + +/** + * Holds if `action` needs `permission` to run. + */ +extensible predicate actionsPermissionsDataModel(string action, string permission); diff --git a/actions/ql/lib/ext/config/actions_permissions.yml b/actions/ql/lib/ext/config/actions_permissions.yml new file mode 100644 index 000000000000..6e0081973de6 --- /dev/null +++ b/actions/ql/lib/ext/config/actions_permissions.yml @@ -0,0 +1,37 @@ +extensions: + - addsTo: + pack: codeql/actions-all + extensible: actionsPermissionsDataModel + data: + - ["actions/checkout", "contents: read"] + - ["actions/setup-node", "contents: read"] + - ["actions/setup-python", "contents: read"] + - ["actions/setup-java", "contents: read"] + - ["actions/setup-go", "contents: read"] + - ["actions/setup-dotnet", "contents: read"] + - ["actions/labeler", "contents: read"] + - ["actions/labeler", "pull-requests: write"] + - ["actions/attest", "id-token: write"] + - ["actions/attest", "attestations: write"] + # No permissions needed for actions/add-to-project + - ["actions/dependency-review-action", "contents: read"] + - ["actions/attest-sbom", "id-token: write"] + - ["actions/attest-sbom", "attestations: write"] + - ["actions/stale", "contents: write"] + - ["actions/stale", "issues: write"] + - ["actions/stale", "pull-requests: write"] + - ["actions/attest-build-provenance", "id-token: write"] + - ["actions/attest-build-provenance", "attestations: write"] + - ["actions/jekyll-build-pages", "contents: read"] + - ["actions/jekyll-build-pages", "pages: write"] + - ["actions/jekyll-build-pages", "id-token: write"] + - ["actions/publish-action", "contents: write"] + - ["actions/versions-package-tools", "contents: read"] + - ["actions/versions-package-tools", "actions: read"] + - ["actions/reusable-workflows", "contents: read"] + - ["actions/reusable-workflows", "actions: read"] + # TODO: Add permissions for actions/download-artifact + # TODO: Add permissions for actions/upload-artifact + # TODO: Add permissions for actions/cache + + diff --git a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql index 4f7e951d7ed6..6d5bd04b3e26 100644 --- a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql +++ b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql @@ -14,7 +14,28 @@ import actions -from Job job +Step stepInJob(Job job) { result = job.(LocalJob).getAStep() } + +bindingset[fullActionSelector] +string versionedAction(string fullActionSelector) { + result = fullActionSelector.substring(0, fullActionSelector.indexOf("@")) + or + not exists(fullActionSelector.indexOf("@")) and + result = fullActionSelector +} + +string stepUses(Step step) { result = step.getUses().(ScalarValue).getValue() } + +string jobNeedsPersmission(Job job) { + actionsPermissionsDataModel(versionedAction(stepUses(stepInJob(job))), result) +} + +string permissionsForJob(Job job) { + result = + "{" + concat(string permission | permission = jobNeedsPersmission(job) | permission, ", ") + "}" +} + +from Job job, string permissions where not exists(job.getPermissions()) and not exists(job.getEnclosingWorkflow().getPermissions()) and @@ -22,5 +43,7 @@ where exists(Event e | e = job.getATriggerEvent() and not e.getName() = "workflow_call" - ) -select job, "Actions Job or Workflow does not set permissions" + ) and + permissions = permissionsForJob(job) +select job, + "Actions Job or Workflow does not set permissions. A minimal set might be " + permissions diff --git a/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms6.yml b/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms6.yml new file mode 100644 index 000000000000..2824ca14a7e1 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms6.yml @@ -0,0 +1,13 @@ +on: + workflow_call: + workflow_dispatch: + +jobs: + build: + name: Build and test + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - uses: actions/jekyll-build-pages + + diff --git a/actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected b/actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected index 8f94d0dc45a6..c7103d6f8ddc 100644 --- a/actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected +++ b/actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected @@ -1,3 +1,4 @@ -| .github/workflows/perms1.yml:6:5:9:32 | Job: build | Actions Job or Workflow does not set permissions | -| .github/workflows/perms2.yml:6:5:10:2 | Job: build | Actions Job or Workflow does not set permissions | -| .github/workflows/perms5.yml:7:5:10:32 | Job: build | Actions Job or Workflow does not set permissions | +| .github/workflows/perms1.yml:6:5:9:32 | Job: build | Actions Job or Workflow does not set permissions. A minimal set might be {contents: read} | +| .github/workflows/perms2.yml:6:5:10:2 | Job: build | Actions Job or Workflow does not set permissions. A minimal set might be {contents: read} | +| .github/workflows/perms5.yml:7:5:10:32 | Job: build | Actions Job or Workflow does not set permissions. A minimal set might be {contents: read} | +| .github/workflows/perms6.yml:7:5:11:39 | Job: build | Actions Job or Workflow does not set permissions. A minimal set might be {contents: read, id-token: write, pages: write} | From 1ec3e8712b5b0daf3a0db52c30137a32fc0bc2e1 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 1 Apr 2025 13:18:30 +0200 Subject: [PATCH 02/10] Apply suggestions from code review Co-authored-by: Aditya Sharad <6874315+adityasharad@users.noreply.github.com> --- actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql index 6d5bd04b3e26..760ef8c1897b 100644 --- a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql +++ b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql @@ -30,6 +30,7 @@ string jobNeedsPersmission(Job job) { actionsPermissionsDataModel(versionedAction(stepUses(stepInJob(job))), result) } +/** Gets a suggestion for the minimal token permissions for `job`, as a JSON string. */ string permissionsForJob(Job job) { result = "{" + concat(string permission | permission = jobNeedsPersmission(job) | permission, ", ") + "}" @@ -46,4 +47,4 @@ where ) and permissions = permissionsForJob(job) select job, - "Actions Job or Workflow does not set permissions. A minimal set might be " + permissions + "Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: " + permissions From 3cdd641b8191590c1416fb325f718243ced6e1f5 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 1 Apr 2025 13:43:00 +0200 Subject: [PATCH 03/10] actions: fix typo --- .../ql/src/Security/CWE-275/MissingActionsPermissions.ql | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql index 760ef8c1897b..bc3b4869a8d1 100644 --- a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql +++ b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql @@ -26,14 +26,14 @@ string versionedAction(string fullActionSelector) { string stepUses(Step step) { result = step.getUses().(ScalarValue).getValue() } -string jobNeedsPersmission(Job job) { +string jobNeedsPermission(Job job) { actionsPermissionsDataModel(versionedAction(stepUses(stepInJob(job))), result) } /** Gets a suggestion for the minimal token permissions for `job`, as a JSON string. */ string permissionsForJob(Job job) { result = - "{" + concat(string permission | permission = jobNeedsPersmission(job) | permission, ", ") + "}" + "{" + concat(string permission | permission = jobNeedsPermission(job) | permission, ", ") + "}" } from Job job, string permissions @@ -47,4 +47,5 @@ where ) and permissions = permissionsForJob(job) select job, - "Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: " + permissions + "Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: " + + permissions From bd7c684c6c5d8a500e5e4ca3f7181fc9e43508d8 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 1 Apr 2025 17:06:32 +0200 Subject: [PATCH 04/10] actions: add test with empty permissions --- .../Security/CWE-275/.github/workflows/perms7.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms7.yml diff --git a/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms7.yml b/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms7.yml new file mode 100644 index 000000000000..0ec255f0d109 --- /dev/null +++ b/actions/ql/test/query-tests/Security/CWE-275/.github/workflows/perms7.yml @@ -0,0 +1,10 @@ +on: + workflow_call: + workflow_dispatch: + +jobs: + build: + name: Build and test + runs-on: ubuntu-latest + steps: + - uses: actions/add-to-project@v2 From ee1eb199b557970e2675c2afd1efaba0949980a0 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 1 Apr 2025 17:07:02 +0200 Subject: [PATCH 05/10] actions: add description of `actionsPermissionsDataModel` --- actions/ql/lib/codeql/actions/config/ConfigExtensions.qll | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll b/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll index b86ce68a5fdd..906cabbd27d1 100644 --- a/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll +++ b/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll @@ -80,5 +80,10 @@ extensible predicate untrustedGhCommandDataModel(string cmd_regex, string flag); /** * Holds if `action` needs `permission` to run. + * - 'action' is the name of the action without any version information. + * E.g. for the action selector `actions/checkout@v2`, `action` is `actions/checkout`. + * - `permission` is of the form `scope-name: read|write`, for example `contents: read`. + * - see https://github.com/actions/checkout?tab=readme-ov-file#recommended-permissions + * for an example of recommended permissions. */ extensible predicate actionsPermissionsDataModel(string action, string permission); From 6fd8aba5609b84979a7c3ade3c94acfe37b9167c Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 1 Apr 2025 17:07:21 +0200 Subject: [PATCH 06/10] actions: simplify using existing `UsesStep` --- .../Security/CWE-275/MissingActionsPermissions.ql | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql index bc3b4869a8d1..aedf65bc564e 100644 --- a/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql +++ b/actions/ql/src/Security/CWE-275/MissingActionsPermissions.ql @@ -16,18 +16,8 @@ import actions Step stepInJob(Job job) { result = job.(LocalJob).getAStep() } -bindingset[fullActionSelector] -string versionedAction(string fullActionSelector) { - result = fullActionSelector.substring(0, fullActionSelector.indexOf("@")) - or - not exists(fullActionSelector.indexOf("@")) and - result = fullActionSelector -} - -string stepUses(Step step) { result = step.getUses().(ScalarValue).getValue() } - string jobNeedsPermission(Job job) { - actionsPermissionsDataModel(versionedAction(stepUses(stepInJob(job))), result) + actionsPermissionsDataModel(stepInJob(job).(UsesStep).getCallee(), result) } /** Gets a suggestion for the minimal token permissions for `job`, as a JSON string. */ From d83f35ff648942215dd5db69151cca947eb2f8e3 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 1 Apr 2025 17:07:43 +0200 Subject: [PATCH 07/10] actions: remove unneded API --- actions/ql/lib/codeql/actions/Ast.qll | 2 -- actions/ql/lib/codeql/actions/ast/internal/Ast.qll | 2 -- 2 files changed, 4 deletions(-) diff --git a/actions/ql/lib/codeql/actions/Ast.qll b/actions/ql/lib/codeql/actions/Ast.qll index a50febdea973..8c1925f3288c 100644 --- a/actions/ql/lib/codeql/actions/Ast.qll +++ b/actions/ql/lib/codeql/actions/Ast.qll @@ -242,8 +242,6 @@ class Step extends AstNode instanceof StepImpl { If getIf() { result = super.getIf() } - AstNode getUses() { result = super.getUses() } - StepsContainer getContainer() { result = super.getContainer() } Step getNextStep() { result = super.getNextStep() } diff --git a/actions/ql/lib/codeql/actions/ast/internal/Ast.qll b/actions/ql/lib/codeql/actions/ast/internal/Ast.qll index 44ae082a34db..b0cbb8a1d79e 100644 --- a/actions/ql/lib/codeql/actions/ast/internal/Ast.qll +++ b/actions/ql/lib/codeql/actions/ast/internal/Ast.qll @@ -1194,8 +1194,6 @@ class StepImpl extends AstNodeImpl, TStepNode { /** Gets the value of the `if` field in this step, if any. */ IfImpl getIf() { result.getNode() = n.lookup("if") } - AstNodeImpl getUses() { result.getNode() = n.lookup("uses") } - /** Gets the Runs or LocalJob that this step is in. */ StepsContainerImpl getContainer() { result = this.getParentNode().(RunsImpl) or From 80ae8794f55a6bcedf90e12c6757270845a50e82 Mon Sep 17 00:00:00 2001 From: yoff Date: Tue, 1 Apr 2025 17:07:57 +0200 Subject: [PATCH 08/10] actions: update test expectations --- .../Security/CWE-275/MissingActionsPermissions.expected | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected b/actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected index c7103d6f8ddc..1a3c36c78ca1 100644 --- a/actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected +++ b/actions/ql/test/query-tests/Security/CWE-275/MissingActionsPermissions.expected @@ -1,4 +1,5 @@ -| .github/workflows/perms1.yml:6:5:9:32 | Job: build | Actions Job or Workflow does not set permissions. A minimal set might be {contents: read} | -| .github/workflows/perms2.yml:6:5:10:2 | Job: build | Actions Job or Workflow does not set permissions. A minimal set might be {contents: read} | -| .github/workflows/perms5.yml:7:5:10:32 | Job: build | Actions Job or Workflow does not set permissions. A minimal set might be {contents: read} | -| .github/workflows/perms6.yml:7:5:11:39 | Job: build | Actions Job or Workflow does not set permissions. A minimal set might be {contents: read, id-token: write, pages: write} | +| .github/workflows/perms1.yml:6:5:9:32 | Job: build | Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read} | +| .github/workflows/perms2.yml:6:5:10:2 | Job: build | Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read} | +| .github/workflows/perms5.yml:7:5:10:32 | Job: build | Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read} | +| .github/workflows/perms6.yml:7:5:11:39 | Job: build | Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {contents: read, id-token: write, pages: write} | +| .github/workflows/perms7.yml:7:5:10:38 | Job: build | Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {} | From 7bf4a4754903698b1a2ebf1360d9665e3c35c3f7 Mon Sep 17 00:00:00 2001 From: yoff Date: Wed, 2 Apr 2025 08:43:29 +0200 Subject: [PATCH 09/10] Apply suggestions from code review Co-authored-by: Aditya Sharad <6874315+adityasharad@users.noreply.github.com> --- actions/ql/lib/codeql/actions/config/Config.qll | 4 ++-- actions/ql/lib/codeql/actions/config/ConfigExtensions.qll | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/actions/ql/lib/codeql/actions/config/Config.qll b/actions/ql/lib/codeql/actions/config/Config.qll index e55d36f1bab8..e6359c142582 100644 --- a/actions/ql/lib/codeql/actions/config/Config.qll +++ b/actions/ql/lib/codeql/actions/config/Config.qll @@ -158,8 +158,8 @@ predicate untrustedGhCommandDataModel(string cmd_regex, string flag) { /** * MaD models for permissions needed by actions * Fields: - * - action: action name - * - permission: permission name + * - action: action name, e.g. `actions/checkout` + * - permission: permission name, e.g. `contents: read` */ predicate actionsPermissionsDataModel(string action, string permission) { Extensions::actionsPermissionsDataModel(action, permission) diff --git a/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll b/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll index 906cabbd27d1..87a919359404 100644 --- a/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll +++ b/actions/ql/lib/codeql/actions/config/ConfigExtensions.qll @@ -85,5 +85,6 @@ extensible predicate untrustedGhCommandDataModel(string cmd_regex, string flag); * - `permission` is of the form `scope-name: read|write`, for example `contents: read`. * - see https://github.com/actions/checkout?tab=readme-ov-file#recommended-permissions * for an example of recommended permissions. + * - see https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/controlling-permissions-for-github_token for documentation of token permissions. */ extensible predicate actionsPermissionsDataModel(string action, string permission); From c18529086af3b0f0c4a1452944a9cf338564a734 Mon Sep 17 00:00:00 2001 From: yoff Date: Wed, 2 Apr 2025 08:50:05 +0200 Subject: [PATCH 10/10] actions: add change note --- .../change-notes/2025-02-04-suggest-actions-permissions.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 actions/ql/src/change-notes/2025-02-04-suggest-actions-permissions.md diff --git a/actions/ql/src/change-notes/2025-02-04-suggest-actions-permissions.md b/actions/ql/src/change-notes/2025-02-04-suggest-actions-permissions.md new file mode 100644 index 000000000000..c775b70274fb --- /dev/null +++ b/actions/ql/src/change-notes/2025-02-04-suggest-actions-permissions.md @@ -0,0 +1,4 @@ +--- +category: fix +--- +* Alerts produced by the query `actions/missing-workflow-permissions` now include a minimal set of recommended permissions in the alert message, based on well-known actions seen within the workflow file. \ No newline at end of file