SG0016 (CSRF) is displayed even for method not bound to a view

[xperiandri]

Could you add a check that if view is not returned from controller then the warning is not displayed?
As it is extremely annoying

[h3xstream]

Can you provided an example of controller method for illustration of the false positive. It does not have to be a complete implementation.

[xperiandri]

Can I send you a reproduction in private? Write me email or accept LinkedIn request

[h3xstream]

I would expect it to fit in a couple of lines of code just to illustrate the false positive.

[xperiandri]

// POST: 1/Passengers/Account/Register
/// <summary>Creates a new passenger.</summary>
/// <remarks>Only application/x-www-form-urlencoded POST allowed.</remarks>
/// <returns>Passenger entity id in the location header.</returns>
[AllowAnonymous]
[HttpPost("[action]")]
[Consumes("application/x-www-form-urlencoded")]
[ProducesResponseType(typeof(void), (int)HttpStatusCode.Created)]
[ProducesResponseType(typeof(IdentityError[]), (int)HttpStatusCode.BadRequest)]
public async Task<IActionResult> Register(
    [FromForm, Required] string userName,
    [FromForm] string email,
    [FromForm, Required] string phone,
    [FromForm, Required] string password)
{
    if (ModelState.IsValid)
    {
        var passenger = new Passenger
        {
            UserName = userName,
            Email = email,
            PhoneNumber = phone,
        };

        try
        {
            var result = await userManager.CreateAsync(passenger, password);
            if (result.Succeeded)
            {
                // For more information on how to enable account confirmation and password reset please visit http://go.microsoft.com/fwlink/?LinkID=532713
                // Send an email with this link
                //var code = await _userManager.GenerateEmailConfirmationTokenAsync(user);
                //var callbackUrl = Url.Action("ConfirmEmail", "Account", new { userId = user.Id, code = code }, protocol: Context.Request.Scheme);
                //await _emailSender.SendEmailAsync(model.Email, "Confirm your account",
                //    "Please confirm your account by clicking this link: <a href=\"" + callbackUrl + "\">link</a>");
                await signInManager.SignInAsync(passenger, isPersistent: false);
                return Created(new Uri($"1/Passengers/{passenger.Id}", UriKind.Relative), null);
            }

            return BadRequest(result.Errors);
        }
        catch (Exception ex)
        {
            // TODO: Write log message
            throw;
        }
    }

    return BadRequest(ModelState);
}

[xperiandri]

Am I right that this check is relevant only for posts coming from views and they must return views. Otherwise they appear to be API calls and anti-forgery token can't be supplied or is irrelevant.
Is my definition correct?

[h3xstream]

Content type allowed

A malicious form could easily submit to this endpoint. [Consumes("application/x-www-form-urlencoded")]

On the other hand, I am thinking about method that will specify [Consumes("application/json")]. These are theoretically not reachable from other domain because the header Content-Type can not be forged.
Does not seems to be the case of the previous method.

Accessibility / Risk

For a registration form, a CSRF has very small benefit for an attacker since the form is most likely public (see [AllowAnonymus]). If a critical function (password change, profile update, etc) has AllowAnonymous, the vulnerability will be that it is public not the requirement for CSRF token.

TODO:

• Do not rise a warning if the method is annotated with [AllowAnonymus]
• Do not rise a warning if the method accept a content-type that can not be forged (some research needed)

[xperiandri]

• Do not raise warning if controller does not have [Authorize] attribute or method has [AllowAnonymus]
• Do not raise warning if no one code branch returns ViewResult from method

[xperiandri]

So, could you fix that or point me to a piece of code where I can fix it myself to make pull request?

[h3xstream]

@xperiandri This is the analyzer https://github.com/dotnet-security-guard/roslyn-security-guard/blob/master/RoslynSecurityGuard/Analyzers/OutputCacheAnnotationAnalyzer.cs