Rails: SQL Injection over Configuration

It was an interesting weekend for the Github team, the Rails core team, and lots of Rails users who worked at all through the weekend. There are a lot of details about the weekend to discuss, but my main discussion point is one of philosophy and intention of the Rails project. We’ll get to that towards the end. First, a bit of background.

Hacking Github

This weekend, a Github user named Egor Homakov hacked Github in such a way that allowed him to commit directly to the Rails core project. Since Homakov is not a Rails team member, this is a really big deal.

Since this happened, there’s been a lot of talk about the Rails core being fundamentally insecure. In fact, Homakov has been harping on this for at least 1000 years. A few days ago he filed an issue about a mass assignment vulnerability in the Rails core, and later he illustrated this vulnerability by filing an issue report from the future. He was illustrating that you can inject attributes into a Rails model with an HTTP request.

So, when no-one took him seriously, he took the next step. He created HTTP PUT requests adding his SSH key to a Rails core user id, then pushed a commit directly to Rails core.

How to Hack Rails

It turns out that Rails apps, by default, are easy to hack. Peter Nixey wrote a very detailed post about how Homakov hacked Github and the one line of code that could have prevented it, so if you want the full details, read there. The summary is much shorter.

Let’s create a simple User model which has two attributes, name and role. By default, every attribute of every model can be modified by using update_attributes. We all know this, and we’ve known it for a while. What it means is that any User, even if they don’t have permission, can update their own role. Imagine if I log in to our example app and submit a PUT request to the UsersController with the package {'params': {'id': 23, 'role': 'superadmin'} }. By default, the app will accept this and update the user with the new role.

This is exactly what Homakov did. He sent an HTTP request to Github and used update_attributes to change the Github database. All of this could, he argued, be prevented by adding attr_accessor to the User model.

Rails: Convention over Configuration Security

Now, the philosophical point I want to make about this is that the Rails core team seems to be ignoring their own mantra. All of us know that Rails adheres strongly to the Convention over Configuration design pattern. It’s a pattern that Rails users are taught from day one. In fact, it’s embedded in the framework’s name.¹

One of the outcomes of Convention over Configuration is that sensible defaults should be, well, sensible. The Rails core team feel strongly that attr_accessor should not be a default² and that security is the responsibility of the app developer. I agree that security is our responsibility, but disagree that the dominant Rails design-pattern-come-mantra supports this.

It’s almost as if no-one wants to say “Yeah, we should really take care of this.” Everyone is saying “You know, you should really take care of this.”

Stop the world! We’ve found a SQL injection

There’s a term that makes all software developers give pause: “SQL Injection.” It’s a phrase that keeps us lying awake at night, and gives us nightmares when we finally fall asleep. The idea that someone, using nothing more than a web browser, can change our database willy nilly. It’s a terrifying thought.

I can’t help but take an initial read of all the hubbub and think that we’re not giving it the importance that it’s due. Everyone sees a headline saying Rails has a ‘mass assignment vulnerability’ and says to themselves ‘I should probably look into that at some point.’ It’s too vague, to uncertain. To unemotional.³

I have to assume that everyone would be treating this differently if we called it what it is. Imagine your thoughts (and actions) if you read a different headline, something like “Rails apps prone to SQL injection by default”

Don’t you think you’d get something done? You should, because that’s what we’re talking about. This is a path to SQL injection, full stop.

Who owns this issue

I love Ruby on Rails. Like Python, Scala, Node.js, and a host of other technologies that we have at our disposal, Ruby’s web framework makes being a developer both powerful and fun. There’s so much we can do– and so much we can do very quickly. But there’s a cost to pay. Convention over Configuration is a good thing, but we can’t expect people to learn fast, develop fast– create fast– if we don’t respect the logical outcome of that philosophy.

That outcome is this: We, Rails developers, understand that we are responsible for our security; however, we also believe in you, the Rails core team. We trust you. We believe it when you tell us to follow “Convention over Configuration,” and so we naturally believe that the defaults you give us will be a safe, or at least not horribly, dangerously wrong. We, all of us– developers and Rails core team both– can’t have it both ways. We are telling ourselves to follow Convention over Configuration, but we’re also telling ourselves that SQL Injection is a viable convention, thus, that we have security as a configuration.

Now, personally, I don’t believe that attr_accessor belongs in the model– or at least that security from view-based actions belongs in the model. Rails is an MVC framework, and therefore I don’t like forcing myself to define my model based on actions in the view. I don’t want my view owning control of my model that way. I think the controller should manage these permissions and there are others who feel the same. In fact Yehuda Katz argues this as well.

Still, the overall question remains: How can we reconcile the mantra of Convention over Configuration if we support the standard of dangerous insecurity by convention?

1. Look, just stay on these rails and you’ll move fast. We’re not responsible for what happens if you go over there [↩]
2. Incidentally, I don’t either, but you can set this as the default by using ActiveRecord::Base.send(:attr_accessible, nil), but this causes all sorts of problems [↩]
3. In fact, to be honest, that’s how I was reading it. [↩]

Comments are closed.