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

Add XSS support for Freemarker prior 2.3.24-incubating #7497

Merged
merged 21 commits into from
Sep 9, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
1fb0b74
Create smoke test for xss freemarker
Mariovido Aug 21, 2024
ff9e444
Add instrumentation for DollarVariable class
Mariovido Aug 23, 2024
7302294
Merge branch 'master' into mario.vidal/xss_freemarker
Mariovido Aug 23, 2024
f1877f8
Add instrumentation for freemarker
Mariovido Aug 26, 2024
c8574a3
Merge branch 'master' into mario.vidal/xss_freemarker
Mariovido Aug 26, 2024
9dee0c0
Modify instrumentation WIP smoke tests
Mariovido Aug 27, 2024
8195137
Add smoke test test + clean not needed stuff
Mariovido Aug 28, 2024
d31332a
Fix check_inst pipeline job
Mariovido Aug 28, 2024
d3a20ae
Fix smoke test to be compatible with different versions
Mariovido Aug 28, 2024
e735736
Fix DollarVariableInstrumentation to be compatible with different ver…
Mariovido Aug 28, 2024
8c74d0f
Refactor module of freemarker
Mariovido Aug 28, 2024
60fc532
Refactor freemarker instrumentation by version + make smoke tests
Mariovido Aug 29, 2024
6e7c279
Fix test in freemarker-2.3.9
Mariovido Aug 29, 2024
cab25f9
Merge branch 'master' into mario.vidal/xss_freemarker
Mariovido Aug 29, 2024
02c0dae
Merge branch 'master' into mario.vidal/xss_freemarker
Mariovido Sep 3, 2024
96cc296
Remove uncessary files
Mariovido Sep 3, 2024
44cc556
Update instrumentation for older versions
Mariovido Sep 3, 2024
966be3e
Refactor advice + style changes in helper
Mariovido Sep 9, 2024
cda2dd6
Change latestDep configuration
Mariovido Sep 9, 2024
b645688
Merge branch 'master' into mario.vidal/xss_freemarker
Mariovido Sep 9, 2024
e955dad
Add testImplementation of previous version
Mariovido Sep 9, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions dd-java-agent/instrumentation/freemarker-2.3.9/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
muzzle {
fail {
amarziali marked this conversation as resolved.
Show resolved Hide resolved
name = 'freemarker-2.3.9'
group = 'org.freemarker'
module = 'freemarker'
versions = '[2.3.24-incubating,]'
amarziali marked this conversation as resolved.
Show resolved Hide resolved
assertInverse = true
}
}

apply from: "$rootDir/gradle/java.gradle"

addTestSuiteForDir('latestDepTest', 'test')

