Mass Assignment Vulnerability in ASP.NET MVC

On March 5, 2012, in development, by Josh Bush

By now you may have seen what happened to github last night. In case you didn’t, let me bring you up to speed.

In a Ruby on Rails application, you can make a call to update your model directly from request parameters. Once you’ve loaded an ActiveRecord model into memory, you can poke its values by calling update_attributes and passing in the request parameters. This is bad because sometimes your model might have properties which you don’t want to be updated by just anyone. In a rails application, you can protect this by adding attr_accessible to your model and explicitly stating which properties can be updated via mass assignment.

I’m not going to pretend to be a Ruby dev and try to explain this with a Rails example. Github already linked to this fantastic post on the subject regarding Rails here. What I’m here to tell you is that this situation exists in ASP.NET MVC also. If you aren’t careful, you too could end up with a visit from Bender in the future.

So, let’s see this vulnerability in action on an ASP.NET MVC project.

First, let’s set up a model:

public class User {
    public int Id { get; set; }
    public string UserName { get; set; }
    public string FirstName { get; set; }
    public string LastName { get; set; }
    public bool IsAdmin { get; set; }
}

Then let’s scaffold out a controller to edit this user:

public class UserController : Controller {
    IUserRepository _userRepository;
    public UserController(IUserRepository userRepository) {
        _userRepository = userRepository;
    }

    public ActionResult Edit(int id) {
        var user = _userRepository.GetUserById(id);
        return View(user);
    }

    [HttpPost]
    public ActionResult Edit(int id, FormCollection collection) {
        try {
            var user = _userRepository.GetUserById(id);
            UpdateModel(user);
            _userRepository.SaveUser(user);
            return RedirectToAction("Index");
        } catch {
            return View();
        }
    }
}

Do you see that UpdateModel call in the POST to ‘/User/Edit’. Pay attention to that. It looks innocent enough, but we’ll see in a minute why that is bad.

Next, we scaffold up a view and remove the checkbox that allows us to update the user’s Admin status. Once we’re done, it looks like this:

That works. We can ship it, right? Nope. Look what happens when we doctor up the URL by adding a query parameter:

I bet you guess what’s about to happen now. Here, I’ll break execution right at the problematic line so you can watch the carnage:

Okay, you can see the current values to the right. We’ve loaded user #42 from the database and we’re about to update all of his values based on the incoming request. Step to the next line and we see this:

UH OH. That’s not good at all. User #42 is now an administrator. All it takes is an industrious user guessing the names of properties on your entities for you to get burned here.

So, what can we do to prevent it? One way would be to change the way we call UpdateModel. You can use the overload which allows you to pass in an array of properties you want to include. That looks like this:

UpdateModel(user,new[]{"FirstName","LastName","Email"});

We’ve just created a whitelist of properties we will allow to be updated. That works, but it’s ugly and would become unmanageable for a large entity. Aesthetics aside, using this method isn’t secure by default. The developer has to actively do something here to be safe. It should be the other way around, it should be hard to fail and easy to succeed. The Pit of Success is what we want.

So, what can we really do to prevent it? The approach I typically take is to model bind to an object with only the properties I’m willing to accept. After I’ve validated that the input is well formed, I use AutoMapper to apply that to my entities. There are other ways to achieve what we want too, but I don’t have time to enumerate all of the scenarios.

Wrapping up
The point of all of this is that you need to understand exactly what your framework is doing for you. Just because there is a gun available, it doesn’t mean you have to shoot it. Remember folks, frameworks don’t kill people; developers with frameworks kill people. Stay safe out there friends, it’s a crazy world.

Tagged with:  
  • http://twitter.com/timalmond Tim Almond

    thanks Josh… that’s a really interesting article. Most helpful.

    (I use Automapper by default anyway, but good to see something that could be a problem).

  • Michael Dillon

    How do you use automapper to push a subset back into an existing entity?

  • http://imakimak.myopenid.com/ imak

    Not sure if I will call it MVC vulnerability. It is just a poor design. I probably will have the Id in session so that it never posted back from client. 

    • http://digitalbush.com/ Josh Bush

      It turns into a vulnerability if it’s used in this manner. The take home here is that you should always be aware of what data you are accepting.

    • http://www.facebook.com/people/Guy-Incawgnito/100002249142638 Guy Incawgnito

      It’s not just the primary key property (such as UserId) or an authorization flag (such as IsAdmin), it could be any public property of the model object that could be overwritten by the default model binder with a value from the Request/Form objects.  It could be an ‘ownership’ foreign key property (such as CreatedByUserId on a blog Post object) that could be used to assign fraudulent ownership of a protected resource to a normal user.  The attacker might have to know a bit about how your models are structured, as was the case with the github exploit.  Brad Wilson covered this issue at least a year ago in a blog post about MVC2: 
      http://bradwilson.typepad.com/blog/2010/01/input-validation-vs-model-validation-in-aspnet-mvc.html

  • http://twitter.com/BVitale Ben Vitale

    What we can _really_ do to prevent it? How about a more de-coupled architecture, where you’re not binding directly to your data entities in your web layer?

    • http://digitalbush.com/ Josh Bush

      Prevention is just a matter of being explicit about what you are willing to accept from the user. 

  • Anon
    • http://digitalbush.com/ Josh Bush

      I hadn’t looked into ReadOnlyAttribute because I’m never binding directly to my data model. Even with this, it requires a developer to actively do something to be secure. All it takes is for someone to forget to add an attribute and you’re back in the same boat.

  • Ramon

    When you use ViewModels instead of the business models, this could never happen.

    • Steve Fenton

       I agree – with the caveat / reminder that you only put stuff on the ViewModel that you actually need and you also need to think about what happens if you have “Id” on there – what if I change the Id to someone else’s Id etc.

      • http://digitalbush.com/ Josh Bush

        Yep, it all boils down to just understanding the data. No framework can do that for us.

  • Pingback: The Morning Brew - Chris Alcock » The Morning Brew #1058

  • http://twitter.com/koenwillemse Koen Willemse

    I think this vulnerability is only available if you do a poor design in combination with not being aware of the issue of overposting. If you just use view specific models that you bind and also apply the bind attribute on the model ([Bind(Include = "FirstName,LastName")]), there should be no problem.

    • http://digitalbush.com/ Josh Bush

      If you are using view specific models, you likely won’t need the Bind attribute. By binding only a subset, there should be no issue of overposting.

  • Sheep

    Have you tried replacing the FormCollection with a input model? That should stop this happening – in MVC 1 this was an issue, but with the latest MVC versions you can now create proper input objects which also makes any type validation easier (and you can make the framework do more of the type-validation, etc work on the client side)

    • http://digitalbush.com/ Josh Bush

      Yep, that’s exactly what I’m doing too. I mentioned it towards the end of the article: “The approach I typically take is to model bind to an object with only the properties I’m willing to accept.”

      The point of this article was to just point out the parallels between Rails and ASP.NET MVC with this particular issue. I figure if it can affect a site like github, then it’s worth talking about.

  • Yarx

    How do you handle the many mappings you’ll need to have in an app that uses Automapper, especially if they are being mapped to complex objects? Do you just define them as you need them or do you have them all defined in a central location

  • Pingback: About the mass assignment vulnerability in Asp.Net MVC framework « thewayofcode

  • Rocky LIU

    we should operate view model and data model .but take us lots of work for this convert.

  • Pingback: The Pull Request » The Pull Request Episode 3

  • Ron Krauter
  • Ron Krauter