-
Notifications
You must be signed in to change notification settings - Fork 503
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
Should security-events: read
be considered a dangerous permission?
#3131
Comments
I'm leaning towards "dangerous".
In my opinion yes. If you look at the restricted permissions in GitHub docs, they take a much more restricted approach for defaults. I think the |
According to https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token
which means that it's possible to just open a PR, set |
If by "security issues that are privately reported to the repository" you meant the security dashboard looks like its possible for random PRs to dump it using https://docs.github.com/en/rest/code-scanning?apiVersion=2022-11-28#list-code-scanning-alerts-for-a-repository: evverx/elfutils#120. If that dashboard is considered private it should be reported to the GitHub security probably first because there is nothing projects can do to fix that. Let me see if I can dump the advisories :-) |
Luckily it seems only the security dashboard can be dumped: https://github.com/evverx/elfutils/actions/runs/5219302340/jobs/9420978964. (The second curl command was responsible for dumping the advisories there). |
Just to sump up as long as random PRs can freely elevate their read privileges like that it doesn't matter what projects do. I guess it would be better if projects use, say, |
Thanks a lot for the investigation work @evverx! That's really revealing that a forked PR can create and run a workflow with But giving your discovery that it's possible to dump the security-dashboard with
|
so I'd say this particular privilege boundary can be crossed :-) If projects don't use anything apart from CodeQL they should be fine because it's possible to see the same CodeQL alerts by forking repos and triggering CodeQL there :-) Third-party analyzers with their own tokens writing stuff to the security dashboard are much more interesting. |
For the record I reported that issue to GitHub. I don't participate in bug bounty programs so I reported it via their usual bug tracker. I think either all the alerts including comments explaining why stuff is dismissed should be private as advertised or the alerts should be public so as not to mislead people into thinking that they can keep sensitive stuff there. I'll post their reply here once it's resolved one way or another. |
Thanks @evverx! Do you have any link to the bug you created? I could "thumbs up" if possible |
It's ticket #2199640 but I'm not sure it can be viewed by anyone other than me and GH. I linked that ticket to this issue though so it should be visible there at least. |
Hm sad, but I guess that's what can be done by now. Thanks again, great job! |
Stale issue message |
This issue is stale because it has been open for 60 days with no activity. |
Is your feature request related to a problem? Please describe.
The
security-events: read
grants permissions to read security issues that are privately reported to the repository. This can became dangerous as attackers might have access to vulnerabilities that were not yet resolved and/or not publicly disclosed.Impacts of this decision
permissions: read-all
, as it includes thesecurity-events: read
.The text was updated successfully, but these errors were encountered: