-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C#: Adds check for Server Side Template Injection vulnerabilities in RazorEngine #4313
base: main
Are you sure you want to change the base?
Conversation
…ASP.NET applications using RazorEngine Adds check for Server Side Template Injection vulnerabilities in MVC ASP.NET applications using RazorEngine
class RazorEngineClass extends Class { | ||
RazorEngineClass() { this.hasQualifiedName("RazorEngine.Razor") } | ||
|
||
Method getIsValidMethod() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called getIsValidMethod
and not e.g. get(A)ParseMethod
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. Old function name. Changed it to getParseMethod()
*/ | ||
|
||
class ControllerMVC extends Class { | ||
ControllerMVC() { this.hasQualifiedName("System.Web.Mvc", "Controller") } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New ASP.NET Core MVC has a different namespace. It's possible that this query can cover new MVC if you import codeql/csharp/ql/src/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll and include the new namespace controller ('this.hasQualifiedName("Microsoft.AspNetCore.Mvc", "Controller")' or 'this instanceof MicrosoftAspNetCoreMvcController' )
Also, there could be custom controllers derived from ControllerBase. Consider defining namespace separately from the controller (see codeql/csharp/ql/src/semmle/code/csharp/frameworks/microsoft/AspNetCore.qll ):
class MicrosoftAspNetCoreMvcControllerBaseClass extends Class { MicrosoftAspNetCoreMvcControllerBaseClass() { getNamespace() = any(MicrosoftAspNetCoreMvcNamespace h) and (hasName("Controller") or hasName("ControllerBase")) } }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I've added the support for both the new namespace and ControllerBase classes.
Adds new ASP.NET Core MVC namespace and ControllerBase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. I have a few comments. Out of interest, did this query return any results on https://github.com/cnotin/RazorVulnerableApp? Also, is there a good reason why sources are restricted to parameters of controller methods, and not just use the generic RemoteFlowSource
(which would also include e.g. cookie values and URL parameters)?
Hi @cldrn, The query unfortunately didn't find anything, so I took a deeper look into the library and found out that even though the blog post that inspired the query only talks about /**
* @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.frameworks.microsoft.AspNetCore
import semmle.code.csharp.dataflow.TaintTracking
import semmle.code.csharp.dataflow.flowsources.Remote
import DataFlow::PathGraph
class RazorEngineServiceExtensionsClass extends Class {
RazorEngineServiceExtensionsClass() { this.hasQualifiedName("RazorEngine.Templating.RazorEngineServiceExtensions") }
Method getRunCompileMethod() {
result.getDeclaringType() = this and
result.hasName("RunCompile")
}
Method getAddTemplateMethod() {
result.getDeclaringType() = this and
result.hasName("AddTemplate")
}
Method getCompileMethod() {
result.getDeclaringType() = this and
result.hasName("Compile")
}
Method getCompileRunnerMethod() {
result.getDeclaringType() = this and
result.hasName("CompileRunner")
}
}
class LoadedTemplateSourceClass extends Class {
LoadedTemplateSourceClass() { this.hasQualifiedName("RazorEngine.Templating.LoadedTemplateSource") }
}
class RazorEngineClass extends Class {
RazorEngineClass() { this.hasQualifiedName("RazorEngine.Razor") }
Method getParseMethod() {
result.getDeclaringType() = this and
result.hasName("Parse")
}
Method getParseManyMethod() {
result.getDeclaringType() = this and
result.hasName("ParseMany")
}
Method getCompileMethod() {
result.getDeclaringType() = this and
result.hasName("Compile")
}
Method getCreateTemplateMethod() {
result.getDeclaringType() = this and
result.hasName("CreateTemplate")
}
Method getCreateTemplatesMethod() {
result.getDeclaringType() = this and
result.hasName("CreateTemplates")
}
Method getGetTemplateMethod() {
result.getDeclaringType() = this and
result.hasName("GetTemplate")
}
Method getGetTemplatesMethod() {
result.getDeclaringType() = this and
result.hasName("GetTemplates")
}
Method getCreateTemplateTypeMethod() {
result.getDeclaringType() = this and
result.hasName("CreateTemplateType")
}
Method getCreateTemplateTypesMethod() {
result.getDeclaringType() = this and
result.hasName("CreateTemplateTypes")
}
}
class RazorEngineInjection extends TaintTracking::Configuration {
RazorEngineInjection() { this = "RazorEngineInjection" }
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
succ.asExpr().(ArrayInitializer).getAnElement() = pred.asExpr()
or
exists(AssignableDefinition def |
pred.asExpr() = def.getSource() and
succ.asExpr() = def.getTargetAccess().(ArrayWrite).getQualifier()
)
or
exists(MethodCall mc |
mc.getQualifiedDeclaration().getReturnType().getQualifiedName().matches("System.Collections.Generic.IEnumerable%") and
succ.asExpr() = mc and
pred.asExpr() = mc.getAnArgument()
)
}
override predicate isSource(DataFlow::Node source) {
source instanceof RemoteFlowSource
}
override predicate isSink(DataFlow::Node sink) {
exists(RazorEngineClass rec, MethodCall mc |
(
mc.getTarget() = rec.getParseMethod() or
mc.getTarget() = rec.getParseManyMethod() or
mc.getTarget() = rec.getCompileMethod() or
mc.getTarget() = rec.getCreateTemplateMethod() or
mc.getTarget() = rec.getCreateTemplatesMethod() or
mc.getTarget() = rec.getGetTemplateMethod() or
mc.getTarget() = rec.getGetTemplatesMethod() or
mc.getTarget() = rec.getCreateTemplateTypeMethod() or
mc.getTarget() = rec.getCreateTemplateTypesMethod()
)
and
sink.asExpr() = mc.getArgument(0)
)
or
exists(RazorEngineServiceExtensionsClass rec, MethodCall mc |
(
mc.getTarget() = rec.getRunCompileMethod() or
mc.getTarget() = rec.getCompileMethod() or
mc.getTarget() = rec.getAddTemplateMethod() or
mc.getTarget() = rec.getCompileRunnerMethod()
)
and
sink.asExpr() = mc.getArgumentForName("templateSource")
)
or
exists(ObjectCreation oc, LoadedTemplateSourceClass t |
oc.getTarget().getDeclaringType() = t and
sink.asExpr() = oc.getArgumentForName("template")
)
}
}
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" |
Woah, I did not expect that. I'll take a look to check if I'm missing something. I thought it was going to be a good simple example for a blog post and that it would yield more findings but with no findings is not worth writing about it. We got hit a storm and a hurricane in a 5 day period so I'm catching up with everything. I will have more time to look into this on the weekend. |
Hey @cldrn, did you have a chance to look into this? |
Hi, not really. Q4 got in the way and I'm just getting a chance to do fun things again hehe. |
Hey, @cldrn any update on this? |
Hi @cldrn, any ideas on improving the query or maybe you found a real project vulnerable to this pattern? If not we'll discuss with CodeQL team if they want the query merged and try to finalize the PR this week. |
Hi, Originally, I modeled the query using the project RazorVulnerableApp and I was hoping to discover some projects affected by this with your larger project db. I wonder if there is an issue while building-analyzing these types of projects in batch. I will keep playing around/looking for other projects using this framework to see if I am missing something else. Thanks a lot for all your help while merging this query. |
Adds check for Server Side Template Injection that leads to Remote Code Execution vulnerabilities in MVC ASP.NET applications using the RazorEngine class. MVC controller methods that receive user input that is parsed by the template engine is flagged with this rule.
ASP.NET Razor templates is listed as supported by CodeQL, yet only XSS is detected and we are missing this old yet critical vulnerability.
More info: Server Side Template Injection (SSTI) in ASP.NET Razor