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

ci: fix RCE vulnerability in file overwrite #10985

Merged
merged 1 commit into from
May 20, 2024
Merged

ci: fix RCE vulnerability in file overwrite #10985

merged 1 commit into from
May 20, 2024

Conversation

sxzz
Copy link
Member

@sxzz sxzz commented May 20, 2024

Critical RCE Vulnerability:

An attacker can exploit this vulnerability by uploading files named scripts/size-report.ts, which will overwrite the existing file and subsequently execute the injected code.

This flaw grants the attacker the ability to execute arbitrary code in a secure environment that has write permissions to both pull-requests and issues.

Special thanks to @RedYetiDev

@sxzz sxzz added security Pull requests that address a security vulnerability 🔥 p5-urgent Priority 5: this fixes build-breaking bugs that affect most users and should be released ASAP. labels May 20, 2024
Copy link

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 90.7 kB 34.5 kB 31.1 kB
vue.global.prod.js 148 kB 53.7 kB 48.1 kB

Usages

Name Size Gzip Brotli
createApp 50.8 kB 19.9 kB 18.1 kB
createSSRApp 54.1 kB 21.2 kB 19.3 kB
defineCustomElement 53.1 kB 20.6 kB 18.8 kB
overall 64.5 kB 24.9 kB 22.6 kB

@sxzz sxzz changed the title ci: fix overwrite existing files ci: fix RCE vulnerability in file overwrite May 20, 2024
@sxzz sxzz merged commit 8bf1469 into main May 20, 2024
15 checks passed
@sxzz sxzz deleted the ci/fix-overwrite branch May 20, 2024 23:05
@RedYetiDev
Copy link
Contributor

RedYetiDev commented May 20, 2024

It was an pleasure to help :-)

sxzz added a commit to vuejs/core-vapor that referenced this pull request May 21, 2024
@Disservin
Copy link
Contributor

Disservin commented May 21, 2024

out of curiosity, what is wrong with ${{ github.event.number }} ?

@sxzz
Copy link
Member Author

sxzz commented May 21, 2024

size-data and size-report are two distinct workflows. The former runs in an untrusted environment (lacking permissions to write issue/PR comments), while the latter operates in a trusted environment with the necessary permissions.

To link these workflows, we need to pass the target PR number as an argument to the trusted environment via artifacts.

@Disservin
Copy link
Contributor

Sure yes the environments are different, but to me it looks like size-report still knows about the pr environment which triggered it because it still thinks it is a pr

    if: >
      github.event.workflow_run.event == 'pull_request' &&
      github.event.workflow_run.conclusion == 'success'

in my mind replacing

number: ${{ steps.pr-number.outputs.content }}

with

number: ${{ github.event.number }}

should have the same end result without having to upload the pr number in workflow a) and then downloading it in workflow b)

@sxzz
Copy link
Member Author

sxzz commented May 21, 2024

Did you try it? IIRC, it doesn't work. According to GitHub Docs, Webhook event payload is workflow_run object

@Disservin
Copy link
Contributor

I haven't that's why I first asked if i missed anything. I can give it a shot later, the webhook event payload also seems to have an array of objects named pull_requests these objects seem to have the pr number.. i wonder why it is returning an array though

@RedYetiDev
Copy link
Contributor

RedYetiDev commented May 21, 2024

As far as I know, pull_requests[0] with the pull_request event will always be the submitted PR.

@sxzz sxzz mentioned this pull request May 28, 2024
wangdaoo pushed a commit to wangdaoo/core that referenced this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔥 p5-urgent Priority 5: this fixes build-breaking bugs that affect most users and should be released ASAP. security Pull requests that address a security vulnerability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants