By now you may have seen what happened to github last night. In case you didn’t, let me bring you up to speed.
In a Ruby on Rails application, you can make a call to update your model directly from request parameters. Once you’ve loaded an ActiveRecord model into memory, you can poke its values by calling
update_attributes and passing in the request parameters. This is bad because sometimes your model might have properties which you don’t want to be updated by just anyone. In a rails application, you can protect this by adding attr_accessible to your model and explicitly stating which properties can be updated via mass assignment.
I’m not going to pretend to be a Ruby dev and try to explain this with a Rails example. Github already linked to this fantastic post on the subject regarding Rails here. What I’m here to tell you is that this situation exists in ASP.NET MVC also. If you aren’t careful, you too could end up with a visit from Bender in the future.
So, let’s see this vulnerability in action on an ASP.NET MVC project.
First, let’s set up a model:
public class User {
public int Id { get; set; }
public string UserName { get; set; }
public string FirstName { get; set; }
public string LastName { get; set; }
public bool IsAdmin { get; set; }
}
Then let’s scaffold out a controller to edit this user:
public class UserController : Controller {
IUserRepository _userRepository;
public UserController(IUserRepository userRepository) {
_userRepository = userRepository;
}
public ActionResult Edit(int id) {
var user = _userRepository.GetUserById(id);
return View(user);
}
[HttpPost]
public ActionResult Edit(int id, FormCollection collection) {
try {
var user = _userRepository.GetUserById(id);
UpdateModel(user);
_userRepository.SaveUser(user);
return RedirectToAction("Index");
} catch {
return View();
}
}
}
Do you see that
UpdateModel call in the POST to ‘/User/Edit’. Pay attention to that. It looks innocent enough, but we’ll see in a minute why that is bad.
Next, we scaffold up a view and remove the checkbox that allows us to update the user’s Admin status. Once we’re done, it looks like this:
That works. We can ship it, right? Nope. Look what happens when we doctor up the URL by adding a query parameter:
I bet you guess what’s about to happen now. Here, I’ll break execution right at the problematic line so you can watch the carnage:
Okay, you can see the current values to the right. We’ve loaded user #42 from the database and we’re about to update all of his values based on the incoming request. Step to the next line and we see this:
UH OH. That’s not good at all. User #42 is now an administrator. All it takes is an industrious user guessing the names of properties on your entities for you to get burned here.
So, what can we do to prevent it? One way would be to change the way we call
UpdateModel. You can use the overload which allows you to pass in an array of properties you want to include. That looks like this:UpdateModel(user,new[]{"FirstName","LastName","Email"});
We’ve just created a whitelist of properties we will allow to be updated. That works, but it’s ugly and would become unmanageable for a large entity. Aesthetics aside, using this method isn’t secure by default. The developer has to actively do something here to be safe. It should be the other way around, it should be hard to fail and easy to succeed. The Pit of Success is what we want.
So, what can we really do to prevent it? The approach I typically take is to model bind to an object with only the properties I’m willing to accept. After I’ve validated that the input is well formed, I use AutoMapper to apply that to my entities. There are other ways to achieve what we want too, but I don’t have time to enumerate all of the scenarios.
Wrapping up
The point of all of this is that you need to understand exactly what your framework is doing for you. Just because there is a gun available, it doesn’t mean you have to shoot it. Remember folks, frameworks don’t kill people; developers with frameworks kill people. Stay safe out there friends, it’s a crazy world.
The point of all of this is that you need to understand exactly what your framework is doing for you. Just because there is a gun available, it doesn’t mean you have to shoot it. Remember folks, frameworks don’t kill people; developers with frameworks kill people. Stay safe out there friends, it’s a crazy world.
