ASP.NET - Overposting/Mass Assignment Model Binding Security
This little post is just a reminder that while Model Binding in ASP.NET is very cool, you should be aware of the properties (and semantics of those properties) that your object has, and whether or not your HTML form includes all your properties, or omits some.
OK, that's a complex - and perhaps poorly written - sentence. Let me back up.
Let's say you have this horrible class. Relax, yes, it's horrible. It's an example. It'll make sense in a moment.
public class Person
{
public int ID { get; set; }
public string First { get; set; }
public string Last { get; set; }
public bool IsAdmin { get; set; }
}
Then you've got an HTML Form in your view that lets folks create a Person. That form has text boxes/fields for First, and Last. ID is handled by the database on creation, and IsAdmin is a property that the user doesn't need to know about. Whatever. It's secret and internal. It could be Comment.IsApproved or Product.Discount. You get the idea.
Then you have a PeopleController that takes in a Person via a POST:
[HttpPost]
[ValidateAntiForgeryToken]
public async Task<IActionResult> Create(Person person)
{
if (ModelState.IsValid)
{
_context.Add(person);
await _context.SaveChangesAsync();
return RedirectToAction("Index");
}
return View(person);
}
If a theoretical EvilUser found out that Person had an "IsAdmin" property, they could "overpost" and add a field to the HTTP POST and set IsAdmin=true. There's nothing in the code here to prevent that. ModelBinding makes your code simpler by handling the "left side -> right side" boring code of the past. That was all that code where you did myObject.Prop = Request.Form["something"]. You had lines and lines of code digging around in the QueryString or Form POST.
Model Binding gets rid of that and looks at the properties of the object and lines them up with HTTP Form POST name/value pairs of the same names.
NOTE: Just a friendly reminder that none of this "magic" is magic or is secret. You can even write your own custom model binders if you like.
The point here is that folks need to be aware of the layers of abstraction when you use them. Yes, it's convenient, but it's hiding something from you, so you should know the side effects.
How do we fix the problem? Well, a few ways. You can mark the property as [ReadOnly]. More commonly, you can use a BindAttribute on the method parameters and just include (whitelist) the properties you want to allow for binding:
public async Task<IActionResult> Create([Bind("First,Last")] Person person)
Or, the correct answer. Don't let models that look like this get anywhere near the user. This is the case for ViewModels. Make a model that looks like the View. Then do the work. You can make the work easier with something like AutoMapper.
Some folks find ViewModels to be too cumbersome for basic stuff. That's valid. There are those that are "All ViewModels All The Time," but I'm more practical. Use what works, use what's appropriate, but know what's happening underneath so you don't get some scriptkiddie overposting to your app and a bit getting flipped in your Model as a side effect.
Use ViewModels when possible or reasonable, and when not, always whitelist your binding if the model doesn't line up one to one (1:1) with your HTML Form.
What are your thoughts?
Sponsor: Check out JetBrains Rider: a new cross-platform .NET IDE. Edit, refactor, test, build and debug ASP.NET, .NET Framework, .NET Core, or Unity applications. Learn more and get access to early builds!
About Scott
Scott Hanselman is a former professor, former Chief Architect in finance, now speaker, consultant, father, diabetic, and Microsoft employee. He is a failed stand-up comic, a cornrower, and a book author.
About Newsletter
It takes one minute to do it properly so just do it properly.
I separate my API and my MVC code completely and this type of thing just doesn't happen, my API handles each request by accepting only objects with a full set of "this request accepted only" properties on it resulting in me barely using "Models" at all.
Most of the time I either have an entity or a simple DTO and that's about it.
In short ... over-posting just doesn't work in our web stack because the opportunity is simply not there, I consider offering up such opportunity as basically bad practice.
I've also taken to using OData on top of WebAPI so usually I get CRUD behaviour effectively as out of the box on all my API's.
@Scott:
Maybe you could blog about good separation of concerns, I bump in to so many people that for some reason still think using string concats in controllers to write SQL and then update the DB is ok and don't understand why I tell them to re-think their design.
I also like the overarching message of "when you use an abstraction, make sure you understand how it works"
Personally, I hate, hate, hate writing mapping code. Even with something like AutoMapper, it's just one more class to build and tests to write, with not enough real value. Realistic models are often much larger than two or three properties. In most cases I want to accept all but maybe one or two properties. Feels like the better approach would be a small blacklist of what NOT to accept. Something like the [BindNever] attribute, but usable on the method parameters.
My poor man's approach is to simply reset those few "secret" properties in the first lines of my action method. Maybe while also detecting and logging the fact that the user had tried something shady.
[HttpPost]
[ValidateAntiForgeryToken]
public async Task<IActionResult> Create(Person person)
{
if(Person.IsAdmin){
Person.IsAdmin = false;
_log.LogWarning("User acting shady");
}
}
This paragraph is especially confusing:
> Or, the correct answer. Don't let models that look like this get anywhere near the user. This is the case for ViewModels. Make a model that looks like the View. Then do the work. You can make the work easier with something like AutoMapper.
I have no idea what you're saying here.
Scott, Please please add a way to catch bad route paths at the individual controller level so that, in debug compile, one can add a CatchBadRoutePath(string path) in your controller. Right now, one has to do lame error handler at the global level and then hope that the path can be gotten out of the web request information. That's a huge waste of time since I should be able to put in a debug compile only mehtod to handle and assert(false) for the bad paths.
A way to dump the path table used showing the order in which paths are searched would be of great use.
Far, far too much of modern asp.net mvc and EF is attribute driven and not scalable to a 500,000+ line solution.
Tackle that and then get the WPF team to do the same with to text string as binding property fud.
Attribute useage for code binding, validtaion, etc all prevent the compiler from checking/enforcing a contract a compile time and result in harder to develop, more costly code.
The solution is elegant in my mind because it works cleanly with the model-binding in WebApi for example, and inheritance does a great job of separating model responsibility. The one problem is when you need to transfer child-to-parent or parent-to-child, and rather than using ICloneable I just created a Copy function for storage models.
Perhaps I'm worried about a corner case, but I think you'll find that even if you put your [Bind()] decorator on there, because you're passing an ID (which may be hidden, sure), you're still vulnerable even if you've used a [ValidateAntiForgeryToken] attribute as well, because your user can simply get into the dev console and modify the ID. I have confirmed this works in FF, as of 6 months or so ago.
Another thing is that I find myself wanting to identify, at the view model level, is which properties are modifiable and by which controller methods - to give myself a way of seeing immediately that certain properties are modifiable by method X or method Y - and to let me put that control into the View model so that it's centralized & actively prevents the developer of the controller method from doing anything stupid.
Preventing cross domain posting isn't everything though anyway. Picture I'm just an ordinary user on the web site. I am in control of my browser. Using my F12 tools I can change what's posted (or fiddle with it in Fiddler, etc) and make myself an admin.
You can't trust what's coming from the end user as you're not in control of their device and what it sends.
In his example, the Person is a database object, yet it's also bound directly to a post - so a user if able to effectively control what's in your database. In *most* web apps, this is bad.
You should design your code in a way where the Person object (and anything else reading/writing from the database) is in it's own separate layer, where you *control* everything that happens with your code. That way you achieve single responsibility AND it comes with added security built in to your design. Your controllers then just map the "ViewModels" into things that this other layer expects.
Totally agree with most people here - separation is the way to go. It may seem unnecessarily complicated at first, but if you will run into far less problems if you do it right. Look into Onion Architecture, it's pretty awesome.
Inside a view model (preferably) even if that view model doesn't contain anything else, rare though, there is usually some kind of dynamic data or some kind or select/dropdown list/s that make up that view, but again only bind input values to an input model.
Yeah, so something like IsAdmin would most likely have no semantic meaning in my view and would not be Model-bound (would not be in the object which is the action method parameter).
This approach, of course, would be overkill if you are just making an app for your local book-club. It is an Enterprise app architecture. Even for small ones.
[Bind(nameof(Person.First) + "," nameof(Person.Last)]
The issue with using a blacklist is that adding a new property to the model, which needs to be blacklisted, requires you to remember to blacklist it. This means that there is scope for human error revealing a bit of a security hole.
A whitelist is preferable, as forgetting to add a new property to this list would cause the binding to fail, and you would definitely notice this at development time.
As for the poor man's approach, this is just treating the symptoms, instead of applying a cure, and still requires you to remember to update the list of secret properties that are reset.
Ultimately, though, the approach that uses an input model without any of the blacklisted properties is even more preferable.
I hear what you are saying. And I totally get that are many cases where specific View/Input Models are the right answer, for all the reasons the smart folks posted above. If I can forget to blacklist a property, then eventually I will. So for really critical apps or models, it may not be the right choice.
However, my experience has been that 99 times out of 100*, that new property I just added is something I want to expose to the View. The set of hidden/secret properties is very small (often zero), while the set of public properties is usually very large. In that scenario, the whitelist is a significant burden. Whether the whitelist is a ViewModel or a Bind()] attribute, you still have to spend real effort updating it.
In either approach, the process of adding a new property to the underlying domain model is likely to trigger a "does this go on the view?" question. I choose to optimize for the "yes" case, because it's the most common and safe enough for me.
But you definitely have to be aware of what the impact is of the choice you make. So thanks to Scott for posting this, as it is always good to keep fresh in our minds.
*made up stats on the internet
Comments are closed.