search

tinted-theming / home

Style systems and smart build tooling for crafting high fidelity color schemes and easily using them in all your favorite apps.
MIT License
254 stars 12 forks source link

Security implications of auto-publishing executable code (in built assets) #21

Closed joshgoebel closed 2 years ago

joshgoebel commented 2 years ago

Maybe be slightly more careful with anything that auto-publishes (Go, NPM, etc) but I'm not sure we have things that auto-publish yet?

Originally posted by @joshgoebel in https://github.com/base16-project/base16/issues/12#issuecomment-1172614478

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...

Things worth considering... some of this is partly mitigated just by protecting the main branch and requiring PR review for builders.

belak commented 2 years ago

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.

joshgoebel commented 2 years ago

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.

belak commented 2 years ago

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.

joshgoebel commented 2 years ago

if you feel strongly about that

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 sudo rm -rf / or rm -rf $HOME when someone installs a Base16 shell color scheme.

if it's an issue in the future for template repos

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.

belak commented 2 years ago

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).

belak commented 2 years ago

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 :laughing:.

joshgoebel commented 2 years ago

(EDIT: I don't see an option for it to be bypassed by certain groups so that idea isn't an option

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"...

belak commented 2 years ago

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.