From 8707d34dde5b7c7ac39ef76ef42d3199304f7052 Mon Sep 17 00:00:00 2001 From: deacon-mp Date: Mon, 18 May 2026 18:50:53 -0400 Subject: [PATCH] security(ci): fix SONAR_TOKEN exfiltration via pull_request_target sonar_fork_pr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sonar_fork_pr job in .github/workflows/quality.yml (introduced in #3213) ran under pull_request_target, checked out the fork's PR HEAD into pr/, then invoked the Sonar scanner with projectBaseDir pointing at that fork-controlled directory while SONAR_TOKEN and GITHUB_TOKEN were in the environment. The scanner resolves sonar-project.properties from projectBaseDir, so a fork PR could ship its own properties file overriding sonar.host.url and have the scanner authenticate to an attacker-controlled host — leaking SONAR_TOKEN in the Authorization header. Reported externally on 2026-05-18. Fix splits trusted and untrusted work along the GitHub-recommended pattern: * .github/workflows/quality.yml - drop the pull_request_target trigger entirely - drop the sonar_fork_pr job entirely - in the existing (secret-less) fork build, upload coverage.xml + PR metadata + a source tarball as a 3-day artifact - fork-controlled values pass through env vars + jq, never inline ${{ }} interpolation into a run: block * .github/workflows/sonar-fork-pr.yml (new) - workflow_run trigger (always uses the default-branch definition, cannot be replaced by a fork) - checks out master (trusted root sonar-project.properties) - downloads the fork artifact as DATA, validates pr-meta.json fields against strict regexes, cross-checks pr_number and head_sha with the GitHub-supplied workflow_run context - runs the scanner from the trusted base root with sonar.host.url / .organization / .projectKey pinned on the CLI (CLI overrides any properties file, belt + suspenders) - feeds PR source as -Dsonar.sources only — never as scanner config SONAR_TOKEN must be rotated in SonarCloud independently of this commit; the job has been exploitable on public master since 2025-10-06. Refs: #3213 --- .github/workflows/quality.yml | 86 +++++++-------- .github/workflows/sonar-fork-pr.yml | 158 ++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 43 deletions(-) create mode 100644 .github/workflows/sonar-fork-pr.yml diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 1c6935d00..edcc90689 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -6,8 +6,6 @@ on: - master pull_request: types: [opened, synchronize, reopened, ready_for_review] - pull_request_target: - types: [opened, synchronize, reopened, ready_for_review] workflow_dispatch: permissions: @@ -16,7 +14,6 @@ permissions: jobs: build: runs-on: ubuntu-latest - if: github.event_name != 'pull_request_target' permissions: contents: read pull-requests: read @@ -68,48 +65,51 @@ jobs: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} - sonar_fork_pr: - runs-on: ubuntu-latest - if: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.fork }} - permissions: - contents: read - pull-requests: write - steps: - - name: Checkout base repo - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 - with: - ref: ${{ github.event.pull_request.base.sha }} - fetch-depth: 0 - - - name: Checkout PR HEAD (fork) - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 - with: - repository: ${{ github.event.pull_request.head.repo.full_name }} - ref: ${{ github.event.pull_request.head.sha }} - path: pr - fetch-depth: 0 - submodules: recursive - - - name: Detect Sonar base dir - id: detect + # ------------------------------------------------------------------ + # Fork PR analysis is handed off to the trusted .github/workflows/ + # sonar-fork-pr.yml workflow via workflow_run, which executes against + # a TRUSTED base checkout and treats the artifacts uploaded below as + # untrusted data only. See SECURITY note in that workflow. + # ------------------------------------------------------------------ + - name: Stage fork PR Sonar artifacts + if: ${{ matrix.python-version == '3.13' && github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true }} + env: + # Fork-controlled fields. Pass through env vars and jq so they + # cannot inject into the shell or the resulting JSON. The trusted + # sonar-fork-pr.yml workflow re-validates everything before use. + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + HEAD_REF: ${{ github.event.pull_request.head.ref }} + BASE_SHA: ${{ github.event.pull_request.base.sha }} + BASE_REF: ${{ github.event.pull_request.base.ref }} + HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} run: | set -euo pipefail - if [ -f pr/caldera/sonar-project.properties ]; then - echo "base=pr/caldera" >> "$GITHUB_OUTPUT" - elif [ -f pr/sonar-project.properties ]; then - echo "base=pr" >> "$GITHUB_OUTPUT" - else - echo "base=pr" >> "$GITHUB_OUTPUT" + mkdir -p sonar-fork-pr-artifact + if [ -f coverage.xml ]; then + cp coverage.xml sonar-fork-pr-artifact/coverage.xml fi + jq -n \ + --arg pr_number "$PR_NUMBER" \ + --arg head_sha "$HEAD_SHA" \ + --arg head_ref "$HEAD_REF" \ + --arg base_sha "$BASE_SHA" \ + --arg base_ref "$BASE_REF" \ + --arg head_repo "$HEAD_REPO" \ + '{pr_number:$pr_number, head_sha:$head_sha, head_ref:$head_ref, base_sha:$base_sha, base_ref:$base_ref, head_repo:$head_repo}' \ + > sonar-fork-pr-artifact/pr-meta.json + tar --exclude='./.git' \ + --exclude='./node_modules' \ + --exclude='./plugins/magma/node_modules' \ + --exclude='./plugins/magma/dist' \ + --exclude='./sonar-fork-pr-artifact' \ + -czf sonar-fork-pr-artifact/pr-source.tar.gz ./app ./sonar-project.properties || true - - name: SonarQube Scan (fork PR) - uses: SonarSource/sonarqube-scan-action@v6.0.0 - env: - SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Upload fork PR Sonar artifacts + if: ${{ matrix.python-version == '3.13' && github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == true }} + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: - projectBaseDir: ${{ steps.detect.outputs.base }} - args: | - -Dsonar.pullrequest.key=${{ github.event.pull_request.number }} - -Dsonar.pullrequest.branch=${{ github.event.pull_request.head.ref }} - -Dsonar.pullrequest.base=${{ github.event.pull_request.base.ref }} + name: sonar-fork-pr + path: sonar-fork-pr-artifact/ + retention-days: 3 + if-no-files-found: warn diff --git a/.github/workflows/sonar-fork-pr.yml b/.github/workflows/sonar-fork-pr.yml new file mode 100644 index 000000000..5316dc6bd --- /dev/null +++ b/.github/workflows/sonar-fork-pr.yml @@ -0,0 +1,158 @@ +name: SonarQube Scan (Fork PR) + +# ---------------------------------------------------------------------------- +# SECURITY MODEL — read before editing. +# +# This workflow analyses fork pull requests with SonarCloud while holding the +# SONAR_TOKEN. The previous design (pull_request_target + checkout of PR HEAD +# + projectBaseDir pointing at fork-controlled content) was a textbook +# pwn-request: a fork PR could ship its own sonar-project.properties under the +# detected base dir and redirect the scanner's sonar.host.url, exfiltrating +# SONAR_TOKEN to an attacker host. CVE-class. Do not reintroduce that pattern. +# +# This workflow is safe because: +# +# 1. workflow_run executes against the workflow definition on the DEFAULT +# branch, never the PR HEAD. A fork cannot replace this file. +# 2. The repository is checked out at the BASE commit (master), so the +# trusted root sonar-project.properties is what the scanner reads. +# 3. The fork's coverage report and source archive are downloaded as +# ARTIFACTS — treated as untrusted DATA. We never execute anything from +# them (no pip/npm install, no shell). The source is only fed to the +# scanner as files to analyse. +# 4. sonar.host.url / sonar.organization / sonar.projectKey are forced on +# the scanner CLI. CLI arguments take precedence over any properties +# file, so even if the fork smuggled one through it cannot redirect the +# scanner's authentication target. +# 5. PR metadata fields that flow into the scanner CLI (head_ref, base_ref, +# pr_number) are validated against strict regexes before use. Untrusted +# strings never expand directly inside a `run:` block. +# +# If you need to relax any of these constraints, get a security review first. +# ---------------------------------------------------------------------------- + +on: + workflow_run: + workflows: ["Code Quality"] + types: [completed] + +permissions: + contents: read + +jobs: + sonar_fork_pr: + runs-on: ubuntu-latest + if: >- + ${{ github.event.workflow_run.event == 'pull_request' + && github.event.workflow_run.conclusion == 'success' + && github.event.workflow_run.head_repository.fork == true }} + permissions: + contents: read + pull-requests: write + steps: + - name: Checkout trusted base (master) + uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4 + with: + ref: master + fetch-depth: 0 + + - name: Download fork PR artifact + uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4 + with: + name: sonar-fork-pr + path: fork-artifact + github-token: ${{ secrets.GITHUB_TOKEN }} + run-id: ${{ github.event.workflow_run.id }} + + - name: Validate fork PR metadata + id: meta + env: + # workflow_run-provided values are trusted (they come from GitHub, + # not the fork). Use these for cross-checks. + WR_PR_NUMBER: ${{ github.event.workflow_run.pull_requests[0].number }} + WR_HEAD_SHA: ${{ github.event.workflow_run.head_sha }} + run: | + set -euo pipefail + + if [ ! -f fork-artifact/pr-meta.json ]; then + echo "::error::pr-meta.json missing from fork artifact" + exit 1 + fi + + # Parse with jq, then validate each field with a strict regex + # BEFORE writing to GITHUB_OUTPUT. Anything failing validation is + # fatal — better to skip the analysis than to feed an attacker + # string into the scanner CLI. + pr_number=$(jq -r '.pr_number // empty' fork-artifact/pr-meta.json) + head_sha=$( jq -r '.head_sha // empty' fork-artifact/pr-meta.json) + head_ref=$( jq -r '.head_ref // empty' fork-artifact/pr-meta.json) + base_sha=$( jq -r '.base_sha // empty' fork-artifact/pr-meta.json) + base_ref=$( jq -r '.base_ref // empty' fork-artifact/pr-meta.json) + + # PR number must match the one GitHub itself reports for this run. + if [ -z "$pr_number" ] || [ "$pr_number" != "$WR_PR_NUMBER" ]; then + echo "::error::pr_number from artifact ($pr_number) does not match workflow_run PR ($WR_PR_NUMBER)" + exit 1 + fi + # Head SHA must match the one GitHub reports. + if [ -z "$head_sha" ] || [ "$head_sha" != "$WR_HEAD_SHA" ]; then + echo "::error::head_sha from artifact ($head_sha) does not match workflow_run head ($WR_HEAD_SHA)" + exit 1 + fi + # Branch names: git allows a lot, but Sonar only needs something + # that round-trips through a CLI arg. Keep it conservative. + ref_re='^[A-Za-z0-9._/-]{1,250}$' + if ! [[ "$head_ref" =~ $ref_re ]]; then + echo "::error::head_ref failed validation: $head_ref" + exit 1 + fi + if ! [[ "$base_ref" =~ $ref_re ]]; then + echo "::error::base_ref failed validation: $base_ref" + exit 1 + fi + sha_re='^[0-9a-f]{40}$' + if ! [[ "$base_sha" =~ $sha_re ]]; then + echo "::error::base_sha failed validation: $base_sha" + exit 1 + fi + + { + echo "pr_number=$pr_number" + echo "head_ref=$head_ref" + echo "base_ref=$base_ref" + } >> "$GITHUB_OUTPUT" + + - name: Stage PR sources for analysis + run: | + set -euo pipefail + mkdir -p pr-src + if [ -f fork-artifact/pr-source.tar.gz ]; then + tar -xzf fork-artifact/pr-source.tar.gz -C pr-src + else + echo "::warning::pr-source.tar.gz missing; scanner will run with no source overlay" + fi + if [ -f fork-artifact/coverage.xml ]; then + cp fork-artifact/coverage.xml ./coverage.xml + else + echo "::warning::coverage.xml missing; running scanner without coverage data" + fi + + - name: SonarQube Scan (fork PR, trusted base) + uses: SonarSource/sonarqube-scan-action@v6.0.0 + env: + SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }} + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + with: + # Run from the TRUSTED base checkout so the trusted root + # sonar-project.properties is what gets loaded. The fork's source + # tree is fed in below via -Dsonar.sources only. + projectBaseDir: . + args: >- + -Dsonar.host.url=https://sonarcloud.io + -Dsonar.organization=mitre + -Dsonar.projectKey=mitre_caldera + -Dsonar.sources=pr-src/app + -Dsonar.python.coverage.reportPaths=coverage.xml + -Dsonar.pullrequest.key=${{ steps.meta.outputs.pr_number }} + -Dsonar.pullrequest.branch=${{ steps.meta.outputs.head_ref }} + -Dsonar.pullrequest.base=${{ steps.meta.outputs.base_ref }}