Skip XSS check for freemarker built-in escaping expressions in 2.3.24 instrumentation (#10865)

fix(iast): skip XSS check for freemarker built-in escaping expressions in 2.3.24 instrumentation

Fixed. Replaced instanceof BuiltIn with instanceof BuiltInForLegacyEscaping, the abstract base class that covers exactly ?html, ?xml, and ?xhtml in freemarker 2.3.24. Non-escaping built-ins like ?upper_case and ?js_string extend different base classes and are unaffected. Added
   parameterized unit tests to cover all cases.

Merge branch 'master' into alejandro.gonzalez/fix-iast-freemarker

Merge branch 'master' into alejandro.gonzalez/fix-iast-freemarker

Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

Author: jandro996

dd-java-agent/instrumentation/freemarker/freemarker-2.3.24/src/main/java/datadog/trace/instrumentation/freemarker24/DollarVariableDatadogAdvice.java

@@ -27,6 +27,9 @@ public static void onEnter(
         return;
       }
       String charSec = DollarVariable24Helper.fetchCharSec(self, environment);
+      if (charSec == null) {
+        return;
+      }
       final String templateName = environment.getMainTemplate().getName();
       final int line = DollarVariable24Helper.fetchBeginLine(self);
       xssModule.onXss(charSec, templateName, line);

dd-java-agent/instrumentation/freemarker/freemarker-2.3.24/src/main/java/freemarker/core/DollarVariable24Helper.java

@@ -12,6 +12,7 @@ private DollarVariable24Helper() {}
   private static final Logger log = LoggerFactory.getLogger(DollarVariable24Helper.class);
 
   private static final Field AUTO_ESCAPE = prepareAutoEscape();
+  private static final Field ESCAPED_EXPRESSION = prepareEscapedExpression();
 
   private static Field prepareAutoEscape() {
     Field autoEscape = null;
@@ -25,6 +26,17 @@ private static Field prepareAutoEscape() {
     return autoEscape;
   }
 
+  private static Field prepareEscapedExpression() {
+    try {
+      Field escapedExpression = DollarVariable.class.getDeclaredField("escapedExpression");
+      escapedExpression.setAccessible(true);
+      return escapedExpression;
+    } catch (Throwable e) {
+      log.debug("Failed to get DollarVariable escapedExpression", e);
+      return null;
+    }
+  }
+
   public static boolean fetchAutoEscape(Object dollarVariable) {
     if (AUTO_ESCAPE == null || !(dollarVariable instanceof DollarVariable)) {
       return true;
@@ -40,6 +52,15 @@ public static String fetchCharSec(Object object, Environment environment) {
     if (!(object instanceof DollarVariable)) {
       return null;
     }
+    if (ESCAPED_EXPRESSION != null) {
+      try {
+        if (ESCAPED_EXPRESSION.get(object) instanceof BuiltInForLegacyEscaping) {
+          return null;
+        }
+      } catch (IllegalAccessException e) {
+        throw new UndeclaredThrowableException(e);
+      }
+    }
     try {
       return (String) ((DollarVariable) object).calculateInterpolatedStringOrMarkup(environment);
     } catch (TemplateException e) {

dd-java-agent/instrumentation/freemarker/freemarker-2.3.24/src/test/groovy/datadog/trace/instrumentation/freemarker24/DollarVariableInstrumentationTest.groovy

@@ -15,24 +15,60 @@ class DollarVariableInstrumentationTest extends InstrumentationSpecification {
     injectSysConfig('dd.iast.enabled', 'true')
   }
 
-  void 'test freemarker process'() {
+  void 'plain interpolation reports xss'() {
     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 Template template = new Template("test", new StringReader('test ${name}'), cfg)
     final TemplateHashModel rootDataModel = new SimpleHash(cfg.getObjectWrapper())
-    rootDataModel.put(stringExpression, expression)
+    rootDataModel.put("name", "<script>alert(1)</script>")
 
     when:
     template.process(rootDataModel, Mock(FileWriter))
 
     then:
     1 * module.onXss(_, _, _)
+  }
+
+  void 'non-escaping built-in still reports xss'() {
+    given:
+    final module = Mock(XssModule)
+    InstrumentationBridge.registerIastModule(module)
+
+    final Configuration cfg = new Configuration()
+    final Template template = new Template("test", new StringReader("test \${name?$builtIn}"), cfg)
+    final TemplateHashModel rootDataModel = new SimpleHash(cfg.getObjectWrapper())
+    rootDataModel.put("name", "<script>alert(1)</script>")
+
+    when:
+    template.process(rootDataModel, Mock(FileWriter))
+
+    then:
+    1 * module.onXss(_, _, _)
+
+    where:
+    builtIn << ['upper_case', 'js_string', 'j_string']
+  }
+
+  void 'html/xml escaping built-ins do not report xss'() {
+    given:
+    final module = Mock(XssModule)
+    InstrumentationBridge.registerIastModule(module)
+
+    final Configuration cfg = new Configuration()
+    final Template template = new Template("test", new StringReader("test \${name?$builtIn}"), cfg)
+    final TemplateHashModel rootDataModel = new SimpleHash(cfg.getObjectWrapper())
+    rootDataModel.put("name", "<script>alert(1)</script>")
+
+    when:
+    template.process(rootDataModel, Mock(FileWriter))
+
+    then:
+    0 * module.onXss(_, _, _)
 
     where:
-    stringExpression | expression
-    "test"           | "test"
+    builtIn << ['html', 'xml', 'xhtml']
   }
 }