Always check all fields in the model

Today, Github was hit by a security vulnerability that was caused by not properly dealing with model class fields that were not passed in by the form. Basically, by forging a HTTP Request, it was possible to set values for any field in the model, even the ones the forms on the website normally don’t pass in.

This type of issue is not limited to Ruby on Rails though, a similar issue can happen with any framework. Let me demonstrate how this could affect ASP.net MVC. Assume you have a Business Object:

public class BusinessObject
{
    public int? UserId { get; set; }
    public string Content { get; set; }
}

Your underlying service makes sure to set the UserId to the current user if it’s not set:

public void Upsert(BusinessObject business)
{
    if (!business.UserId.HasValue)
    {
        business.UserId = _userService.GetCurrentUserId();
    }

    using (var repo = _businessObjectRepositoryFactory())
    {
        repo.Upsert(business);
    }
}

On your form, you don’t set the user id:

<form action="@Url.Action("Index")" method="post">
    @Html.TextBoxFor(m => m.Content)
    <input type="submit" value="send" />
</form>

And in your controller, you just pass through the model to the service, since the service deals with it:

[HttpPost]
public ActionResult Index(BusinessObject model)
{
    try
    {
        _businessService.Upsert(model);
        return RedirectToAction("Index");
    }
    catch (Exception ex)
    {
        Trace.TraceError(ex.ToDebuggingOutput());
        return RedirectToAction("Error");
    }
}

This works great, your service enters the branch that sets the current user id, so you can move on to the next feature!

image

But wait, what if I just forge a HTTP POST using cUrl or Fiddler or JavaScript or the Firefox Poster plugin or any of the other many many ways to create arbitrary HTTP Requests? Let’s try!

image

Well, look at that!

image

Since no one checks that the UserId should be null, this would happily let me post as User 12.

This isn’t a vulnerability in the Framework, and it is not a new issue. In ASP.net MVC, there are many ways to avoid this, one example being the BindAttribute.

[HttpPost]
public ActionResult Index(
    [Bind(Include="Content")] BusinessObject model
)

With that one set, UserId will remain null even if I POST it in:

image

There are other methods for this: Creating specialized business classes for each form POST, using a custom model binder, setting each field to default… A Whitelist on the Bind is easy, but can get complex for large models, and you won’t get a Compiler Error if someone renames a field. Specialized business classes give you compiler errors if stuff changes, but mean 1 class + mapping code per Form POST action.

While this example seems trivial, it’s very easy to miss over-posting vulnerabilities on larger models, and while attackers can just try guessing common model property names like UserID, the more forms your application has the more data an attacker has as well to correctly guess property names.