Skip to content

Commit

Permalink
Add XSS support for Velocity (#7546)
Browse files Browse the repository at this point in the history
  • Loading branch information
Mariovido authored Sep 16, 2024
1 parent e7631e6 commit 3f5051e
Show file tree
Hide file tree
Showing 16 changed files with 433 additions and 0 deletions.
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 16 additions & 0 deletions dd-java-agent/instrumentation/velocity/build.gradle
Original file line number Diff line number Diff line change
@@ -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: '+'
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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 << ["<script>alert(1)</script>", "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 << ["<script>alert(1)</script>", "name"]
}
}
Original file line number Diff line number Diff line change
@@ -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 | '&Oslash;-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 | '&#216;-This is a quote'
'sql' | ['Ø-This is a quote'] | SQL_INJECTION_MARK | 'Ø-This is a quote'
}
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<html>
I am vulnerable $param
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<html>
I am vulnerable $esc.html($param)
</html>
25 changes: 25 additions & 0 deletions dd-smoke-tests/springboot-velocity/build.gradle
Original file line number Diff line number Diff line change
@@ -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()}"
}
Original file line number Diff line number Diff line change
@@ -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);
}
}
Original file line number Diff line number Diff line change
@@ -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());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<html>
Is this vulnerable? $param
</html>
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<html>
Is this vulnerable? $esc.html($param)
</html>
Loading

0 comments on commit 3f5051e

Please sign in to comment.