diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index cf34b255730..d375f8f30ad 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -45,6 +45,7 @@ dd-java-agent/instrumentation/*iast* @DataDog/asm-java dd-java-agent/instrumentation/*appsec* @DataDog/asm-java dd-java-agent/instrumentation/json/ @DataDog/asm-java dd-java-agent/instrumentation/snakeyaml/ @DataDog/asm-java +dd-java-agent/instrumentation/velocity/ @DataDog/asm-java dd-java-agent/instrumentation/freemarker/ @DataDog/asm-java dd-smoke-tests/iast-util/ @DataDog/asm-java dd-smoke-tests/spring-security/ @DataDog/asm-java diff --git a/dd-java-agent/instrumentation/velocity/build.gradle b/dd-java-agent/instrumentation/velocity/build.gradle new file mode 100644 index 00000000000..5ec6e1735b7 --- /dev/null +++ b/dd-java-agent/instrumentation/velocity/build.gradle @@ -0,0 +1,16 @@ + +apply from: "$rootDir/gradle/java.gradle" +apply plugin: 'call-site-instrumentation' +addTestSuiteForDir('latestDepTest', 'test') + +dependencies { + compileOnly group: 'org.apache.velocity', name: 'velocity', version: '1.5' + compileOnly group: 'org.apache.velocity', name: 'velocity-tools', version: '1.3' + testImplementation group: 'org.apache.velocity', name: 'velocity', version: '1.5' + testImplementation group: 'org.apache.velocity', name: 'velocity-tools', version: '1.3' + + testRuntimeOnly project(':dd-java-agent:instrumentation:iast-instrumenter') + + latestDepTestImplementation group: 'org.apache.velocity', name: 'velocity', version: '+' + latestDepTestImplementation group: 'org.apache.velocity', name: 'velocity-tools', version: '+' +} diff --git a/dd-java-agent/instrumentation/velocity/src/main/java/datadog/trace/instrumentation/velocity/ASTReferenceInstrumentation.java b/dd-java-agent/instrumentation/velocity/src/main/java/datadog/trace/instrumentation/velocity/ASTReferenceInstrumentation.java new file mode 100644 index 00000000000..74c7814ccd9 --- /dev/null +++ b/dd-java-agent/instrumentation/velocity/src/main/java/datadog/trace/instrumentation/velocity/ASTReferenceInstrumentation.java @@ -0,0 +1,66 @@ +package datadog.trace.instrumentation.velocity; + +import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.Sink; +import datadog.trace.api.iast.VulnerabilityTypes; +import datadog.trace.api.iast.sink.XssModule; +import net.bytebuddy.asm.Advice; +import org.apache.velocity.context.InternalContextAdapter; +import org.apache.velocity.runtime.parser.node.ASTReference; + +@AutoService(InstrumenterModule.class) +public class ASTReferenceInstrumentation extends InstrumenterModule.Iast + implements Instrumenter.ForSingleType { + public ASTReferenceInstrumentation() { + super("velocity"); + } + + @Override + public String instrumentedType() { + return "org.apache.velocity.runtime.parser.node.ASTReference"; + } + + @Override + public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvice( + named("render") + .and(isMethod()) + .and( + takesArgument(0, named("org.apache.velocity.context.InternalContextAdapter")) + .and(takesArgument(1, named("java.io.Writer")))), + ASTReferenceInstrumentation.class.getName() + "$ASTReferenceAdvice"); + } + + public static class ASTReferenceAdvice { + + @Advice.OnMethodEnter(suppress = Throwable.class) + @Sink(VulnerabilityTypes.XSS) + public static void onEnter( + @Advice.Argument(0) final InternalContextAdapter context, + @Advice.This final ASTReference self) { + if (self == null) { + return; + } + final XssModule xssModule = InstrumentationBridge.XSS; + if (xssModule == null) { + return; + } + Object variable = self.getVariableValue(context, self.getRootString()); + // For cases when you have a variable that is not a string such as the EscapeTool variable + if (!(variable instanceof String)) { + return; + } + final String charSec = (String) variable; + final String file = context.getCurrentTemplateName(); + final int line = self.getLine(); + xssModule.onXss(charSec, file, line); + } + } +} diff --git a/dd-java-agent/instrumentation/velocity/src/main/java/datadog/trace/instrumentation/velocity/EscapeToolCallSite.java b/dd-java-agent/instrumentation/velocity/src/main/java/datadog/trace/instrumentation/velocity/EscapeToolCallSite.java new file mode 100644 index 00000000000..85c2e5a384d --- /dev/null +++ b/dd-java-agent/instrumentation/velocity/src/main/java/datadog/trace/instrumentation/velocity/EscapeToolCallSite.java @@ -0,0 +1,55 @@ +package datadog.trace.instrumentation.velocity; + +import datadog.trace.agent.tooling.csi.CallSite; +import datadog.trace.api.iast.IastCallSites; +import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.Propagation; +import datadog.trace.api.iast.VulnerabilityMarks; +import datadog.trace.api.iast.propagation.PropagationModule; +import javax.annotation.Nullable; +import org.apache.velocity.tools.generic.EscapeTool; + +@Propagation +@CallSite(spi = IastCallSites.class) +public class EscapeToolCallSite { + + @CallSite.After( + "java.lang.String org.apache.velocity.tools.generic.EscapeTool.html(java.lang.Object)") + @CallSite.After( + "java.lang.String org.apache.velocity.tools.generic.EscapeTool.javascript(java.lang.Object)") + @CallSite.After( + "java.lang.String org.apache.velocity.tools.generic.EscapeTool.url(java.lang.Object)") + @CallSite.After( + "java.lang.String org.apache.velocity.tools.generic.EscapeTool.xml(java.lang.Object)") + public static String afterEscape( + @CallSite.This final EscapeTool self, + @CallSite.Argument(0) @Nullable final Object input, + @CallSite.Return final String result) { + final PropagationModule module = InstrumentationBridge.PROPAGATION; + if (module != null) { + try { + module.taintStringIfTainted(result, input, false, VulnerabilityMarks.XSS_MARK); + } catch (final Throwable e) { + module.onUnexpectedException("afterEscape threw", e); + } + } + return result; + } + + @CallSite.After( + "java.lang.String org.apache.velocity.tools.generic.EscapeTool.sql(java.lang.Object)") + public static String afterEscapeSQL( + @CallSite.This final EscapeTool self, + @CallSite.Argument(0) @Nullable final Object input, + @CallSite.Return final String result) { + final PropagationModule module = InstrumentationBridge.PROPAGATION; + if (module != null) { + try { + module.taintStringIfTainted(result, input, false, VulnerabilityMarks.SQL_INJECTION_MARK); + } catch (final Throwable e) { + module.onUnexpectedException("afterEscapeSQL threw", e); + } + } + return result; + } +} diff --git a/dd-java-agent/instrumentation/velocity/src/test/groovy/datadog/trace/instrumentation/velocity/ASTReferenceInstrumentationTest.groovy b/dd-java-agent/instrumentation/velocity/src/test/groovy/datadog/trace/instrumentation/velocity/ASTReferenceInstrumentationTest.groovy new file mode 100644 index 00000000000..02f11e79e3a --- /dev/null +++ b/dd-java-agent/instrumentation/velocity/src/test/groovy/datadog/trace/instrumentation/velocity/ASTReferenceInstrumentationTest.groovy @@ -0,0 +1,69 @@ +package datadog.trace.instrumentation.velocity + +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.api.iast.InstrumentationBridge +import datadog.trace.api.iast.sink.XssModule +import org.apache.velocity.Template +import org.apache.velocity.VelocityContext +import org.apache.velocity.app.VelocityEngine +import org.apache.velocity.runtime.RuntimeConstants +import org.apache.velocity.tools.generic.EscapeTool + +class ASTReferenceInstrumentationTest extends AgentTestRunner { + + @Override + protected void configurePreAgent() { + injectSysConfig('dd.iast.enabled', 'true') + } + + void 'test ASTReference execute (insecure)'() { + given: + final module = Mock(XssModule) + InstrumentationBridge.registerIastModule(module) + + VelocityEngine velocity = new VelocityEngine() + velocity.setProperty( + RuntimeConstants.RUNTIME_LOG_LOGSYSTEM_CLASS, + "org.apache.velocity.runtime.log.NullLogChute") + velocity.init() + Template template = velocity.getTemplate("src/test/resources/velocity-astreference-insecure.vm") + + VelocityContext context = new VelocityContext() + context.put("param", param) + + when: + template.merge(context, Mock(FileWriter)) + + then: + 1 * module.onXss(_, _, _) + + where: + param << ["", "name"] + } + + void 'test ASTReference execute (secure)'() { + given: + final module = Mock(XssModule) + InstrumentationBridge.registerIastModule(module) + + VelocityEngine velocity = new VelocityEngine() + velocity.setProperty( + RuntimeConstants.RUNTIME_LOG_LOGSYSTEM_CLASS, + "org.apache.velocity.runtime.log.NullLogChute") + velocity.init() + Template template = velocity.getTemplate("src/test/resources/velocity-astreference-secure.vm") + + VelocityContext context = new VelocityContext() + context.put("esc", new EscapeTool()) + context.put("param", param) + + when: + template.merge(context, Mock(FileWriter)) + + then: + 0 * module.onXss(_, _, _) + + where: + param << ["", "name"] + } +} diff --git a/dd-java-agent/instrumentation/velocity/src/test/groovy/datadog/trace/instrumentation/velocity/EscapeToolCallSiteTest.groovy b/dd-java-agent/instrumentation/velocity/src/test/groovy/datadog/trace/instrumentation/velocity/EscapeToolCallSiteTest.groovy new file mode 100644 index 00000000000..bc1ee28386c --- /dev/null +++ b/dd-java-agent/instrumentation/velocity/src/test/groovy/datadog/trace/instrumentation/velocity/EscapeToolCallSiteTest.groovy @@ -0,0 +1,41 @@ +package datadog.trace.instrumentation.velocity + +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.api.iast.InstrumentationBridge +import datadog.trace.api.iast.propagation.PropagationModule +import foo.bar.TestEscapeToolSuite +import groovy.transform.CompileDynamic + +import static datadog.trace.api.iast.VulnerabilityMarks.SQL_INJECTION_MARK +import static datadog.trace.api.iast.VulnerabilityMarks.XSS_MARK + +@CompileDynamic +class EscapeToolCallSiteTest extends AgentTestRunner { + + @Override + protected void configurePreAgent() { + injectSysConfig("dd.iast.enabled", "true") + } + + void 'test #method'() { + given: + final module = Mock(PropagationModule) + InstrumentationBridge.registerIastModule(module) + + when: + final result = TestEscapeToolSuite.&"$method".call(args) + + then: + result == expected + 1 * module.taintStringIfTainted(_ as String, args[0], false, mark) + 0 * _ + + where: + method | args | mark | expected + 'html' | ['Ø-This is a quote'] | XSS_MARK | 'Ø-This is a quote' + 'javascript' | ['Ø-This is a quote'] | XSS_MARK | '\\u00D8-This is a quote' + 'url' | ['Ø-This is a quote'] | XSS_MARK | '%C3%98-This+is+a+quote' + 'xml' | ['Ø-This is a quote'] | XSS_MARK | 'Ø-This is a quote' + 'sql' | ['Ø-This is a quote'] | SQL_INJECTION_MARK | 'Ø-This is a quote' + } +} diff --git a/dd-java-agent/instrumentation/velocity/src/test/java/foo/bar/TestEscapeToolSuite.java b/dd-java-agent/instrumentation/velocity/src/test/java/foo/bar/TestEscapeToolSuite.java new file mode 100644 index 00000000000..a3177332bb9 --- /dev/null +++ b/dd-java-agent/instrumentation/velocity/src/test/java/foo/bar/TestEscapeToolSuite.java @@ -0,0 +1,28 @@ +package foo.bar; + +import org.apache.velocity.tools.generic.EscapeTool; + +public class TestEscapeToolSuite { + + public static EscapeTool escapeTool = new EscapeTool(); + + public static String html(String input) { + return escapeTool.html(input); + } + + public static String javascript(String input) { + return escapeTool.javascript(input); + } + + public static String url(String input) { + return escapeTool.url(input); + } + + public static String xml(String input) { + return escapeTool.xml(input); + } + + public static String sql(String input) { + return escapeTool.sql(input); + } +} diff --git a/dd-java-agent/instrumentation/velocity/src/test/resources/velocity-astreference-insecure.vm b/dd-java-agent/instrumentation/velocity/src/test/resources/velocity-astreference-insecure.vm new file mode 100644 index 00000000000..727f210a29b --- /dev/null +++ b/dd-java-agent/instrumentation/velocity/src/test/resources/velocity-astreference-insecure.vm @@ -0,0 +1,3 @@ + +I am vulnerable $param + diff --git a/dd-java-agent/instrumentation/velocity/src/test/resources/velocity-astreference-secure.vm b/dd-java-agent/instrumentation/velocity/src/test/resources/velocity-astreference-secure.vm new file mode 100644 index 00000000000..107843dd1c2 --- /dev/null +++ b/dd-java-agent/instrumentation/velocity/src/test/resources/velocity-astreference-secure.vm @@ -0,0 +1,3 @@ + +I am vulnerable $esc.html($param) + diff --git a/dd-smoke-tests/springboot-velocity/build.gradle b/dd-smoke-tests/springboot-velocity/build.gradle new file mode 100644 index 00000000000..b2be96af3c3 --- /dev/null +++ b/dd-smoke-tests/springboot-velocity/build.gradle @@ -0,0 +1,25 @@ +plugins { + id 'java' + id 'org.springframework.boot' version '2.7.15' + id 'io.spring.dependency-management' version '1.0.15.RELEASE' + id 'java-test-fixtures' +} + +apply from: "$rootDir/gradle/java.gradle" +description = 'SpringBoot Velocity Smoke Tests.' + +dependencies { + implementation group: 'org.springframework.boot', name: 'spring-boot-starter-web', version: '1.5.18.RELEASE' + implementation group: 'org.apache.velocity', name: 'velocity', version: '1.5' + implementation(group: 'org.apache.velocity', name: 'velocity-tools', version: '1.3') { + exclude group: 'javax.servlet', module: 'servlet-api' + } + + testImplementation project(':dd-smoke-tests') + testImplementation(testFixtures(project(":dd-smoke-tests:iast-util"))) +} + +tasks.withType(Test).configureEach { + dependsOn "bootJar" + jvmArgs "-Ddatadog.smoketest.springboot.shadowJar.path=${tasks.bootJar.archiveFile.get()}" +} diff --git a/dd-smoke-tests/springboot-velocity/src/main/java/datadog/smoketest/springboot/SpringbootApplication.java b/dd-smoke-tests/springboot-velocity/src/main/java/datadog/smoketest/springboot/SpringbootApplication.java new file mode 100644 index 00000000000..2b4e4de452a --- /dev/null +++ b/dd-smoke-tests/springboot-velocity/src/main/java/datadog/smoketest/springboot/SpringbootApplication.java @@ -0,0 +1,11 @@ +package datadog.smoketest.springboot; + +import org.springframework.boot.SpringApplication; +import org.springframework.boot.autoconfigure.SpringBootApplication; + +@SpringBootApplication +public class SpringbootApplication { + public static void main(String[] args) { + SpringApplication.run(SpringbootApplication.class, args); + } +} diff --git a/dd-smoke-tests/springboot-velocity/src/main/java/datadog/smoketest/springboot/XssController.java b/dd-smoke-tests/springboot-velocity/src/main/java/datadog/smoketest/springboot/XssController.java new file mode 100644 index 00000000000..f1f9ca860bd --- /dev/null +++ b/dd-smoke-tests/springboot-velocity/src/main/java/datadog/smoketest/springboot/XssController.java @@ -0,0 +1,42 @@ +package datadog.smoketest.springboot; + +import javax.servlet.http.HttpServletResponse; +import org.apache.velocity.Template; +import org.apache.velocity.VelocityContext; +import org.apache.velocity.app.VelocityEngine; +import org.apache.velocity.runtime.RuntimeConstants; +import org.apache.velocity.tools.generic.EscapeTool; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestParam; + +@Controller +@RequestMapping("/xss") +public class XssController { + + private static final String DIRECTORY_TEMPLATES_TEST = "resources/main/templates/"; + private static final String DIRECTORY_TEMPLATES_RUN = + "dd-smoke-tests/springboot-velocity/src/main/resources/templates/"; + + @GetMapping("/velocity") + public void xssVelocity( + @RequestParam("velocity") String param, + @RequestParam("templateName") String templateName, + HttpServletResponse response) + throws Exception { + VelocityEngine velocity = new VelocityEngine(); + // To avoid the creation of a Velocity log file + velocity.setProperty( + RuntimeConstants.RUNTIME_LOG_LOGSYSTEM_CLASS, + "org.apache.velocity.runtime.log.NullLogChute"); + velocity.init(); + Template template = velocity.getTemplate(DIRECTORY_TEMPLATES_TEST + templateName); + + VelocityContext context = new VelocityContext(); + context.put("esc", new EscapeTool()); + context.put("param", param); + + template.merge(context, response.getWriter()); + } +} diff --git a/dd-smoke-tests/springboot-velocity/src/main/resources/templates/velocity-insecure.vm b/dd-smoke-tests/springboot-velocity/src/main/resources/templates/velocity-insecure.vm new file mode 100644 index 00000000000..016b8a865b6 --- /dev/null +++ b/dd-smoke-tests/springboot-velocity/src/main/resources/templates/velocity-insecure.vm @@ -0,0 +1,3 @@ + +Is this vulnerable? $param + diff --git a/dd-smoke-tests/springboot-velocity/src/main/resources/templates/velocity-secure.vm b/dd-smoke-tests/springboot-velocity/src/main/resources/templates/velocity-secure.vm new file mode 100644 index 00000000000..c18915a41b1 --- /dev/null +++ b/dd-smoke-tests/springboot-velocity/src/main/resources/templates/velocity-secure.vm @@ -0,0 +1,3 @@ + +Is this vulnerable? $esc.html($param) + diff --git a/dd-smoke-tests/springboot-velocity/src/test/groovy/datadog/smoketest/springboot/IastSpringBootVelocitySmokeTest.groovy b/dd-smoke-tests/springboot-velocity/src/test/groovy/datadog/smoketest/springboot/IastSpringBootVelocitySmokeTest.groovy new file mode 100644 index 00000000000..a16f3b4297a --- /dev/null +++ b/dd-smoke-tests/springboot-velocity/src/test/groovy/datadog/smoketest/springboot/IastSpringBootVelocitySmokeTest.groovy @@ -0,0 +1,65 @@ +package datadog.smoketest.springboot + +import datadog.smoketest.AbstractIastServerSmokeTest +import okhttp3.Request + +import static datadog.trace.api.config.IastConfig.IAST_DEBUG_ENABLED +import static datadog.trace.api.config.IastConfig.IAST_DETECTION_MODE +import static datadog.trace.api.config.IastConfig.IAST_ENABLED +import static datadog.trace.api.config.IastConfig.IAST_REDACTION_ENABLED + +class IastSpringBootVelocitySmokeTest extends AbstractIastServerSmokeTest { + + @Override + ProcessBuilder createProcessBuilder() { + String springBootShadowJar = System.getProperty('datadog.smoketest.springboot.shadowJar.path') + + List command = [] + command.add(javaPath()) + command.addAll(defaultJavaProperties) + command.addAll([ + withSystemProperty(IAST_ENABLED, true), + withSystemProperty(IAST_DETECTION_MODE, 'FULL'), + withSystemProperty(IAST_DEBUG_ENABLED, true), + withSystemProperty(IAST_REDACTION_ENABLED, false) + ]) + command.addAll((String[]) ['-jar', springBootShadowJar, "--server.port=${httpPort}"]) + ProcessBuilder processBuilder = new ProcessBuilder(command) + processBuilder.directory(new File(buildDirectory)) + // Spring will print all environment variables to the log, which may pollute it and affect log assertions. + processBuilder.environment().clear() + return processBuilder + } + + void 'xss is present'() { + setup: + final url = "http://localhost:${httpPort}/xss/velocity?velocity=${param}&templateName=${templateName}" + final request = new Request.Builder().url(url).get().build() + + when: + client.newCall(request).execute() + + then: + hasVulnerability { vul -> vul.type == 'XSS' && vul.location.path.contains(templateName) && vul.location.line == line } + + where: + param | templateName | line + 'name' | 'velocity-insecure.vm' | 2 + } + + void 'xss is not present'() { + setup: + final url = "http://localhost:${httpPort}/xss/velocity?velocity=${param}&templateName=${templateName}" + final request = new Request.Builder().url(url).get().build() + + when: + client.newCall(request).execute() + + then: + noVulnerability { vul -> vul.type == 'XSS' && vul.location.path.contains(templateName) && vul.location.line == line } + + where: + param | templateName | line + 'name' | 'velocity-secure.vm' | 2 + } +} diff --git a/settings.gradle b/settings.gradle index 279384630b7..66f943b22ad 100644 --- a/settings.gradle +++ b/settings.gradle @@ -147,6 +147,7 @@ include ':dd-smoke-tests:springboot-openliberty-23' include ':dd-smoke-tests:springboot-thymeleaf' include ':dd-smoke-tests:springboot-tomcat' include ':dd-smoke-tests:springboot-tomcat-jsp' +include ':dd-smoke-tests:springboot-velocity' include ':dd-smoke-tests:vertx-3.4' include ':dd-smoke-tests:vertx-3.9' include ':dd-smoke-tests:vertx-3.9-resteasy' @@ -462,6 +463,7 @@ include ':dd-java-agent:instrumentation:unbescape' include ':dd-java-agent:instrumentation:undertow' include ':dd-java-agent:instrumentation:undertow:undertow-2.0' include ':dd-java-agent:instrumentation:undertow:undertow-2.2' +include ':dd-java-agent:instrumentation:velocity' include ':dd-java-agent:instrumentation:vertx-mysql-client-3.9' include ':dd-java-agent:instrumentation:vertx-mysql-client-4.0' include ':dd-java-agent:instrumentation:vertx-mysql-client-4.4.2'