Practical .NET

The Heisenberg Developer

There’s a potential security exploit that ASP.NET MVC leaves you open to. However, in Peter’s opinion, all the proposed solutions miss the point.

• By Peter Vogel
• 10/03/2017

There are problems in the world, and we should take defensive action to protect ourselves from those problems. In some cases, however, I think the cost of the defensive measures is more expensive than the problem ... and, furthermore, that those solutions don’t actually solve the problem.

Consider the problem of the over-posting hack that ASP.NET’s model binding creates (you’ll also find this problem referred to as "mass assignment"). Yes, it can open your page up to attack. Yes, there are several solutions. In the end, however: I think there’s a simpler solution than the ones commonly suggested; I don’t think there actually is a solution; and even if there was a solution, I think the person who would apply it doesn’t exist.

Understanding the Hack
To understand the problem, imagine you have a Customer object that you pass to a View. In that Customer object, you have a property called IsPremiumCustomer that you don’t include in the page. Instead, you use that property to control what’s displayed in the page (premium customers are allowed access to information and operations that ordinary customers are not).

Typical code in View that uses the IsPremiumCustomer property might look like this:

@If Model.IsPremiumCustomer Then
  <input type="button" name="AutoApproveCredit" value="One Click Unlimited"/>
End If

There’s nothing inherently wrong with this approach. In fact, the alternative is to have two separate Views: One for premium customers and one for ordinary customers. Assuming those two Views share a lot of their content -- and, as a result, must be kept in sync through the life of the application -- having two Views is just an accident looking for a place to happen (though clever use of Partial Views might reduce the pain).

Using a property in the View that you don’t include in the View is a common thing to do (even, I’d suggest, a smart thing to do). However, it does open you up to a hack if you use that same Customer object as one of the parameters to the Action method that accepts data from the View for processing when the page is posted back to the server, like this:

[HttpPost]
Public Function UpdateCustomerInformation(cust As Customer) As ActionResult

When the data comes back from the browser, ASP.NET almost blindly maps data from the browser into the cust parameter’s properties. Because of that, it’s surprisingly easy for a malicious user to slip a value into the IsPremiumCustomer property of the cust parameter (there are any number of ways to do this -- Abhi Jain demonstrates the problem using a Chrome plug-in called Request Maker).

Granted, the malicious user has to know the name of the property on your Model class in order to provide a value for that property. However, that malicious user (let’s call that user "Darth") may be able guess the name of the property, research its name from some other page where the property is updateable, or recognize the name because the property is part of some utility or third-party package. In fact, I’ll say it may be trivially easy for Darth to get the name of the property. In the most obvious case where you’ll expose yourself to this hack, you’ll tell Darth exactly what the name of the property is. I’ll come back to this later.

Of course, for this to be a problem, that server-side method has to use that IsPremiumCustomer property. Dangerous server-side code might code look like this:

If cust.IsPremiumCustomer Then
  '... do something stupid based on the value of IsPremiumCustomer sent from the browser
End If

In the worst-case scenario, the server-side code might update the database with that IsPremiumCustomer property and permanently promote the customer.

Solutions
There’ve been lots of descriptions on how to protect yourself from this problem, including columns from Scott Hanselman and Andrew Lock. Their solutions are well described and I can’t improve on them, so I won’t. The most popular solution is to have two Model classes: a view class to pass data to the View and a binding class used in the update method to accept data sent back from the server.

I will add that both columns imply (but don’t explicitly state) the simplest solution: Omit the IsPremiumCustomer property from the Model object and pass the value to the View through the ViewBag. In the Action method that prepares the View, you would do this:

Dim retreivedCustomer As Customer
retrievedCustomer = CustomerRepository.GetCustomerById("A123")
ViewBag.IsPremium = retreiveCustomer.IsPremium
Dim modelCustomer As CustomerModelClass
modelCustomer = New CustomerModelClass
modelCustomer.CustomerId = retrievedCustomer.CustomerId
'... and so on ...

And, in the View called from the Action method, you’d do this:

@If ViewBag.IsPremiumCustomer Then
  <input type="button" name="AutoApproveCredit" value="One Click Unlimited"/>
End If

