IronShay

Ironing code, geek t-shirts and even presentations!

NAVIGATION - SEARCH

Mass Assignment Vulnerability in ASP.NET MVC

A couple of days ago the Ruby on Rails world got shocked by an old bug (or feature?) that could cause massive security issues sometimes. You can read about it here.

While reading about this vulnerability, I figured out that ASP.NET MVC worked in a very similar way… would it reproduce in an ASP.NET MVC environment? well, of course!

The Problematic Feature

ASP.NET MVC has this very convenient way of getting parameters from the request named Model Binding. The very simple example of Model Binding is controller actions with parameters. For instance:

public ActionResult Create(string name, string email)
{ 
  // ... do stuff ...
}

In this sample, the Model Binding feature will automatically fill in the name and email parameters with data from the request. Very similar to doing something like that:

public ActionResult Create()
{
  string name = Request["name"];
  string email = Request["Email"];

  /// ... do stuff ...			
}

This is already very helpful and it’s getting even better – you can set the parameter to a type of your own, and ASP.NET MVC will create an instance and fill it up for you. For instance, if you have a class named Person like this one:

public class Person
{
  public string Name { get; set; }
  public string Email { get; set; }
}

You can change the Create method to:

public ActionResult Create(Person person)
{			
  /// ... do stuff ...
}

By doing this, the Person.Name and Person.Email properties will automatically be filled in by ASP.NET MVC Model Binding.

OK, now that we understand what the essence of model binding is, let’s move on to the problem it represents…

Reproducing the Vulnerability

  1. Create a new ASP.NET MVC Application (I tried it with ASP.NET MVC 3 and 4).
  2. Add a new model class named User, as follows:
    public class User
    {
      public int Id { get; set; }
      public string Name { get; set; }
      public string Email { get; set; }
      public bool IsAdmin { get; set; }
    }
  3. Use the Add Controller dialog box to create a database context and a controller. Call it UsersController. Set the dialog properties as follows:
    Add Controller UsersController
  4. We don’t want the users to change the IsAdmin boolean value. It will be set somehow by the logics of the application later on. Therefore, open the Create.cshtml and Edit.cshtml views (they’re located under the Views/Users folder), and remove the IsAdmin part from them. The part to remove should look something like that:
    <div class="editor-label">
        @Html.LabelFor(model => model.IsAdmin)
    </div>
    <div class="editor-field">
        @Html.EditorFor(model => model.IsAdmin)
        @Html.ValidationMessageFor(model => model.IsAdmin)
    </div>

 

Now to the interesting part:

  1. Run the application and browse to /Users/Create
  2. Fill up the form and send. You’ve got a new user. IsAdmin is false.
  3. Go to the Edit page for this user. The URL will be something like /Users/Edit/1.
  4. Change the URL to /Users/Edit/1?IsAdmin=true and click enter to browse to it.
  5. Now click Save
  6. IsAdmin is now saved as True to the database. Oops.

This example is very very simple, but think about real world scenarios… this might get ugly. Very ugly. The biggest site that suffered the consequences of this vulnerability(based on Rails, but it’s the same thing) is GitHub – you can read their announcement here.

How to Defend

ASP.NET MVC offers a very simple solution to that problem – the Bind(Exclude=””) Attribute.  However, most people never use this feature. So… make a new habit from today – start using it. ALL THE TIME. And when I say ALL THE TIME, I mean that from now on you use it ALL THE F***ING TIME.

For my small sample, add [Bind(Exclude = "IsAdmin")] to the top of the model class (User.cs). After this change the model class should look like that:

[Bind(Exclude = "IsAdmin")]
public class User
{
  public int Id { get; set; }
  public string Name { get; set; }
  public string Email { get; set; }
  public bool IsAdmin { get; set; }
}

Rebuild and try our little hack again. It won’t work this time. Phew.

Stay safe,
Shay.



Comments (10) -

Sweden Johannes Gustafsson

I honestly don't see the problem. If you don't want to bind to some field then don't put it in your viewmodel.

Reply

The problem here is that this will affect anyone who used MVC in a straight forward way (no viewmodels, no special binding attributes...). And this is the vast majority of the people.

Reply

Take a look at this excellent blog posts for more ways to defend against mass assignment in ASP.NET MVC: odetocode.com/.../...ssignment-in-asp-net-mvc.aspx

Reply

MVC works as expected, news at 11. Tong
Seriously, this is the way things should work. Its a fundamental concept of MVC to only send and receive the data that is required for the view and its a fundamental concept of the web to not trust data from the client/user.

you wouldnt create a hidden field in web forms and check the value of that on postback to see if the user is an admin. Never trust user input. its the same pattern with mvc. Your Isadmin variable should not be exposed, either through making it(or its setter) private or using exclude or just simply not including it in the model because it's not required..

If you really must send sensitive properties in your view (e.g. Its an external class and you're too lazy to do the right thing and make a model of it), you should be doing server side validation of those objects anyway.

Its good to spread the word, but i would love if you explained/linked to some best practices and not just help people stumble along with their incorrect code.

Cheers

Reply

This is not MVC issue. This is a nature of object. It depends how you define object.  From my point of view, 'IsAdmin' bool type property should not be used for User object.  

There should have UserInRoles table for saving userId and roleId. Basically, the following table should be existed:

- User table
_ Role table

- UserInRole table.   That's the way it should be.

Hope It helps.

Reply

Great post, wasn't aware of this hack. Thanks for the good example! Smile

Reply

Israel Ilya Vinokurov

Thanks Shay for a great article!

Reply

I dont think anyone wud include IsAdmin in Model. Never trust user input. Infact IsAdmin sud be resolved within the controller via. security service Smile

Reply

You can solve this another ways by creating a viewmodel and then use Automapper http://www.automapper.org to map to viewmodel to the entity model , this way you willl only specify what is needed in your viewmodel i.e only the input the users would need.

Reply

Pingbacks and trackbacks (4)+

Add comment