Trusting too Much
After forking Spacebar into Harmony, there were a lot of fixes, early on it was mostly bug fixes and just making the codebase nicer to work with, until we found the barrage of security vulnerabilities in the codebase. there were so many to fix, and while it took a while to find and patch them all, we eventually go through at least most of them.
This blog post is going to go over a few examples, and what exactly went wrong with each, so these mistakes don't need to be repeated, I won't go over literally everything or even most of it, as most of them are pretty similar and it would take way too long to write up about all of them individually.
Just to note, we have notified Spacebar many times, and we gave them a copy of this blog post a week before it was supposed to come out so they could have even more time to fix these issues in their codebase. (edit before posting this, they still have not been patched despite this) I've wanted to write this for a few months now, and I'm tired of waiting, with this blog being fixed, it is the perfect time to write this up.
Trusting IDs
The most common mistake in the codebase was trusting IDs, for example, the case of the wandering webhook. The wandering webhook was a bug that was caused due to trusting an ID, normally this would be fine, as most IDs are checked by the middleware, but IDs passed in the bodies of requests are not checked.
On the PATCH /webhooks/#webhook_id API you can set a new channel for the webhook to be in, this should be fine right? it checks that you have the proper permissions in the guild to edit the webhook, which is the main important check. But no one thought about what happens if the channel_id is from another guild, which means you can move the webhook to a guild which it was not from, nor that you had permissions to set it to. This oversight allows you to send a message in any channel you wanted, so long as you had an ID, and a webhook to edit.
The whole interaction system was plagued with this exact bug too, where the client would send a message ID, along with the bot ID, the message ID was properly checked, but the bot ID was never checked, so you could in effect start an interaction with anyone's message and say that someone else should get the token that allows them to edit, reply, and more that message. This could've been avoided just by checking that the author of the message and the passed in bot ID were the same.
And many more vulnerabilities like this, including pinning messages, automod endpoints, command edit endpoints, system channels, adding people to group DMs, message deletion, and much more. This is an easy mistake to make, assuming that auth happened once, so you're done, when sometimes you need to go through extra steps of authorization, even when your first round of auth was right.
Forgetting to Authenticate
Another one of the common problems was forgetting to authenticate some endpoints. The way the codebase was set up, authentication was only done when you asked for it by the middleware, instead of only not doing it when you asked for it not to be done, which caused many issues in the codebase, where the authentication checks needed to be moved from not checking permissions, to checking that you have access to the resource in the first place.
One endpoint that was effected was the endpoint to add reactions, which before would let you add reactions to any message you wanted, even for messages that you had no access to, or getting the threads within a channel was unchecked, along with viewing the guild templates for a guild. Activating webhooks would not check their auth token, which caused many security issues, allowing anyone to send messages from webhooks.
These were rarer and were easily mitigated by just making it so the endpoints just checked the needed permissions, making it opt out made the compiler complain about all of the places that were missing checks.
Gateway
For the most part, gateway was screwed up in ways that are hard to really describe with examples, but was generally messy. I had to redo many parts of the gateway to get it working in a much more secure way.
The easiest to talk about is the channel subscription process for people who joined a guild, which is rather simple, it subscribes you to events for the guild and all of the channels within the guild, sounds correct, right? It's rather incorrect, it should only subscribe you two events for channels you have permission to view, which means new members can read messages in channels they are not actually able to view.
Permission checking was also sorta broken too, with channel permissions being recalculated from the previous permission state instead of from the guild permission state, which has some weird consequences where someone may not have permission to view a channel without refreshing on the gateway, or the other way around, mostly a source of some buggy behaviors, but it does have the potential of having some major security vulnerabilities.
The last thing is the checking of gateway objects was rather lax, which while I didn't search for what exactly that meant for security, but there are likely some serious implications. The codebase never checked for extra properties on objects, which means they were preserved, and some objects it neglects to check outside of checking if they're an object, which is not nearly enough. This was fixed by using the APIs type checker on the gateway, which was a bit of a hassle but it did end up simplifying gateway logic and overall making it more secure.
Conclusions
Overall, this whole process has been a real show of what not to do, and a lot of common security flaws, along with a few more complicated ones. This partly helps to justify the fork and what all that entails, as some of these flaws require sweeping changes to the codebase, which would be hard to get merged on Spacebar. We also tried to disclose these flaws many times to Spacebar, giving the various resources to them, initially giving them links to our patches, then links to a list of every flaw, then a week early edition of this blog post, but most of the problems still remain.
I don't think Spacebar is a bad project, and please do not attack them with these active vulnerabilities.
There were many other issues that I had found and have not talked about here, mostly to do with logic errors, mostly with the whole MFA system, not that they're very exploitable as is.