Skip to content
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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

cldrn
Copy link
Contributor

@cldrn cldrn commented Sep 22, 2020

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

…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() {
Copy link
Contributor

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?

Copy link
Contributor Author

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()

@RasmusWL RasmusWL changed the title Adds check for Server Side Template Injection vulnerabilities in RazorEngine C#: Adds check for Server Side Template Injection vulnerabilities in RazorEngine Sep 22, 2020
*/

class ControllerMVC extends Class {
ControllerMVC() { this.hasQualifiedName("System.Web.Mvc", "Controller") }
Copy link
Contributor

@denislevin denislevin Sep 22, 2020

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")) } }

Copy link
Contributor Author

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
@hvitved hvitved self-assigned this Sep 28, 2020
Copy link
Contributor

@hvitved hvitved left a 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)?

csharp/ql/src/experimental/CWE-095/razor_parse.ql Outdated Show resolved Hide resolved
csharp/ql/src/experimental/CWE-095/razor_parse.ql Outdated Show resolved Hide resolved
@JarLob
Copy link
Contributor

JarLob commented Oct 7, 2020

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 Razor.Parse, there are more sinks in the library like ParseMany, Compile, CreateTemplate, etc. Also the methods from RazorEngine.Razor are deprecated and it is recommended to use the IRazorEngineService interface. I tried to improve the query (see below) by adding all these sinks and also by using any RemoteFlowSource as a source (not only arguments from Post action) however it also didn't find anything.
It is a valid query, but the pattern is not an example of regular Razor usage. The vulnerable app must use the specific library in order to be vulnerable. On the one hand the project is quite popular (1.9k stars), on the other hand "searching for new maintainers". This may be the reason there were no findings.
Do you have more ideas how to improve the query or add other patterns of "extracted Razor" usage?

/**
 * @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"

@cldrn
Copy link
Contributor Author

cldrn commented Oct 14, 2020

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.

@JarLob
Copy link
Contributor

JarLob commented Jan 12, 2021

Hey @cldrn, did you have a chance to look into this?

@cldrn
Copy link
Contributor Author

cldrn commented Jan 27, 2021

Hi, not really. Q4 got in the way and I'm just getting a chance to do fun things again hehe.

@JarLob
Copy link
Contributor

JarLob commented Mar 1, 2021

Hey, @cldrn any update on this?

@JarLob
Copy link
Contributor

JarLob commented Apr 19, 2021

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.

@cldrn
Copy link
Contributor Author

cldrn commented Apr 20, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants