hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2026-01-07 19:50:22 +01:00
Code Issues Packages Projects Releases Wiki Activity

Improve ControlCheck for untrusted checkouts

Browse Source
This commit is contained in:
Alvaro Muñoz
2024-05-24 09:33:35 +02:00
parent 16a7522807
commit 1fc45eb296
10 changed files with 177 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
ql/lib/codeql/actions/security/UntrustedCheckoutQuery.qll
View File

@@ -227,7 +227,14 @@ class GhSHACheckout extends SHACheckoutStep instanceof Run {
}
/** An If node that contains an actor, user or label check */
abstract class ControlCheck extends If { }
abstract class ControlCheck extends If {
predicate dominates(Step step) {
step.getIf() = this or
step.getEnclosingJob().getIf() = this or
step.getEnclosingJob().getANeededJob().(LocalJob).getAStep().getIf() = this or
step.getEnclosingJob().getANeededJob().(LocalJob).getIf() = this
}
}
class LabelControlCheck extends ControlCheck {
LabelControlCheck() {
@@ -244,15 +251,28 @@ class LabelControlCheck extends ControlCheck {
class ActorControlCheck extends ControlCheck {
ActorControlCheck() {
// eg: contains(github.actor, 'dependabot')
// eg: github.triggering_actor != 'CI Agent'
// eg: github.actor == 'dependabot[bot]'
// eg: github.triggering_actor == 'CI Agent'
// eg: github.event.pull_request.user.login == 'mybot'
exists(
normalizeExpr(this.getCondition())
.regexpFind([
"\\bgithub\\.actor\\b", "\\bgithub\\.triggering_actor\\b",
"\\bgithub\\.event\\.comment\\.user\\.login\\b",
"\\bgithub\\.event\\.pull_request\\.user\\.login\\b",
"\\bgithub\\.event\\.pull_request\\.user\\.login\\b",
], _, _)
)
}
}
class RepositoryControlCheck extends ControlCheck {
RepositoryControlCheck() {
// eg: github.repository == 'test/foo'
exists(
normalizeExpr(this.getCondition())
.regexpFind([
"\\bgithub\\.repository\\b",
"\\bgithub\\.repository_owner\\b",
], _, _)
)
}

6
ql/src/Security/CWE-367/UntrustedCheckoutTOCTOUHigh.ql
View File

@@ -2,9 +2,9 @@
* @name Untrusted Checkout TOCTOU
* @description Untrusted Checkout is protected by a security check but the checked-out branch can be changed after the check.
* @kind problem
* @problem.severity warning
* @precision medium
* @security-severity 5.3
* @problem.severity error
* @precision high
* @security-severity 7.5
* @id actions/untrusted-checkout-toctou/high
* @tags actions
* security

7
ql/src/Security/CWE-829/UntrustedCheckoutCritical.ql
View File

@@ -21,10 +21,11 @@ from LocalJob j, PRHeadCheckoutStep checkout
where
j = checkout.getEnclosingJob() and
j.getAStep() = checkout and
// the checkout is followed by a known poisonable step
checkout.getAFollowingStep() instanceof PoisonableStep and
not exists(ControlCheck check |
checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check
) and
// the checkout is not controlled by an access check
not exists(ControlCheck check | check.dominates(checkout)) and
// the checkout occurs in a privileged context
(
inPrivilegedCompositeAction(checkout)
or

13
ql/src/Security/CWE-829/UntrustedCheckoutHigh.ql
View File

@@ -4,9 +4,9 @@
* By explicitly checking out and running the build script from a fork the untrusted code is running in an environment
* that is able to push to the base repository and to access secrets.
* @kind problem
* @problem.severity warning
* @precision medium
* @security-severity 5.3
* @problem.severity error
* @precision high
* @security-severity 7.5
* @id actions/untrusted-checkout/high
* @tags actions
* security
@@ -21,10 +21,11 @@ from LocalJob j, PRHeadCheckoutStep checkout
where
j = checkout.getEnclosingJob() and
j.getAStep() = checkout and
// the checkout is NOT followed by a known poisonable step
not checkout.getAFollowingStep() instanceof PoisonableStep and
not exists(ControlCheck check |
checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check
) and
// the checkout is not controlled by an access check
not exists(ControlCheck check | check.dominates(checkout)) and
// the checkout occurs in a privileged context
(
inPrivilegedCompositeAction(checkout)
or

6
ql/src/Security/CWE-829/UntrustedCheckoutMedium.ql
View File

@@ -21,9 +21,9 @@ from LocalJob j, PRHeadCheckoutStep checkout
where
j = checkout.getEnclosingJob() and
j.getAStep() = checkout and
not exists(ControlCheck check |
checkout.getIf() = check or checkout.getEnclosingJob().getIf() = check
) and
// the checkout is not controlled by an access check
not exists(ControlCheck check | check.dominates(checkout)) and
// the checkout occurs in a non-privileged context
(
inNonPrivilegedCompositeAction(checkout) or
inNonPrivilegedJob(checkout)

45
ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot1.yml vendored Normal file
View File

@@ -0,0 +1,45 @@
name: Check dist
on:
pull_request:
push:
branches:
- main
- 'releases/*'
jobs:
verify-build: # make sure the checked in dist/ folder matches the output of a rebuild
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
- name: Read .nvmrc
id: nvm
run: echo "NVMRC=$(cat .nvmrc)" >> $GITHUB_OUTPUT
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: ${{ steps.nvm.outputs.NVMRC }}
- name: Install npm dependencies
run: npm clean-install
- name: Rebuild the dist/ directory
run: npm run package
- name: Compare the expected and actual dist/ directories
run: script/check-diff
verify-index-js: # make sure the entrypoint js files run on a clean machine without compiling first
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.sha }}
- uses: ./
with:
milliseconds: 1000

68
ql/test/query-tests/Security/CWE-829/.github/workflows/dependabot2.yml vendored Normal file
View File

@@ -0,0 +1,68 @@
name: Compile dependabot updates
on:
pull_request:
permissions:
pull-requests: write
contents: write
jobs:
fetch-dependabot-metadata:
runs-on: ubuntu-latest
# We only want to check the metadata on pull_request events from Dependabot itself,
# any subsequent pushes to the PR should just skip this step so we don't go into
# a loop on commits created by the `build-dependabot-changes` job
if: ${{ github.actor == 'dependabot[bot]' }}
# Map the step output to a job output for subsequent jobs
outputs:
dependency-type: ${{ steps.dependabot-metadata.outputs.dependency-type }}
package-ecosystem: ${{ steps.dependabot-metadata.outputs.package-ecosystem }}
steps:
- name: Fetch dependabot metadata
id: dependabot-metadata
uses: dependabot/fetch-metadata@c9c4182bf1b97f5224aee3906fd373f6b61b4526 # v1.6.0
with:
github-token: "${{ secrets.GITHUB_TOKEN }}"
build-dependabot-changes:
runs-on: ubuntu-latest
needs: [fetch-dependabot-metadata]
# We only need to build the dist/ folder if the PR relates to Docker or an npm dependency
if: needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'docker' || needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'npm_and_yarn'
steps:
# Check out using a PAT so any pushed changes will trigger checkruns
- uses: actions/checkout@v4
with:
ref: ${{ github.event.pull_request.head.ref }}
token: ${{ secrets.DEPENDABOT_AUTOBUILD }}
- name: Read .nvmrc
id: nvm
run: echo "NVMRC=$(cat .nvmrc)" >> $GITHUB_OUTPUT
- name: Setup Node.js
uses: actions/setup-node@v4
with:
node-version: ${{ steps.nvm.outputs.NVMRC }}
- name: Install npm dependencies
run: npm clean-install
# If we're reacting to a Docker PR, we have on extra step to refresh and check in the container manifest,
# this **must** happen before rebuilding dist/ so it uses the new version of the manifest
- name: Rebuild docker/containers.json
if: needs.fetch-dependabot-metadata.outputs.package-ecosystem == 'docker'
run: |
npm run update-container-manifest
git add docker/containers.json
- name: Rebuild the dist/ directory
run: npm run package
- name: Check in any change to dist/
run: |
git add dist/
# Specifying the full email allows the avatar to show up: https://github.com/orgs/community/discussions/26560
git config user.name "github-actions[bot]"
git config user.email "41898282+github-actions[bot]@users.noreply.github.com"
git commit -m "[dependabot skip] Update dist/ with build changes" || exit 0
git push

20
ql/test/query-tests/Security/CWE-829/.github/workflows/test2.yml vendored Normal file
View File

@@ -0,0 +1,20 @@
name: "Frogbot Scan Pull Request"
on:
pull_request_target:
types: [ opened, synchronize ]
permissions:
pull-requests: write
contents: read
jobs:
scan-pull-request:
runs-on: ubuntu-latest
environment: frogbot
steps:
- uses: actions/checkout@v2
with:
ref: ${{ github.event.pull_request.head.sha }}
- uses: jfrog/frogbot@8fbeca612957ae5f5f0c03a19cb6e59e237026f3 # v2.10.0
env:
JF_URL: ${{ secrets.JF_URL }}
JF_ACCESS_TOKEN: ${{ secrets.JF_ACCESS_TOKEN }}
JF_GIT_TOKEN: ${{ secrets.GITHUB_TOKEN }}

2
ql/test/query-tests/Security/CWE-829/UntrustedCheckoutCritical.expected
View File

@@ -1,5 +1,7 @@
| .github/workflows/auto_ci.yml:20:9:27:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/auto_ci.yml:67:9:74:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/dependabot1.yml:15:9:19:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/dependabot1.yml:39:9:43:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/gitcheckout.yml:10:11:18:8 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/level0.yml:99:9:103:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/level0.yml:125:9:129:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |

1
ql/test/query-tests/Security/CWE-829/UntrustedCheckoutHigh.expected
View File

@@ -15,6 +15,7 @@
| .github/workflows/issue_comment_octokit.yml:79:9:83:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/issue_comment_octokit.yml:95:9:100:2 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/issue_comment_octokit.yml:109:9:114:66 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/test2.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/untrusted_checkout2.yml:14:9:19:72 | Run Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/workflow_run_untrusted_checkout.yml:13:9:16:6 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |
| .github/workflows/workflow_run_untrusted_checkout.yml:16:9:18:31 | Uses Step | Potential unsafe checkout of untrusted pull request on privileged workflow. |