What Grails developers can learn from the Github/Rails Mass Assignment Vulnerability

In short: Github's security was breached due to a "vulnerability" in Rails. Grails also suffers from the same vulnerability, but there are ways to protect your app. Check your code for instances of:

new DomainModel(params)

and replace them with bindData or command objects. When using bindData, use the "includes" option - it is safer than "excludes". The following regular expression might help you find some offending code:

new .*?\(params\)

In addition, make sure that all your domain objects have comprehensive constraints to protect from malicious users. For more info, read on.

---

Over the weekend, Github suffered a security breach that allowed an unauthorized user to make a commit to the main rails/rails repo. Fortunately, the user had no malicious intent, and only made the commit to bring awareness to the issue. Whether this was the best approach to achieve this is another discussion, and not the subject of this post.

Github quickly resolved the issue, but thousands of other Rails sites out there are vulnerable to the same attack. Unfortunately, many Grails apps are also liable to have the same issues. Before I go into why this is, and how you can mitigate it, here's a bit of background.

Weapons of Mass Assignment

Rails, just like many modern web frameworks, allows you to quickly create an object using the request's parameters. In Rails, this code looks a little like this:

@user.update_attributes(params[:user])

- or -

@user = User.new(params[:user])

Basically, what is happening here is that any of the user instance's properties that have a corresponding request parameter get the value of that parameter. In this case, imagine the User object class had at least two properties:

• name
• isAdmin

Now imagine that the request to create a user had one parameter, name. Under normal operating circumstances, this would work - but it is inherently insecure. An attacker only has to guess that a property exists on the User object with the name 'isAdmin', then add isAdmin=true to the HTTP request. When they do this, the user will be created with isAdmin set to true. Bad news.

This is known as the "mass-assignment vulnerability" and is, along with mitigation strategies, described in many places - including the Ruby on Rails Security Guide.

What happened at Github?

According to the github blog, the "malicious" user used the mass-assignment vulnerability to compromise the form that allows you to set authorized SSH public keys for your repo, and added his public key to the rails repo, effectively giving him permission to directly commit there.

What about Grails?

Grails also has the concept of mass assignment, but tends to refer to it as "data binding" or "batch updating". It is covered in detail in the Grails Reference, but i'll summarize it here.

//Create a user object, initializing it with values taken directly from the request.
def user = new User(params)

- or -

def user = new User()
//Update the user object, with values taken directly from the request.
user.properties = params

This is exactly equivalent to the ruby code I posted above. It also suffers from the same vulnerability. Any property on the User object that grants that user elevated privileges will be open to attack via HTTP.

If it's insecure, why would anyone use it?

Well, two reasons:

1. Many people don't know it's insecure
2. They RTFM. But only briefly.

To address my first point, most Grails developers, like those using any other language/frameworks, are mid-level devs under pressure to get features out. In this situation, it's also unlikely that their code will get peer-reviewed, and unlikely that they'll benefit from an experienced developer pointing out the error of their ways. This is what has happened in the Rails world, and I'm sure it's happened with Grails too.

So, what about the documentation? Grails docs are awesome. They've improved leaps and bounds in the last couple of years. They're clear, concise and accurate. However, in this case, the security risks and mitigation strategies are buried deep down in the topic of data binding. "Data Binding and Security concerns" appears as the 7th and final section, after the section entitled "Data binding and type conversion errors". Up until this point, every example shows the use of the new User(params) method of object creation.

So this in combination with inexperienced developers pushed for time, eager to please the powers that be, gives us a perfect storm. Very fews devs will know about the risks of batch updating and fewer will know how to avoid them. It also doesn't help that Grails scaffolding also uses the new User(params) method of object creation. It doesn't really have a choice right now, since the scaffolding scripts can't make a good guess which properties to include in the batch-update, and which to exclude. They could perhaps just use bindData (see below), including all of the object's properties. But that doesn't seem to illustrate much of a point.

So, what should I do?

