Mass Assignment Insecure Binder Configuration System

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.

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.


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”!

Valerio

Like this:

LikeLoading...

Related

0 Thoughts to “Mass Assignment Insecure Binder Configuration System

Leave a comment

L'indirizzo email non verrà pubblicato. I campi obbligatori sono contrassegnati *