Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize PR number from artifact #616

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AdnaneKhan
Copy link

Technically the PR number could be anything from the first workflow if from a PR and therefore could be used to inject code. This fixes it.

@everdimension
Copy link
Member

Can you please give more details about what problem this update is solving?

@AdnaneKhan
Copy link
Author

Can you please give more details about what problem this update is solving?

The artifact uploaded on a pull_request workflow can be modified so that it contains a PR number that is in fact a script injection payload. You can see in the previous step it just reads the PR number from a file and sets it to the output variable.

   pr_number=$(cat ./pr_number)
          build_artifact_id=$(curl -H "Accept: application/vnd.github+json" -H "Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}" "https://api.github.com/repos/${{ github.repository }}/actions/runs/${{ github.event.workflow_run.id }}/artifacts" | jq ".artifacts[1].id")
          # We can not use "url" or "archive_download_url" here,
          # hence we need to figure out the artifact's URL "manually"
          build_download_url=${repository_url}/suites/${check_suite_id}/artifacts/${build_artifact_id}
          set +x
          echo "pr_number=$pr_number" >> $GITHUB_OUTPUT
          echo "build_download_url=$build_download_url" >> $GITHUB_OUTPUT

So if someone modified the pr.yml workflow in their fork pull request to upload a malicious PR number that contained javascript code, they could conduct a poisoned pipeline execution attack.

Reference

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants