From 9a29cebe5869b02f9a1e03c97c2512695223d97d Mon Sep 17 00:00:00 2001 From: Martin Costello Date: Fri, 14 Feb 2025 12:35:55 +0000 Subject: [PATCH 1/9] Fix docker SHA false positive Fix false positives for pinned Docker container images. --- .../Security/CWE-829/UnpinnedActionsTag.ql | 19 ++++++++++++++++++- .../.github/workflows/unpinned_tags.yml | 2 ++ .../CWE-829/UnpinnedActionsTag.expected | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql index 4c4480d8ad19..6e10e3c906b4 100644 --- a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql +++ b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql @@ -23,6 +23,12 @@ private predicate isTrustedOwner(string nwo) { trustedActionsOwnerDataModel(nwo.substring(0, nwo.indexOf("/"))) } +bindingset[version] +private predicate isPinnedContainer(string version) { version.regexpMatch("^sha256:[A-Fa-f0-9]{64}$") } + +bindingset[nwo] +private predicate isContainerImage(string nwo) { version.regexpMatch("^docker:\/\/.*") } + from UsesStep uses, string nwo, string version, Workflow workflow, string name where uses.getCallee() = nwo and @@ -33,9 +39,20 @@ where not exists(workflow.getName()) and workflow.getLocation().getFile().getBaseName() = name ) and uses.getVersion() = version and + isContainerImage(nwo) = isContainer and not isTrustedOwner(nwo) and - not isPinnedCommit(version) and not isImmutableAction(uses, nwo) + not ( + ( + isContainer and + isPinnedCommit(version) + ) + or + ( + not isContainer and + isPinnedCommit(version) + ) + ) select uses.getCalleeNode(), "Unpinned 3rd party Action '" + name + "' step $@ uses '" + nwo + "' with ref '" + version + "', not a pinned commit hash", uses, uses.toString() diff --git a/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/unpinned_tags.yml b/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/unpinned_tags.yml index 992686fb5aa8..f204816eed4e 100644 --- a/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/unpinned_tags.yml +++ b/actions/ql/test/query-tests/Security/CWE-829/.github/workflows/unpinned_tags.yml @@ -9,3 +9,5 @@ jobs: - uses: foo/bar - uses: foo/bar@v1 - uses: foo/bar@25b062c917b0c75f8b47d8469aff6c94ffd89abb + - uses: docker://foo/bar@latest + - uses: docker://foo/bar@sha256:887a259a5a534f3c4f36cb02dca341673c6089431057242cdc931e9f133147e9 diff --git a/actions/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected b/actions/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected index 848962e26bd6..ed35f1546171 100644 --- a/actions/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected +++ b/actions/ql/test/query-tests/Security/CWE-829/UnpinnedActionsTag.expected @@ -32,3 +32,4 @@ | .github/workflows/test17.yml:20:21:20:63 | sonarsource/sonarcloud-github-action@master | Unpinned 3rd party Action 'Sonar' step $@ uses 'sonarsource/sonarcloud-github-action' with ref 'master', not a pinned commit hash | .github/workflows/test17.yml:19:15:23:58 | Uses Step | Uses Step | | .github/workflows/test18.yml:37:21:37:63 | sonarsource/sonarcloud-github-action@master | Unpinned 3rd party Action 'Sonar' step $@ uses 'sonarsource/sonarcloud-github-action' with ref 'master', not a pinned commit hash | .github/workflows/test18.yml:36:15:40:58 | Uses Step | Uses Step | | .github/workflows/unpinned_tags.yml:10:13:10:22 | foo/bar@v1 | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'foo/bar' with ref 'v1', not a pinned commit hash | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | Uses Step | +| .github/workflows/unpinned_tags.yml:12:13:12:35 | docker://foo/bar@latest | Unpinned 3rd party Action 'unpinned_tags.yml' step $@ uses 'docker://foo/bar' with ref 'latest', not a pinned commit hash | .github/workflows/unpinned_tags.yml:12:7:13:4 | Uses Step | Uses Step | From 71bc89bedaceb1c8629c58a47c8c132ef13dc9f4 Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 14 Feb 2025 12:59:02 +0000 Subject: [PATCH 2/9] Fix query Fix various issues with the query. --- .../ql/src/Security/CWE-829/UnpinnedActionsTag.ql | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql index 6e10e3c906b4..e6cb94713b19 100644 --- a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql +++ b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql @@ -27,7 +27,7 @@ bindingset[version] private predicate isPinnedContainer(string version) { version.regexpMatch("^sha256:[A-Fa-f0-9]{64}$") } bindingset[nwo] -private predicate isContainerImage(string nwo) { version.regexpMatch("^docker:\/\/.*") } +private predicate isContainerImage(string nwo) { nwo.regexpMatch("^docker://.+") } from UsesStep uses, string nwo, string version, Workflow workflow, string name where @@ -39,17 +39,17 @@ where not exists(workflow.getName()) and workflow.getLocation().getFile().getBaseName() = name ) and uses.getVersion() = version and - isContainerImage(nwo) = isContainer and not isTrustedOwner(nwo) and - not isImmutableAction(uses, nwo) - not ( + not isImmutableAction(uses, nwo) and + not + ( ( - isContainer and - isPinnedCommit(version) + isContainerImage(nwo) and + isPinnedContainer(version) ) or ( - not isContainer and + not isContainerImage(nwo) and isPinnedCommit(version) ) ) From cf8abb79892882d8c8af04cf53e5a212f69bc230 Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 14 Feb 2025 13:27:36 +0000 Subject: [PATCH 3/9] Add change note Add change note. --- .../ql/src/change-notes/2025-02-14-docker-false-positives.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 actions/ql/src/change-notes/2025-02-14-docker-false-positives.md diff --git a/actions/ql/src/change-notes/2025-02-14-docker-false-positives.md b/actions/ql/src/change-notes/2025-02-14-docker-false-positives.md new file mode 100644 index 000000000000..9dadea510f76 --- /dev/null +++ b/actions/ql/src/change-notes/2025-02-14-docker-false-positives.md @@ -0,0 +1,5 @@ +--- +category: minorAnalysis +--- + +* Fix CWE-829 false positives for Docker GitHub actions pinned by the container's SHA256 digest. From 99bb0f0b4ff7dd38bb5d281fd2b3fc24eb36d133 Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 14 Feb 2025 13:30:55 +0000 Subject: [PATCH 4/9] Use if then else Apply code review suggestion. Co-Authored-By: Taus <1104778+tausbn@users.noreply.github.com> --- .../ql/src/Security/CWE-829/UnpinnedActionsTag.ql | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql index e6cb94713b19..642d778ca2f1 100644 --- a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql +++ b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql @@ -41,18 +41,7 @@ where uses.getVersion() = version and not isTrustedOwner(nwo) and not isImmutableAction(uses, nwo) and - not - ( - ( - isContainerImage(nwo) and - isPinnedContainer(version) - ) - or - ( - not isContainerImage(nwo) and - isPinnedCommit(version) - ) - ) + not if isContainerImage(nwo) then isPinnedContainer(version) else isPinnedCommit(version) select uses.getCalleeNode(), "Unpinned 3rd party Action '" + name + "' step $@ uses '" + nwo + "' with ref '" + version + "', not a pinned commit hash", uses, uses.toString() From 9a7ed7f3f7f23716daa615c7220daa37b53a5d33 Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 14 Feb 2025 13:35:20 +0000 Subject: [PATCH 5/9] Re-order conditions Makes for a neater diff. --- actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql index 642d778ca2f1..85dc480dfbc2 100644 --- a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql +++ b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql @@ -40,8 +40,8 @@ where ) and uses.getVersion() = version and not isTrustedOwner(nwo) and - not isImmutableAction(uses, nwo) and not if isContainerImage(nwo) then isPinnedContainer(version) else isPinnedCommit(version) + not isImmutableAction(uses, nwo) and select uses.getCalleeNode(), "Unpinned 3rd party Action '" + name + "' step $@ uses '" + nwo + "' with ref '" + version + "', not a pinned commit hash", uses, uses.toString() From 5d2409e652b1dc647ba9530bef5160da795bd2f8 Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 14 Feb 2025 13:36:09 +0000 Subject: [PATCH 6/9] Fix query Forgot to move the `and`. --- actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql index 85dc480dfbc2..90f4b53eb173 100644 --- a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql +++ b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql @@ -40,8 +40,8 @@ where ) and uses.getVersion() = version and not isTrustedOwner(nwo) and - not if isContainerImage(nwo) then isPinnedContainer(version) else isPinnedCommit(version) - not isImmutableAction(uses, nwo) and + not if isContainerImage(nwo) then isPinnedContainer(version) else isPinnedCommit(version) and + not isImmutableAction(uses, nwo) select uses.getCalleeNode(), "Unpinned 3rd party Action '" + name + "' step $@ uses '" + nwo + "' with ref '" + version + "', not a pinned commit hash", uses, uses.toString() From 979d604bf602907f1a3ac4b7879df702c0e3a038 Mon Sep 17 00:00:00 2001 From: Martin Costello Date: Fri, 14 Feb 2025 17:21:24 +0000 Subject: [PATCH 7/9] Apply suggestions from code review Co-authored-by: Aditya Sharad <6874315+adityasharad@users.noreply.github.com> --- actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql | 2 +- .../ql/src/change-notes/2025-02-14-docker-false-positives.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql index 90f4b53eb173..4c07f09b2fac 100644 --- a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql +++ b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql @@ -40,7 +40,7 @@ where ) and uses.getVersion() = version and not isTrustedOwner(nwo) and - not if isContainerImage(nwo) then isPinnedContainer(version) else isPinnedCommit(version) and + not (if isContainerImage(nwo) then isPinnedContainer(version) else isPinnedCommit(version)) and not isImmutableAction(uses, nwo) select uses.getCalleeNode(), "Unpinned 3rd party Action '" + name + "' step $@ uses '" + nwo + "' with ref '" + version + diff --git a/actions/ql/src/change-notes/2025-02-14-docker-false-positives.md b/actions/ql/src/change-notes/2025-02-14-docker-false-positives.md index 9dadea510f76..387472462205 100644 --- a/actions/ql/src/change-notes/2025-02-14-docker-false-positives.md +++ b/actions/ql/src/change-notes/2025-02-14-docker-false-positives.md @@ -2,4 +2,4 @@ category: minorAnalysis --- -* Fix CWE-829 false positives for Docker GitHub actions pinned by the container's SHA256 digest. +* Fixed false positives in the query `actions/unpinned-tag` (CWE-829), which will no longer flag uses of Docker-based GitHub actions pinned by the container's SHA256 digest. From f1723321fa7a2e7aaf6575bd6948a4996757ddc2 Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 14 Feb 2025 18:06:00 +0000 Subject: [PATCH 8/9] Format Document Fix lint warning. --- actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql index 4c07f09b2fac..c8512e5a1c0c 100644 --- a/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql +++ b/actions/ql/src/Security/CWE-829/UnpinnedActionsTag.ql @@ -24,7 +24,9 @@ private predicate isTrustedOwner(string nwo) { } bindingset[version] -private predicate isPinnedContainer(string version) { version.regexpMatch("^sha256:[A-Fa-f0-9]{64}$") } +private predicate isPinnedContainer(string version) { + version.regexpMatch("^sha256:[A-Fa-f0-9]{64}$") +} bindingset[nwo] private predicate isContainerImage(string nwo) { nwo.regexpMatch("^docker://.+") } From 31913c4a55831bd1fab7da5ee0fb74df8d8193da Mon Sep 17 00:00:00 2001 From: martincostello Date: Fri, 14 Feb 2025 19:46:46 +0000 Subject: [PATCH 9/9] Fix test Fix failing test. --- .../Security/CWE-829/UntrustedCheckoutCritical.expected | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected b/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected index 365e9c823fac..5d7a94aead0b 100644 --- a/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected +++ b/actions/ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected @@ -299,7 +299,9 @@ edges | .github/workflows/test.yml:14:9:25:6 | Run Step | .github/workflows/test.yml:25:9:33:6 | Run Step | | .github/workflows/test.yml:25:9:33:6 | Run Step | .github/workflows/test.yml:33:9:37:34 | Run Step | | .github/workflows/unpinned_tags.yml:9:7:10:4 | Uses Step | .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | -| .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | .github/workflows/unpinned_tags.yml:11:7:11:61 | Uses Step | +| .github/workflows/unpinned_tags.yml:10:7:11:4 | Uses Step | .github/workflows/unpinned_tags.yml:11:7:12:4 | Uses Step | +| .github/workflows/unpinned_tags.yml:11:7:12:4 | Uses Step | .github/workflows/unpinned_tags.yml:12:7:13:4 | Uses Step | +| .github/workflows/unpinned_tags.yml:12:7:13:4 | Uses Step | .github/workflows/unpinned_tags.yml:13:7:13:101 | Uses Step | | .github/workflows/untrusted_checkout2.yml:7:9:14:6 | Run Step: pr_number | .github/workflows/untrusted_checkout2.yml:14:9:19:72 | Run Step | | .github/workflows/untrusted_checkout3.yml:11:9:12:6 | Uses Step | .github/workflows/untrusted_checkout3.yml:12:9:13:6 | Uses Step | | .github/workflows/untrusted_checkout3.yml:12:9:13:6 | Uses Step | .github/actions/dangerous-git-checkout/action.yml:6:7:11:4 | Uses Step |