-
Notifications
You must be signed in to change notification settings - Fork 12
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
Security implications of auto-publishing executable code (in built assets) #21
Comments
I went through and required everything going to main to be via a PR, but I didn't require a review for now. I'm open to changing that if other people have strong opinions. |
Review for review sake is it's own discussion (as brought up elsewhere), but you only close the security hole here by actually requiring review... otherwise a bad actor can just approve their own PR... to be clear I'm specifically talking about builders - which (via GH action) can publish malicious code (into applications via executable theme configs) if it was compromised. The more relaxed we are about commit bits for our builders the more important the review step becomes. I think we either keep the commit list tiny or you go the more trusting route but then put the review protections in place. Admins still (by default) have the ability to bypass review so for emergencies the "admins" could still push a quick fix thru, etc. |
I'm OK with requiring review for builders only if you feel strongly about that - if it's an issue in the future for template repos, we can look at doing it there as well. |
It seems prudent (see the other issue I just filed)... if a builder can generate Bash themes (and any of those themes are just shell scripting - as I imagine some are) they you're just one bad day/night away from
Of course if the built assets are just laying around in the template repos and those bits are wide - then one doesn't even need to attack the builder, they could just commit the bad code directly to the template repos built assets, ugh... It seems far too easy to exploit. Would love another security minded person to weigh in on this. |
You are right that it's easy to exploit. Maybe we can require all repos have PRs reviewed by at least one person, but it's bypassable by admins and maintainers of that template (EDIT: I don't see an option for it to be bypassed by certain groups so that idea isn't an option)? I'd like to avoid review requirements slowing down what momentum we currently have, when it's unlikely to be an issue (since we still need someone we've added to the org to merge it). |
I've updated the builder repos. I really wish there was a way to apply branch restrictions to all repos in an org - I'm getting a little tired of clicking into every repo 😆. |
Yeah I think it's only those with admin permission that can bypass. If we have a few people in the organization with time to do quick reviews I'm not sure that needing a single review will be all that onerous of a requirement... I'm happy to review most things across the organization for now. Scheme and template reviews shouldn't require a ton of "expertise"... |
I'm going to close this for now - if it becomes an issue, we can require approvals on all template repos as well, but I hope we won't need to. |
Originally posted by @joshgoebel in #12 (comment)
Vim is actually an example of the auto-publishing I was getting at here... we should be careful because a single bad actor in the organization (or a compromise of their account) could perhaps insert executable code on a person's computer merely by hacking a color scheme (assuming some yet unknown builder vulnerability).
And if that seems unlikely (lets say we trust our builders implicitly) one could definitely cause issues if they had commit bits to both the builder and the color schemes...
base16-vim/colors
Things worth considering... some of this is partly mitigated just by protecting the
main
branch and requiring PR review for builders.The text was updated successfully, but these errors were encountered: