From 2a3455902ffe3ec2623b4d973cf2a0fa49532fa5 Mon Sep 17 00:00:00 2001 From: Paulino Calderon Date: Tue, 22 Sep 2020 00:51:15 -0500 Subject: [PATCH 1/4] Adds check for Server Side Template Injection vulnerabilities in MVC ASP.NET applications using RazorEngine Adds check for Server Side Template Injection vulnerabilities in MVC ASP.NET applications using RazorEngine --- .../src/experimental/CWE-095/razor_parse.ql | 71 +++++++++++++++++++ .../experimental/CWE-095/razor_parse.qlhelp | 19 +++++ 2 files changed, 90 insertions(+) create mode 100644 csharp/ql/src/experimental/CWE-095/razor_parse.ql create mode 100644 csharp/ql/src/experimental/CWE-095/razor_parse.qlhelp diff --git a/csharp/ql/src/experimental/CWE-095/razor_parse.ql b/csharp/ql/src/experimental/CWE-095/razor_parse.ql new file mode 100644 index 000000000000..68caaaa2d8ab --- /dev/null +++ b/csharp/ql/src/experimental/CWE-095/razor_parse.ql @@ -0,0 +1,71 @@ +/** + * @name Server-Side Template Injection in RazorEngine + * @description User-controlled data may be evaluated, leading to arbitrary code execution. + * @kind path-problem + * @problem.severity error + * @precision high + * @id csharp/razor-injection + * @tags security + */ + +import csharp +import semmle.code.csharp.dataflow.TaintTracking +import DataFlow::PathGraph + +/* + * The offending method is Parse from RazorEngine.Razor. + */ + +class RazorEngineClass extends Class { + RazorEngineClass() { this.hasQualifiedName("RazorEngine.Razor") } + + Method getIsValidMethod() { + result.getDeclaringType() = this and + result.hasName("Parse") + } +} + +/* + * We are only interested in ASP.NET MVC Controller classes + */ + +class ControllerMVC extends Class { + ControllerMVC() { this.hasQualifiedName("System.Web.Mvc", "Controller") } +} + +/* + * We filter by the ActionResult. I think this might not be needed. + */ + +class ActionResultCall extends Call { + ActionResultCall() { + this.getEnclosingCallable().getAnnotatedReturnType().toString() = "ActionResult" + } +} + +/* + * TaintTracking configuration that will track any public method in MVC controllers that takes arguments that are parsed later by RazorEngine.Parse. + */ + +class RazorEngineInjection extends TaintTracking::Configuration { + RazorEngineInjection() { this = "RazorEngineInjection" } + + override predicate isSource(DataFlow::Node source) { + exists(ActionResultCall ar | + ar.getEnclosingCallable().getDeclaringType().getABaseType() instanceof ControllerMVC and + source.asParameter() = ar.getEnclosingCallable().getAParameter() + ) + } + + override predicate isSink(DataFlow::Node sink) { + exists(RazorEngineClass rec, MethodCall mc | + mc.getQualifiedDeclaration() = rec.getIsValidMethod() and + sink.asExpr() = mc.getArgument(0) + ) + } +} + +from RazorEngineInjection cfg, DataFlow::PathNode source, DataFlow::PathNode sink +where cfg.hasFlowPath(source, sink) +select sink, source, sink, + "Server-Side Template Injection in RazorEngine leads to Remote Code Execution" diff --git a/csharp/ql/src/experimental/CWE-095/razor_parse.qlhelp b/csharp/ql/src/experimental/CWE-095/razor_parse.qlhelp new file mode 100644 index 000000000000..e23c52867da8 --- /dev/null +++ b/csharp/ql/src/experimental/CWE-095/razor_parse.qlhelp @@ -0,0 +1,19 @@ + + + +

This rule finds public ActualResults methods that have user controlled parameters that could contain malicious code that +is parsed by RazorEngine.Parse() leading to a Server-Side Template Injection vulnerability. + + + +

Do not use the class RazorEngine. If not possible, do not parse user input into the template.

