New C# rule: validate antiforgery tokens in MVC actions

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.

Hi Chris

Thanks for your suggestion and the examples, this is an easy attribute to miss that can have serious consequences!

I’ve chatted to our developers about this and it seems that this was considered in the design of
Disabling CSRF protections is security-sensitive. As described on this page, it’s not just a simple case that the attribute should be specified on all non-get actions, it may not always be required or the check may be applied in a different way, and by enforcing the attribute we risk a lot of false positives.

If in your application it’s the case that it is always required, you can enable the CA3147 and this should be imported into SonarQube or SonarCloud (although at the moment FPs can’t be suppressed). I’d be interested to know if this is something you have already done and how that’s worked out?

It may be the case that we can enhance Disabling CSRF protections is security-sensitive and do a more complete check around anti-forgery tokens. It’s something we’ll consider and further feedback and ideas are always welcome :slight_smile:

Thanks

Tom

1 Like

Thanks! I had no idea that I can add Roslyn rules fairly easily, and those then get auto-imported to SonarQube. This is perfect!

To handle false positives I can still add things like #pragma warning disable/restore for the CA3147 rule in the rare cases where alternative protections are in place.

This topic was automatically closed 7 days after the last reply. New replies are no longer allowed.