Reminder: Your publics are public.

There has been quite a lot of activiy in the blogosphere in the last days, following github’s Mass Assignment Vulnerability hit. I have spent parts of the day looking into this vulnerability, and some of the suggestions for mitigation.

What is the issue?

Ruby on Rails, which github is built upon, has a feature known as Mass Assignment, which automagically maps your http request parameters to object properties. The same feature is available in .NET MVC. The feature and the problematic side of the feature is described in detail here:

In short, the problem is, that if you have an action with the following signature, which is intended to update the address of the person:

public ActionResult UpdateAddress(Person person)
{			
  SaveToDatabase(person);
}

which is called from a view, and Person is defined like this:

public class User
{
  public string Name { get; set; }
  public string Address { get; set; }
  public bool IsAdmin { get; set; }
}

even though you just just (mean to) expose the Address in the edit view (via a text field); if someone supplied a request parameter or a POST variable with the same name as one of the properties, the object is automatically populated with the values from the request/form parameters even though this was not your intention.

This has been described as a vulnerability in .NET MVC. And lots of different mitigation strategies are suggested in the links above, among others. However, I think all the suggestions overlook a suttle, but important feature of MVC:

  • If you make something public, it is really public

This applies to your action methods, which, if they are public, can be accessed to a URL with the controller and action names. It also applies to your models if you expose them in your views and controllers. When you think about it, it makes sense. If you declare something as public, you are fine with other parts of the system updating it. The special case with ASP .NET MVC is that this “other part of the system” could also be the user’s browser.

As you might have guessed, this leads us to the maybe simplest solution to the “vulnerability” (which is in fact, when you think it over, a feature rather than a vulnerability):

Make your view model’s setters internal or private:

public class User
{
  public string Name { get; internal set; }
  public string Address { get; set; }
  public bool IsAdmin { get; internal set; }
}

Simple. This prevents UpdateModel from automatically assigning any other variables than Address, which has a public setter, from the request parameters, because it respects the visibility of your model’s methods.

Advertisements
This entry was posted in .NET, C#, MVC and tagged , , , , , , , . Bookmark the permalink.

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