Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* Added a new extensible predicate `pinnedByLockfileDataModel(workflow_path, nwo, ref)`, which records `uses:` references that are pinned by a repository's Actions lockfile (`.github/workflows/actions.lock`). The CodeQL Actions extractor generates this data at database-creation time into a database-local model pack (`codeql/actions-lockfile-pins`); it has no effect unless that pack is supplied to analysis (for example via `--model-packs`), so behavior is unchanged for repositories without a lockfile or analyses that do not opt in.
13 changes: 13 additions & 0 deletions actions/ql/lib/codeql/actions/config/Config.qll
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,19 @@ predicate trustedActionsOwnerDataModel(string owner) {
Extensions::trustedActionsOwnerDataModel(owner)
}

/**
* MaD models for `uses` references pinned by the repository's Actions lockfile
* (`.github/workflows/actions.lock`). Populated by the CodeQL Actions extractor; see
* `pinnedByLockfileDataModel` in `ConfigExtensions.qll`.
* Fields:
* - workflow_path: repo-relative path of the file containing the `uses:` reference
* - nwo: owner and name of the referenced action (e.g. `actions/checkout`)
* - ref: the ref as written in `uses:` (e.g. `v4`)
*/
predicate pinnedByLockfileDataModel(string workflow_path, string nwo, string ref) {
Extensions::pinnedByLockfileDataModel(workflow_path, nwo, ref)
}

/**
* MaD models for untrusted git commands
* Fields:
Expand Down
20 changes: 20 additions & 0 deletions actions/ql/lib/codeql/actions/config/ConfigExtensions.qll
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,26 @@ extensible predicate immutableActionsDataModel(string action);
*/
extensible predicate trustedActionsOwnerDataModel(string owner);

/**
* Holds if the `uses` reference `nwo`@`ref` in the workflow or composite action file at
* `workflow_path` is pinned by an entry in the repository's Actions lockfile
* (`.github/workflows/actions.lock`).
*
* This predicate is intended to be populated by the CodeQL Actions extractor, which parses
* `actions.lock` at database-creation time using the canonical lockfile parser at
* `github.com/github/actions-lockfile/go`. Each lockfile entry binds an `nwo`@`ref` to a
* verified commit SHA, which is exactly the pinning evidence the `actions/unpinned-tag` query
* otherwise lacks. Until the extractor populates this predicate it is empty, so any clause that
* consumes it is a clean no-op and behavior is unchanged for repositories without a lockfile.
*
* Fields:
* - `workflow_path`: repo-relative path of the file containing the `uses:` reference,
* e.g. `.github/workflows/ci.yml`.
* - `nwo`: owner and name of the referenced action, e.g. `actions/checkout`.
* - `ref`: the ref (tag or branch) as written in `uses:`, e.g. `v4`.
*/
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
extensible predicate pinnedByLockfileDataModel(string workflow_path, string nwo, string ref);

