Home > Security, Uncategorized > Preventing over posting or mass assignments vulnerability in ASP.Net MVC

Preventing over posting or mass assignments vulnerability in ASP.Net MVC


In the previous post, I showed how the over posting or mass assignments vulnerability works in ASP.NET MVC. In this post, I will show how to prevent this vulnerability.

Never use your database object as a parameter for in an action method

Developers may get tempted to use the object they pass to the database. Here is an example

[HttpPost]
[ValidateAntiForgeryToken]
public async Task IActionResult  Create(User user)
{
    if (ModelState.IsValid)
    {
	_context.Add(userl);
	await _context.SaveChangesAsync();
	return RedirectToAction(nameof(Index));
    }
    return View(user);
} 

This is dangerous. With the above code, you will end up hurting the integrity of the data in your database. You should never, in my opinion, expose your database object to the outside world.

Recently, when scaffolding the Controller, the code generator adds a comment to warn developers about the overposting attack I don’t recall seeing it in ASP.NET MVC 4. It may have been added in ASP.NET MVC 5 or ASP.NET Core. The scaffolder also using the BindAttribute when generating the code which will discuss later on in this post.

Use different mode/view model for the Get and Post action methods

I like to use a model or view model which contains a subset of the properties that are in the database model. I use a different view model for the Get and Post request depending on the fields I want to show on the screen or capture from the user. I added new properties to the User model. I also created two view models. One for creating and updating a user (you can different VM for create and update if you want) and one for viewing a user.

public class CreateUserViewModel
    {
	public string Firstname { get; set; }
	public string Lastname { get; set; }
	public string Role { get; set; }
    }
public class UserViewModel
    {
	public int Id { get; set; }
	public string Firstname { get; set; }
	public string Lastname { get; set; }
	public string Role { get; set; }
	public DateTime LastUpdatedDate { get; set; }
    }

I created a branch in the code repository to demonstrate that https://github.com/lajak/SecureAspMvc/tree/OverPostingPreventionSeparateModel

With using the above code, we guarantee that the binder will only limited to the properties we have in the CreateUserViewModel for post request while for get request we have exposing more properties. Btw, creating a separate view model for get is not really necessary if your action method returns a view because your action method is returning the html which will be rendered by the browser. It becomes more crucial to have separate view model for read if your action returns data in json format.

In the controller, we need to map the values from CreateserViewModel to the DB user object.

  public async Task IActionResult Create(CreateUserViewModel user)
{
	var dbUser = new User();
	if (!await TryUpdateModelAsync(dbUser))
	    return NotFound();

	if (ModelState.IsValid)
	{
	    dbUser.LastUpdatedDate = DateTime.Now;
	    dbUser.CreatedDate = dbUser.LastUpdatedDate;
	    _context.Add(dbUser);
	    await _context.SaveChangesAsync();
	    return RedirectToAction(nameof(Index));
	}
	return View(dbUser);
}

Decorate properties with attributes

If you want to use one model, you can decorate the properties you don’t want the binder to bind with the [BindNever] attribute.

public class User
    {
	public int Id { get; set; }
	public string Firstname { get; set; }
	public string Lastname { get; set; }
	public string Role { get; set; }
	[BindNever]
	public DateTime CreatedDate { get; set; }
	[BindNever]
	public DateTime LastUpdatedDate { get; set; }
    }

Properties that are decorated with the BindNever attribute will not be assigned the value that is posted to the server. In other words, the binder will ignore it. There are few potential issues with using BindNever attribute.

  • High maintenance: developers need to remember adding BindNever property when adding new attribute if required and developers need to make sure that BindNever is not removed by mistake from one of the attribute
  • TryUpdateModelAsync: if you decide to have let’s say an admin page where you want administrator to be able to modify those attributes and you create a view model. The TryUpdateModelAsync will still ignore those properties that are decorated with BindNever and you need to assign those properties manually
  • I have read few reported issues that the Binder doesn’t respect BindNever or BindRequired when using the FromBody attribute

Use BindAttribute

One other option is to use the Bind attribute on the action method as such

public async Task IActionResult> Create([Bind("Firstname", "Lastname", "Role")] User user)

With this approach, you need to add the Bind attribute on every action method you want to prevent the over posting attack. You may have to revisit all your action methods every time you add or remove a property to the model.

Conclusion

As we have seen, there are few ways to protect our application from the overposting attack. I prefer to have view models. I feel it is the most flexible option as I can have a different view model per view or action method if required.

 

Advertisements
Categories: Security, Uncategorized Tags: ,
  1. No comments yet.
  1. No trackbacks yet.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

w

Connecting to %s

%d bloggers like this: