Scott Hanselman

ASP.NET - Overposting/Mass Assignment Model Binding Security

April 19, 2017 Comment on this post [35] Posted in ASP.NET | ASP.NET MVC
Sponsored By

imageThis 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.

facebook twitter subscribe
About   Newsletter
Hosting By
Hosted in an Azure App Service
April 19, 2017 7:46
Andrew wrote a very similar post a month ago: https://andrewlock.net/preventing-mass-assignment-or-over-posting-in-asp-net-core/
April 19, 2017 7:51
I'm all about MediatR so I set up commands as what is bound in the controller and then just pass that off to the mediator. Prevents overbinding and keeps a nice separation of concerns.
April 19, 2017 8:31
View/Input/Output models all the time. No exceptions. That's my position. The Bind attribute is horrible (property names as strings? urgh. Sure, nameof(), but still, urgh).
April 19, 2017 8:33
Whether I'm using commands or direct domain models, I'm never taking the form input for granted - it could be some view model or other dto, but in any case the real action will happen via a domain object (or service) and will never be abused thanks to strict constraints/invariants/guard clauses in the domain context. These are always supported by tests so a developer can be confident however it chooses to wire up the controllers (+1 for the pragmatic approach Scott), he's not going to put the domain into an inconsistent state.
April 19, 2017 9:25
Have a CreatePerson class with First and Last properties on it. In the action instantiate a Person and assign First/Last.

It takes one minute to do it properly so just do it properly.
April 19, 2017 11:24
It is such a simple thing to overlook if you are not aware of the behavior. In their rush to get code out, some developers simply don't consider the security side of things.
April 19, 2017 11:36
Yip, Damian Edwards is spot on.
April 19, 2017 14:15
I also agree with Damian Edwards. You should always have a separate model for inputs. It's cleaner, clearer, safer, and ultimately easier.
April 19, 2017 14:43
Is it me or am I the only one saying that having any form of Db access code in the MVC controller is bad?

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.
April 19, 2017 15:48
I'm of the opinion that nothing that comes from a post should ever be directly saved. Even if you use an entity directly as your binding parameters, you should map over the properties from that onto an instance that will actually get saved to the database. This may kind of negate some of the benefit of the modelbinder, as you're still doing the leftside-rightside stuff, but it's infinitely safer and if you just make it standard, you never need worry about overposting. For the love of all things good and holy, Bind should just die a fiery death, though.
April 19, 2017 16:16
That opening sentence did make a lot more sense after I read the rest of the article! Thanks, Scott; attacks of this nature hadn't really been on my radar previously.
April 19, 2017 17:21
I see a lot of "always" and "never" in the comments, but like Scott said in the article: Do what is practical, as long as you do it safely.

I also like the overarching message of "when you use an abstraction, make sure you understand how it works"
April 19, 2017 17:29
What about the case where SOME users are allowed to flip that bit, but others aren't? Your model, view model or whitelisted attributes will still have to contain that property. Another case for putting your validation in a service.
April 19, 2017 17:31
I think the [Bind("First,Last")] only works when you are doing regular form posts (application/x-www-form-urlencoded). If you are doing pure application/json [FromBody] posts, it seems to ignore the [Bind] attribute.

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");
}
}
April 19, 2017 17:33
Usually your articles are well written and thorough Scott, but I'm not really following much of this post. I'm not really familiar with any of these attributes or what you're even talking about with "ModelBinding" or "ViewModel" in this context. I haven't used ASP .NET Core extensively but even if I had, it's possible to use it without touching any of this stuff so some more context would certainly be nice.

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.
Sam
April 19, 2017 18:45
Wouldn't the AntiForgeryToken prevent X domain posting?
April 19, 2017 18:56
Agree 100% with Damien. Strings in attributes should not be used for binding, code generation or other purposes.

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.


Ted
April 19, 2017 21:07
I'm not sure if this works with ASP.NET model binding (been years since I've used it), but I tend to rely on inheritance to solve this kind of issue. Assuming you stick with the "IsAdmin" boolean property to identify administrators, you could have a PersonWithCredentials : Person. PersonWithCredentials adds the IsAdmin and any other security you might need.

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.
April 19, 2017 21:30
I've found that those bind attribute lists are a bit of a pain to maintain (strings = typos), and don't actually solve the underlying problem in a durable way. I've opened an issue about it on Git at https://github.com/aspnet/Mvc/issues/5412.

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.
April 19, 2017 23:04
Just curious, what was the impetus behind this blog post? It's a cool bit of information but the topic isn't one that one would normally think about :)
April 20, 2017 3:26
Are we doing server side web apps yet?
April 20, 2017 3:30
@Travis

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.
April 20, 2017 6:43
@Sam, Scott is saying that separation of concerns is important. Not only for code design, but it helps with security.

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.
April 20, 2017 7:03
Using different models is the best practise in most circumstances. And by the way, the front-end code is not posting a Person to the server, but a CreatePersonCommand, then it makes sense that CreatePersonCommand has no property named IsAdmin.
April 20, 2017 14:42
The qualification for the use of a ViewModel is very simple: use ViewModels all the time *unless* you're doing demos at MS events or writing books about how to use ASP.NET.
April 20, 2017 19:10
+1 for input models.

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.
April 21, 2017 5:50
I agree with Damien Edwards and I'll go even further to say that I don't use Domain Models as members of my ViewModels (or EditModels). The properties of my ViewModels contain DTOs only, which I map from domain models using Automapper. No Domain Model exist in my View project (ASP.NET or whatever).

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.
April 22, 2017 1:16
Thanks Scott for this post. I feel like I learned a lot (and I also learned stuff from some of these comments). Does it make sense usually for the viewmodel to inherit from a model ?
April 24, 2017 20:48
the nameof(...) API can also help here for compile time sanity
[Bind(nameof(Person.First) + "," nameof(Person.Last)]
April 25, 2017 17:36
I agree with Joe G

Thank you good idea
April 25, 2017 18:23
@Joe G

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.
April 26, 2017 17:54
@Robert A

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
April 29, 2017 5:57
Very good article, congratulations
April 29, 2017 6:07
Thanks Scott Great article,
May 02, 2017 17:36
Rule of thumb; use a database-model, a domain-model and a view-model in the appropriate layers. Model-binding is great and a timesaver, but it does come with it's own risks.

Comments are closed.

Disclaimer: The opinions expressed herein are my own personal opinions and do not represent my employer's view in any way.