Fix PWN vulnerability in workflow files#4657
Conversation
| paths: ["adapters/*/*.go"] | ||
|
|
||
| permissions: | ||
| contents: read |
There was a problem hiding this comment.
Write access is required because the results of the code coverage scan are uploaded to a branch for viewability. Does this PR offer a safer alternative?
There was a problem hiding this comment.
Hi @SyntaxNode, @jun06t,
I responsibly disclosed this vulnerability via email (and not through a public PR), specifically to avoid exposing the details to the world and to prevent potential abuse. It’s been over a week and I haven’t received a response yet.
Could you please confirm whether this fix is related to my disclosure?
There was a problem hiding this comment.
@SyntaxNode
Thanks for your review.
I've just fixed it.
@barakharyati
Thank you for your report.
I'm not a member of the Prebid team, but this fix addresses that vulnerability.
Since I wasn't able to create a repository security advisory in this repository, I created this PR instead.
There was a problem hiding this comment.
@SyntaxNode
Now that it's public, I strongly recommend turning off the vulnerable workflows ASAP until they're fixed.
@jun06t, if you run it in PR and not PRT, the Job will not have access to the base repo for any update.
Running checkout without the head ref will scan the base repo code.
The right way to solve is to separate Jobs,
First Job with no permission that will run the tainted code that will create an artifact or output
Second, with permissions that will download the artifact or output and commit \ change PR.
There was a problem hiding this comment.
@barakharyati
Thank you for your suggestion.
I've split the workflow into two stages. Could you please verify if this implementation aligns with your intention?
Change pull_request_target to pull_request trigger and remove explicit checkout of PR head ref to prevent potential code injection attacks. - adapter-code-coverage.yml: Change trigger and remove ref/repository params - semgrep.yml: Change trigger and remove ref/repository params
jun06t
force-pushed
the
fix/pwn-vulnerability-workflow
branch
from
d203747 to
15b8df9
Compare
Split the workflow into two stages for better security: Stage 1 (adapter-code-coverage.yml): - Runs on pull_request trigger with read-only permissions - Executes coverage tests on PR code - Uploads coverage results and PR metadata as artifacts Stage 2 (adapter-code-coverage-upload.yml): - Runs on workflow_run trigger after stage 1 completes - Has write permissions but never executes PR code - Downloads artifacts, commits to coverage-preview branch - Posts coverage summary comment to PR This approach prevents untrusted PR code from accessing repository secrets while maintaining full functionality for fork PRs.
3 participants
Summary
adapter-code-coverage.ymlandsemgrep.ymlpull_request_targettrigger topull_requestref/repositoryparameters)Background
When using
pull_request_targettrigger with checkout of PR code, malicious PRs from forks can execute code with access to base repository secrets.Workflow Analysis
pull_request_targethead.ref)head.ref)pull_request)pull_request)Changes
pull_request_target→pull_request, remove ref/repository paramspull_request_target→pull_request, remove ref/repository paramsTest plan