About the mass assignment vulnerability in Asp.Net MVC framework


Heads up!

The blog has moved!
The new URL to bookmark is http://blog.valeriogheri.com/


Following what happened to github in the past days (someone was able to hack a github repository exploiting a Ruby on Rails vulnerability to proof the point that this is in fact a vulnerability of the framework and a very dangerous one) there has been a lot of buzz and discussion in the web about  it and also about Asp.Net MVC framework, if it suffers from the same vulnerability and if it is to be considered a vulnerability in the first place.

What could actually happen using Asp.Net MVC model binding feature is explained very well in this blog article by Josh Bush, so I won’t repeat it.

A good solution to this problem is already offered by the framework, DataAnnotations! I’m using the same ViewModel as in Josh Bush’s example, with an extra row at the beginning:

[Bind(Exclude = "IsAdmin")]
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; }

By using the Bind attribute, [Bind(Exclude = “IsAdmin”)], we are explicitly asking the framework not to update the value of property IsAdmin when we use the UpdateModel(user) instruction.

So even if the attacker will attempt to change the query string in the URL, the Model Binder will simply not use it for this particular property and your system will be “safe”!


4 thoughts on “About the mass assignment vulnerability in Asp.Net MVC framework

  1. The major major major issue in going route is that you’re ignoring the underlying problem. Security MUST always be OPT OUT, it should NEVER be OPT IN.

    Once you make security opt in you open yourself up to potentially very serious leaks due to somewhere someone forgetting to slap that bind exclude attribute.

    The much better solution is you use a view model (or even just a specific post model) that inherently doesn’t have that property and then map from your view model to your actual model.

    • I generally agree with you, 1 ViewModel per View is my preferred way as well.
      But what if you have to display the property as a read only field in the UI? Or you use DataAnnotations (readonly, bind(Exclude)) or similar, being still able to use the Model Binding engine, or you use the ViewBag, losing the Model Binding feature.
      Is there any other option?

      • As long as you’re mapping from your view model to your actual model your map should specifically NOT map the IsAdmin from ViewModel -> Model.

        Also generally for an important property like IsAdmin I would never expose a public setter I would only allow it to be set through the constructor. Any property that i want to be held immutable never has a public setter on my models things like Id, Created, Creator. This prevents a whole host of accidents from occurring. Generally I’ll make those setters internal and since my models are always in a standalone project with nothing but models it has little room for developer error.

        You can then easily use a class to mediate the updating of modified times etc that exists inside the models project.

        I also follow this pattern for all reference types on my models. There is almost never a case on this planet that I want a property to be null that is not a nullable primitive (i even hate that strings are allowed to be null)

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s