+ + + +
  • RazorParser.
    • +
  • RazorEngine Injection ASP.NET.
    • + +     +     \ No newline at end of file From b0dd573f62fd7dec93e09f1f93c0def65364e380 Mon Sep 17 00:00:00 2001 From: Paulino Calderon Date: Tue, 22 Sep 2020 11:39:55 -0500 Subject: [PATCH 2/4] Fixes method name --- csharp/ql/src/experimental/CWE-095/razor_parse.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csharp/ql/src/experimental/CWE-095/razor_parse.ql b/csharp/ql/src/experimental/CWE-095/razor_parse.ql index 68caaaa2d8ab..39d7e4dbbd1f 100644 --- a/csharp/ql/src/experimental/CWE-095/razor_parse.ql +++ b/csharp/ql/src/experimental/CWE-095/razor_parse.ql @@ -19,7 +19,7 @@ import DataFlow::PathGraph class RazorEngineClass extends Class { RazorEngineClass() { this.hasQualifiedName("RazorEngine.Razor") } - Method getIsValidMethod() { + Method getParseMethod() { result.getDeclaringType() = this and result.hasName("Parse") } From b36b07cb4ee3d44e616d395e810a684e7dd86e45 Mon Sep 17 00:00:00 2001 From: Paulino Calderon Date: Tue, 22 Sep 2020 14:06:39 -0500 Subject: [PATCH 3/4] Extends possible sources Adds new ASP.NET Core MVC namespace and ControllerBase --- csharp/ql/src/experimental/CWE-095/razor_parse.ql | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/csharp/ql/src/experimental/CWE-095/razor_parse.ql b/csharp/ql/src/experimental/CWE-095/razor_parse.ql index 39d7e4dbbd1f..162ed72ca15f 100644 --- a/csharp/ql/src/experimental/CWE-095/razor_parse.ql +++ b/csharp/ql/src/experimental/CWE-095/razor_parse.ql @@ -9,6 +9,7 @@ */ import csharp +import semmle.code.csharp.frameworks.microsoft.AspNetCore import semmle.code.csharp.dataflow.TaintTracking import DataFlow::PathGraph @@ -26,11 +27,16 @@ class RazorEngineClass extends Class { } /* - * We are only interested in ASP.NET MVC Controller classes + * We are only interested in ASP.NET MVC Controller or ControllerBase classes */ class ControllerMVC extends Class { - ControllerMVC() { this.hasQualifiedName("System.Web.Mvc", "Controller") } + ControllerMVC() { + this.hasQualifiedName("System.Web.Mvc", "Controller") or + this.hasQualifiedName("Microsoft.AspNetCore.Mvc", "Controller") or + this instanceof MicrosoftAspNetCoreMvcController or + this instanceof MicrosoftAspNetCoreMvcControllerBaseClass + } } /* @@ -59,7 +65,7 @@ class RazorEngineInjection extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { exists(RazorEngineClass rec, MethodCall mc | - mc.getQualifiedDeclaration() = rec.getIsValidMethod() and + mc.getQualifiedDeclaration() = rec.getParseMethod() and sink.asExpr() = mc.getArgument(0) ) } From 859079fcd1e29c9d9cc44a876d7a0a16bd6e8022 Mon Sep 17 00:00:00 2001 From: Paulino Calderon Date: Wed, 30 Sep 2020 23:25:51 -0500 Subject: [PATCH 4/4] Updates sources to accept both POST requests and MVC methods --- .../src/experimental/CWE-095/razor_parse.ql | 37 +++++++------------ 1 file changed, 14 insertions(+), 23 deletions(-) diff --git a/csharp/ql/src/experimental/CWE-095/razor_parse.ql b/csharp/ql/src/experimental/CWE-095/razor_parse.ql index 162ed72ca15f..b389c4784ad1 100644 --- a/csharp/ql/src/experimental/CWE-095/razor_parse.ql +++ b/csharp/ql/src/experimental/CWE-095/razor_parse.ql @@ -11,6 +11,8 @@ import csharp import semmle.code.csharp.frameworks.microsoft.AspNetCore import semmle.code.csharp.dataflow.TaintTracking +import semmle.code.csharp.dataflow.flowsources.Remote +import semmle.code.csharp.frameworks.system.web.Mvc as Mvc import DataFlow::PathGraph /* @@ -26,26 +28,18 @@ class RazorEngineClass extends Class { } } -/* - * We are only interested in ASP.NET MVC Controller or ControllerBase classes - */ - -class ControllerMVC extends Class { - ControllerMVC() { - this.hasQualifiedName("System.Web.Mvc", "Controller") or - this.hasQualifiedName("Microsoft.AspNetCore.Mvc", "Controller") or - this instanceof MicrosoftAspNetCoreMvcController or - this instanceof MicrosoftAspNetCoreMvcControllerBaseClass +class Controller extends Class { + Controller() { + this instanceof Mvc::Controller + or + this instanceof MicrosoftAspNetCoreMvcController } -} -/* - * We filter by the ActionResult. I think this might not be needed. - */ - -class ActionResultCall extends Call { - ActionResultCall() { - this.getEnclosingCallable().getAnnotatedReturnType().toString() = "ActionResult" + Method getAPostActionMethod() { + result = this.(Mvc::Controller).getAPostActionMethod() + or + result = this.(MicrosoftAspNetCoreMvcController).getAnActionMethod() and + result.getAnAttribute() instanceof MicrosoftAspNetCoreMvcHttpPostAttribute } } @@ -57,15 +51,12 @@ class RazorEngineInjection extends TaintTracking::Configuration { RazorEngineInjection() { this = "RazorEngineInjection" } override predicate isSource(DataFlow::Node source) { - exists(ActionResultCall ar | - ar.getEnclosingCallable().getDeclaringType().getABaseType() instanceof ControllerMVC and - source.asParameter() = ar.getEnclosingCallable().getAParameter() - ) + source.asParameter() = any(Controller c).getAPostActionMethod().getAParameter() } override predicate isSink(DataFlow::Node sink) { exists(RazorEngineClass rec, MethodCall mc | - mc.getQualifiedDeclaration() = rec.getParseMethod() and + mc.getTarget() = rec.getParseMethod() and sink.asExpr() = mc.getArgument(0) ) }