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

Quote $GITHUB_HEAD_REF on release.yml #1448

Merged
merged 2 commits into from
May 9, 2024
Merged

Quote $GITHUB_HEAD_REF on release.yml #1448

merged 2 commits into from
May 9, 2024

Conversation

h2oa
Copy link
Contributor

@h2oa h2oa commented May 9, 2024

Hi cml security team,

I submitted a report of vulnerability on huntr.com. I see your product run a bug bounty program on this platform. You can connect to the huntr admin to see details of the report at https://huntr.com/bounties/2113dbb3-8427-4b77-913a-15a95bf68922. This pull request is a patch for this vulnerability. Because this is a dangerous vulnerability, please consider it as quickly as possible!

@0x2b3bfa0
Copy link
Member

Thanks for the report, @h2oa! Unfortunately, it seems like we can't access it on huntr.com due to lack of permissions, like in other report you submitted previously.

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allegedly vulnerable job can only run if the following condition is met:

if: github.event.pull_request.merged && startsWith(github.head_ref, 'bump/')

This implies that, at very least, you'd need someone to approve & merge a pull request with a malicious branch name to trigger the vulnerable job.

Proof of concept

Trick a maintainer into merging this, if you can. 🙃

Minimal vulnerable workflow

on: pull_request
jobs:
  test:
    runs-on: ubuntu-latest
    steps:
    - uses: actions/checkout@v4
    - run: echo ${{ github.head_ref }}

Minimal malicious pull request

  • Branch name: bump/$(bash<script)
  • Contents: create a file named script with arbitrary code

.github/workflows/release.yml Outdated Show resolved Hide resolved
@0x2b3bfa0 0x2b3bfa0 self-assigned this May 9, 2024
@0x2b3bfa0 0x2b3bfa0 added the security Sensitive flaws label May 9, 2024
@h2oa
Copy link
Contributor Author

h2oa commented May 9, 2024

Hi @0x2b3bfa0,

Yes, the vulnerability issue will occur if a pull request is accepted by someone. Therefore, in my report at huntr.com, the User interaction field has the value Required. Can you notify to the admin of huntr.com to consider and change my report on huntr.com to valid, this will help me receive a reward commensurate with my efforts to find vulnerabilities. Thanks a lot!

Your product is running a bug bounty program on the huntr.com platform, so I believe you have the authority to request permission from the huntr.com admin to view the report details.

Best regards,
@h2oa

@0x2b3bfa0 0x2b3bfa0 changed the title Fix vulnerability at release.yml Quote $GITHUB_HEAD_REF on release.yml May 9, 2024
@0x2b3bfa0
Copy link
Member

0x2b3bfa0 commented May 9, 2024

Your product is running a bug bounty program on the huntr.com platform, so I believe you have the authority to request permission from the huntr.com admin to view the report details.

Thanks for clarifying this, @h2oa! Our only official bug bounty program is this one, and we aren't affiliated in any way with huntr.com; I'm trying to contact them, hoping to find out how to access and triage those reports.

@0x2b3bfa0
Copy link
Member

Yes, the vulnerability issue will occur if a pull request is accepted by someone.

If someone managed to trick us into accepting a malicious pull request1 they could find a thousand more subtle and more impactful ways of executing code than this one, e.g. adding a malicious dependency to package-lock.json

Footnotes

  1. It's already too hard to get a legitimate pull request accepted, let alone a malicious one. 🙈

Copy link
Member

@0x2b3bfa0 0x2b3bfa0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Irrelevant security-wise, but using an environment variable is definitely nicer. Thanks for the report and the effort!

@0x2b3bfa0 0x2b3bfa0 merged commit 2675110 into iterative:master May 9, 2024
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
security Sensitive flaws
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants