Missing cross-site request forgery token validation query (experimental) #16942
Conversation
|
QHelp previews: csharp/ql/src/experimental/CWE-352/MissingAntiForgeryTokenValidationAspNetCore.qhelpMissing cross-site request forgery token validationWeb applications that use tokens to prevent cross-site request forgery (CSRF) should validate the tokens for all HTTP POST requests. Although login and authentication methods are not vulnerable to traditional CSRF attacks, they still need to be protected with a token or other mitigation. This because an unprotected login page can be used by an attacker to force a login using an account controlled by the attacker. Subsequent requests to the site are then made using this account, without the user being aware that this is the case. This can result in the user associating private information with the attacker-controlled account. RecommendationThe appropriate attribute should be added to this method to ensure the anti-forgery token is validated when this action method is called. If using the MVC-provided anti-forgery framework this will be the Alternatively, you may consider including a global filter that applies token validation to all POST requests. ExampleIn the following example an ASP.NET MVC using System.Web.Mvc;
public class HomeController : Controller
{
// BAD: Anti forgery token has been forgotten
[HttpPost]
public ActionResult Login()
{
return View();
}
// GOOD: Anti forgery token is validated
[HttpPost]
[ValidateAntiForgeryToken]
public ActionResult UpdateDetails()
{
return View();
}
}References
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private import semmle.code.csharp.frameworks.system.web.Mvc as Mvc | ||
| private import semmle.code.csharp.frameworks.microsoft.AspNetCore as AspNetCore | ||
|
|
||
| /** Defines classes used for anti-forgery tokens. */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
| result = this.(AspNetCore::AntiForgeryClass).getValidateMethod() | ||
| } | ||
| } | ||
| /** Defines the anti-forgery token attribute. */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
| } | ||
| } | ||
|
|
||
| /** Defines a generic controller including Mvc and AspNetCore */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
| } | ||
| } | ||
|
|
||
| /** Find a filter that auto-validates anti-forgery tokens. */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
| } | ||
| } | ||
|
|
||
| /** Accounts for custom auto-validation classes with a similar name */ |
Check warning
Code scanning / CodeQL
Class QLDoc style. Warning
…github.com/aegilops/codeql into cs/web/missing-token-validation-aspnetcore
|
I should expand this to cover other definitely state-changing HTTP verbs, such as This will not cover CSRF with |
There was a problem hiding this comment.
Thank you very much for the contribution - it would be really great if we can get ASP.NET coverage for this query as well! Good work!
Some overall questions and comments
(1) The comments about the QL doc need to be addressed.
(2) It looks like the primary “controversial” difference between the new experimental and the existing query is the removal of the assumption about the presence of at least one of the “interesting” attributes before we start to produce alerts. Do you see any reason why the original query wouldn’t benefit from also producing alerts for missing token validation of POST methods in ASP controllers? If not, then maybe we should consider unifying the logic and incorporate the ASP parts into the existing query and re-use this for the experimental query where the only difference is the part about the presence of at least one “interesting” attribute.
| @@ -0,0 +1,27 @@ | |||
| using System.Web.Mvc; | |||
There was a problem hiding this comment.
Maybe add some more tests.
(1) Since ASP.NET is now covered by this query, then maybe add a test for an ASP like controller (using Microsoft.AspNetCore.Mvc).
(2) Maybe add a test where one of the CSRF attributes is added to the controller (or a super type of the controller). (I know that this is not covered by the test for the original query - but it would be great to cover it as well).
(3) The most complicated logic is in the "global" add filter logic - I think it would be great to add some tests for this as well.
| exists(Attribute attribute | | ||
| ( | ||
| attribute instanceof ValidateAntiForgeryTokenAttribute or | ||
| attribute instanceof IgnoreAntiforgeryTokenAttribute or |
There was a problem hiding this comment.
Why do we allow this? The description of the attribute states that "A filter that skips antiforgery token validation.".
| import csharp | ||
| import experimental.code.csharp.MissingAntiForgeryTokenValidationAspNetCore | ||
|
|
||
| from Method postMethod, Controller c |
There was a problem hiding this comment.
The existing query assumes that if none of the attributes to ensure token validation is identified, other means of validation are performed (contrary to this query). Do you think that was a bad assumption on the original query?
| @@ -0,0 +1,19 @@ | |||
| using System.Web.Mvc; | |||
There was a problem hiding this comment.
Maybe an ASP example would make sense instead (this is also what is mentioned in the qlhelp).
|
|
||
| /** The ignore anti-forgery token attribute. */ | ||
| class IgnoreAntiforgeryTokenAttribute extends Attribute { | ||
| IgnoreAntiforgeryTokenAttribute() { this.getType().getName() = "IgnoreAntiforgeryTokenAttribute" } |
There was a problem hiding this comment.
Use hasFullyQualifiedName instead.
| --- | ||
| category: minorAnalysis | ||
| --- | ||
| * A "Missing cross-site request forgery token validation" query for AspNetCore (`cs/web/missing-token-validation-aspnetcore`) was added as an `experimental` query. |
There was a problem hiding this comment.
Maybe mention that this query also covers MVC.
|
|
||
| <p>Although login and authentication methods are not vulnerable to traditional | ||
| CSRF attacks, they still need to be protected with a token or other mitigation. | ||
| This because an unprotected login page can be used by an attacker to force a |
| MethodCall addControllers, LambdaExpr lambda, ParameterAccess options, PropertyCall filters, | ||
| MethodCall add | ||
| | | ||
| // "AddMvc", "AddControllersWithViews", "AddMvcCore", "AddControllers", "AddRazorPages", so generalised to "Add*" to future-proof |
There was a problem hiding this comment.
The the second argument of AddRazorPages is of type Action<RazorPageOptions> and RazorPageOptions doesn't have a Filter property.
Should the logic also check that the first parameter to the lambda is of type MvcOptions?
| MethodCall addControllers, LambdaExpr lambda, ParameterAccess options, PropertyCall filters, | ||
| MethodCall add | ||
| | | ||
| // "AddMvc", "AddControllersWithViews", "AddMvcCore", "AddControllers", "AddRazorPages", so generalised to "Add*" to future-proof |
There was a problem hiding this comment.
If all the methods are in Microsoft.Extensions.DependencyInjection then maybe we should check the qualifier?
| ) | ||
| or | ||
| // Add<AutoValidateAntiforgeryTokenAttribute>() | ||
| add.getTarget().getName() = "Add<AutoValidateAntiforgeryTokenAttribute>" |
There was a problem hiding this comment.
Should we also cover the method Add<TFilterType>(Int32)?
New version of the existing
cs/web/missing-token-validationquery that adds:Any
POSTmethod without either an explicit CSRF attribute, or an identifable global CSRF filter, will be reported as an alert.This is in contrast to the existing approach, which only alerts if one method is missing a CSRF filter, where others are not.
The existing approach is quite permissive, and results in missing results in both synthetic web applications and in real applications.