feat: security test #55
Conversation
github-actions
bot
added
the
action-request
|
hmm, this should not be run for first-time contributors. Did someone approve the workflows? @nodejs/bluesky @nodejs/tsc |
|
Hey @joyeecheung, I hope you are doing good. I'm just trying to do some tests there (I'm part of NodeJS H1 program), if it's bothering you I can stop right now. |
|
Hi @4bg0P I understand that you are testing the security of this repo, thanks. Do I understand correctly that you have write permission to this repository and approved the validation workflow? I am just trying to figure out if there is a loophole in which people without write permission to the repository can send in a PR and trigger the validation workflow with it, without anyone else pushing the approve button. |
|
Hi @joyeecheung, Indeed I don't have any write access to this repository. I was trying to test the In this specific workflow, this trigger is combined with a checkout of the forked repository. It means the all the steps of the workflow will be done based on the fork i.e from files controlled by a potential attacker. This is what I did here, and to be 100% transparent, I tried to make a command injection using the names of the JSON files (as those names are used to fill an environment variable, later used and printed). I hope this will be clearer! If you want to add safety on this workflow, I think the best would be to set the "pull_request" trigger instead of the "pull_request_target"; it would at least require the attacker to have been approved one time, and to have contribute to the repository. |
|
cc @aduh95 |
|
Thanks for the heads up! The workflow was changed in #31 to use |
|
Indeed once the command injection in place, I wanted to send it to my server (in a secure way) and then test it, to report it on https://hackerone.com/nodejs Have a great day, |
See #55 - using pull_request_target would allow the workflow to run without authorization. While there are some code in this workflow to defend against naive attacks e.g. adding scripts to the actions, there could be other attack vectors e.g. via specially crafted branch names or file names that evade GitHub's escape rules. It would be too hard to wrap our head around this, so the easiest way to defend against it would be to restrict full validation that require access to secrets to PRs opened from branches in this repo. For PRs opened from forks, I think we should go back to what I proposed in #10 - that is, only run local validations that require no access to secrets, and use `pull_request` for it. cc @aduh95 @nodejs/tsc
See #55 - using pull_request_target would allow the workflow to run without authorization. While there are some code in this workflow to defend against naive attacks e.g. adding scripts to the actions, there could be other attack vectors e.g. via specially crafted branch names or file names that evade GitHub's escape rules. It would be too hard to wrap our head around this, so the easiest way to defend against it would be to restrict full validation that require access to secrets to PRs opened from branches in this repo. For PRs opened from forks, I think we should go back to what I proposed in #10 - that is, only run local validations that require no access to secrets, and use `pull_request` for it. cc @aduh95 @nodejs/tsc
This is a security test