6 Ways To Avoid Mass Assignment in ASP.NET MVC

Monday, March 12, 2012

One of the scenarios that I always demonstrate during an ASP.NET MVC class is how to create a mass assignment vulnerability and then execute an over-posting attack. It is a mass assignment vulnerability that led to a severe problem on github last week.

Let's say you have the following model.

public class User
{
    public string FirstName { get; set; }
    public bool IsAdmin { get; set; }
}

When you want to let a regular user change their first name, you give them the following form.

@using (Html.BeginForm()) {
   
     @Html.EditorFor(model => model.FirstName)
    <input type="submit" value="Save" />    
    
}

There is no input in the form to let a user set the IsAdmin flag, but this won't stop someone from crafting an HTTP request with IsAdmin in the query string or request body. Maybe they saw the "IsAdmin" name somewhere in a request displaying account details, or maybe they just got lucky and guessed the name.

composing the attack

If you use the MVC model binder with the above request and the previous model, then the model binder will happily move the IsAdmin value into the IsAdmin property of the model. Assuming you save the model values into a database, then any user can become an administrator by sending the right request. It's not enough to leave an IsAdmin input out of the edit form.

Fortunately, there are at least 6 different approaches you can use to remove the vulnerability. Some approaches are architectural, others just involve adding some metadata or using the right API.

Weakly Typed Approaches

The [Bind] attribute will let you specify the exact properties a model binder should include in binding (a whitelist).

[HttpPost]
public ViewResult Edit([Bind(Include = "FirstName")] User user)
{
    // ...
}

Alternatively, you could use a blacklist approach by setting the Exclude parameter on the attribute.

[HttpPost]
public ViewResult Edit([Bind(Exclude = "IsAdmin")] User user)
{
    // ...
}

If you prefer explicit binding with the UpdateModel and TryUpdateModel API, then these methods also support whitelist and blacklist parameters.

[HttpPost]
public ViewResult Edit()
{
    var user = new User();
    TryUpdateModel(user, includeProperties: new[] { "FirstName" });
    // ...
}

Strongly Typed Approaches

TryUpdateModel will take a generic type parameter.  You can use the generic type parameter and an interface definition to restrict the model binder to a subset of properties.

[HttpPost]
public ViewResult Edit()
{
    var user = new User();
    TryUpdateModel<IUserInputModel>(user);

    return View("detail", user);
}

This assumes your interface definition looks like the following.

public interface IUserInputModel
{
    string FirstName { get; set; }
}

Of course, the model will also have to implement the interface.

public class User : IUserInputModel
{
    public string FirstName { get; set; }
    public bool IsAdmin { get; set; }
}

There is also a [ReadOnly] attribute the model binder will respect. ReadOnly metadata might be want you want to use if you never want to bind the IsAdmin property. (Note: I remember ReadOnly not working in MVC 2 or MVC 1, but it is working in 3 & 4 (beta)).

public class User 
{
    public string FirstName { get; set; }

    [ReadOnly(true)]
    public bool IsAdmin { get; set; }
}

An Architectural Approach

One of many architectural approaches to solve the problem is to always put user input into a model designed for user input only.

public class UserInputViewModel
{
    public string FirstName { get; set; }
}

In this approach you'll never bind against business objects or entities, and you'll only have properties available for the input you expect. Once the model is validated you can move values from the input model to the object you use in the next layer of software.

Whatever approach you use, remember to treat any data in an HTTP request as malicious until proven otherwise.


Comments
gravatar Shay Friedman Monday, March 12, 2012
I wrote a post about the same subject a few days ago, showing a very simple way to reproduce this behavior: www.ironshay.com/....

Shay.
gravatar Eduardo Monday, March 12, 2012
Another solution could be that MVC generate a hash of the INPUT names on Html.BeginForm() and then let us compare that hash in the model binding.
gravatar James Culbertson Monday, March 12, 2012
Thanks K. Scott, that's a nice overview of all of the approaches. The View model approach is a good one as it is explicit and will help avoid accidents with change, e.g. if properties are later added to the base entity and the developer forgets to update all of the bindings in the controllers.
gravatar tobi Monday, March 12, 2012
You can make the setter internal.

I think it is unbelievable that the official guidance is to let the model binder bind to entities. This such an outrageous vulnerability. I have noticed it immediately in the first sample I saw. How can people build such stuff?
gravatar Robert Monday, March 12, 2012
Thank you for this post. Isn't it a very bad idea store inputs in database directly anyway? Having an abstraction with input sanitization between the models exposed by ORM frameworks like LINQ or EF and your application is not only a good for increasing security but also for coupling data layer more loosely.
gravatar James Culbertson Monday, March 12, 2012
@Robert, it can still happen with ORMs and other storage abstractions depending on how the updates are implemented against the underlying class, so it's still worth doing one of the safeguards outlined here.

@Tobi, yes, but in some cases you might need to update it from the app, e.g. from an admin page. The example given here can also apply to other properties that might need to be protected at the controller level that aren't a security risk but possibly a data-consistency risk. BTW, the model-binder in Rails Active Record binds to the entity as well and similar measures must be taken.
gravatar tobi Monday, March 12, 2012
@James, you are correct. I want to stress that binding to entities is almost exclusively useful in toy examples. And admin pages which usually have a very little share of the total project effort.
Lee Tuesday, March 13, 2012
AntiForgeryToken?
gravatar scott Tuesday, March 13, 2012
@Lee: that could prevent some scenarios, but not the github type scenario where the user is authenticated to the site - all they need to do is add some additional form data to a request they have.
Elias Wednesday, March 14, 2012
Great tips as usual Scott!
Good designs could in most cases prevent such issues - that is speaking Architecturally.
gravatar Tarik Guney Sunday, March 18, 2012
Nice catch there. Bind is making even more sense right now.
gravatar Stuart Sunday, April 1, 2012
@Lee, How exactly does an antiforgery token make a difference here? Form overposting (or seemingly now know as mass assignment) is not necessarily a CSRF attack.
gravatar Ajander Singh Monday, May 7, 2012
Great tips Scott
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!