dependencies {
compileOnly group: 'org.freemarker', name: 'freemarker', version: '2.3.9'

testImplementation group: 'org.freemarker', name: 'freemarker', version: '2.3.9'

testRuntimeOnly project(':dd-java-agent:instrumentation:iast-instrumenter')

latestDepTestImplementation group: 'org.freemarker', name: 'freemarker', version: '2.3.23'
Mariovido marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package datadog.trace.instrumentation.freemarker9;

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 freemarker.core.DollarVariable9Helper;
import freemarker.core.Environment;
import net.bytebuddy.asm.Advice;

public final class DollarVariableDatadogAdvice {
Mariovido marked this conversation as resolved.
Show resolved Hide resolved

public static class DollarVariableAdvice {
Mariovido marked this conversation as resolved.
Show resolved Hide resolved

@Advice.OnMethodEnter(suppress = Throwable.class)
@Sink(VulnerabilityTypes.XSS)
public static void onEnter(
@Advice.Argument(0) final Environment environment, @Advice.This final Object self) {
if (environment == null || self == null) {
return;
}
final XssModule xssModule = InstrumentationBridge.XSS;
if (xssModule == null) {
return;
}
String charSec = DollarVariable9Helper.fetchCharSec(self, environment);
final String templateName = environment.getTemplate().getName();
final int line = DollarVariable9Helper.fetchBeginLine(self);
xssModule.onXss(charSec, templateName, line);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package datadog.trace.instrumentation.freemarker9;

import static datadog.trace.agent.tooling.bytebuddy.matcher.ClassLoaderMatchers.hasClassNamed;
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.isMethod;
import static net.bytebuddy.matcher.ElementMatchers.not;
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 net.bytebuddy.matcher.ElementMatcher;

@AutoService(InstrumenterModule.class)
public class DollarVariableInstrumentation extends InstrumenterModule.Iast
implements Instrumenter.ForSingleType {
static final String FREEMARKER_CORE = "freemarker.core";

public DollarVariableInstrumentation() {
super("freemarker");
}

@Override
public String muzzleDirective() {
return "freemarker-2.3.9";
}

static final ElementMatcher.Junction<ClassLoader> NOT_VERSION_PRIOR_2_3_24 =
not(hasClassNamed("freemarker.cache.ByteArrayTemplateLoader"));

@Override
public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
return NOT_VERSION_PRIOR_2_3_24;
}

@Override
public String instrumentedType() {
return FREEMARKER_CORE + ".DollarVariable";
}

@Override
public String[] helperClassNames() {
return new String[] {FREEMARKER_CORE + ".DollarVariable9Helper"};
}

@Override
public void methodAdvice(MethodTransformer transformer) {
transformer.applyAdvice(
named("accept")
.and(isMethod())
.and(takesArgument(0, named(FREEMARKER_CORE + ".Environment"))),
packageName + ".DollarVariableDatadogAdvice$DollarVariableAdvice");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
package freemarker.core;

import freemarker.template.TemplateModelException;
import java.lang.reflect.Field;
import java.lang.reflect.UndeclaredThrowableException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public final class DollarVariable9Helper {
Mariovido marked this conversation as resolved.
Show resolved Hide resolved
private DollarVariable9Helper() {}

private static final Logger log = LoggerFactory.getLogger(DollarVariable9Helper.class);

private static final Field ESCAPED_EXPRESSION = prepareEscapedExpression();

private static Field prepareEscapedExpression() {
Field autoEscape = null;
try {
autoEscape = DollarVariable.class.getDeclaredField("escapedExpression");
autoEscape.setAccessible(true);
} catch (Throwable e) {
log.debug("Failed to get DollarVariable escapedExpression", e);
return null;
}
return autoEscape;
}

public static Expression fetchEscapeExpression(Object object) {
if (ESCAPED_EXPRESSION == null || !(object instanceof DollarVariable)) {
return null;
}
try {
return (Expression) ESCAPED_EXPRESSION.get(object);
} catch (IllegalAccessException e) {
throw new UndeclaredThrowableException(e);
}
}

public static String fetchCharSec(Object object, Environment environment) {
if (!(object instanceof DollarVariable)) {
return null;
}
final Expression expression = DollarVariable9Helper.fetchEscapeExpression(object);
if (expression instanceof BuiltIn) {
return null;
}
try {
return environment.getDataModel().get(expression.toString()).toString();
} catch (TemplateModelException e) {
throw new UndeclaredThrowableException(e);
}
}

public static Integer fetchBeginLine(Object object) {
if (!(object instanceof DollarVariable)) {
return null;
}
return ((DollarVariable) object).beginLine;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package datadog.trace.instrumentation.freemarker9

import datadog.trace.agent.test.AgentTestRunner
import datadog.trace.api.iast.InstrumentationBridge
import datadog.trace.api.iast.sink.XssModule
import freemarker.template.Configuration
import freemarker.template.SimpleHash
import freemarker.template.Template
import freemarker.template.TemplateHashModel

class DollarVariableInstrumentationTest extends AgentTestRunner {

@Override
protected void configurePreAgent() {
injectSysConfig('dd.iast.enabled', 'true')
}

void 'test freemarker process'() {
given:
final module = Mock(XssModule)
InstrumentationBridge.registerIastModule(module)

final Configuration cfg = new Configuration()
final Template template = new Template("test", new StringReader("test \${$stringExpression}"), cfg)
final TemplateHashModel rootDataModel = new SimpleHash(cfg.getObjectWrapper())
rootDataModel.put(stringExpression, expression)

when:
template.process(rootDataModel, Mock(FileWriter))

then:
1 * module.onXss(_, _, _)

where:
stringExpression | expression
"test" | "test"
}
}
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ include ':dd-java-agent:instrumentation:elasticsearch:transport-7.3'
include ':dd-java-agent:instrumentation:enable-wallclock-profiling'
include ':dd-java-agent:instrumentation:exception-profiling'
include ':dd-java-agent:instrumentation:finatra-2.9'
include ':dd-java-agent:instrumentation:freemarker-2.3.9'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when multiple version of the same instrumentation exists we tent to group them under a top folder (i.e. freemarker) (see conventions)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is being done in the following PR #7579. Once merged I'll do the change in this one

include ':dd-java-agent:instrumentation:freemarker-2.3.24'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have added the freemarker-2.3.9 module as a test implementation of this one in order to test that only one instrumentation applies as expected. Even if muzzle checks that, adding this is a good practice

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already added :)

include ':dd-java-agent:instrumentation:glassfish'
include ':dd-java-agent:instrumentation:google-http-client'
Expand Down