Dinis Cruz Blog: Can you spot the security implications/vulnerability of a small change to an ASP.NET MVC 3.0+ Model Binder?

Wednesday, 17 July 2013

Can you spot the security implications/vulnerability of a small change to an ASP.NET MVC 3.0+ Model Binder?

This post contains an example of a serious security vulnerability that is common on ASP.NET MVC applications.

There are two versions of a Model Binder (CartModelBinder) class shown below, one or both are vulnerable.

Your job is to find out how to exploit them :)

Ideally you should provide your answer/results/proof using UnitTests :)

Note 1: The vulnerability shown in this post is a variation of real-world vulnerability that I helped to find a couple weeks ago (on an UK-based financial services company).

Note 2: I’m yet to to look/review/see a large MVC application that doesn’t have similar vulnerabilities (on both ASP.NET MVC or Spring MVC)

Note 3: the code is from the http://sportsstoremvc3.codeplex.com/ sample application, which is used as an ‘real-world application’ example by the Pro ASP.NET MVC 4 book (the code was compiled using .NET 4.0 and executed using the ASP.NET MVC 3.0 and ASP.NET MVC 4.0 versions).

Code to Review:

Let’s say that you have an ASP.NET MVC 4.0 (or 3.0) project with this configuration:

protected void Application_Start()
{
    AreaRegistration.RegisterAllAreas();

    RegisterGlobalFilters(GlobalFilters.Filters);
    RegisterRoutes       (RouteTable.Routes);

    ModelBinders.Binders.Add(typeof(Cart), new CartModelBinder());            
    DependencyResolver.SetResolver(new NinjectDependencyResolver());
}

With the CartModelBinder (used to create instances of the Cart Object) looking like this:

public class CartModelBinder : DefaultModelBinder
{
    private const string sessionKey = "Cart";
    
    protected override object CreateModel(ControllerContext controllerContext, ModelBindingContext bindingContext, Type modelType)
    {
        // get the Cart from the session
        Cart cart = (Cart)controllerContext.HttpContext.Session[sessionKey];
        // create the Cart if there wasn't one in the session data
        if (cart == null)
        {
            cart = new Cart();
            controllerContext.HttpContext.Session[sessionKey] = cart;
        }
        // return the cart
        return cart;
    }
}

With these Cart, CartLine and Product Model classes:

public class Cart
{
    private List<CartLine> lineCollection = new List<CartLine>();

    public IEnumerable<CartLine> Lines
    {
        get { return lineCollection; }
    }
}

public class CartLine
{
    public Product Product { get; set; }
    public int Quantity { get; set; }
}

public class Product
{
    [HiddenInput(DisplayValue = false)]
    public int ProductID { get; set; }
    
    [Required(ErrorMessage = "Please enter a product name")]
    public string Name { get; set; }        
    
    [DataType(DataType.MultilineText)]
    [Required(ErrorMessage = "Please enter a description")]
    public string Description { get; set; }
    
    [Required]
    [Range(0.01, double.MaxValue, ErrorMessage = "Please enter a positive price")]
    public decimal Price { get; set; }

    //[HiddenInput(DisplayValue = false)]
    public int CategoryID { get; set; }

    public virtual Category Category { get; set; }
}

And finally, with these controllers:

public RedirectToRouteResult AddToCart(Cart cart, int productId, string returnUrl)
{
    Product product = repository.Products
        .FirstOrDefault(p => p.ProductID == productId);

    if (product != null)
    {
        cart.AddItem(product, 1);
    }
    return RedirectToAction("Index", new { returnUrl });
}

public PartialViewResult Summary(Cart cart)
{
    return PartialView(cart);
}

public ViewResult Index(Cart cart,string returnUrl)
{
    return View(new CartIndexViewModel
    {
        Cart = cart,
        ReturnUrl = returnUrl
    });
}

Can you spot any security vulnerability(ies)?

What about if we change the CartModelBinder to use the IModelBinder (and BindModel) instead of DefaultModelBinder (and CreateModel)?

public class CartModelBinder : IModelBinder
{
    private const string sessionKey = "Cart";

    public object BindModel(ControllerContext controllerContext,ModelBindingContext bindingContext)
    {

        // get the Cart from the session
        Cart cart = (Cart)controllerContext.HttpContext.Session[sessionKey];
        // create the Cart if there wasn't one in the session data
        if (cart == null)
        {
            cart = new Cart();
            controllerContext.HttpContext.Session[sessionKey] = cart;
        }
        // return the cart
        return cart;
    }
}

Anything changed? (from a security point of view, since the app works ok with both versions)

Can you spot any security vulnerability with this 2nd version of the CartModelBinder?

Or are both vulnerable?

:)

Wrapping up: Here is the question being asked in this post:

What is are the security implications of these two variations of an CartModelBinder (which both provide the same business-logic functionality)

Related Threads:

Some references and related articles:

External: