CWE 915: Improperly Controlled Modification of Dynamically-Determined Object Attributes

Flaw

CWE 915: Improperly Controlled Modification of Dynamically-Determined Object Attributes, also known as overpost or mass-assignment, is a flaw in which an application accepts input data and does not properly control which elements are allowed to be modified. In ASP.NET MVC model binding simplifies the mapping of incoming (untrusted) data to the controller action method parameters. Although it is very helpful you can use it to perform validation, you need to use it carefully. For example:

public class UserAccount
{
    [Key]
    public int UserID { get; set; }
    public string UserName { get; set; }
    public string Password { get; set; }
    public string Location { get; set; }
    public bool IsAdmin { get; set; }
}
...
public class UserDbcontext : DbContext
{
    ...
    public DbSet<UserAccount> Users { get; set; }
    ...
}

The above code shows an entity framework datbase-context called UserDbContext that exposes a generic DbSet collection for UserAccount. Entity Framwork is an Object Relational Mapper (ORM) that helps the developer storing data in a database. The following is a ASP.NET MVC controller implementation called AccountController that contains the update method UpdateUserAccount() that updates user details.

public class AccountController : Controller
{
    ...
    [HttpPost]
    [ValidateAntiForgeryToken]
    public async Task<ActionResult> UpdateUserAccount(UserAccount account)
    {
        if (!ModelState.IsValid)
        {
            return View();
        }

        try
        {
            using (UserDbcontext db = new UserDbcontext())
            {
                db.Users.Attach(account);
                await db.SaveChangesAsync();
                return View("Index");
            }
        }
        catch (Exception ex)
        {
            LogError(ex);
            ModelState.AddModelError("Error when saving data", ex);
            return View();
        }
    }
    ...
}

The above HTTP POST action accepts the method argument account, which is of type UserAccount. The ASP.NET MVC model binder will populates account instance variable with all the requested data elements using the request form-, route-, and query string fields. After that, the model validates and attaches the given UserAccount instance to the database context and store its changes.

If the form used to HTTP POST these data fields does not contain a HTML checkbox to edit the IsAdmin field it is still possible to add a value for that field in either a form- or query-string field. Because the field is not present in any of the webpages that edit the user data, it is still possible for someone to find the field using other methods.

Fix

There are different ways to fix an overpost or mass-assignment issue. It is possible to instruct (with help of annotation attribute) the model binder to ignore certain fields when processing. The second approach is to separate the data model from the way the view delivers the data to the controller. You can use a view model for this solution.

BindAttribute Include Whitelist

In the first approach for fixing this problem, you annotate either the method argument userAccount or the class UserAccount with an instance of System.Web.Mvc.BindAttribute. The Include property of that attribute contains a comma separated list of properties that are included during binding. This approach is useful because that property newly added to UserAccount is not populated by default, and the developer has to explicitly add that name in the include list.

public class AccountController : Controller
{
    ...
    [HttpPost]
    [ValidateAntiForgeryToken]
    public async Task<ActionResult> UpdateUserAccount([Bind(Include="UserName,Password,Location")] UserAccount account)
    {
        if (!ModelState.IsValid)
        {
            return View();
        }

        try
        {
            using (UserDbcontext db = new UserDbcontext())
            {
                db.Users.Attach(account);
                await db.SaveChangesAsync();
                return View("Index");
            }
        }
        catch (Exception ex)
        {
            LogError(ex);
            ModelState.AddModelError("Error when saving data", ex);
            return View();
        }
    }
    ...
}

view diff

...
     [HttpPost]
     [ValidateAntiForgeryToken]
-    public async Task<ActionResult> UpdateUserAccount(UserAccount account)
+    public async Task<ActionResult> UpdateUserAccount([Bind(Include="UserName,Password,Location")] UserAccount account)
     {
         if (!ModelState.IsValid)
         {

view fixed code only

NB: It is also possible to use the System.Web.Mvc.BindAttribute in combination with a blacklist property on that attribute called Exclude. However, this will not solve the problem if a new (internal only) field is added to UserAccount because it needs to be explicitly added to the blacklist to not be bound to any external untrusted data.

Separate ViewModel

The best approach is to fully separate the model that collects the data from the UserAccount that stores the data in the database. In the following code you see a ChangeUserViewModel that collects the data and mapped explicitly to the internal UserAccount instance that then stores the data. Any field you add to the model is not be exposed to the outside by default, so the developer should explicitly add the mapping in the processing logic to store that data element.

public class AccountController : Controller
{
    ...
    [HttpPost]
    [ValidateAntiForgeryToken]
    public async Task<ActionResult> UpdateUserAccount(ChangeUserViewModel changeUserModel)
    {
        if (!ModelState.IsValid)
        {
            return View();
        }

        try
        {
            using (UserDbcontext db = new UserDbcontext())
            {
                var user = db.Users.Where(x => x.UserID == changeUserModel.UserID).FirstOrDefault();
                if (user != null)
                {
                    user.UserName = changeUserModel.UserName;
                    user.Password = changeUserModel.Password;
                    user.Location = changeUserModel.Location;

                    await db.SaveChangesAsync();
                    return View("Index");
                }
                return View();
            }
        }
        catch (Exception ex)
        {
            LogError(ex);
            ModelState.AddModelError("Error when saving data", ex);
            return View();
        }
    }
    ...
}