Rule description: When designing an ASP.NET MVC controller, be mindful of cross-site request forgery attacks. A cross-site request forgery attack can send malicious requests from an authenticated user to your ASP.NET MVC controller.
The rule should check that actions either:
- Have the [ValidateAntiforgeryTokenAttribute] and specify allowed HTTP verbs, not including HTTP GET, or
- Specify HTTP GET as an allowed verb.
To identify actions and limit potential false positives, the rule should only apply to methods that return an ActionResult type or a type that inherits from ActionResult (e.g. JsonResult).
Snippet of noncompliant code:
namespace TestNamespace
{
using System.Web.Mvc;
public class TestController : Controller
{
public ActionResult TransferMoney(string toAccount, string amount)
{
// You don't want an attacker to specify to who and how much money to transfer.
return null;
}
}
}
or, worse:
namespace TestNamespace
{
using System.Web.Mvc;
public class TestController : Controller
{
[HttpPost]
public ActionResult TransferMoney(string toAccount, string amount)
{
// You don't want an attacker to specify to who and how much money to transfer.
return null;
}
}
}
Snippet of compliant code:
using System;
using System.Xml;
namespace TestNamespace
{
using System.Web.Mvc;
public class TestController : Controller
{
[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult TransferMoney(string toAccount, string amount)
{
return null;
}
}
}
And an example for HTTP GET requests:
Snippet of noncompliant code:
namespace TestNamespace
{
using System.Web.Mvc;
public class TestController : Controller
{
public ActionResult Help(int topicId)
{
// This Help method is an example of a read-only operation with no harmful side effects.
return null;
}
}
}
Snippet of compliant code:
namespace TestNamespace
{
using System.Web.Mvc;
public class TestController : Controller
{
[HttpGet]
public ActionResult Help(int topicId)
{
return null;
}
}
}
External references:
Type: Vulnerability, specifically CSRF
I just came across a bunch of [HttpPost] actions without the [ValidateAntiForgeryToken] attribute in the legacy codebase for one of our projects. While easy to fix, this rule would be a great help to make sure this doesn’t happen in the first place.
