ASP.NET MVC UpdateModel vulnerable to hacking?

Question

I have an ASP.NET MVC application that is calendar-like. As per the NerdDinner example, I'm updating the results of my edit page using UpdateMethod()

In my app, certain events are fully customizable and certain ones are only partially customizable. Even though the edit form for editing the partially customizable events only have those fields available, obviously someone could create their own form with the missing data and post to my site. If they do so, what's to keep someone from changing any/all fields? Worse, what if they tried to change the id (primary key)?

It feels like UpdateModel() is vulnerable to very basic hacking. Are my fears legitimate or is there something I'm missing?

// POST: /MyEvents/Edit/2
[AcceptVerbs(HttpVerbs.Post), Authorize]
public ActionResult Edit(int id, FormCollection formValues)
{
    MyEvent myevent = eventRepository.GetMyEvent(id);

    try
    {
        UpdateModel(myevent);
        eventRepository.Save();
        return RedirectToAction("Details", new { id = myevent.MyEventId });
    }
    catch
    {
        ModelState.AddRuleViolations(myevent.GetRuleViolations());
        return View(new MyEventFormViewModel(myevent));
    }
}

Montoya

easy/safe mode = create form (in)models, then map those to your entities via Automapper. – Cayenne

Answer 1

You're missing the section on "Model Binding Security". You should always include a whitelist of properties that can be updated by any of your user input methods.

For example, from NerdDinner:

[AcceptVerbs(HttpVerbs.Post)]
public ActionResult Create( [Bind(Include="Title, Address")] Dinner dinner)
{

}

or if you're calling UpdateModel, you can create a string array of allowed properties, and do

UpdateModel(myObject, allowedProperties);

You can lock down the classes themselves so that only certain properties are updateable as well.

[Bind(Include="MyProp1,MyProp2,MyProp3")]
public partial class MyEntity { }

Renascent

Great until you have 20 or 30 elements on a data entry screen, then it becomes a pain =) – Ooze

You can also use a blacklist :) – Renascent

Answer 2

Your fears are right. This is called mass assignment. You can protect your code by marking your class with BindAttribute and setting Exclude / Include properties.

Waistband

Answer 3

It's perfectly possible for someone enterprising / malicious to map the fields to any of the properties on your model. There are a few ways around this

The most simple is to use the exclude / include properties overloads of UpdateModel as has been mentioned before. The downside of this is that the method only accepts a string array which can sometimes mean your code gets out of sync if you do any renaming.

Another way is to use a simple DTO which holds the bound fields, you can then take the DTO and do what you want with your event object, this obviously adds another class and is much more manual but gives you a lot more control

public ActionResult(int id, EditForm form) {
    MyEvent event = _eventRepository.GetMyEvent(id);
    event.Name = form.Name; //etc;
    if (User.IsInRole("Organiser")) {
        event.Date = form.Date;
    }
    return ...
}

Another way could be via a customer model binder for your MyEvent class which only binds your desired field, probably overkill though.

Obligato

Answer 4

There are overloads of UpdateModel that take an array of strings naming properties to update. These overloads will only update the named properties.

There may be other easier / more declarative ways to accomplish this, I'm not an expert on MVC data binding.

Ichthyo

Answer 5

You can mark fields on your model that should be ignored by update or pass a list of included/excluded fields using one of the other UpdateModel overloads.

Electrothermal