There are costs: Because you’re using the ViewBag, you give up IntelliSense support for this property in your View. However, someone in the comments to these columns always says "You can’t be too secure!" so this cost (or any cost) is, apparently, always worthwhile (to be fair -- in his column, Hanselman points out that you need to be rational about applying these solutions).

You’ve also shut off the possibility of having a single Model/View combination that supports multiple classes of users by excluding or including property values, based on the user’s class (for example, guests vs. logged-in users).

Assuming that last issue isn’t a deal-breaker for you, there’s a more critical issue: All of the "solutions" miss the point by ignoring that the problem is, in fact, unavoidable. To put it another way: None of the solutions actually solve the problem.

The Unavoidable Problem
For example, consider the CustomerId property. I must include the CustomerId in the page sent to the user because I need to get it back from the browser so that I can update the right customer at the server (I could, I suppose, keep the CustomerId in the Session object and sync that data with the page, simultaneously increasing the user’s footprint on the server and the complexity of the application -- not an attractive option). Assuming I don't want the user to update the CustomerId, I put in a hidden element:

<input type="hidden" name="CustomerId" value="A123"/>

Except, by embedding the CusotmerId in the page, I've now made it trivially easy for Darth to change it -- I've even provided the name of the property (though, if Darth knows I'm using Entity Framework, Darth can probably guess the property's name, thanks to EF conventions). Now, by replacing the CustomerId, the malicious user can convince the server to update a customer of choice with the data returned from the browser.

Assuming, of course, I’m foolish enough to use the CustomerId or IsPremiumCustomer returned from the browser, as is.

The Real Answer
The real solution is to only use the data returned from the browser that the user should be expected to update (in fact, all these discussions always include a note to that effect). The only way the IsPremiumCustomer property should be used at the server in the update method is from a Customer object fetched on the server. The safe IsPremiumCustomer code looks like this:

[HttpPost]
Public Function UpdateCustomerInformation(cust As Customer) As ActionResult
  Dim retreivedCustomer As Customer
  retrievedCustomer = CustomerRepository.GetCustomerById(cust.CustomerId)
  If retrievedCustomer.IsPremiumCustomer Then
    '...do something based on the value of IsPremium fetched on the server
  End If

Remember, the reason we commonly have non-user-updateable properties in the Model object is because we’re trying to avoid having multiple Views that we have to keep in sync. Creating multiple classes that we have to keep in sync, as most solutions suggest, isn’t actually solving the problem -- it’s just moving it around.

The Non-Existent Developer
The only sensible reason I can think of for any of the proposed solutions is they reduce the opportunities for over posting to just the unavoidable cases, like my CustomerId property.

But, even then, I’m not clear to whom this solution is applicable. These solutions seem to assume the existence of the Heisenberg Developer: A developer who’s simultaneously foolish enough to blindly use all the data returned from the browser while being smart enough to distinguish between dangerous and non-dangerous data and implement a solution.

I guess there’s an argument to be made for there being two developers: the first, wise developer who recognizes the problem and sets up the solution; and the second, foolish developer who uses data blindly. Except the second developer, finding a property he needs that’s present in the view class but is missing from the binding class, would simply solve his problem by adding the property to the binding class (remember: we’ve already acknowledged that the second developer is foolish).

Protecting Yourself
If you’re worried about this problem, solve it by only using data from the browser that the user should’ve updated. This means, among other downsides, that you shouldn’t use AutoMapper to update your entity objects from the objects passed to your update method (that is, unless you configure AutoMapper to only copy property values you explicitly tell it to copy).

If you don’t trust yourself or subsequent developers to follow this rule, I’d recommend either using the ViewBag more or consider having two separate Views and being clever with partial Views (or both). Neither of these solutions solve the CustomerId property, though. And I’d also suggest that there’s no defense against foolish people, so your attempts to constrain subsequent, foolish developers are probably doomed ... other than education and code reviews, of course.

For any non-user-updateable data that you must include in the View (for example, Customer Id), encrypt the data as part of putting it in the View and, back on the server, decrypt it before use. I’ve discussed encrypting and decrypting in ASP.NET in an earlier column.

If you want to do the extra work to protect yourself from the Heisenberg Developer, I can’t stop you. I just think, when you open the box, the Heisenberg Developer isn’t alive or dead -- there’s no one in the box at all.