As a developer who is using Grails, you should use this as a wake up call. Never write code that uses this style of batch updating, and review your controller code, removing any examples of it you can find. Take a look at the docs and understand the mitigation strategies for this issue. In short, they are:

Using bindData

//Update the Person object, with values taken directly from the request - including only properties known to be safe
def p = new Person()
bindData(p, params, [include: ['firstName', 'lastName]])

For more, see the bindData docs. You'll notice that there are ways to blacklist/exclude certain properties - this is ok, but it is more prudent to always white-list/include allowed properties instead.

Using the subscript operator

//Retrieve a Person object, and update it with values taken directly from the request - including only properties known to be safe
def p = Person.get(1)
p.properties['firstName','lastName'] = params

This is a "white-list" approach, that achieves the same as the bindData method above, but only works on Domain Objects. It is also not as flexible as bindData.

Using Command Objects or Action Arguments

Command Objects are explained in the docs too, but in a different section. They enable the developer to create a class that has very similar characteristics to domain objects. By including the command object in the parameter list of a an action, Grails will automatically initialize the command object with the values taken from the request. This is not dangerous, because this object will not be directly saved to the DB, but instead probably passed down to a service layer where its properties will be accessed directly. For example:

class UserController {
    def authService

    def create = { CreateUserCommand cmd ->
        if (cmd.hasErrors()) {
            redirect(action: 'createForm')
            return
        }

        authService.createUser(cmd)
    }
}

class CreateUserCommand {
    String username
    String password

    static constraints = {
        username(blank: false, minSize: 6)
        password(blank: false, minSize: 6)
    }
}

If you have one or two properties you need to update, you could use action arguments (new in Grails 2.0). They allow you to do something similar:

def create = { String firstName,  String lastName->
    def user = new User(firstName: firstName, lastName: lastName)
    // save the user, checking for validation errors
}

Important: None of these mechanisms guarantee safety. You can never trust any input that comes from a user, even if using one of the above strategies. Use the Grails validation mechanism to its fullest. Sanitize your input before saving it down to the DB. Basically, assume your user is out to get you.

What about the Grails Framework?

Thankfully, the Grails core team is wanting to address this, and are asking for feedback from the community. Here's mine...

Update the docs for all Grails versions

Remove examples of the insecure method of creating objects, and replace it with bindData. Move the "security" paragraphs to the top of the section on data-binding. This will make it clear that security is of primary importance, not an after-thought. As I mentioned earlier, the docs are great - this minor tweak will make them even better.

Introduce a dataBindable static property

Grails doesn't have an equivalent of Rails's attr_accessible or attr_protected attributes. But it should. For example:

class User {
    //Fields listed here would not be processed bindData, or new User(params)
    static dataBindable = ["username", "firstName", "lastName"]
    
    String username
    String firstName
    String lastName
 
    boolean isActive = false
    int failedPasswordAttemptCount = 0
 
    Date dateCreated
    Date lastUpdated

    static constraints = {
        //strict constraints go here
    }
}

Crucially, a class with an empty or missing dataBindable property would not be processed at all by bindData or other batch updating mechanisms. This is a harsh breaking change, but makes it clear that the developer must think about security. Since it is a breaking change, there should be a configurable "legacy mode" that could be configured to "true" to enable old behavior for all objects, or it could also be configured to a list of classes or namespaces to enable gradual migration. "dateCreated" and "lastUpdated" should never be updated via bindData or similar.

Update the scaffolding scripts

With the introduction of the dataBindable property, the scaffolding scripts can be smart enough to inspect each domain class and understand which properties should be included in a generated "bindData" statement in the controller code. As a consequence of this, the scaffolding UI would do one of:

• Not display properties that are not in the "dataBindable" property
• Update these properties using explicit setters in the controller code, with comments explaining why this is so.

In Conclusion

Grails isn't immune from this kind of vulnerability - in fact it is likely that many Grails apps suffer from it. Use what happened to Github as inspiration for auditing your code, and make sure that you use sensible methods of handling data from HTTP requests. I hope both the Grails framework and its community are able to benefit from this weekend's events.