From b93aaea3d17b8898f17757c8a79aac1bae8c4b4b Mon Sep 17 00:00:00 2001 From: Evgenii Date: Sun, 18 Jul 2021 21:12:20 +0300 Subject: [PATCH 1/3] Introduce Freemarker --- .../Security/CWE/CWE-094/Freemarker.qll | 179 ++++++++++++++++++ .../CWE-094/FreemarkerTaintedTemplate.java | 24 +++ .../CWE-094/FreemarkerTaintedTemplate.qhelp | 31 +++ .../CWE/CWE-094/FreemarkerTaintedTemplate.ql | 34 ++++ .../CWE-094/FreemarkerUnsafeConfiguration.ftl | 17 ++ .../FreemarkerUnsafeConfiguration.java | 28 +++ .../FreemarkerUnsafeConfiguration.qhelp | 36 ++++ .../CWE-094/FreemarkerUnsafeConfiguration.ql | 21 ++ 8 files changed, 370 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.ql create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.ftl create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll b/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll new file mode 100644 index 000000000000..f1acb1b3e8aa --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll @@ -0,0 +1,179 @@ +/** + * This library models Apache FreeMarker template engine + */ + +import java +import semmle.code.java.dataflow.DataFlow2 +import semmle.code.java.dataflow.DataFlow3 + +module Freemarker { + class FreemarkerConfiguration extends RefType { + FreemarkerConfiguration() { this.hasQualifiedName("freemarker.core", "Configurable") } + } + + class TemplateClassResolver extends RefType { + TemplateClassResolver() { this.hasQualifiedName("freemarker.core", "TemplateClassResolver") } + } + + class FreemarkerTemplate extends RefType { + FreemarkerTemplate() { this.hasQualifiedName("freemarker.template", "Template") } + } + + // https://github.com/sanluan/PublicCMS/blob/d617de930d78e5ca17357614c1209ce410eae403/publiccms-parent/publiccms-core/src/main/java/com/publiccms/logic/component/site/DirectiveComponent.java + // https://github.com/Rekoe/Rk_Cms/blob/999854b156e4d7c8627095066e8f80f053645528/src/main/java/com/rekoe/service/FileService.java + class FreeMarkerConfigurer extends RefType { + FreeMarkerConfigurer() { + exists(string package | + this.hasQualifiedName(package, "FreeMarkerConfigurer") and + package.matches("%freemarker%") + ) + } + } + + // https://github.com/hibernate/hibernate-tools/blob/71fa3dae6ac1ac1b9c59f4fef5ba056c5ac36b34/orm/src/main/java/org/hibernate/tool/internal/export/common/TemplateHelper.java + class FreemarkerTemplateConfiguration extends RefType { + FreemarkerTemplateConfiguration() { + this.hasQualifiedName("freemarker.template", "Configuration") + } + } + + class FreemarkerStringTemplateLoader extends RefType { + FreemarkerStringTemplateLoader() { + this.hasQualifiedName("freemarker.cache", "StringTemplateLoader") + } + } + + Expr getAllowNothingResolverExpr() { + exists(Field f | + result = f.getAnAccess() and + f.hasName("ALLOWS_NOTHING_RESOLVER") and + f.getDeclaringType() instanceof TemplateClassResolver + ) + } + + class FreemarkerSetClassResolver extends MethodAccess { + FreemarkerSetClassResolver() { + exists(Method m | + m = this.getMethod() and + m.getDeclaringType() instanceof FreemarkerConfiguration and + m.hasName("setNewBuiltinClassResolver") and + m.getAParameter().getAnArgument() = getAllowNothingResolverExpr() + ) + } + } + + class FreemarkerSetAPIBuiltinEnabled extends MethodAccess { + FreemarkerSetAPIBuiltinEnabled() { + exists(Method m | + m = this.getMethod() and + m.getDeclaringType() instanceof FreemarkerConfiguration and + m.hasName("setAPIBuiltinEnabled") and + m.getAParameter().getAnArgument().(BooleanLiteral).getBooleanValue() = true + ) + } + } + + class GetConfigurationCall extends MethodAccess { + GetConfigurationCall() { + exists(Method m | + m = this.getMethod() and + m.getDeclaringType() instanceof FreeMarkerConfigurer and + m.hasName("getConfiguration") + ) + } + } + + class SafeFreemarkerConfiguration extends DataFlow2::Configuration { + SafeFreemarkerConfiguration() { this = "SafeFreemarkerConfiguration" } + + override predicate isSource(DataFlow2::Node src) { + src.asExpr() instanceof FreemarkerTemplateConfigurationSource + } + + override predicate isSink(DataFlow2::Node sink) { + sink.asExpr() = any(FreemarkerSetClassResolver r).getQualifier() + } + // override int fieldFlowBranchLimit() { result = 0 } + } + + class UnsafeFreemarkerConfiguration extends DataFlow3::Configuration { + UnsafeFreemarkerConfiguration() { this = "UnsafeFreemarkerConfiguration" } + + override predicate isSource(DataFlow3::Node src) { + src.asExpr() instanceof FreemarkerTemplateConfigurationSource + } + + override predicate isSink(DataFlow3::Node sink) { + sink.asExpr() = any(FreemarkerSetAPIBuiltinEnabled r).getQualifier() + } + // override int fieldFlowBranchLimit() { result = 0 } + } + + class FreemarkerTemplateConfigurationSource extends Expr { + FreemarkerTemplateConfigurationSource() { + this.(ClassInstanceExpr).getConstructedType() instanceof FreemarkerTemplateConfiguration + or + this instanceof GetConfigurationCall + } + + predicate isSafe() { + exists(SafeFreemarkerConfiguration safeConfig | + safeConfig + .hasFlow(DataFlow2::exprNode(this), + DataFlow2::exprNode(any(FreemarkerSetClassResolver r).getQualifier())) + ) and + not exists(UnsafeFreemarkerConfiguration unsafeConfig | + unsafeConfig + .hasFlow(DataFlow3::exprNode(this), + DataFlow3::exprNode(any(FreemarkerSetAPIBuiltinEnabled r).getQualifier())) + ) + } + } + + /** + * Template created using new expr + * `Template t = new Template("name", templateStr, cfg);` + * ref: https://freemarker.apache.org/docs/api/index.html Class Template + */ + class NewTemplate extends ClassInstanceExpr { + NewTemplate() { this.getConstructedType() instanceof FreemarkerTemplate } + + predicate isReaderArg(int index) { + this.getConstructor() + .getParameter(index) + .getType() + .(RefType) + .hasQualifiedName("java.io", "Reader") + } + + Expr getSink() { + // All constructors accept java.io.Reader as template source string. + exists(int index | + isReaderArg(index) and + result = this.getArgument(index) + ) + or + // in one case constructor accepts java.lang.String as second arg instead of java.io.Reader + this.getNumArgument() = 3 and + not isReaderArg(1) and + result = this.getArgument(1) + } + } + + /** + * Pass the template via StringTemplateLoader. + * `StringTemplateLoader stringLoader = new StringTemplateLoader();` + * `stringLoader.putTemplate("myTemplate", templateStr);` + */ + class FreemarkerPutTemplate extends MethodAccess { + FreemarkerPutTemplate() { + exists(Method m | + m = this.getMethod() and + m.getDeclaringType() instanceof FreemarkerStringTemplateLoader and + m.hasName("putTemplate") + ) + } + + Expr getSink() { result = this.getArgument(1) } + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.java b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.java new file mode 100644 index 000000000000..18a0adf7de49 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.java @@ -0,0 +1,24 @@ +package com.example.freemarkertest; + +import freemarker.template.Configuration; +import freemarker.template.Template; +import freemarker.template.TemplateExceptionHandler; +import freemarker.template.Version; +import freemarker.core.TemplateClassResolver; +import freemarker.cache.StringTemplateLoader; + +{ + Configuration cfg = new Configuration(); + cfg.setDefaultEncoding("UTF-8"); + cfg.setLocale(Locale.US); + cfg.setTemplateExceptionHandler(TemplateExceptionHandler.RETHROW_HANDLER); + + // cfg.setNewBuiltinClassResolver(TemplateClassResolver.ALLOWS_NOTHING_RESOLVER); + + // String templateStr="<#assign ex=\"freemarker.template.utility.Execute\"?new()> ${ex(\"id\")}"; + String templateStr=argv[1]; + Template t = new Template("name", new StringReader(templateStr), cfg); + Writer consoleWriter3 = new OutputStreamWriter(System.out); + t.process(input, consoleWriter3); + +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.qhelp new file mode 100644 index 000000000000..57c16eead7e7 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.qhelp @@ -0,0 +1,31 @@ + + + + +

+Template Injection occurs when user input is interpreted as template. +When an attacker is able to use native template syntax to inject a malicious payload into a template, +which is then executed server-side is results in Server Side Template Injection and Information Disclosure. +

+ + + +

+To fix this, ensure that an untrusted value is not used as a template. +

+ + + +

+Consider the example given below, an untrusted data is used to generate a template string. +This can lead to remote code execution. +Even if you disable class resolver it may lead to sensitive information disclosure through data model global variable. +

+ + + + +
  • Portswigger : [Server Side Template Injection](https://portswigger.net/web-security/server-side-template-injection)
    • +     + +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.ql b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.ql new file mode 100644 index 000000000000..b2fe74c6d303 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.ql @@ -0,0 +1,34 @@ +/** + * @id java/freemarker-tainted + * @name Tainted Freemarker Template + * @description Building a template from user-controlled sources is vulnerable to insertion of + * malicious code by the user. This may lead up to remote code execution and data leakage. + * @kind path-problem + * @problem.severity error + * @tags security + * external/cwe/cwe-094 + * @precision high + */ + +import java +import semmle.code.java.dataflow.FlowSources +import DataFlow::PathGraph +import semmle.code.java.dataflow.TaintTracking +import Freemarker + +class FreemarkerTaintedTemplateConfig extends TaintTracking::Configuration { + FreemarkerTaintedTemplateConfig() { this = "FreemarkerTaintedTemplateConfig" } + + override predicate isSource(DataFlow::Node src) { src instanceof RemoteFlowSource } + + override predicate isSink(DataFlow::Node sink) { + any(Freemarker::NewTemplate t).getSink() = sink.asExpr() + or + any(Freemarker::FreemarkerPutTemplate t).getSink() = sink.asExpr() + } + // override int fieldFlowBranchLimit() { result = 0 } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, FreemarkerTaintedTemplateConfig conf +where conf.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Template is built using $@.", source.getNode(), "user input" diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.ftl b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.ftl new file mode 100644 index 000000000000..8e640ceef93a --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.ftl @@ -0,0 +1,17 @@ + + + + + +<#function msg text args...> + <#assign directive=title?interpret> + <#assign msg> + <@directive/> + + <#return msg> + + +

    ${m.msg(title)}

    + + + diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.java b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.java new file mode 100644 index 000000000000..6851db7ae5ae --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.java @@ -0,0 +1,28 @@ +package com.example.freemarkertest; + +import freemarker.template.Configuration; +import freemarker.template.Template; +import freemarker.template.TemplateExceptionHandler; +import freemarker.template.Version; +import freemarker.core.TemplateClassResolver; +import freemarker.cache.StringTemplateLoader; + +{ + Configuration cfg = new Configuration(); + cfg.setDirectoryForTemplateLoading(new File("/home/templates")); + + cfg.setDefaultEncoding("UTF-8"); + cfg.setLocale(Locale.US); + cfg.setTemplateExceptionHandler(TemplateExceptionHandler.RETHROW_HANDLER); + + // cfg.setAPIBuiltinEnabled(true); + // cfg.setNewBuiltinClassResolver(TemplateClassResolver.ALLOWS_NOTHING_RESOLVER); + + Map input = new HashMap(); + input.put("title", argv[1]); + + Template template = cfg.getTemplate("FreemarkerUnsafeConfiguration.ftl"); + Writer consoleWriter = new OutputStreamWriter(System.out); + template.process(input, consoleWriter); + +} \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.qhelp new file mode 100644 index 000000000000..65ea0c2826ff --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.qhelp @@ -0,0 +1,36 @@ + + + + +

    +Apache FreeMarker is a template engine: a Java library to generate text output based on templates. +Usually user input is passed throught key-value data model, and it is safe to use them. +But sometimes developers allow to intepret user input as user directive with interpret filter function. +It results in executing user-controlled data as part of template, which may lead up to remote code execution. +

    +     + + +

    +It is generally recommended to avoid using interpret filter function. +Additionally you should configure template engine by setting class resolver to ALLOWS_NOTHING_RESOLVER. +

    +

    +Also there is method setAPIBuiltinEnabled which enables usage of builtin API. +By default this property is set to false. And setting this value to true is unsafe. +

    +     + + +

    +The following example pass untrusted data through data model, then interpret it as user definec directive and execute it. +

    + +     + + +
  • +CVE-2021-25770:BeanShell Injection. +
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.ql b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.ql new file mode 100644 index 000000000000..d3b6fda945aa --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.ql @@ -0,0 +1,21 @@ +/** + * @id java/freemarker-unsafe-configuration + * @name Unsafe Freemarker Configuration + * @description There is an unsafe Freemarker Configuration, that may lead to SSTI vulnerability + * that results in RCE. To protect against this + * 1) set class resolver to ALLOWS_NOTHING_RESOLVER, + * 2) dont set setAPIBuiltinEnabled to true + * 3) dont interpret user-input inside of template. + * @kind problem + * @problem.severity warning + * @tags security + * external/cwe/cwe-094 + * @precision high + */ + +import java +import Freemarker + +from Freemarker::FreemarkerTemplateConfigurationSource c +where not c.isSafe() +select c, "Unsafe Freemarker Configuration" From e715c170d9fa2cfb220c70d7643023e331f2b3b6 Mon Sep 17 00:00:00 2001 From: Evgenii Date: Wed, 28 Jul 2021 20:12:01 +0300 Subject: [PATCH 2/3] add freemarker setSetting flow --- .../Security/CWE/CWE-094/Freemarker.qll | 15 +++++++++++++++ .../CWE/CWE-094/FreemarkerTaintedTemplate.java | 1 + .../CWE-094/FreemarkerUnsafeConfiguration.java | 1 + 3 files changed, 17 insertions(+) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll b/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll index f1acb1b3e8aa..157fa508cf33 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll @@ -62,6 +62,19 @@ module Freemarker { } } + // setSetting method configuration method: https://freemarker.apache.org/docs/api/freemarker/template/Configuration.html#setSetting-java.lang.String-java.lang.String- + class FreemarkerSetSettingClassResolver extends MethodAccess { + FreemarkerSetSettingClassResolver() { + exists(Method m | + m = this.getMethod() and + m.getDeclaringType() instanceof FreemarkerTemplateConfiguration and + m.hasName("setSetting") and + m.getParameter(0).getAnArgument().(Literal).getValue().matches("new_builtin_class_resolver") and + m.getParameter(1).getAnArgument().(Literal).getValue().matches("allows_nothing") + ) + } + } + class FreemarkerSetAPIBuiltinEnabled extends MethodAccess { FreemarkerSetAPIBuiltinEnabled() { exists(Method m | @@ -92,6 +105,8 @@ module Freemarker { override predicate isSink(DataFlow2::Node sink) { sink.asExpr() = any(FreemarkerSetClassResolver r).getQualifier() + or + sink.asExpr() = any(FreemarkerSetSettingClassResolver r).getQualifier() } // override int fieldFlowBranchLimit() { result = 0 } } diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.java b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.java index 18a0adf7de49..389f213b9460 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.java +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerTaintedTemplate.java @@ -14,6 +14,7 @@ cfg.setTemplateExceptionHandler(TemplateExceptionHandler.RETHROW_HANDLER); // cfg.setNewBuiltinClassResolver(TemplateClassResolver.ALLOWS_NOTHING_RESOLVER); + // cfg.setSetting("new_builtin_class_resolver", "allows_nothing"); // String templateStr="<#assign ex=\"freemarker.template.utility.Execute\"?new()> ${ex(\"id\")}"; String templateStr=argv[1]; diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.java b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.java index 6851db7ae5ae..dd3889ee7623 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.java +++ b/java/ql/src/experimental/Security/CWE/CWE-094/FreemarkerUnsafeConfiguration.java @@ -17,6 +17,7 @@ // cfg.setAPIBuiltinEnabled(true); // cfg.setNewBuiltinClassResolver(TemplateClassResolver.ALLOWS_NOTHING_RESOLVER); + // cfg.setSetting("new_builtin_class_resolver", "allows_nothing"); Map input = new HashMap(); input.put("title", argv[1]); From 0427366321909826a812d10fffdd50c1ad3bdd9a Mon Sep 17 00:00:00 2001 From: Evgenii Date: Thu, 5 Aug 2021 17:11:44 +0300 Subject: [PATCH 3/3] java. freemarker. add allowNothing variation --- java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll b/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll index 157fa508cf33..e572bfa1d4ea 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-094/Freemarker.qll @@ -65,12 +65,13 @@ module Freemarker { // setSetting method configuration method: https://freemarker.apache.org/docs/api/freemarker/template/Configuration.html#setSetting-java.lang.String-java.lang.String- class FreemarkerSetSettingClassResolver extends MethodAccess { FreemarkerSetSettingClassResolver() { - exists(Method m | + exists(Method m, string s | m = this.getMethod() and m.getDeclaringType() instanceof FreemarkerTemplateConfiguration and m.hasName("setSetting") and m.getParameter(0).getAnArgument().(Literal).getValue().matches("new_builtin_class_resolver") and - m.getParameter(1).getAnArgument().(Literal).getValue().matches("allows_nothing") + m.getParameter(1).getAnArgument().(Literal).getValue().matches(s) and + s in ["allows_nothing", "allowsNothing"] ) } }