/**
* Holds for git commands that may introduce untrusted data when called on an attacker controlled branch.
*/
Expand Down
20 changes: 20 additions & 0 deletions actions/ql/lib/ext/config/pinned_by_lockfile.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
extensions:
- addsTo:
pack: codeql/actions-all
extensible: pinnedByLockfileDataModel
# `pinnedByLockfileDataModel` records `uses:` references that are pinned by the repository's
# Actions lockfile (`.github/workflows/actions.lock`). It is intended to be populated by the
# CodeQL Actions extractor, which parses the lockfile at database-creation time using the
# canonical Go parser at `github.com/github/actions-lockfile/go`. Each row is
# `[workflow_path, nwo, ref]`:
# - workflow_path: repo-relative path of the file containing the `uses:` reference
# - nwo: owner/name of the referenced action (e.g. `actions/checkout`)
# - ref: the ref as written in `uses:` (e.g. `v4`)
#
# Example of the intended shape (commented out; real data is supplied by the extractor):
# - [".github/workflows/ci.yml", "actions/checkout", "v4"]
#
# Until the extractor populates this predicate it stays empty, so the
# `not pinnedByLockfile(...)` clause in `actions/unpinned-tag` is a no-op and behavior is
# unchanged for repositories without a lockfile.
data: []
23 changes: 23 additions & 0 deletions actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,27 @@ private predicate isPinnedContainer(string version) {
bindingset[nwo]
private predicate isContainerImage(string nwo) { nwo.regexpMatch("^docker://.+") }

// A `$/` reference is a same-repository (self repository) reference (e.g. `$/path/to/action`),
// resolved at the commit the calling workflow is running. Like `./` local (self workspace)
// references, it is inherently pinned and can never be an unpinned-tag finding, so we never flag it.
bindingset[nwo]
private predicate isSelfRepository(string nwo) { nwo.matches("$/%") }

// Holds if `uses` (calling action `nwo` at `version`) is pinned by an entry in the repository's
// Actions lockfile (`.github/workflows/actions.lock`). The underlying `pinnedByLockfileDataModel`
// predicate is populated by the CodeQL Actions extractor when it parses the lockfile at
// database-creation time; until then this is a clean no-op and no lockfile-pinned refs are
// suppressed. See `pinnedByLockfileDataModel` in `ConfigExtensions.qll` for the intended shape.
bindingset[nwo]
private predicate pinnedByLockfile(UsesStep uses, string nwo, string version) {
// The extractor populates this predicate with lower-cased owner/repo (GitHub treats
// them case-insensitively) but preserves the ref, so match `nwo` case-insensitively
// and `version` exactly. `nwo` keeps its source casing everywhere else (e.g. the
// alert message) so authors still see the ref as written.
pinnedByLockfileDataModel(uses.getLocation().getFile().getRelativePath(), nwo.toLowerCase(),
version)
}

private predicate getStepContainerName(UsesStep uses, string name) {
exists(Workflow workflow |
uses.getEnclosingWorkflow() = workflow and
Expand All @@ -55,6 +76,8 @@ where
getStepContainerName(uses, name) and
uses.getVersion() = version and
not isTrustedOwner(nwo) and
not isSelfRepository(nwo) and
not pinnedByLockfile(uses, nwo, version) and
not (if isContainerImage(nwo) then isPinnedContainer(version) else isPinnedCommit(version)) and
not isImmutableAction(uses, nwo)
select uses.getCalleeNode(),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The `actions/unpinned-tag` query no longer reports `uses:` references that are recorded as pinned by a repository's Actions lockfile (`.github/workflows/actions.lock`) via the new `pinnedByLockfileDataModel` extensible predicate. The CodeQL Actions extractor generates this data at database-creation time into a database-local model pack (`codeql/actions-lockfile-pins`); references are suppressed only when that pack is supplied to analysis (for example via `--model-packs`), so behavior is unchanged for repositories without a lockfile or analyses that do not opt in. Because the lockfile keys its pins transitively by workflow path, a stale lockfile could in rare cases suppress a directly-written unpinned tag that shares an action and major version with a transitively-pinned dependency of the same workflow; keeping the lockfile current avoids this.
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The `actions/unpinned-tag` query no longer reports `$/` self repository references (e.g. `uses: $/path/to/action`), which resolve to the same repository at the running commit and are therefore inherently pinned, just like `./` self workspace (local) references.
2 changes: 2 additions & 0 deletions actions/ql/test/qlpack.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,6 @@ dependencies:
codeql/immutable-actions-list: ${workspace}
extractor: actions
tests: .
dataExtensions:
- query-tests/Security/CWE-829/*.model.yml
warnOnImplicitThis: true
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
on:
pull_request

jobs:
build:
name: Build and test
runs-on: ubuntu-latest
steps:
# `some-owner/pinned-action@v1` is a tag ref that would normally be reported as an unpinned
# tag. The test data extension `pinned_by_lockfile.model.yml` records it as pinned by the
# repository's Actions lockfile, so the `not pinnedByLockfile(...)` clause suppresses it (this
# fixture is expected to produce no findings). This mirrors what the CodeQL Actions extractor
# will do by parsing `.github/workflows/actions.lock`.
#
# Negative control is provided for free by the many other fixtures in this directory whose tag
# refs are NOT recorded in the data extension and therefore remain reported.
- uses: some-owner/pinned-action@v1
# `Mixed-Owner/Pinned-Action@v1` is pinned by the lockfile too, but written with the mixed-case
# owner/repo that authors commonly use (Azure, GoogleCloudPlatform, ...). The extractor emits
# the lockfile nwo lower-cased, so the query must match owner/repo case-insensitively; this ref
# is therefore expected to be suppressed (no finding).
- uses: Mixed-Owner/Pinned-Action@v1
# Negative control: a mixed-case ref that is NOT recorded in the lockfile data must still be
# reported, guarding against over-suppression of every mixed-case ref.
- uses: Mixed-Owner/Unpinned-Action@v2
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
on:
pull_request

jobs:
build:
name: Build and test
runs-on: ubuntu-latest
steps:
# `$/` is a same-repository (self repository) reference resolved at the running commit. It is
# inherently pinned (like `./` self workspace refs) and must never be reported as an unpinned tag.
- uses: $/actions/foo
# `$/…@ref` is rejected by the `$/` rule, but a user could still write it. It must also
# never be flagged; this case exercises the `not isSelfRepository(nwo)` suppression, since
# without it `$/actions/foo@v1` would otherwise be reported as an unpinned tag.
- uses: $/actions/foo@v1
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
| .github/workflows/label_trusted_checkout2.yml:21:13:21:36 | completely/fakeaction@v2 | Unpinned 3rd party Action 'label_trusted_checkout2.yml' step $@ uses 'completely/fakeaction' with ref 'v2', not a pinned commit hash | .github/workflows/label_trusted_checkout2.yml:21:7:25:4 | Uses Step | Uses Step |
| .github/workflows/label_trusted_checkout2.yml:25:13:25:37 | fakerepo/comment-on-pr@v1 | Unpinned 3rd party Action 'label_trusted_checkout2.yml' step $@ uses 'fakerepo/comment-on-pr' with ref 'v1', not a pinned commit hash | .github/workflows/label_trusted_checkout2.yml:25:7:28:21 | Uses Step | Uses Step |
| .github/workflows/level0.yml:36:15:36:47 | rlespinasse/github-slug-action@v4 | Unpinned 3rd party Action 'Poutine Level 0' step $@ uses 'rlespinasse/github-slug-action' with ref 'v4', not a pinned commit hash | .github/workflows/level0.yml:36:9:39:6 | Uses Step | Uses Step |
| .github/workflows/lockfile_pinned.yml:25:13:25:42 | Mixed-Owner/Unpinned-Action@v2 | Unpinned 3rd party Action 'lockfile_pinned.yml' step $@ uses 'Mixed-Owner/Unpinned-Action' with ref 'v2', not a pinned commit hash | .github/workflows/lockfile_pinned.yml:25:7:25:43 | Uses Step | Uses Step |
| .github/workflows/mend.yml:31:15:31:34 | ruby/setup-ruby@v1 | Unpinned 3rd party Action 'Test' step $@ uses 'ruby/setup-ruby' with ref 'v1', not a pinned commit hash | .github/workflows/mend.yml:29:9:33:28 | Uses Step | Uses Step |
| .github/workflows/pr-workflow.yml:60:15:60:52 | amannn/action-semantic-pull-request@v5 | Unpinned 3rd party Action 'pr-workflow' step $@ uses 'amannn/action-semantic-pull-request' with ref 'v5', not a pinned commit hash | .github/workflows/pr-workflow.yml:60:9:70:6 | Uses Step | Uses Step |
| .github/workflows/pr-workflow.yml:109:15:109:42 | actionsdesk/lfs-warning@v3.2 | Unpinned 3rd party Action 'pr-workflow' step $@ uses 'actionsdesk/lfs-warning' with ref 'v3.2', not a pinned commit hash | .github/workflows/pr-workflow.yml:109:9:124:6 | Uses Step | Uses Step |
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ edges
| .github/workflows/level0.yml:122:9:125:6 | Uses Step | .github/workflows/level0.yml:125:9:129:6 | Uses Step |
| .github/workflows/level0.yml:125:9:129:6 | Uses Step | .github/workflows/level0.yml:129:9:133:6 | Uses Step |
| .github/workflows/level0.yml:129:9:133:6 | Uses Step | .github/workflows/level0.yml:133:9:135:23 | Run Step |
| .github/workflows/lockfile_pinned.yml:17:7:22:4 | Uses Step | .github/workflows/lockfile_pinned.yml:22:7:25:4 | Uses Step |
| .github/workflows/lockfile_pinned.yml:22:7:25:4 | Uses Step | .github/workflows/lockfile_pinned.yml:25:7:25:43 | Uses Step |
| .github/workflows/mend.yml:13:9:22:6 | Run Step: set_ref | .github/workflows/mend.yml:22:9:29:6 | Uses Step |
| .github/workflows/mend.yml:22:9:29:6 | Uses Step | .github/workflows/mend.yml:29:9:33:28 | Uses Step |
| .github/workflows/poc2.yml:28:9:37:6 | Uses Step: branch-deploy | .github/workflows/poc2.yml:37:9:42:6 | Uses Step |
Expand Down Expand Up @@ -196,6 +198,7 @@ edges
| .github/workflows/resolve-args.yml:20:9:22:6 | Uses Step | .github/actions/download-artifact/action.yaml:6:7:25:4 | Uses Step |
| .github/workflows/resolve-args.yml:20:9:22:6 | Uses Step | .github/workflows/resolve-args.yml:22:9:36:13 | Run Step: resolve-step |
| .github/workflows/reusable_local.yml:23:9:26:6 | Uses Step | .github/workflows/reusable_local.yml:26:9:29:7 | Run Step |
| .github/workflows/self_ref_dollar.yml:11:7:15:4 | Uses Step | .github/workflows/self_ref_dollar.yml:15:7:15:29 | Uses Step |
| .github/workflows/test1.yml:18:9:21:6 | Uses Step | .github/workflows/test1.yml:21:9:24:6 | Run Step |
| .github/workflows/test1.yml:21:9:24:6 | Run Step | .github/workflows/test1.yml:24:9:25:39 | Run Step |
| .github/workflows/test2.yml:13:9:16:6 | Uses Step | .github/workflows/test2.yml:16:9:20:52 | Uses Step |
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
extensions:
- addsTo:
pack: codeql/actions-all
extensible: pinnedByLockfileDataModel
# Test data standing in for what the CodeQL Actions extractor will emit from
# `.github/workflows/actions.lock`. Records `some-owner/pinned-action@v1` in
# `lockfile_pinned.yml` as pinned by the lockfile so the `actions/unpinned-tag`
# query suppresses it.
data:
- [".github/workflows/lockfile_pinned.yml", "some-owner/pinned-action", "v1"]
# Lower-cased form of `Mixed-Owner/Pinned-Action@v1`, as the extractor emits it. Exercises the
# case-insensitive owner/repo match in the `actions/unpinned-tag` query.
- [".github/workflows/lockfile_pinned.yml", "mixed-owner/pinned-action", "v1"]
Loading