From 0f9c22c4f46ee082102af5552b344274e6e96168 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 15 Nov 2024 14:36:17 +0100 Subject: [PATCH 01/38] formatter and config POC --- .../datadog/trace/api/config/IastConfig.java | 5 +- .../main/java/datadog/trace/api/Config.java | 14 +++ .../trace/api/iast/VulnerabilityMarks.java | 40 +++++++ .../iast/securitycontrol/SecurityControl.java | 73 +++++++++++ .../SecurityControlFormatter.java | 113 ++++++++++++++++++ .../securitycontrol/SecurityControlType.java | 5 + .../SecurityControlFormatterTest.groovy | 44 +++++++ 7 files changed, 292 insertions(+), 2 deletions(-) create mode 100644 internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java create mode 100644 internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java create mode 100644 internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlType.java create mode 100644 internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/IastConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/IastConfig.java index 73f0b93a4c9..411a49ee91f 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/IastConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/IastConfig.java @@ -17,7 +17,6 @@ public final class IastConfig { public static final String IAST_REDACTION_NAME_PATTERN = "iast.redaction.name.pattern"; public static final String IAST_REDACTION_VALUE_PATTERN = "iast.redaction.value.pattern"; public static final String IAST_STACKTRACE_LEAK_SUPPRESS = "iast.stacktrace-leak.suppress"; - public static final String IAST_HARDCODED_SECRET_ENABLED = "iast.hardcoded-secret.enabled"; public static final String IAST_MAX_RANGE_COUNT = "iast.max-range-count"; public static final String IAST_TRUNCATION_MAX_VALUE_LENGTH = "iast.truncation.max.value.length"; @@ -27,8 +26,10 @@ public final class IastConfig { public static final String IAST_SOURCE_MAPPING_MAX_SIZE = "iast.source-mapping.max-size"; public static final String IAST_EXPERIMENTAL_PROPAGATION_ENABLED = "iast.experimental.propagation.enabled"; - public static final String IAST_STACK_TRACE_ENABLED = "iast.stacktrace.enabled"; + public static final String IAST_SECURITY_CONTROLS_ENABLED = "iast.security-controls.enabled"; + public static final String IAST_SECURITY_CONTROLS_CONFIGURATION= "iast.security-controls.configuration"; + private IastConfig() {} } diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index f27c6cf0272..aecdb38b73d 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -307,6 +307,8 @@ public static String getHostName() { private final int iastSourceMappingMaxSize; private final boolean iastStackTraceEnabled; private final boolean iastExperimentalPropagationEnabled; + private final boolean iastSecurityControlsEnabled; + private final String iastSecurityControlsConfiguration; private final boolean ciVisibilityTraceSanitationEnabled; private final boolean ciVisibilityAgentlessEnabled; @@ -1332,6 +1334,10 @@ PROFILING_DATADOG_PROFILER_ENABLED, isDatadogProfilerSafeInCurrentEnvironment()) configProvider.getBoolean(IAST_STACK_TRACE_ENABLED, DEFAULT_IAST_STACK_TRACE_ENABLED); iastExperimentalPropagationEnabled = configProvider.getBoolean(IAST_EXPERIMENTAL_PROPAGATION_ENABLED, false); + iastSecurityControlsEnabled = + configProvider.getBoolean(IAST_SECURITY_CONTROLS_ENABLED, false); + iastSecurityControlsConfiguration = + configProvider.getString(IAST_SECURITY_CONTROLS_CONFIGURATION, null); ciVisibilityTraceSanitationEnabled = configProvider.getBoolean(CIVISIBILITY_TRACE_SANITATION_ENABLED, true); @@ -2633,6 +2639,14 @@ public boolean isIastExperimentalPropagationEnabled() { return iastExperimentalPropagationEnabled; } + public boolean isIastSecurityControlsEnabled() { + return iastSecurityControlsEnabled; + } + + public String getIastSecurityControlsConfiguration() { + return iastSecurityControlsConfiguration; + } + public boolean isCiVisibilityEnabled() { return instrumenterConfig.isCiVisibilityEnabled(); } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityMarks.java b/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityMarks.java index 59b493e3f59..643f9cabbec 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityMarks.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityMarks.java @@ -20,4 +20,44 @@ private VulnerabilityMarks() {} public static final int HEADER_INJECTION_MARK = 1 << 9; public static final int REFLECTION_INJECTION_MARK = 1 << 10; public static final int UNTRUSTED_DESERIALIZATION_MARK = 1 << 11; + + public static final int CUSTOM_SECURITY_CONTROL_MARK = 1 << 13; + + + public static int markForAll() { + return XSS_MARK | XPATH_INJECTION_MARK | SQL_INJECTION_MARK | COMMAND_INJECTION_MARK | PATH_TRAVERSAL_MARK | LDAP_INJECTION_MARK | SSRF_MARK | UNVALIDATED_REDIRECT_MARK | TRUST_BOUNDARY_VIOLATION_MARK | HEADER_INJECTION_MARK | REFLECTION_INJECTION_MARK | UNTRUSTED_DESERIALIZATION_MARK | CUSTOM_SECURITY_CONTROL_MARK; + } + + public static int getMarkFromVulnerabitityType(final String vulnerabilityTypeString){ + switch (vulnerabilityTypeString) { + case "XSS": + return XSS_MARK; + case "XPATH_INJECTION": + return XPATH_INJECTION_MARK; + case "SQL_INJECTION": + return SQL_INJECTION_MARK; + case "COMMAND_INJECTION": + return COMMAND_INJECTION_MARK; + case "PATH_TRAVERSAL": + return PATH_TRAVERSAL_MARK; + case "LDAP_INJECTION": + return LDAP_INJECTION_MARK; + case "SSRF": + return SSRF_MARK; + case "UNVALIDATED_REDIRECT": + return UNVALIDATED_REDIRECT_MARK; + case "TRUST_BOUNDARY_VIOLATION": + return TRUST_BOUNDARY_VIOLATION_MARK; + case "HEADER_INJECTION": + return HEADER_INJECTION_MARK; + case "REFLECTION_INJECTION": + return REFLECTION_INJECTION_MARK; + case "UNTRUSTED_DESERIALIZATION": + return UNTRUSTED_DESERIALIZATION_MARK; + case "CUSTOM_SECURITY_CONTROL": + return CUSTOM_SECURITY_CONTROL_MARK; + default: + return NOT_MARKED; + } + } } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java new file mode 100644 index 00000000000..9b66acb25af --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java @@ -0,0 +1,73 @@ +package datadog.trace.api.iast.securitycontrol; + +import datadog.trace.api.iast.VulnerabilityMarks; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; + +public class SecurityControl { + + @Nonnull + private SecurityControlType type; + + private int marks; + + @Nonnull + private String className ; + + @Nonnull + private String method; + + @Nullable + private String[] parameterTypes; + + @Nullable + private int[] parametersToMark; + + + public SecurityControl( + @Nonnull SecurityControlType type, + int marks, + @Nonnull String className, + @Nonnull String method, + @Nullable String[] parameterTypes, + @Nullable int[] parametersToMark) { + this.type = type; + this.marks = marks; + this.className = className; + this.method = method; + this.parameterTypes = parameterTypes; + this.parametersToMark = parametersToMark; + } + + @Nonnull + public SecurityControlType getType() { + return type; + } + + @Nonnull + public int getMarks() { + return marks; + } + + @Nonnull + public String getClassName() { + return className; + } + + @Nonnull + public String getMethod() { + return method; + } + + @Nullable + public String[] getParameterTypes() { + return parameterTypes; + } + + @Nullable + public int[] getParametersToMark() { + return parametersToMark; + } + +} diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java new file mode 100644 index 00000000000..6ec12cf605e --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -0,0 +1,113 @@ +package datadog.trace.api.iast.securitycontrol; + +import datadog.trace.api.iast.VulnerabilityMarks; +import org.slf4j.Logger; + +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.List; + +import static datadog.trace.api.iast.VulnerabilityMarks.NOT_MARKED; + +public class SecurityControlFormatter { + + private static final Logger log = org.slf4j.LoggerFactory.getLogger(SecurityControlFormatter.class); + + private static final String SECURITY_CONTROL_DELIMITER = ";"; + private static final String SECURITY_CONTROL_FIELD_DELIMITER = ":"; + private static final String SECURITY_CONTROL_ELEMENT_DELIMITER = "," ; + + private static final String ALL = "*"; + + + @Nullable + public static List format (final @Nonnull String securityControlString) { + + if (securityControlString.isEmpty()) { + log.warn("Security control configuration is empty"); + return null; + } + + String config = securityControlString.replaceAll("\\s+", ""); + + String[] list = config.split(SECURITY_CONTROL_DELIMITER); + + List securityControls = new ArrayList<>(list.length); + + for (String s : list) { + SecurityControl securityControl = getSecurityControl(s); + if (securityControl != null) { + securityControls.add(securityControl); + } + } + return securityControls.isEmpty() ? null : securityControls; + } + + + private static SecurityControl getSecurityControl(@Nonnull final String config) { + if (config.isEmpty()) { + log.warn("Security control configuration is empty"); + return null; + } + String[] split = config.split(SECURITY_CONTROL_FIELD_DELIMITER); + if (split.length < 4) { + log.warn("Security control configuration is invalid: {}", config); + return null; + } + SecurityControlType type = SecurityControlType.valueOf(split[0]); + if (type == null) { + log.warn("Security control type is invalid: {}", split[0]); + return null; + } + int marks = getMarks(split[1]); + String className = split[2]; + String method = split[3]; + + String[] parameterTypes = null; + int[] parametersToMark = null; + + if (split.length > 4) { + String[] elements = split[4].split(SECURITY_CONTROL_ELEMENT_DELIMITER); + if (elements.length > 0) { + if (isNumeric(elements[0])) { + parametersToMark = getParametersToMark(elements); + }else { + parameterTypes = elements; + } + } + } + if (split.length > 5) { + String[] elements = split[5].split(SECURITY_CONTROL_ELEMENT_DELIMITER); + parametersToMark = getParametersToMark(elements); + } + + return new SecurityControl(type, marks, className, method, parameterTypes, parametersToMark); + } + + private static int getMarks(String s) { + if (s.equals(ALL)) { + return VulnerabilityMarks.markForAll(); + } + String[] elements = s.split(SECURITY_CONTROL_ELEMENT_DELIMITER); + int marks = NOT_MARKED; + for (String element : elements) { + marks |= VulnerabilityMarks.getMarkFromVulnerabitityType(element); + } + return marks; + } + + private static int[] getParametersToMark(String[] elements) { + return Arrays.stream(elements) + .mapToInt(Integer::parseInt) + .toArray(); + } + + private static boolean isNumeric(String str) { + return str.matches("[0-9]+"); + } + + + +} diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlType.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlType.java new file mode 100644 index 00000000000..cf96684825c --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlType.java @@ -0,0 +1,5 @@ +package datadog.trace.api.iast.securitycontrol; + +public enum SecurityControlType { + INPUT_VALIDATOR, SANITIZER +} diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy new file mode 100644 index 00000000000..9167ca9c6c9 --- /dev/null +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy @@ -0,0 +1,44 @@ +package datadog.trace.api.iast.securitycontrol + +import datadog.trace.api.iast.VulnerabilityMarks +import datadog.trace.test.util.DDSpecification + +class SecurityControlFormatterTest extends DDSpecification{ + + void 'test happy path Input validator'() { + setup: + final formatter = new SecurityControlFormatter() + final config = 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate' + final result = formatter.format(config) + + expect: + result.size() == 1 + def securityControl = result.get(0) + securityControl.getType() == SecurityControlType.INPUT_VALIDATOR + securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getClassName() == "bar.foo.CustomInputValidator" + securityControl.getMethod() == "validate" + securityControl.getParameterTypes() == null + securityControl.getParametersToMark() == null + + } + + void 'test happy path sanitizer'() { + setup: + final formatter = new SecurityControlFormatter() + final config = 'SANITIZER:COMMAND_INJECTION:bar.foo.CustomSanitizer:sanitize' + final result = formatter.format(config) + + expect: + result.size() == 1 + def securityControl = result.get(0) + securityControl.getType() == SecurityControlType.SANITIZER + securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getClassName() == "bar.foo.CustomSanitizer" + securityControl.getMethod() == "sanitize" + securityControl.getParameterTypes() == null + securityControl.getParametersToMark() == null + } + + +} From a44168e08658b32a28d8d4bf284c5a4230cec90b Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 19 Nov 2024 08:09:24 +0100 Subject: [PATCH 02/38] more changes --- .../datadog/trace/api/config/IastConfig.java | 4 +- .../main/java/datadog/trace/api/Config.java | 3 +- .../trace/api/iast/VulnerabilityMarks.java | 17 ++++++-- .../iast/securitycontrol/SecurityControl.java | 19 +++------ .../SecurityControlFormatter.java | 29 +++++-------- .../securitycontrol/SecurityControlType.java | 3 +- .../SecurityControlFormatterTest.groovy | 42 ++++++++++++++++++- 7 files changed, 76 insertions(+), 41 deletions(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/config/IastConfig.java b/dd-trace-api/src/main/java/datadog/trace/api/config/IastConfig.java index 411a49ee91f..42dd7e91c4d 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/config/IastConfig.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/config/IastConfig.java @@ -28,8 +28,8 @@ public final class IastConfig { "iast.experimental.propagation.enabled"; public static final String IAST_STACK_TRACE_ENABLED = "iast.stacktrace.enabled"; public static final String IAST_SECURITY_CONTROLS_ENABLED = "iast.security-controls.enabled"; - public static final String IAST_SECURITY_CONTROLS_CONFIGURATION= "iast.security-controls.configuration"; - + public static final String IAST_SECURITY_CONTROLS_CONFIGURATION = + "iast.security-controls.configuration"; private IastConfig() {} } diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index aecdb38b73d..958bb6de001 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -1334,8 +1334,7 @@ PROFILING_DATADOG_PROFILER_ENABLED, isDatadogProfilerSafeInCurrentEnvironment()) configProvider.getBoolean(IAST_STACK_TRACE_ENABLED, DEFAULT_IAST_STACK_TRACE_ENABLED); iastExperimentalPropagationEnabled = configProvider.getBoolean(IAST_EXPERIMENTAL_PROPAGATION_ENABLED, false); - iastSecurityControlsEnabled = - configProvider.getBoolean(IAST_SECURITY_CONTROLS_ENABLED, false); + iastSecurityControlsEnabled = configProvider.getBoolean(IAST_SECURITY_CONTROLS_ENABLED, false); iastSecurityControlsConfiguration = configProvider.getString(IAST_SECURITY_CONTROLS_CONFIGURATION, null); diff --git a/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityMarks.java b/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityMarks.java index 643f9cabbec..adcc65222ed 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityMarks.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityMarks.java @@ -23,12 +23,23 @@ private VulnerabilityMarks() {} public static final int CUSTOM_SECURITY_CONTROL_MARK = 1 << 13; - public static int markForAll() { - return XSS_MARK | XPATH_INJECTION_MARK | SQL_INJECTION_MARK | COMMAND_INJECTION_MARK | PATH_TRAVERSAL_MARK | LDAP_INJECTION_MARK | SSRF_MARK | UNVALIDATED_REDIRECT_MARK | TRUST_BOUNDARY_VIOLATION_MARK | HEADER_INJECTION_MARK | REFLECTION_INJECTION_MARK | UNTRUSTED_DESERIALIZATION_MARK | CUSTOM_SECURITY_CONTROL_MARK; + return XSS_MARK + | XPATH_INJECTION_MARK + | SQL_INJECTION_MARK + | COMMAND_INJECTION_MARK + | PATH_TRAVERSAL_MARK + | LDAP_INJECTION_MARK + | SSRF_MARK + | UNVALIDATED_REDIRECT_MARK + | TRUST_BOUNDARY_VIOLATION_MARK + | HEADER_INJECTION_MARK + | REFLECTION_INJECTION_MARK + | UNTRUSTED_DESERIALIZATION_MARK + | CUSTOM_SECURITY_CONTROL_MARK; } - public static int getMarkFromVulnerabitityType(final String vulnerabilityTypeString){ + public static int getMarkFromVulnerabitityType(final String vulnerabilityTypeString) { switch (vulnerabilityTypeString) { case "XSS": return XSS_MARK; diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java index 9b66acb25af..cfad238b39e 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java @@ -1,29 +1,21 @@ package datadog.trace.api.iast.securitycontrol; -import datadog.trace.api.iast.VulnerabilityMarks; - import javax.annotation.Nonnull; import javax.annotation.Nullable; public class SecurityControl { - @Nonnull - private SecurityControlType type; + @Nonnull private SecurityControlType type; private int marks; - @Nonnull - private String className ; - - @Nonnull - private String method; + @Nonnull private String className; - @Nullable - private String[] parameterTypes; + @Nonnull private String method; - @Nullable - private int[] parametersToMark; + @Nullable private String[] parameterTypes; + @Nullable private int[] parametersToMark; public SecurityControl( @Nonnull SecurityControlType type, @@ -69,5 +61,4 @@ public String[] getParameterTypes() { public int[] getParametersToMark() { return parametersToMark; } - } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index 6ec12cf605e..43b28e60012 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -1,29 +1,28 @@ package datadog.trace.api.iast.securitycontrol; -import datadog.trace.api.iast.VulnerabilityMarks; -import org.slf4j.Logger; +import static datadog.trace.api.iast.VulnerabilityMarks.NOT_MARKED; -import javax.annotation.Nonnull; -import javax.annotation.Nullable; +import datadog.trace.api.iast.VulnerabilityMarks; import java.util.ArrayList; import java.util.Arrays; import java.util.List; - -import static datadog.trace.api.iast.VulnerabilityMarks.NOT_MARKED; +import javax.annotation.Nonnull; +import javax.annotation.Nullable; +import org.slf4j.Logger; public class SecurityControlFormatter { - private static final Logger log = org.slf4j.LoggerFactory.getLogger(SecurityControlFormatter.class); + private static final Logger log = + org.slf4j.LoggerFactory.getLogger(SecurityControlFormatter.class); private static final String SECURITY_CONTROL_DELIMITER = ";"; private static final String SECURITY_CONTROL_FIELD_DELIMITER = ":"; - private static final String SECURITY_CONTROL_ELEMENT_DELIMITER = "," ; + private static final String SECURITY_CONTROL_ELEMENT_DELIMITER = ","; private static final String ALL = "*"; - @Nullable - public static List format (final @Nonnull String securityControlString) { + public static List format(final @Nonnull String securityControlString) { if (securityControlString.isEmpty()) { log.warn("Security control configuration is empty"); @@ -45,7 +44,6 @@ public static List format (final @Nonnull String securityContro return securityControls.isEmpty() ? null : securityControls; } - private static SecurityControl getSecurityControl(@Nonnull final String config) { if (config.isEmpty()) { log.warn("Security control configuration is empty"); @@ -73,7 +71,7 @@ private static SecurityControl getSecurityControl(@Nonnull final String config) if (elements.length > 0) { if (isNumeric(elements[0])) { parametersToMark = getParametersToMark(elements); - }else { + } else { parameterTypes = elements; } } @@ -99,15 +97,10 @@ private static int getMarks(String s) { } private static int[] getParametersToMark(String[] elements) { - return Arrays.stream(elements) - .mapToInt(Integer::parseInt) - .toArray(); + return Arrays.stream(elements).mapToInt(Integer::parseInt).toArray(); } private static boolean isNumeric(String str) { return str.matches("[0-9]+"); } - - - } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlType.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlType.java index cf96684825c..f1b5b6a53da 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlType.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlType.java @@ -1,5 +1,6 @@ package datadog.trace.api.iast.securitycontrol; public enum SecurityControlType { - INPUT_VALIDATOR, SANITIZER + INPUT_VALIDATOR, + SANITIZER } diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy index 9167ca9c6c9..8640b228e42 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy @@ -20,7 +20,6 @@ class SecurityControlFormatterTest extends DDSpecification{ securityControl.getMethod() == "validate" securityControl.getParameterTypes() == null securityControl.getParametersToMark() == null - } void 'test happy path sanitizer'() { @@ -40,5 +39,46 @@ class SecurityControlFormatterTest extends DDSpecification{ securityControl.getParametersToMark() == null } + void 'test multiple security controls'(){ + setup: + final formatter = new SecurityControlFormatter() + final config = 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate;SANITIZER:COMMAND_INJECTION:bar.foo.CustomSanitizer:sanitize' + final result = formatter.format(config) + + expect: + result.size() == 2 + def inputValidator = result.get(0) + inputValidator.getType() == SecurityControlType.INPUT_VALIDATOR + inputValidator.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + inputValidator.getClassName() == "bar.foo.CustomInputValidator" + inputValidator.getMethod() == "validate" + inputValidator.getParameterTypes() == null + inputValidator.getParametersToMark() == null + + def sanitizer = result.get(1) + sanitizer.getType() == SecurityControlType.SANITIZER + sanitizer.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + sanitizer.getClassName() == "bar.foo.CustomSanitizer" + sanitizer.getMethod() == "sanitize" + sanitizer.getParameterTypes() == null + sanitizer.getParametersToMark() == null + } + + void 'test multiple secure marks'() { + setup: + final formatter = new SecurityControlFormatter() + final config = 'INPUT_VALIDATOR:COMMAND_INJECTION,SQL_INJECTION:bar.foo.CustomInputValidator:validate' + final result = formatter.format(config) + + expect: + result.size() == 1 + def securityControl = result.get(0) + securityControl.getType() == SecurityControlType.INPUT_VALIDATOR + securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.SQL_INJECTION_MARK) + securityControl.getClassName() == "bar.foo.CustomInputValidator" + securityControl.getMethod() == "validate" + securityControl.getParameterTypes() == null + securityControl.getParametersToMark() == null + } } From 3b19a1684f08cf7a0f3b0e3493880d251e4820fb Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 20 Nov 2024 11:31:10 +0100 Subject: [PATCH 03/38] Not working maybe need to rollback --- .../java/datadog/trace/bootstrap/Agent.java | 28 +++++++++++++ dd-java-agent/agent-iast/build.gradle | 1 + .../IastSecurityControlTransformer.java | 25 ++++++++++++ .../IastSecurityControlsInstrumenter.java | 28 +++++++++++++ .../SecurityControlMethodAdapter.java | 12 ++++++ .../SecurityControlMethodClassVisitor.java | 39 +++++++++++++++++++ 6 files changed, 133 insertions(+) create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlsInstrumenter.java create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 9baf349f8b9..25a4557dd8e 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -535,11 +535,14 @@ public void execute() { // start debugger before remote config to subscribe to it before starting to poll maybeStartDebugger(instrumentation, scoClass, sco); maybeStartRemoteConfig(scoClass, sco); + maybeStartIastSecurityControls(instrumentation, scoClass, sco); if (telemetryEnabled) { startTelemetry(instrumentation, scoClass, sco); } } + + } protected static class StartProfilingAgentCallback extends ClassLoadCallBack { @@ -607,6 +610,31 @@ private static void maybeStartRemoteConfig(Class scoClass, Object sco) { StaticEventLogger.end("Remote Config"); } + private void maybeStartIastSecurityControls(Instrumentation instrumentation, Class scoClass, Object sco) { + if(!Config.get().isIastSecurityControlsEnabled()) { + return; + } + if (!iastEnabled || iastFullyDisabled) { + log.warn("Error starting IAST Security Controls, IAST should be enabled"); + return; + } + if(Config.get().getIastSecurityControlsConfiguration() == null){ + log.warn("Error starting IAST Security Controls, IAST Security Controls configuration is missing"); + return; + } + StaticEventLogger.begin("IAST Security Controls"); + try { + final Class appSecSysClass = AGENT_CLASSLOADER.loadClass("com.datadog.iast.IastSecurityControlsInstrumenter"); + final Method iastInstallerMethod = + appSecSysClass.getMethod("start"); + iastInstallerMethod.invoke(instrumentation); + } catch (Exception e) { + log.error("Error starting IAST Security Controls", e); + } + + StaticEventLogger.end("IAST Security Controls"); + } + private static synchronized void startDatadogAgent( final InitializationTelemetry initTelemetry, final Instrumentation inst) { if (null != inst) { diff --git a/dd-java-agent/agent-iast/build.gradle b/dd-java-agent/agent-iast/build.gradle index 659a568d947..942f8d5b94a 100644 --- a/dd-java-agent/agent-iast/build.gradle +++ b/dd-java-agent/agent-iast/build.gradle @@ -50,6 +50,7 @@ dependencies { implementation project(':internal-api') implementation project(':internal-api:internal-api-9') implementation libs.moshi + implementation("org.ow2.asm:asm:9.7") testFixturesApi project(':dd-java-agent:testing') testFixturesApi project(':utils:test-utils') diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java new file mode 100644 index 00000000000..96f38e5b37c --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java @@ -0,0 +1,25 @@ +package com.datadog.iast.securitycontrol; + +import datadog.trace.api.iast.securitycontrol.SecurityControl; + +import java.lang.instrument.ClassFileTransformer; +import org.objectweb.asm.ClassReader; +import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.ClassWriter; + +public class IastSecurityControlTransformer implements ClassFileTransformer { + + private final SecurityControl securityControl; + + public IastSecurityControlTransformer(SecurityControl securityControl) { + this.securityControl = securityControl; + } + + @Override + public byte[] transform(ClassLoader loader, String className, Class classBeingRedefined, java.security.ProtectionDomain protectionDomain, byte[] classfileBuffer) { + ClassReader cr = new ClassReader(classfileBuffer); + ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES); + ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, className); + cr.accept(cv, 0); + } +} diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlsInstrumenter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlsInstrumenter.java new file mode 100644 index 00000000000..360bc1ccb6d --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlsInstrumenter.java @@ -0,0 +1,28 @@ +package com.datadog.iast.securitycontrol; + +import datadog.trace.api.Config; +import datadog.trace.api.iast.securitycontrol.SecurityControl; +import datadog.trace.api.iast.securitycontrol.SecurityControlFormatter; +import org.slf4j.Logger; + +import java.lang.instrument.Instrumentation; +import java.util.List; + +public class IastSecurityControlsInstrumenter { + + private static final Logger log = org.slf4j.LoggerFactory.getLogger(IastSecurityControlsInstrumenter.class); + + public static void start(Instrumentation inst) { + + List securityControls = SecurityControlFormatter.format(Config.get().getIastSecurityControlsConfiguration()); + if (securityControls == null) { + log.warn("No security controls to apply"); + return; + } + for (SecurityControl securityControl : securityControls) { + inst.addTransformer(new IastSecurityControlTransformer(securityControl), true); + } + + } + +} diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java new file mode 100644 index 00000000000..fc953ef62f8 --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java @@ -0,0 +1,12 @@ +package com.datadog.iast.securitycontrol; + +import datadog.trace.api.iast.securitycontrol.SecurityControl; +import org.objectweb.asm.MethodVisitor; + +public class SecurityControlMethodAdapter extends MethodVisitor { + + + protected SecurityControlMethodAdapter(int api) { + super(api); + } +} diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java new file mode 100644 index 00000000000..ee0aecdecae --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java @@ -0,0 +1,39 @@ +package com.datadog.iast.securitycontrol; + +import datadog.trace.api.iast.securitycontrol.SecurityControl; +import org.objectweb.asm.ClassVisitor; +import org.objectweb.asm.ClassWriter; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; + +import static org.objectweb.asm.Opcodes.ASM7; +import static org.objectweb.asm.Opcodes.ASM8; + +public class SecurityControlMethodClassVisitor extends ClassVisitor { + + private final SecurityControl securityControl; + + public SecurityControlMethodClassVisitor(final ClassWriter cw, final SecurityControl securityControl) { + super(ASM8, cw); + this.securityControl = securityControl; + } + + @Override + public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { + cv.visit(version, access, name, signature, superName, interfaces); + } + + @Override + public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) { + MethodVisitor mv = super.visitMethod(access, name, desc, signature, exceptions); + if (mv != null) { + mv = new SecurityControlMethodAdapter(ASM8, mv, access, name, desc, securityControl); + } + return mv; + } + + public void visitEnd() { + cv.visitEnd(); + } + +} From 808390879229539c25ad8d264aefa4157a9a9c49 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 20 Nov 2024 13:03:27 +0100 Subject: [PATCH 04/38] use IastSystem --- .../java/datadog/trace/bootstrap/Agent.java | 37 +++---------------- .../java/com/datadog/iast/IastSystem.java | 33 ++++++++++++++++- .../IastSecurityControlTransformer.java | 24 ++++++++++-- .../IastSecurityControlsInstrumenter.java | 28 -------------- 4 files changed, 57 insertions(+), 65 deletions(-) delete mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlsInstrumenter.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 25a4557dd8e..2aadbc5c799 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -529,7 +529,7 @@ public void execute() { installDatadogTracer(initTelemetry, scoClass, sco); maybeStartAppSec(scoClass, sco); - maybeStartIast(scoClass, sco); + maybeStartIast(instrumentation, scoClass, sco); maybeStartCiVisibility(instrumentation, scoClass, sco); maybeStartLogsIntake(scoClass, sco); // start debugger before remote config to subscribe to it before starting to poll @@ -610,31 +610,6 @@ private static void maybeStartRemoteConfig(Class scoClass, Object sco) { StaticEventLogger.end("Remote Config"); } - private void maybeStartIastSecurityControls(Instrumentation instrumentation, Class scoClass, Object sco) { - if(!Config.get().isIastSecurityControlsEnabled()) { - return; - } - if (!iastEnabled || iastFullyDisabled) { - log.warn("Error starting IAST Security Controls, IAST should be enabled"); - return; - } - if(Config.get().getIastSecurityControlsConfiguration() == null){ - log.warn("Error starting IAST Security Controls, IAST Security Controls configuration is missing"); - return; - } - StaticEventLogger.begin("IAST Security Controls"); - try { - final Class appSecSysClass = AGENT_CLASSLOADER.loadClass("com.datadog.iast.IastSecurityControlsInstrumenter"); - final Method iastInstallerMethod = - appSecSysClass.getMethod("start"); - iastInstallerMethod.invoke(instrumentation); - } catch (Exception e) { - log.error("Error starting IAST Security Controls", e); - } - - StaticEventLogger.end("IAST Security Controls"); - } - private static synchronized void startDatadogAgent( final InitializationTelemetry initTelemetry, final Instrumentation inst) { if (null != inst) { @@ -848,14 +823,14 @@ private static boolean isSupportedAppSecArch() { return true; } - private static void maybeStartIast(Class scoClass, Object o) { + private static void maybeStartIast(Instrumentation instrumentation, Class scoClass, Object o) { if (iastEnabled || !iastFullyDisabled) { StaticEventLogger.begin("IAST"); try { SubscriptionService ss = AgentTracer.get().getSubscriptionService(RequestContextSlot.IAST); - startIast(ss, scoClass, o); + startIast(instrumentation, ss, scoClass, o); } catch (Exception e) { log.error("Error starting IAST subsystem", e); } @@ -864,12 +839,12 @@ private static void maybeStartIast(Class scoClass, Object o) { } } - private static void startIast(SubscriptionService ss, Class scoClass, Object sco) { + private static void startIast(Instrumentation instrumentation, SubscriptionService ss, Class scoClass, Object sco) { try { final Class appSecSysClass = AGENT_CLASSLOADER.loadClass("com.datadog.iast.IastSystem"); final Method iastInstallerMethod = - appSecSysClass.getMethod("start", SubscriptionService.class); - iastInstallerMethod.invoke(null, ss); + appSecSysClass.getMethod("start",Instrumentation.class, SubscriptionService.class); + iastInstallerMethod.invoke(null, instrumentation, ss); } catch (final Throwable e) { log.warn("Not starting IAST subsystem", e); } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java index 806fb36b2e8..2af4d5ce18f 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java @@ -8,6 +8,7 @@ import com.datadog.iast.propagation.CodecModuleImpl; import com.datadog.iast.propagation.PropagationModuleImpl; import com.datadog.iast.propagation.StringModuleImpl; +import com.datadog.iast.securitycontrol.IastSecurityControlTransformer; import com.datadog.iast.sink.ApplicationModuleImpl; import com.datadog.iast.sink.CommandInjectionModuleImpl; import com.datadog.iast.sink.HardcodedSecretModuleImpl; @@ -47,12 +48,18 @@ import datadog.trace.api.iast.IastContext; import datadog.trace.api.iast.IastModule; import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.securitycontrol.SecurityControl; +import datadog.trace.api.iast.securitycontrol.SecurityControlFormatter; import datadog.trace.api.iast.telemetry.IastMetricCollector; import datadog.trace.api.iast.telemetry.Verbosity; import datadog.trace.util.AgentTaskScheduler; import datadog.trace.util.stacktrace.StackWalkerFactory; + +import java.lang.instrument.Instrumentation; import java.lang.reflect.Constructor; +import java.lang.reflect.Method; import java.lang.reflect.UndeclaredThrowableException; +import java.util.List; import java.util.function.BiFunction; import java.util.function.Supplier; import java.util.stream.Stream; @@ -66,11 +73,16 @@ public class IastSystem { public static boolean DEBUG = false; public static Verbosity VERBOSITY = Verbosity.OFF; - public static void start(final SubscriptionService ss) { - start(ss, null); + public static void start( final SubscriptionService ss) { + start(null, ss, null); + } + + public static void start(final Instrumentation instrumentation, final SubscriptionService ss) { + start(instrumentation, ss, null); } public static void start( + final Instrumentation instrumentation, final SubscriptionService ss, @Nullable OverheadController overheadController) { final Config config = Config.get(); final ProductActivation iast = config.getIastActivation(); @@ -106,9 +118,26 @@ public static void start( registerRequestEndedCallback(ss, addTelemetry, dependencies); registerHeadersCallback(ss); registerGrpcServerRequestMessageCallback(ss); + maybeApplySecurityControls(instrumentation); LOGGER.debug("IAST started"); } + private static void maybeApplySecurityControls(Instrumentation instrumentation) { + if(!Config.get().isIastSecurityControlsEnabled()) { + return; + } + if(Config.get().getIastSecurityControlsConfiguration() == null){ + LOGGER.warn("Error starting IAST Security Controls, IAST Security Controls configuration is missing"); + return; + } + List securityControls = SecurityControlFormatter.format(Config.get().getIastSecurityControlsConfiguration()); + if (securityControls == null) { + LOGGER.warn("No security controls to apply"); + return; + } + instrumentation.addTransformer(new IastSecurityControlTransformer(securityControls), true); + } + private static IastContext.Provider contextProvider( final ProductActivation iast, final boolean global) { if (iast != FULLY_ENABLED) { diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java index 96f38e5b37c..ef94804dea0 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java @@ -3,23 +3,39 @@ import datadog.trace.api.iast.securitycontrol.SecurityControl; import java.lang.instrument.ClassFileTransformer; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; + import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; + public class IastSecurityControlTransformer implements ClassFileTransformer { - private final SecurityControl securityControl; + private final List securityControls; + private final Set classFilter; - public IastSecurityControlTransformer(SecurityControl securityControl) { - this.securityControl = securityControl; + public IastSecurityControlTransformer(List securityControls) { + this.securityControls = securityControls; + this.classFilter = securityControls.stream().map(SecurityControl::getClassName).collect(Collectors.toSet()); } @Override public byte[] transform(ClassLoader loader, String className, Class classBeingRedefined, java.security.ProtectionDomain protectionDomain, byte[] classfileBuffer) { + if (classFilter.contains(className)) { + return null; // Do not transform classes that are not in the classFilter + } ClassReader cr = new ClassReader(classfileBuffer); ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES); - ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, className); + ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, getSecurityControl(className)); cr.accept(cv, 0); + return cw.toByteArray(); + } + + //TODO remove this and change structure to Map instead of List if this approach works + private SecurityControl getSecurityControl(final String className) { + return securityControls.stream().filter(sc -> sc.getClassName().equals(className)).findFirst().orElse(null); } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlsInstrumenter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlsInstrumenter.java deleted file mode 100644 index 360bc1ccb6d..00000000000 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlsInstrumenter.java +++ /dev/null @@ -1,28 +0,0 @@ -package com.datadog.iast.securitycontrol; - -import datadog.trace.api.Config; -import datadog.trace.api.iast.securitycontrol.SecurityControl; -import datadog.trace.api.iast.securitycontrol.SecurityControlFormatter; -import org.slf4j.Logger; - -import java.lang.instrument.Instrumentation; -import java.util.List; - -public class IastSecurityControlsInstrumenter { - - private static final Logger log = org.slf4j.LoggerFactory.getLogger(IastSecurityControlsInstrumenter.class); - - public static void start(Instrumentation inst) { - - List securityControls = SecurityControlFormatter.format(Config.get().getIastSecurityControlsConfiguration()); - if (securityControls == null) { - log.warn("No security controls to apply"); - return; - } - for (SecurityControl securityControl : securityControls) { - inst.addTransformer(new IastSecurityControlTransformer(securityControl), true); - } - - } - -} From 57134bd1807424e7dd1a82479b472d11135164ce Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 21 Nov 2024 16:22:39 +0100 Subject: [PATCH 05/38] First working version --- .../java/datadog/trace/bootstrap/Agent.java | 8 +-- .../java/com/datadog/iast/IastSystem.java | 21 +++---- .../propagation/PropagationModuleImpl.java | 16 +++++ .../IastSecurityControlTransformer.java | 33 ++++++---- .../SecurityControlMethodAdapter.java | 60 ++++++++++++++++++- .../SecurityControlMethodClassVisitor.java | 45 ++++++++++---- .../springboot/controller/XssController.java | 37 ++++++++++++ .../securitycontrols/InputValidator.java | 12 ++++ .../ddtest/securitycontrols/Sanitizer.java | 8 +++ .../AbstractIastSpringBootTest.groovy | 24 +++++++- .../iast/propagation/PropagationModule.java | 2 + .../iast/securitycontrol/SecurityControl.java | 7 ++- .../SecurityControlFormatter.java | 11 ++-- .../SecurityControlHelper.java | 19 ++++++ .../SecurityControlFormatterTest.groovy | 11 ++-- 15 files changed, 259 insertions(+), 55 deletions(-) create mode 100644 dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/InputValidator.java create mode 100644 dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/Sanitizer.java create mode 100644 internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlHelper.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java index 2aadbc5c799..ec5ab19ca09 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java @@ -535,14 +535,11 @@ public void execute() { // start debugger before remote config to subscribe to it before starting to poll maybeStartDebugger(instrumentation, scoClass, sco); maybeStartRemoteConfig(scoClass, sco); - maybeStartIastSecurityControls(instrumentation, scoClass, sco); if (telemetryEnabled) { startTelemetry(instrumentation, scoClass, sco); } } - - } protected static class StartProfilingAgentCallback extends ClassLoadCallBack { @@ -839,11 +836,12 @@ private static void maybeStartIast(Instrumentation instrumentation, Class sco } } - private static void startIast(Instrumentation instrumentation, SubscriptionService ss, Class scoClass, Object sco) { + private static void startIast( + Instrumentation instrumentation, SubscriptionService ss, Class scoClass, Object sco) { try { final Class appSecSysClass = AGENT_CLASSLOADER.loadClass("com.datadog.iast.IastSystem"); final Method iastInstallerMethod = - appSecSysClass.getMethod("start",Instrumentation.class, SubscriptionService.class); + appSecSysClass.getMethod("start", Instrumentation.class, SubscriptionService.class); iastInstallerMethod.invoke(null, instrumentation, ss); } catch (final Throwable e) { log.warn("Not starting IAST subsystem", e); diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java index 2af4d5ce18f..c7ff57e9407 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java @@ -54,10 +54,8 @@ import datadog.trace.api.iast.telemetry.Verbosity; import datadog.trace.util.AgentTaskScheduler; import datadog.trace.util.stacktrace.StackWalkerFactory; - import java.lang.instrument.Instrumentation; import java.lang.reflect.Constructor; -import java.lang.reflect.Method; import java.lang.reflect.UndeclaredThrowableException; import java.util.List; import java.util.function.BiFunction; @@ -73,7 +71,7 @@ public class IastSystem { public static boolean DEBUG = false; public static Verbosity VERBOSITY = Verbosity.OFF; - public static void start( final SubscriptionService ss) { + public static void start(final SubscriptionService ss) { start(null, ss, null); } @@ -82,8 +80,9 @@ public static void start(final Instrumentation instrumentation, final Subscripti } public static void start( - final Instrumentation instrumentation, - final SubscriptionService ss, @Nullable OverheadController overheadController) { + @Nullable final Instrumentation instrumentation, + final SubscriptionService ss, + @Nullable OverheadController overheadController) { final Config config = Config.get(); final ProductActivation iast = config.getIastActivation(); final ProductActivation appSec = config.getAppSecActivation(); @@ -122,15 +121,17 @@ public static void start( LOGGER.debug("IAST started"); } - private static void maybeApplySecurityControls(Instrumentation instrumentation) { - if(!Config.get().isIastSecurityControlsEnabled()) { + private static void maybeApplySecurityControls(@Nullable Instrumentation instrumentation) { + if (!Config.get().isIastSecurityControlsEnabled() || instrumentation == null) { return; } - if(Config.get().getIastSecurityControlsConfiguration() == null){ - LOGGER.warn("Error starting IAST Security Controls, IAST Security Controls configuration is missing"); + if (Config.get().getIastSecurityControlsConfiguration() == null) { + LOGGER.warn( + "Error starting IAST Security Controls, IAST Security Controls configuration is missing"); return; } - List securityControls = SecurityControlFormatter.format(Config.get().getIastSecurityControlsConfiguration()); + List securityControls = + SecurityControlFormatter.format(Config.get().getIastSecurityControlsConfiguration()); if (securityControls == null) { LOGGER.warn("No security controls to apply"); return; diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java index e0d1668d6fd..1cfe2bd1c04 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java @@ -652,6 +652,22 @@ public Taintable.Source findSource( return highestPrioritySource(to, target); } + @Override + public void markIfTainted(@org.jetbrains.annotations.Nullable Object target, int mark) { + if (target == null) { + return; + } + final IastContext ctx = IastContext.Provider.get(); + if (ctx == null) { + return; + } + TaintedObjects taintedObjects = ctx.getTaintedObjects(); + TaintedObject taintedObject = taintedObjects.get(target); + if (taintedObject != null) { + taintedObject.setRanges(markRanges(taintedObject.getRanges(), mark)); + } + } + @Override public boolean isTainted(@Nullable final Object target) { if (target == null) { diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java index ef94804dea0..d41f78f51a5 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java @@ -1,41 +1,54 @@ package com.datadog.iast.securitycontrol; import datadog.trace.api.iast.securitycontrol.SecurityControl; - import java.lang.instrument.ClassFileTransformer; import java.util.List; import java.util.Set; import java.util.stream.Collectors; - +import javax.annotation.Nullable; import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; - public class IastSecurityControlTransformer implements ClassFileTransformer { - private final List securityControls; + private final List securityControls; private final Set classFilter; public IastSecurityControlTransformer(List securityControls) { this.securityControls = securityControls; - this.classFilter = securityControls.stream().map(SecurityControl::getClassName).collect(Collectors.toSet()); + this.classFilter = + securityControls.stream().map(SecurityControl::getClassName).collect(Collectors.toSet()); } @Override - public byte[] transform(ClassLoader loader, String className, Class classBeingRedefined, java.security.ProtectionDomain protectionDomain, byte[] classfileBuffer) { - if (classFilter.contains(className)) { + @Nullable + public byte[] transform( + ClassLoader loader, + String className, + Class classBeingRedefined, + java.security.ProtectionDomain protectionDomain, + byte[] classfileBuffer) { + if (!classFilter.contains(className)) { return null; // Do not transform classes that are not in the classFilter } + SecurityControl securityControl = getSecurityControl(className); + if (securityControl == null) { + return null; // Do not transform classes that do not have a security control + } ClassReader cr = new ClassReader(classfileBuffer); ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES); - ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, getSecurityControl(className)); + ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, securityControl); cr.accept(cv, 0); return cw.toByteArray(); } - //TODO remove this and change structure to Map instead of List if this approach works + // TODO remove this and change structure to Map instead of List if this approach works + @Nullable private SecurityControl getSecurityControl(final String className) { - return securityControls.stream().filter(sc -> sc.getClassName().equals(className)).findFirst().orElse(null); + return securityControls.stream() + .filter(sc -> sc.getClassName().equals(className)) + .findFirst() + .orElse(null); } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java index fc953ef62f8..28ae7ac0520 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java @@ -1,12 +1,68 @@ package com.datadog.iast.securitycontrol; import datadog.trace.api.iast.securitycontrol.SecurityControl; +import datadog.trace.api.iast.securitycontrol.SecurityControlType; import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; public class SecurityControlMethodAdapter extends MethodVisitor { + private final MethodVisitor mv; + private final SecurityControl securityControl; + private final String desc; - protected SecurityControlMethodAdapter(int api) { - super(api); + public SecurityControlMethodAdapter( + final MethodVisitor mv, final SecurityControl securityControl, final String desc) { + super(Opcodes.ASM8, mv); + this.mv = mv; + this.securityControl = securityControl; + this.desc = desc; + } + + @Override + public void visitInsn(int opcode) { + // Check if the opcode is a return instruction + if (opcode >= Opcodes.IRETURN && opcode <= Opcodes.RETURN) { + if (securityControl.getType() == SecurityControlType.INPUT_VALIDATOR) { + boolean allParameters = securityControl.getParametersToMark() == null; + Type[] types = Type.getArgumentTypes(desc); + for (int i = 0; i < types.length; i++) { + // TODO add check if only some parameters should be marked + if (allParameters || securityControl.getParametersToMark().contains(i)) { + callInputValidation(i); + } + } + } else { // SecurityControlType.SANITIZER + /// Mark the return value as secure for securityControl.getMarks() items + // Duplicate the return value on the stack + mv.visitInsn(Opcodes.DUP); + // Load the marks from securityControl onto the stack + mv.visitLdcInsn(securityControl.getMarks()); + // Insert the call to setSecureMarks with the return value and marks as parameters + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "datadog/trace/api/iast/securitycontrol/SecurityControlHelper", + "setSecureMarks", + "(Ljava/lang/Object;I)V", + false); + } + } + + super.visitInsn(opcode); + } + + private void callInputValidation(int i) { + // Duplicate the parameter value on the stack + mv.visitVarInsn(Opcodes.ALOAD, i); + // Load the marks from securityControl onto the stack + mv.visitLdcInsn(securityControl.getMarks()); + // Insert the call to setSecureMarks with the parameter value and marks as parameters + mv.visitMethodInsn( + Opcodes.INVOKESTATIC, + "datadog/trace/api/iast/securitycontrol/SecurityControlHelper", + "setSecureMarks", + "(Ljava/lang/Object;I)V", + false); } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java index ee0aecdecae..987dd7ecd2e 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java @@ -1,33 +1,29 @@ package com.datadog.iast.securitycontrol; +import static org.objectweb.asm.Opcodes.ASM8; + import datadog.trace.api.iast.securitycontrol.SecurityControl; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; import org.objectweb.asm.MethodVisitor; -import org.objectweb.asm.Opcodes; - -import static org.objectweb.asm.Opcodes.ASM7; -import static org.objectweb.asm.Opcodes.ASM8; +import org.objectweb.asm.Type; public class SecurityControlMethodClassVisitor extends ClassVisitor { private final SecurityControl securityControl; - public SecurityControlMethodClassVisitor(final ClassWriter cw, final SecurityControl securityControl) { + public SecurityControlMethodClassVisitor( + final ClassWriter cw, final SecurityControl securityControl) { super(ASM8, cw); this.securityControl = securityControl; } @Override - public void visit(int version, int access, String name, String signature, String superName, String[] interfaces) { - cv.visit(version, access, name, signature, superName, interfaces); - } - - @Override - public MethodVisitor visitMethod(int access, String name, String desc, String signature, String[] exceptions) { + public MethodVisitor visitMethod( + int access, String name, String desc, String signature, String[] exceptions) { MethodVisitor mv = super.visitMethod(access, name, desc, signature, exceptions); - if (mv != null) { - mv = new SecurityControlMethodAdapter(ASM8, mv, access, name, desc, securityControl); + if (mv != null && shouldBeAdapted(name, desc)) { + mv = new SecurityControlMethodAdapter(mv, securityControl, desc); } return mv; } @@ -36,4 +32,27 @@ public void visitEnd() { cv.visitEnd(); } + private boolean shouldBeAdapted(String name, String desc) { + + if (!securityControl.getMethod().equals(name)) { + return false; + } + + if (securityControl.getParameterTypes() == null) { + return true; + } + + Type[] types = Type.getArgumentTypes(desc); + if (types.length != securityControl.getParameterTypes().length) { + return false; + } + + for (int i = 0; i < types.length; i++) { + if (!types[i].getClassName().equals(securityControl.getParameterTypes()[i])) { + return false; + } + } + + return true; + } } diff --git a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java index 81f708acb55..7ab925a63b4 100644 --- a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java +++ b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java @@ -1,5 +1,7 @@ package datadog.smoketest.springboot.controller; +import ddtest.securitycontrols.InputValidator; +import ddtest.securitycontrols.Sanitizer; import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.IOException; import java.util.Locale; @@ -190,4 +192,39 @@ public void format4(final HttpServletRequest request, final HttpServletResponse public String responseBody(final HttpServletRequest request, final HttpServletResponse response) { return request.getParameter("string"); } + + @GetMapping("/sanitize") + @SuppressFBWarnings + public void sanitize(final HttpServletRequest request, final HttpServletResponse response) { + try { + response.getWriter().write(Sanitizer.sanitize(request.getParameter("string"))); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + @GetMapping("/validate") + @SuppressFBWarnings + public void validate(final HttpServletRequest request, final HttpServletResponse response) { + try { + String s = request.getParameter("string"); + InputValidator.validate(s); + response.getWriter().write(s); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + @GetMapping("/validate2") + @SuppressFBWarnings + public void validate2(final HttpServletRequest request, final HttpServletResponse response) { + try { + String string1 = request.getParameter("string"); + String string2 = request.getParameter("string2"); + InputValidator.validate(string1, string2); + response.getWriter().write(string1 + string2); + } catch (IOException e) { + throw new RuntimeException(e); + } + } } diff --git a/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/InputValidator.java b/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/InputValidator.java new file mode 100644 index 00000000000..182ca8287ea --- /dev/null +++ b/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/InputValidator.java @@ -0,0 +1,12 @@ +package ddtest.securitycontrols; + +public class InputValidator { + + public static boolean validate(String input) { + return true; // dummy implementation + } + + public static boolean validate(String input, String input2) { + return true; // dummy implementation + } +} diff --git a/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/Sanitizer.java b/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/Sanitizer.java new file mode 100644 index 00000000000..3fc4dcf9937 --- /dev/null +++ b/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/Sanitizer.java @@ -0,0 +1,8 @@ +package ddtest.securitycontrols; + +public class Sanitizer { + + public static String sanitize(String input) { + return "Sanitized: " + input; + } +} diff --git a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy index 8dbe33e52cb..075f8055553 100644 --- a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy +++ b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy @@ -7,9 +7,10 @@ import okhttp3.Request import okhttp3.RequestBody import okhttp3.Response -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_SECURITY_CONTROLS_ENABLED +import static datadog.trace.api.config.IastConfig.IAST_SECURITY_CONTROLS_CONFIGURATION abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { @@ -35,7 +36,8 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { return [ withSystemProperty(IAST_ENABLED, true), withSystemProperty(IAST_DETECTION_MODE, 'FULL'), - withSystemProperty(IAST_DEBUG_ENABLED, true), + withSystemProperty(IAST_SECURITY_CONTROLS_ENABLED, true), + withSystemProperty(IAST_SECURITY_CONTROLS_CONFIGURATION, "SANITIZER:XSS:ddtest.securitycontrols.Sanitizer:sanitize;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validate"), ] } @@ -1203,5 +1205,23 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { then: response.body().string().contains("Test") } + void 'security controls avoid vulnerabilities'() { + setup: + final url = "http://localhost:${httpPort}/xss/${method}?string=test&string2=test2" + final request = new Request.Builder().url(url).get().build() + + when: + client.newCall(request).execute() + + then: + noVulnerability { vul -> vul.type == 'XSS' && vul.location.method == method } + + where: + method | _ + 'sanitize' | _ + 'validate' | _ + 'validate2' | _ + } + } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/propagation/PropagationModule.java b/internal-api/src/main/java/datadog/trace/api/iast/propagation/PropagationModule.java index ca54e68ff1b..371c3618835 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/propagation/PropagationModule.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/propagation/PropagationModule.java @@ -345,4 +345,6 @@ int taintObjectDeeply( */ @Nullable Source findSource(@Nullable IastContext ctx, @Nullable Object target); + + void markIfTainted(@Nullable Object target, int mark); } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java index cfad238b39e..ada17d7e432 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java @@ -1,5 +1,6 @@ package datadog.trace.api.iast.securitycontrol; +import java.util.Set; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -15,7 +16,7 @@ public class SecurityControl { @Nullable private String[] parameterTypes; - @Nullable private int[] parametersToMark; + @Nullable private Set parametersToMark; public SecurityControl( @Nonnull SecurityControlType type, @@ -23,7 +24,7 @@ public SecurityControl( @Nonnull String className, @Nonnull String method, @Nullable String[] parameterTypes, - @Nullable int[] parametersToMark) { + @Nullable Set parametersToMark) { this.type = type; this.marks = marks; this.className = className; @@ -58,7 +59,7 @@ public String[] getParameterTypes() { } @Nullable - public int[] getParametersToMark() { + public Set getParametersToMark() { return parametersToMark; } } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index 43b28e60012..ed332222eae 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -6,6 +6,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Set; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.slf4j.Logger; @@ -60,11 +61,11 @@ private static SecurityControl getSecurityControl(@Nonnull final String config) return null; } int marks = getMarks(split[1]); - String className = split[2]; + String className = split[2].replaceAll("\\.", "/"); String method = split[3]; String[] parameterTypes = null; - int[] parametersToMark = null; + Set parametersToMark = null; if (split.length > 4) { String[] elements = split[4].split(SECURITY_CONTROL_ELEMENT_DELIMITER); @@ -96,8 +97,10 @@ private static int getMarks(String s) { return marks; } - private static int[] getParametersToMark(String[] elements) { - return Arrays.stream(elements).mapToInt(Integer::parseInt).toArray(); + private static Set getParametersToMark(String[] elements) { + return Arrays.stream(elements) + .map(Integer::parseInt) + .collect(java.util.stream.Collectors.toSet()); } private static boolean isNumeric(String str) { diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlHelper.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlHelper.java new file mode 100644 index 00000000000..3266f261ce2 --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlHelper.java @@ -0,0 +1,19 @@ +package datadog.trace.api.iast.securitycontrol; + +import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.propagation.PropagationModule; + +public class SecurityControlHelper { + + public static void setSecureMarks(final Object target, int marks) { + + final PropagationModule module = InstrumentationBridge.PROPAGATION; + try { + if (module != null) { + module.markIfTainted(target, marks); + } + } catch (final Throwable e) { + module.onUnexpectedException("afterRepeat threw", e); + } + } +} diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy index 8640b228e42..0516159efe1 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy @@ -16,7 +16,7 @@ class SecurityControlFormatterTest extends DDSpecification{ def securityControl = result.get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK - securityControl.getClassName() == "bar.foo.CustomInputValidator" + securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == null securityControl.getParametersToMark() == null @@ -33,7 +33,7 @@ class SecurityControlFormatterTest extends DDSpecification{ def securityControl = result.get(0) securityControl.getType() == SecurityControlType.SANITIZER securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK - securityControl.getClassName() == "bar.foo.CustomSanitizer" + securityControl.getClassName() == "bar/foo/CustomSanitizer" securityControl.getMethod() == "sanitize" securityControl.getParameterTypes() == null securityControl.getParametersToMark() == null @@ -50,7 +50,7 @@ class SecurityControlFormatterTest extends DDSpecification{ def inputValidator = result.get(0) inputValidator.getType() == SecurityControlType.INPUT_VALIDATOR inputValidator.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK - inputValidator.getClassName() == "bar.foo.CustomInputValidator" + inputValidator.getClassName() == "bar/foo/CustomInputValidator" inputValidator.getMethod() == "validate" inputValidator.getParameterTypes() == null inputValidator.getParametersToMark() == null @@ -58,7 +58,7 @@ class SecurityControlFormatterTest extends DDSpecification{ def sanitizer = result.get(1) sanitizer.getType() == SecurityControlType.SANITIZER sanitizer.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK - sanitizer.getClassName() == "bar.foo.CustomSanitizer" + sanitizer.getClassName() == "bar/foo/CustomSanitizer" sanitizer.getMethod() == "sanitize" sanitizer.getParameterTypes() == null sanitizer.getParametersToMark() == null @@ -75,10 +75,9 @@ class SecurityControlFormatterTest extends DDSpecification{ def securityControl = result.get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.SQL_INJECTION_MARK) - securityControl.getClassName() == "bar.foo.CustomInputValidator" + securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == null securityControl.getParametersToMark() == null } - } From 8e591d44673264252b7b51a3367af080eda053f9 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 22 Nov 2024 08:29:11 +0100 Subject: [PATCH 06/38] Add more stacktrace cases --- .../springboot/controller/XssController.java | 21 +++++++++++++++---- .../securitycontrols/InputValidator.java | 10 ++++++++- .../AbstractIastSpringBootTest.groovy | 5 +++-- 3 files changed, 29 insertions(+), 7 deletions(-) diff --git a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java index 7ab925a63b4..d7087339a2f 100644 --- a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java +++ b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java @@ -203,12 +203,12 @@ public void sanitize(final HttpServletRequest request, final HttpServletResponse } } - @GetMapping("/validate") + @GetMapping("/validateAll") @SuppressFBWarnings - public void validate(final HttpServletRequest request, final HttpServletResponse response) { + public void validateAll(final HttpServletRequest request, final HttpServletResponse response) { try { String s = request.getParameter("string"); - InputValidator.validate(s); + InputValidator.validateAll(s); response.getWriter().write(s); } catch (IOException e) { throw new RuntimeException(e); @@ -221,7 +221,20 @@ public void validate2(final HttpServletRequest request, final HttpServletRespons try { String string1 = request.getParameter("string"); String string2 = request.getParameter("string2"); - InputValidator.validate(string1, string2); + InputValidator.validateAll(string1, string2); + response.getWriter().write(string1 + string2); + } catch (IOException e) { + throw new RuntimeException(e); + } + } + + @GetMapping("/validate") + @SuppressFBWarnings + public void validate(final HttpServletRequest request, final HttpServletResponse response) { + try { + String string1 = request.getParameter("string"); + String string2 = request.getParameter("string2"); + InputValidator.validate(null, string1, string2); response.getWriter().write(string1 + string2); } catch (IOException e) { throw new RuntimeException(e); diff --git a/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/InputValidator.java b/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/InputValidator.java index 182ca8287ea..2786b00be9c 100644 --- a/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/InputValidator.java +++ b/dd-smoke-tests/iast-util/src/main/java/ddtest/securitycontrols/InputValidator.java @@ -2,11 +2,19 @@ public class InputValidator { + public static boolean validateAll(String input) { + return true; // dummy implementation + } + + public static boolean validateAll(String input, String input2) { + return true; // dummy implementation + } + public static boolean validate(String input) { return true; // dummy implementation } - public static boolean validate(String input, String input2) { + public static boolean validate(Object o, String input, String input2) { return true; // dummy implementation } } diff --git a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy index 075f8055553..48797becef0 100644 --- a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy +++ b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy @@ -37,7 +37,7 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { withSystemProperty(IAST_ENABLED, true), withSystemProperty(IAST_DETECTION_MODE, 'FULL'), withSystemProperty(IAST_SECURITY_CONTROLS_ENABLED, true), - withSystemProperty(IAST_SECURITY_CONTROLS_CONFIGURATION, "SANITIZER:XSS:ddtest.securitycontrols.Sanitizer:sanitize;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validate"), + withSystemProperty(IAST_SECURITY_CONTROLS_CONFIGURATION, "SANITIZER:XSS:ddtest.securitycontrols.Sanitizer:sanitize;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validateAll;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validate:1,2;java.lang.Object,java.lang.String,java.lang.String"), ] } @@ -1219,8 +1219,9 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { where: method | _ 'sanitize' | _ - 'validate' | _ + 'validateAll' | _ 'validate2' | _ + 'validate' | _ } From 2c725b9cefb93fd2e72c38fff958826447927e01 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 22 Nov 2024 09:49:53 +0100 Subject: [PATCH 07/38] first review --- .../propagation/PropagationModuleImpl.java | 2 +- .../SecurityControlMethodAdapter.java | 20 ++++++------------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java index 1cfe2bd1c04..1503fd12909 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/propagation/PropagationModuleImpl.java @@ -653,7 +653,7 @@ public Taintable.Source findSource( } @Override - public void markIfTainted(@org.jetbrains.annotations.Nullable Object target, int mark) { + public void markIfTainted(@Nullable Object target, int mark) { if (target == null) { return; } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java index 28ae7ac0520..fc92550b2a7 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java @@ -8,6 +8,10 @@ public class SecurityControlMethodAdapter extends MethodVisitor { + public static final String HELPER = + "datadog/trace/api/iast/securitycontrol/SecurityControlHelper"; + public static final String METHOD = "setSecureMarks"; + public static final String DESCRIPTOR = "(Ljava/lang/Object;I)V"; private final MethodVisitor mv; private final SecurityControl securityControl; private final String desc; @@ -28,24 +32,17 @@ public void visitInsn(int opcode) { boolean allParameters = securityControl.getParametersToMark() == null; Type[] types = Type.getArgumentTypes(desc); for (int i = 0; i < types.length; i++) { - // TODO add check if only some parameters should be marked if (allParameters || securityControl.getParametersToMark().contains(i)) { callInputValidation(i); } } } else { // SecurityControlType.SANITIZER - /// Mark the return value as secure for securityControl.getMarks() items // Duplicate the return value on the stack mv.visitInsn(Opcodes.DUP); // Load the marks from securityControl onto the stack mv.visitLdcInsn(securityControl.getMarks()); // Insert the call to setSecureMarks with the return value and marks as parameters - mv.visitMethodInsn( - Opcodes.INVOKESTATIC, - "datadog/trace/api/iast/securitycontrol/SecurityControlHelper", - "setSecureMarks", - "(Ljava/lang/Object;I)V", - false); + mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); } } @@ -58,11 +55,6 @@ private void callInputValidation(int i) { // Load the marks from securityControl onto the stack mv.visitLdcInsn(securityControl.getMarks()); // Insert the call to setSecureMarks with the parameter value and marks as parameters - mv.visitMethodInsn( - Opcodes.INVOKESTATIC, - "datadog/trace/api/iast/securitycontrol/SecurityControlHelper", - "setSecureMarks", - "(Ljava/lang/Object;I)V", - false); + mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); } } From 831fe648838ce1b9055130a16f4ee9c88a96196c Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 22 Nov 2024 10:17:06 +0100 Subject: [PATCH 08/38] Fix test and forbidden apis --- .../src/test/groovy/com/datadog/iast/IastSystemTest.groovy | 2 +- .../api/iast/securitycontrol/SecurityControlFormatter.java | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/IastSystemTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/IastSystemTest.groovy index 71914a424a2..38014536f16 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/IastSystemTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/IastSystemTest.groovy @@ -136,7 +136,7 @@ class IastSystemTest extends DDSpecification { InstrumentationBridge.clearIastModules() when: - Agent.maybeStartIast(null, null) + Agent.maybeStartIast(null, null, null) then: InstrumentationBridge.iastModules.each { diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index ed332222eae..0b7682a54c6 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -9,8 +9,11 @@ import java.util.Set; import javax.annotation.Nonnull; import javax.annotation.Nullable; + +import de.thetaphi.forbiddenapis.SuppressForbidden; import org.slf4j.Logger; +@SuppressForbidden // Suppresses the warning for using split method public class SecurityControlFormatter { private static final Logger log = From 86024771f96defcfe9e01e179d39d7eb71673bf6 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 22 Nov 2024 10:44:16 +0100 Subject: [PATCH 09/38] fix spotless --- .../api/iast/securitycontrol/SecurityControlFormatter.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index 0b7682a54c6..c19e6c6f654 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -3,14 +3,13 @@ import static datadog.trace.api.iast.VulnerabilityMarks.NOT_MARKED; import datadog.trace.api.iast.VulnerabilityMarks; +import de.thetaphi.forbiddenapis.SuppressForbidden; import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.Set; import javax.annotation.Nonnull; import javax.annotation.Nullable; - -import de.thetaphi.forbiddenapis.SuppressForbidden; import org.slf4j.Logger; @SuppressForbidden // Suppresses the warning for using split method From 325086fcf65806895e70cdc0e57efcd286d4acbb Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 22 Nov 2024 12:22:11 +0100 Subject: [PATCH 10/38] fix spotless --- .../agent-iast/src/main/java/com/datadog/iast/IastSystem.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java index c7ff57e9407..6797c85885f 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java @@ -75,6 +75,10 @@ public static void start(final SubscriptionService ss) { start(null, ss, null); } + public static void start(final SubscriptionService ss, OverheadController overheadController) { + start(null, ss, overheadController); + } + public static void start(final Instrumentation instrumentation, final SubscriptionService ss) { start(instrumentation, ss, null); } From 838d924d32fe3110c11c626510f98fc77af2d819 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 22 Nov 2024 12:44:50 +0100 Subject: [PATCH 11/38] fix test --- .../groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy index 48797becef0..2f2a8d8334b 100644 --- a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy +++ b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy @@ -1,5 +1,6 @@ package datadog.smoketest +import static datadog.trace.api.config.IastConfig.IAST_DEBUG_ENABLED import okhttp3.FormBody import okhttp3.MediaType import okhttp3.MultipartBody @@ -36,6 +37,7 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { return [ withSystemProperty(IAST_ENABLED, true), withSystemProperty(IAST_DETECTION_MODE, 'FULL'), + withSystemProperty(IAST_DEBUG_ENABLED, true), withSystemProperty(IAST_SECURITY_CONTROLS_ENABLED, true), withSystemProperty(IAST_SECURITY_CONTROLS_CONFIGURATION, "SANITIZER:XSS:ddtest.securitycontrols.Sanitizer:sanitize;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validateAll;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validate:1,2;java.lang.Object,java.lang.String,java.lang.String"), ] From f0f8496bd845072656d43c76000ca4f182040f7b Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 22 Nov 2024 17:02:54 +0100 Subject: [PATCH 12/38] fix test --- dd-java-agent/agent-iast/build.gradle | 2 +- .../SecurityControlFormatter.java | 14 ++- .../SecurityControlFormatterTest.groovy | 88 ++++++++++++++++++- 3 files changed, 98 insertions(+), 6 deletions(-) diff --git a/dd-java-agent/agent-iast/build.gradle b/dd-java-agent/agent-iast/build.gradle index 942f8d5b94a..01dd53138ba 100644 --- a/dd-java-agent/agent-iast/build.gradle +++ b/dd-java-agent/agent-iast/build.gradle @@ -50,7 +50,7 @@ dependencies { implementation project(':internal-api') implementation project(':internal-api:internal-api-9') implementation libs.moshi - implementation("org.ow2.asm:asm:9.7") + implementation libs.bundles.asm testFixturesApi project(':dd-java-agent:testing') testFixturesApi project(':utils:test-utils') diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index c19e6c6f654..7dc2dc96adc 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -39,9 +39,13 @@ public static List format(final @Nonnull String securityControl List securityControls = new ArrayList<>(list.length); for (String s : list) { - SecurityControl securityControl = getSecurityControl(s); - if (securityControl != null) { - securityControls.add(securityControl); + try { + SecurityControl securityControl = getSecurityControl(s); + if (securityControl != null) { + securityControls.add(securityControl); + } + } catch (Exception e) { + log.warn("Security control configuration is invalid: {}", s); } } return securityControls.isEmpty() ? null : securityControls; @@ -73,6 +77,10 @@ private static SecurityControl getSecurityControl(@Nonnull final String config) String[] elements = split[4].split(SECURITY_CONTROL_ELEMENT_DELIMITER); if (elements.length > 0) { if (isNumeric(elements[0])) { + if (split.length != 6) { + log.warn("Security control configuration is invalid: {}", config); + return null; + } parametersToMark = getParametersToMark(elements); } else { parameterTypes = elements; diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy index 0516159efe1..e22bf3f0342 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy @@ -5,7 +5,7 @@ import datadog.trace.test.util.DDSpecification class SecurityControlFormatterTest extends DDSpecification{ - void 'test happy path Input validator'() { + void 'test simple Input validator'() { setup: final formatter = new SecurityControlFormatter() final config = 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate' @@ -22,7 +22,7 @@ class SecurityControlFormatterTest extends DDSpecification{ securityControl.getParametersToMark() == null } - void 'test happy path sanitizer'() { + void 'test simple sanitizer'() { setup: final formatter = new SecurityControlFormatter() final config = 'SANITIZER:COMMAND_INJECTION:bar.foo.CustomSanitizer:sanitize' @@ -80,4 +80,88 @@ class SecurityControlFormatterTest extends DDSpecification{ securityControl.getParameterTypes() == null securityControl.getParametersToMark() == null } + + void 'test overcharged methods'() { + setup: + final formatter = new SecurityControlFormatter() + final config = 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate:java.lang.Object,java.lang.String,java.lang.String' + final result = formatter.format(config) + + expect: + result.size() == 1 + def securityControl = result.get(0) + securityControl.getType() == SecurityControlType.INPUT_VALIDATOR + securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getClassName() == "bar/foo/CustomInputValidator" + securityControl.getMethod() == "validate" + securityControl.getParameterTypes() == ["java.lang.Object", "java.lang.String", "java.lang.String"] + securityControl.getParametersToMark() == null + } + + void 'test parameters to mark'() { + setup: + final formatter = new SecurityControlFormatter() + final config = 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate:1,2' + final result = formatter.format(config) + + expect: + result.size() == 1 + def securityControl = result.get(0) + securityControl.getType() == SecurityControlType.INPUT_VALIDATOR + securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getClassName() == "bar/foo/CustomInputValidator" + securityControl.getMethod() == "validate" + securityControl.getParameterTypes() == null + securityControl.getParametersToMark().size() == 2 + securityControl.getParametersToMark().contains(1) + securityControl.getParametersToMark().contains(2) + } + + void 'test overcharged methods with parameters to mark'() { + setup: + final formatter = new SecurityControlFormatter() + final config = 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate:java.lang.Object,java.lang.String,java.lang.String:1,2' + final result = formatter.format(config) + + expect: + result.size() == 1 + def securityControl = result.get(0) + securityControl.getType() == SecurityControlType.INPUT_VALIDATOR + securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getClassName() == "bar/foo/CustomInputValidator" + securityControl.getMethod() == "validate" + securityControl.getParameterTypes() == ["java.lang.Object", "java.lang.String", "java.lang.String"] + securityControl.getParametersToMark().size() == 2 + securityControl.getParametersToMark().contains(1) + securityControl.getParametersToMark().contains(2) + } + + void 'test error control'() { + setup: + final formatter = new SecurityControlFormatter() + Throwable thrown = null + + when: + try { + final result = formatter.format(config) + } catch (Throwable t) { + thrown = t + } + + then: + thrown == null + final result = formatter.format(config) + result == null + + where: + config << [ + '', + 'This is not a valid configuration', + 'INPUT_VALIDATOR', + 'INPUT_VALIDATOR:COMMAND_INJECTION', + 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator', + 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate:1,2', + 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate:1,2:java.lang.Object,java.lang.String,java.lang.String' + ] + } } From 9844124d53c18f6ed0dba4e4e49364b522be0d5c Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 22 Nov 2024 17:41:07 +0100 Subject: [PATCH 13/38] Add test and fix coverage --- internal-api/build.gradle | 2 + .../SecurityControlFormatter.java | 2 +- .../SecurityControlFormatterTest.groovy | 1 - .../SecurityControlHelperTest.groovy | 37 +++++++++++++++++++ 4 files changed, 40 insertions(+), 2 deletions(-) create mode 100644 internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlHelperTest.groovy diff --git a/internal-api/build.gradle b/internal-api/build.gradle index 7176c43d500..55528d74d39 100644 --- a/internal-api/build.gradle +++ b/internal-api/build.gradle @@ -190,6 +190,8 @@ excludedClassesCoverage += [ 'datadog.trace.util.stacktrace.StackTraceBatch', 'datadog.trace.util.stacktrace.StackTraceEvent', 'datadog.trace.util.stacktrace.StackTraceFrame', + 'datadog.trace.api.iast.VulnerabilityMarks', + 'datadog.trace.api.iast.securitycontrol.SecurityControlHelper' ] excludedClassesBranchCoverage = [ 'datadog.trace.api.ProductActivationConfig', diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index 7dc2dc96adc..3a5b41b4941 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -77,7 +77,7 @@ private static SecurityControl getSecurityControl(@Nonnull final String config) String[] elements = split[4].split(SECURITY_CONTROL_ELEMENT_DELIMITER); if (elements.length > 0) { if (isNumeric(elements[0])) { - if (split.length != 6) { + if (split.length != 5) { log.warn("Security control configuration is invalid: {}", config); return null; } diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy index e22bf3f0342..d6908e3f5c0 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy @@ -160,7 +160,6 @@ class SecurityControlFormatterTest extends DDSpecification{ 'INPUT_VALIDATOR', 'INPUT_VALIDATOR:COMMAND_INJECTION', 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator', - 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate:1,2', 'INPUT_VALIDATOR:COMMAND_INJECTION:bar.foo.CustomInputValidator:validate:1,2:java.lang.Object,java.lang.String,java.lang.String' ] } diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlHelperTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlHelperTest.groovy new file mode 100644 index 00000000000..135d535d391 --- /dev/null +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlHelperTest.groovy @@ -0,0 +1,37 @@ +package datadog.trace.api.iast.securitycontrol + +import datadog.trace.api.iast.InstrumentationBridge +import datadog.trace.api.iast.VulnerabilityMarks +import datadog.trace.api.iast.propagation.PropagationModule +import datadog.trace.test.util.DDSpecification + +class SecurityControlHelperTest extends DDSpecification { + + + void 'test no module'(){ + setup: + final toValidate = 'test' + final marks = VulnerabilityMarks.XSS_MARK + + when: + SecurityControlHelper.setSecureMarks(toValidate, marks) + + then: + 0 * _ + } + + void 'test'(){ + setup: + final iastModule = Mock(PropagationModule) + InstrumentationBridge.registerIastModule(iastModule) + final toValidate = 'test' + final marks = VulnerabilityMarks.XSS_MARK + + when: + SecurityControlHelper.setSecureMarks(toValidate, marks) + + then: + 1 * iastModule.markIfTainted(toValidate, marks) + 0 * _ + } +} From 744cdbb98faf11628c433720d3b9ef9eb1b00761 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 25 Nov 2024 09:09:53 +0100 Subject: [PATCH 14/38] remove security control enabled env var --- .../src/main/java/com/datadog/iast/IastSystem.java | 7 +------ .../datadog/smoketest/AbstractIastSpringBootTest.groovy | 1 - internal-api/src/main/java/datadog/trace/api/Config.java | 6 ------ 3 files changed, 1 insertion(+), 13 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java index 6797c85885f..69842235d3d 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java @@ -126,12 +126,7 @@ public static void start( } private static void maybeApplySecurityControls(@Nullable Instrumentation instrumentation) { - if (!Config.get().isIastSecurityControlsEnabled() || instrumentation == null) { - return; - } - if (Config.get().getIastSecurityControlsConfiguration() == null) { - LOGGER.warn( - "Error starting IAST Security Controls, IAST Security Controls configuration is missing"); + if (Config.get().getIastSecurityControlsConfiguration() == null || instrumentation == null) { return; } List securityControls = diff --git a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy index 2f2a8d8334b..dd4c243e73e 100644 --- a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy +++ b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy @@ -38,7 +38,6 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { withSystemProperty(IAST_ENABLED, true), withSystemProperty(IAST_DETECTION_MODE, 'FULL'), withSystemProperty(IAST_DEBUG_ENABLED, true), - withSystemProperty(IAST_SECURITY_CONTROLS_ENABLED, true), withSystemProperty(IAST_SECURITY_CONTROLS_CONFIGURATION, "SANITIZER:XSS:ddtest.securitycontrols.Sanitizer:sanitize;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validateAll;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validate:1,2;java.lang.Object,java.lang.String,java.lang.String"), ] } diff --git a/internal-api/src/main/java/datadog/trace/api/Config.java b/internal-api/src/main/java/datadog/trace/api/Config.java index 958bb6de001..60a64f9e3d9 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -307,7 +307,6 @@ public static String getHostName() { private final int iastSourceMappingMaxSize; private final boolean iastStackTraceEnabled; private final boolean iastExperimentalPropagationEnabled; - private final boolean iastSecurityControlsEnabled; private final String iastSecurityControlsConfiguration; private final boolean ciVisibilityTraceSanitationEnabled; @@ -1334,7 +1333,6 @@ PROFILING_DATADOG_PROFILER_ENABLED, isDatadogProfilerSafeInCurrentEnvironment()) configProvider.getBoolean(IAST_STACK_TRACE_ENABLED, DEFAULT_IAST_STACK_TRACE_ENABLED); iastExperimentalPropagationEnabled = configProvider.getBoolean(IAST_EXPERIMENTAL_PROPAGATION_ENABLED, false); - iastSecurityControlsEnabled = configProvider.getBoolean(IAST_SECURITY_CONTROLS_ENABLED, false); iastSecurityControlsConfiguration = configProvider.getString(IAST_SECURITY_CONTROLS_CONFIGURATION, null); @@ -2638,10 +2636,6 @@ public boolean isIastExperimentalPropagationEnabled() { return iastExperimentalPropagationEnabled; } - public boolean isIastSecurityControlsEnabled() { - return iastSecurityControlsEnabled; - } - public String getIastSecurityControlsConfiguration() { return iastSecurityControlsConfiguration; } From 74a93b8a7d22790f725491100bde0ffd8a2afa17 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 25 Nov 2024 10:11:42 +0100 Subject: [PATCH 15/38] fix classVisitor --- .../IastSecurityControlTransformer.java | 13 ++++------ .../SecurityControlMethodClassVisitor.java | 25 ++++++++++++++----- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java index d41f78f51a5..da737badec0 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java @@ -32,23 +32,20 @@ public byte[] transform( if (!classFilter.contains(className)) { return null; // Do not transform classes that are not in the classFilter } - SecurityControl securityControl = getSecurityControl(className); - if (securityControl == null) { + List match = getSecurityControl(className); + if (match == null || match.isEmpty()) { return null; // Do not transform classes that do not have a security control } ClassReader cr = new ClassReader(classfileBuffer); ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES); - ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, securityControl); + ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, match); cr.accept(cv, 0); return cw.toByteArray(); } - // TODO remove this and change structure to Map instead of List if this approach works - @Nullable - private SecurityControl getSecurityControl(final String className) { + private List getSecurityControl(final String className) { return securityControls.stream() .filter(sc -> sc.getClassName().equals(className)) - .findFirst() - .orElse(null); + .collect(Collectors.toList()); } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java index 987dd7ecd2e..520a6acb51b 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java @@ -3,6 +3,8 @@ import static org.objectweb.asm.Opcodes.ASM8; import datadog.trace.api.iast.securitycontrol.SecurityControl; +import java.util.List; +import javax.annotation.Nullable; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; import org.objectweb.asm.MethodVisitor; @@ -10,20 +12,31 @@ public class SecurityControlMethodClassVisitor extends ClassVisitor { - private final SecurityControl securityControl; + private final List securityControls; public SecurityControlMethodClassVisitor( - final ClassWriter cw, final SecurityControl securityControl) { + final ClassWriter cw, final List securityControls) { super(ASM8, cw); - this.securityControl = securityControl; + this.securityControls = securityControls; } @Override + @Nullable public MethodVisitor visitMethod( int access, String name, String desc, String signature, String[] exceptions) { MethodVisitor mv = super.visitMethod(access, name, desc, signature, exceptions); - if (mv != null && shouldBeAdapted(name, desc)) { - mv = new SecurityControlMethodAdapter(mv, securityControl, desc); + if (mv == null) { + return null; + } + SecurityControl match = null; + for (SecurityControl securityControl : securityControls) { + if (shouldBeAdapted(securityControl, name, desc)) { + match = securityControl; + break; + } + } + if (match != null) { + return new SecurityControlMethodAdapter(mv, match, desc); } return mv; } @@ -32,7 +45,7 @@ public void visitEnd() { cv.visitEnd(); } - private boolean shouldBeAdapted(String name, String desc) { + private boolean shouldBeAdapted(SecurityControl securityControl, String name, String desc) { if (!securityControl.getMethod().equals(name)) { return false; From 82bb7df802495822daaaaf1ad3a1afe608c5cd91 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 25 Nov 2024 10:22:11 +0100 Subject: [PATCH 16/38] change array for list --- .../securitycontrol/SecurityControlMethodClassVisitor.java | 4 ++-- .../trace/api/iast/securitycontrol/SecurityControl.java | 7 ++++--- .../api/iast/securitycontrol/SecurityControlFormatter.java | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java index 520a6acb51b..ba8d07cf477 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java @@ -56,12 +56,12 @@ private boolean shouldBeAdapted(SecurityControl securityControl, String name, St } Type[] types = Type.getArgumentTypes(desc); - if (types.length != securityControl.getParameterTypes().length) { + if (types.length != securityControl.getParameterTypes().size()) { return false; } for (int i = 0; i < types.length; i++) { - if (!types[i].getClassName().equals(securityControl.getParameterTypes()[i])) { + if (!types[i].getClassName().equals(securityControl.getParameterTypes().get(i))) { return false; } } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java index ada17d7e432..f991baaa25a 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java @@ -1,5 +1,6 @@ package datadog.trace.api.iast.securitycontrol; +import java.util.List; import java.util.Set; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -14,7 +15,7 @@ public class SecurityControl { @Nonnull private String method; - @Nullable private String[] parameterTypes; + @Nullable private List parameterTypes; @Nullable private Set parametersToMark; @@ -23,7 +24,7 @@ public SecurityControl( int marks, @Nonnull String className, @Nonnull String method, - @Nullable String[] parameterTypes, + @Nullable List parameterTypes, @Nullable Set parametersToMark) { this.type = type; this.marks = marks; @@ -54,7 +55,7 @@ public String getMethod() { } @Nullable - public String[] getParameterTypes() { + public List getParameterTypes() { return parameterTypes; } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index 3a5b41b4941..bbc16ce5cf9 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -70,7 +70,7 @@ private static SecurityControl getSecurityControl(@Nonnull final String config) String className = split[2].replaceAll("\\.", "/"); String method = split[3]; - String[] parameterTypes = null; + List parameterTypes = null; Set parametersToMark = null; if (split.length > 4) { @@ -83,7 +83,7 @@ private static SecurityControl getSecurityControl(@Nonnull final String config) } parametersToMark = getParametersToMark(elements); } else { - parameterTypes = elements; + parameterTypes = Arrays.asList(elements); } } } From 7d54511d28396a5fff34ee573b557fd5590b4aac Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 25 Nov 2024 10:51:46 +0100 Subject: [PATCH 17/38] fix test --- .../iast/securitycontrol/SecurityControlFormatterTest.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy index d6908e3f5c0..5d74b0d1d56 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy @@ -140,17 +140,17 @@ class SecurityControlFormatterTest extends DDSpecification{ setup: final formatter = new SecurityControlFormatter() Throwable thrown = null + def result = null when: try { - final result = formatter.format(config) + result = formatter.format(config) } catch (Throwable t) { thrown = t } then: thrown == null - final result = formatter.format(config) result == null where: From 851687142d33d150ad80b63fb36f490c18ad1e13 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 25 Nov 2024 12:45:57 +0100 Subject: [PATCH 18/38] Add unit test for IastSecurityControlTransformer --- .../IastSecurityControlTransformerTest.groovy | 60 +++++++++++++++++++ .../SecurityControlTestSuite.java | 24 ++++++++ 2 files changed, 84 insertions(+) create mode 100644 dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy create mode 100644 dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy new file mode 100644 index 00000000000..cf8dc31e737 --- /dev/null +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy @@ -0,0 +1,60 @@ +package com.datadog.iast.securitycontrol + +import datadog.trace.api.iast.InstrumentationBridge +import datadog.trace.api.iast.VulnerabilityMarks +import datadog.trace.api.iast.propagation.PropagationModule +import datadog.trace.api.iast.securitycontrol.SecurityControl +import datadog.trace.api.iast.securitycontrol.SecurityControlFormatter +import datadog.trace.test.util.DDSpecification +import foo.bar.securitycontrol.SecurityControlTestSuite +import net.bytebuddy.agent.ByteBuddyAgent + +import java.lang.instrument.Instrumentation + +class IastSecurityControlTransformerTest extends DDSpecification{ + + def setupSpec() { + final config = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitize;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateAll;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validate:1,2;java.lang.Object,java.lang.String,java.lang.String' + Instrumentation instrumentation = ByteBuddyAgent.install() + List securityControls = + SecurityControlFormatter.format(config) + assert securityControls != null + instrumentation.addTransformer(new IastSecurityControlTransformer(securityControls), true) + } + + + void 'test sanitize'(){ + given: + final iastModule = Mock(PropagationModule) + InstrumentationBridge.registerIastModule(iastModule) + final marks = VulnerabilityMarks.XSS_MARK + + when: + final result = SecurityControlTestSuite.sanitize('test') + + then: + 1 * iastModule.markIfTainted('Sanitized test', marks) + 0 * _ + } + + void 'test validate'(){ + given: + final iastModule = Mock(PropagationModule) + InstrumentationBridge.registerIastModule(iastModule) + final marks = VulnerabilityMarks.XSS_MARK + + when: + SecurityControlTestSuite.&"$method".call(args) + + then: + expected * iastModule.markIfTainted(toValidate, marks) + 0 * _ + + where: + method | args | toValidate | expected + 'validateAll' | ['test'] | 'test' | 1 + 'validateAll' | ['test1', "test2"] | _ | 2 + 'validate' | ['test'] | 'test' | 0 + 'validate' | [new Object(), 'test1', "test2"] | _ | 2 + } +} diff --git a/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java new file mode 100644 index 00000000000..1938b9030b6 --- /dev/null +++ b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java @@ -0,0 +1,24 @@ +package foo.bar.securitycontrol; + +public class SecurityControlTestSuite { + + public static boolean validateAll(String input) { + return true; // dummy implementation + } + + public static boolean validateAll(String input, String input2) { + return true; // dummy implementation + } + + public static boolean validate(String input) { + return true; // dummy implementation + } + + public static boolean validate(Object o, String input, String input2) { + return true; // dummy implementation + } + + public static String sanitize(String input) { + return "Sanitized " + input; + } +} From 2aa1c7206e03d350e9507223296ebfacd7a4800e Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 25 Nov 2024 13:18:00 +0100 Subject: [PATCH 19/38] improve test --- .../IastSecurityControlTransformerTest.groovy | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy index cf8dc31e737..874cb413d5f 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy @@ -47,14 +47,16 @@ class IastSecurityControlTransformerTest extends DDSpecification{ SecurityControlTestSuite.&"$method".call(args) then: - expected * iastModule.markIfTainted(toValidate, marks) + for (final validate : toValidate){ + expected * iastModule.markIfTainted(validate, marks) + } 0 * _ where: - method | args | toValidate | expected - 'validateAll' | ['test'] | 'test' | 1 - 'validateAll' | ['test1', "test2"] | _ | 2 - 'validate' | ['test'] | 'test' | 0 - 'validate' | [new Object(), 'test1', "test2"] | _ | 2 + method | args | toValidate | expected + 'validateAll' | ['test'] | [args[0]] | 1 + 'validateAll' | ['test1', "test2"] | [args[0], args[1]] | 1 + 'validate' | ['test'] | args[0] | 0 + 'validate' | [new Object(), 'test1', "test2"] | [args[1], args[2]] | 1 } } From 21d7ed1488880977f9d999f9be74b1842ca40f1d Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 25 Nov 2024 13:40:10 +0100 Subject: [PATCH 20/38] fix onUnexpectedException message --- .../trace/api/iast/securitycontrol/SecurityControlHelper.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlHelper.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlHelper.java index 3266f261ce2..babf9b20edc 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlHelper.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlHelper.java @@ -13,7 +13,7 @@ public static void setSecureMarks(final Object target, int marks) { module.markIfTainted(target, marks); } } catch (final Throwable e) { - module.onUnexpectedException("afterRepeat threw", e); + module.onUnexpectedException("setSecureMarks threw", e); } } } From e6225ec774dc3bedf7ecba61fd1a403d18e18b67 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 26 Nov 2024 09:32:49 +0100 Subject: [PATCH 21/38] fix codenarc --- .../securitycontrol/IastSecurityControlTransformerTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy index 874cb413d5f..329a7e393fd 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy @@ -30,7 +30,7 @@ class IastSecurityControlTransformerTest extends DDSpecification{ final marks = VulnerabilityMarks.XSS_MARK when: - final result = SecurityControlTestSuite.sanitize('test') + SecurityControlTestSuite.sanitize('test') then: 1 * iastModule.markIfTainted('Sanitized test', marks) From 867ab0f8c9581db9a9894648e97d29f11c6585e4 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 26 Nov 2024 09:37:04 +0100 Subject: [PATCH 22/38] change to forked tests --- ...t.groovy => IastSecurityControlTransformerForkedTest.groovy} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/{IastSecurityControlTransformerTest.groovy => IastSecurityControlTransformerForkedTest.groovy} (96%) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy similarity index 96% rename from dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy rename to dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy index 329a7e393fd..24ad8908e46 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy @@ -11,7 +11,7 @@ import net.bytebuddy.agent.ByteBuddyAgent import java.lang.instrument.Instrumentation -class IastSecurityControlTransformerTest extends DDSpecification{ +class IastSecurityControlTransformerForkedTest extends DDSpecification{ def setupSpec() { final config = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitize;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateAll;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validate:1,2;java.lang.Object,java.lang.String,java.lang.String' From 800cc0c3bdf3576dbbe048747e4922063d206242 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 26 Nov 2024 09:54:03 +0100 Subject: [PATCH 23/38] fix spotless --- .../IastSecurityControlTransformerForkedTest.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy index 24ad8908e46..4b994807bc5 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy @@ -30,7 +30,7 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ final marks = VulnerabilityMarks.XSS_MARK when: - SecurityControlTestSuite.sanitize('test') + SecurityControlTestSuite.sanitize('test') then: 1 * iastModule.markIfTainted('Sanitized test', marks) From 1c0221875ef3be1a2ba60031e95a0738d278832d Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 26 Nov 2024 14:39:02 +0100 Subject: [PATCH 24/38] fix coverage and tests --- .../IastSecurityControlTransformerForkedTest.groovy | 6 +++--- .../datadog/smoketest/AbstractIastSpringBootTest.groovy | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy index 4b994807bc5..6ce340a7246 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy @@ -14,7 +14,7 @@ import java.lang.instrument.Instrumentation class IastSecurityControlTransformerForkedTest extends DDSpecification{ def setupSpec() { - final config = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitize;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateAll;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validate:1,2;java.lang.Object,java.lang.String,java.lang.String' + final config = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitize;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateAll;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validate:java.lang.Object,java.lang.String,java.lang.String:1,2' Instrumentation instrumentation = ByteBuddyAgent.install() List securityControls = SecurityControlFormatter.format(config) @@ -55,8 +55,8 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ where: method | args | toValidate | expected 'validateAll' | ['test'] | [args[0]] | 1 - 'validateAll' | ['test1', "test2"] | [args[0], args[1]] | 1 + 'validateAll' | ['test1', 'test2'] | [args[0], args[1]] | 1 'validate' | ['test'] | args[0] | 0 - 'validate' | [new Object(), 'test1', "test2"] | [args[1], args[2]] | 1 + 'validate' | [new Object(), 'test1', 'test2'] | [args[1], args[2]] | 1 } } diff --git a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy index dd4c243e73e..418a79beea4 100644 --- a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy +++ b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy @@ -38,7 +38,7 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { withSystemProperty(IAST_ENABLED, true), withSystemProperty(IAST_DETECTION_MODE, 'FULL'), withSystemProperty(IAST_DEBUG_ENABLED, true), - withSystemProperty(IAST_SECURITY_CONTROLS_CONFIGURATION, "SANITIZER:XSS:ddtest.securitycontrols.Sanitizer:sanitize;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validateAll;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validate:1,2;java.lang.Object,java.lang.String,java.lang.String"), + withSystemProperty(IAST_SECURITY_CONTROLS_CONFIGURATION, "SANITIZER:XSS:ddtest.securitycontrols.Sanitizer:sanitize;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validateAll;INPUT_VALIDATOR:XSS:ddtest.securitycontrols.InputValidator:validate:java.lang.Object,java.lang.String,java.lang.String:1,2"), ] } From 963f7c8edc7cb90cd205e2435dc35d53392d8b12 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 26 Nov 2024 15:07:35 +0100 Subject: [PATCH 25/38] fix secure marks in formatter --- ...stSecurityControlTransformerForkedTest.groovy | 4 ++-- .../SecurityControlFormatter.java | 4 ++-- .../SecurityControlFormatterTest.groovy | 16 ++++++++-------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy index 6ce340a7246..03291479403 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy @@ -27,7 +27,7 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ given: final iastModule = Mock(PropagationModule) InstrumentationBridge.registerIastModule(iastModule) - final marks = VulnerabilityMarks.XSS_MARK + final marks = (VulnerabilityMarks.XSS_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) when: SecurityControlTestSuite.sanitize('test') @@ -41,7 +41,7 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ given: final iastModule = Mock(PropagationModule) InstrumentationBridge.registerIastModule(iastModule) - final marks = VulnerabilityMarks.XSS_MARK + final marks = (VulnerabilityMarks.XSS_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) when: SecurityControlTestSuite.&"$method".call(args) diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index bbc16ce5cf9..2c13bcddd07 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -1,6 +1,6 @@ package datadog.trace.api.iast.securitycontrol; -import static datadog.trace.api.iast.VulnerabilityMarks.NOT_MARKED; +import static datadog.trace.api.iast.VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK; import datadog.trace.api.iast.VulnerabilityMarks; import de.thetaphi.forbiddenapis.SuppressForbidden; @@ -100,7 +100,7 @@ private static int getMarks(String s) { return VulnerabilityMarks.markForAll(); } String[] elements = s.split(SECURITY_CONTROL_ELEMENT_DELIMITER); - int marks = NOT_MARKED; + int marks = CUSTOM_SECURITY_CONTROL_MARK; for (String element : elements) { marks |= VulnerabilityMarks.getMarkFromVulnerabitityType(element); } diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy index 5d74b0d1d56..4a6a07664e2 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy @@ -15,7 +15,7 @@ class SecurityControlFormatterTest extends DDSpecification{ result.size() == 1 def securityControl = result.get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR - securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == null @@ -32,7 +32,7 @@ class SecurityControlFormatterTest extends DDSpecification{ result.size() == 1 def securityControl = result.get(0) securityControl.getType() == SecurityControlType.SANITIZER - securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomSanitizer" securityControl.getMethod() == "sanitize" securityControl.getParameterTypes() == null @@ -49,7 +49,7 @@ class SecurityControlFormatterTest extends DDSpecification{ result.size() == 2 def inputValidator = result.get(0) inputValidator.getType() == SecurityControlType.INPUT_VALIDATOR - inputValidator.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + inputValidator.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) inputValidator.getClassName() == "bar/foo/CustomInputValidator" inputValidator.getMethod() == "validate" inputValidator.getParameterTypes() == null @@ -57,7 +57,7 @@ class SecurityControlFormatterTest extends DDSpecification{ def sanitizer = result.get(1) sanitizer.getType() == SecurityControlType.SANITIZER - sanitizer.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + sanitizer.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) sanitizer.getClassName() == "bar/foo/CustomSanitizer" sanitizer.getMethod() == "sanitize" sanitizer.getParameterTypes() == null @@ -74,7 +74,7 @@ class SecurityControlFormatterTest extends DDSpecification{ result.size() == 1 def securityControl = result.get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR - securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.SQL_INJECTION_MARK) + securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.SQL_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == null @@ -91,7 +91,7 @@ class SecurityControlFormatterTest extends DDSpecification{ result.size() == 1 def securityControl = result.get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR - securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == ["java.lang.Object", "java.lang.String", "java.lang.String"] @@ -108,7 +108,7 @@ class SecurityControlFormatterTest extends DDSpecification{ result.size() == 1 def securityControl = result.get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR - securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == null @@ -127,7 +127,7 @@ class SecurityControlFormatterTest extends DDSpecification{ result.size() == 1 def securityControl = result.get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR - securityControl.getMarks() == VulnerabilityMarks.COMMAND_INJECTION_MARK + securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == ["java.lang.Object", "java.lang.String", "java.lang.String"] From 69a34665827a275e029859678e49a353e0a68f08 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 12 Dec 2024 19:15:23 +0100 Subject: [PATCH 26/38] Add more tests --- ...ecurityControlTransformerForkedTest.groovy | 34 +++++++++++--- .../SecurityControlTestSuite.java | 46 ++++++++++++++++++- 2 files changed, 72 insertions(+), 8 deletions(-) diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy index 03291479403..bf3dee019be 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy @@ -13,8 +13,18 @@ import java.lang.instrument.Instrumentation class IastSecurityControlTransformerForkedTest extends DDSpecification{ + private static final String SANITIZER = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitize' + private static final String SANITIZE_VALIDATE_OBJECT = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeObject' + private static final String SANITIZE_INPUTS= 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeInputs' + private static final String SANITIZE_MANY_INPUTS= 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeManyInputs' + private static final String INPUT_VALIDATOR_VALIDATE_ALL = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateAll' + private static final String INPUT_VALIDATOR_VALIDATE_OVERLOADED = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validate:java.lang.Object,java.lang.String,java.lang.String:1,2' + private static final String INPUT_VALIDATOR_VALIDATE_RETURNING_INT = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateReturningInt' + private static final String INPUT_VALIDATOR_VALIDATE_OBJECT = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateObject' + + def setupSpec() { - final config = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitize;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateAll;INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validate:java.lang.Object,java.lang.String,java.lang.String:1,2' + final config = "${SANITIZER};${SANITIZE_VALIDATE_OBJECT};${SANITIZE_INPUTS};${SANITIZE_MANY_INPUTS};${INPUT_VALIDATOR_VALIDATE_ALL};${INPUT_VALIDATOR_VALIDATE_OVERLOADED};${INPUT_VALIDATOR_VALIDATE_RETURNING_INT};${INPUT_VALIDATOR_VALIDATE_OBJECT}" Instrumentation instrumentation = ByteBuddyAgent.install() List securityControls = SecurityControlFormatter.format(config) @@ -30,11 +40,18 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ final marks = (VulnerabilityMarks.XSS_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) when: - SecurityControlTestSuite.sanitize('test') + SecurityControlTestSuite.&"$method".call(*args) then: - 1 * iastModule.markIfTainted('Sanitized test', marks) + 1 * iastModule.markIfTainted( 'Sanitized', marks) 0 * _ + + where: + method | args + 'sanitize' | ['test'] + 'sanitizeObject' | ['test'] + 'sanitizeInputs' | ['test1', new Object(), 27i] + 'sanitizeManyInputs' | ['test', 'test2', 'test3', 'test4', 'test5', 'test6', 'test7', 'test8', 'test9', 'test10'] } void 'test validate'(){ @@ -44,7 +61,7 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ final marks = (VulnerabilityMarks.XSS_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) when: - SecurityControlTestSuite.&"$method".call(args) + SecurityControlTestSuite.&"$method".call(*args) then: for (final validate : toValidate){ @@ -54,9 +71,12 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ where: method | args | toValidate | expected - 'validateAll' | ['test'] | [args[0]] | 1 - 'validateAll' | ['test1', 'test2'] | [args[0], args[1]] | 1 - 'validate' | ['test'] | args[0] | 0 + 'validateAll' | ['test'] | args | 1 + 'validateAll' | ['test1', 'test2'] | args | 1 + 'validateAll' | ['test', 'test2', 'test3', 'test4', 'test5', 'test6', 'test7', 'test8', 'test9', 'test10'] | args | 1 + 'validate' | ['test'] | args | 0 'validate' | [new Object(), 'test1', 'test2'] | [args[1], args[2]] | 1 + 'validateReturningInt' | ['test'] | args | 1 + 'validateObject' | [new Object()] | args | 1 } } diff --git a/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java index 1938b9030b6..ff6d0565d57 100644 --- a/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java +++ b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java @@ -10,6 +10,20 @@ public static boolean validateAll(String input, String input2) { return true; // dummy implementation } + public static boolean validateAll( + String input, + String input2, + String input3, + String input4, + String input5, + String input6, + String input7, + String input8, + String input9, + String input10) { + return true; // dummy implementation + } + public static boolean validate(String input) { return true; // dummy implementation } @@ -18,7 +32,37 @@ public static boolean validate(Object o, String input, String input2) { return true; // dummy implementation } + public static int validateReturningInt(String input) { + return 1; // dummy implementation + } + + public static int validateObject(Object input) { + return 1; // dummy implementation + } + public static String sanitize(String input) { - return "Sanitized " + input; + return "Sanitized"; + } + + public static Object sanitizeObject(String input) { + return "Sanitized"; + } + + public static String sanitizeInputs(String input, Object input2, int input3) { + return "Sanitized"; + } + + public static String sanitizeManyInputs( + String input, + String input2, + String input3, + String input4, + String input5, + String input6, + String input7, + String input8, + String input9, + String input10) { + return "Sanitized"; } } From dfef84311132edef747d59897bb6ec297dd77ffb Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 12 Dec 2024 20:37:55 +0100 Subject: [PATCH 27/38] upgrade Opcodes.ASM9 --- .../iast/securitycontrol/SecurityControlMethodAdapter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java index fc92550b2a7..ab302460087 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java @@ -18,7 +18,7 @@ public class SecurityControlMethodAdapter extends MethodVisitor { public SecurityControlMethodAdapter( final MethodVisitor mv, final SecurityControl securityControl, final String desc) { - super(Opcodes.ASM8, mv); + super(Opcodes.ASM9, mv); this.mv = mv; this.securityControl = securityControl; this.desc = desc; From 7c7e642d36824a42a4a80380218e02943a1882dd Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 16 Dec 2024 18:04:16 +0100 Subject: [PATCH 28/38] remove unnecessary change --- .../groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy index 418a79beea4..55902d45f33 100644 --- a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy +++ b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy @@ -1,6 +1,5 @@ package datadog.smoketest -import static datadog.trace.api.config.IastConfig.IAST_DEBUG_ENABLED import okhttp3.FormBody import okhttp3.MediaType import okhttp3.MultipartBody @@ -8,9 +7,9 @@ import okhttp3.Request import okhttp3.RequestBody import okhttp3.Response +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_SECURITY_CONTROLS_ENABLED import static datadog.trace.api.config.IastConfig.IAST_SECURITY_CONTROLS_CONFIGURATION abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { From 722e3f6138e8480a41ad8feaf4d6d7c535760d6e Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 16 Dec 2024 18:18:04 +0100 Subject: [PATCH 29/38] rename endpoint --- .../datadog/smoketest/springboot/controller/XssController.java | 2 +- .../groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java index d7087339a2f..32cad411098 100644 --- a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java +++ b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/XssController.java @@ -215,7 +215,7 @@ public void validateAll(final HttpServletRequest request, final HttpServletRespo } } - @GetMapping("/validate2") + @GetMapping("/validateAll2") @SuppressFBWarnings public void validate2(final HttpServletRequest request, final HttpServletResponse response) { try { diff --git a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy index 55902d45f33..bcd17d182d0 100644 --- a/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy +++ b/dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy @@ -1220,7 +1220,7 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { method | _ 'sanitize' | _ 'validateAll' | _ - 'validate2' | _ + 'validateAll2' | _ 'validate' | _ } From fff5baf3deb4604786735553ab7555765f641c9f Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 17 Dec 2024 12:44:33 +0100 Subject: [PATCH 30/38] Add safeguard to security control class transform --- .../IastSecurityControlTransformer.java | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java index da737badec0..4743ed841af 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java @@ -9,9 +9,14 @@ import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class IastSecurityControlTransformer implements ClassFileTransformer { + private static final Logger LOGGER = + LoggerFactory.getLogger(IastSecurityControlTransformer.class); + private final List securityControls; private final Set classFilter; @@ -36,11 +41,16 @@ public byte[] transform( if (match == null || match.isEmpty()) { return null; // Do not transform classes that do not have a security control } - ClassReader cr = new ClassReader(classfileBuffer); - ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES); - ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, match); - cr.accept(cv, 0); - return cw.toByteArray(); + try { + ClassReader cr = new ClassReader(classfileBuffer); + ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES); + ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, match); + cr.accept(cv, 0); + return cw.toByteArray(); + } catch (Exception e) { + LOGGER.warn("Failed to transform class: {}", className, e); + return null; + } } private List getSecurityControl(final String className) { From fab351a4e26f725b0a3a8f85b20cadb7c8e37204 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 17 Dec 2024 19:10:20 +0100 Subject: [PATCH 31/38] Fix method adapter add logs and more test cases --- .../IastSecurityControlTransformer.java | 2 +- .../SecurityControlMethodAdapter.java | 69 +++++++++++++++---- ...ecurityControlTransformerForkedTest.groovy | 43 ++++++++---- .../SecurityControlTestSuite.java | 32 +++++++++ .../iast/securitycontrol/SecurityControl.java | 20 ++++++ 5 files changed, 137 insertions(+), 29 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java index 4743ed841af..f75156e42d4 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java @@ -47,7 +47,7 @@ public byte[] transform( ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, match); cr.accept(cv, 0); return cw.toByteArray(); - } catch (Exception e) { + } catch (Throwable e) { LOGGER.warn("Failed to transform class: {}", className, e); return null; } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java index ab302460087..0b926e8dad1 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java @@ -5,9 +5,13 @@ import org.objectweb.asm.MethodVisitor; import org.objectweb.asm.Opcodes; import org.objectweb.asm.Type; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class SecurityControlMethodAdapter extends MethodVisitor { + private static final Logger LOGGER = LoggerFactory.getLogger(SecurityControlMethodAdapter.class); + public static final String HELPER = "datadog/trace/api/iast/securitycontrol/SecurityControlHelper"; public static final String METHOD = "setSecureMarks"; @@ -29,26 +33,59 @@ public void visitInsn(int opcode) { // Check if the opcode is a return instruction if (opcode >= Opcodes.IRETURN && opcode <= Opcodes.RETURN) { if (securityControl.getType() == SecurityControlType.INPUT_VALIDATOR) { - boolean allParameters = securityControl.getParametersToMark() == null; - Type[] types = Type.getArgumentTypes(desc); - for (int i = 0; i < types.length; i++) { - if (allParameters || securityControl.getParametersToMark().contains(i)) { - callInputValidation(i); - } - } + processInputValidator(); } else { // SecurityControlType.SANITIZER - // Duplicate the return value on the stack - mv.visitInsn(Opcodes.DUP); - // Load the marks from securityControl onto the stack - mv.visitLdcInsn(securityControl.getMarks()); - // Insert the call to setSecureMarks with the return value and marks as parameters - mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); + processSanitizer(); } } super.visitInsn(opcode); } + private void processSanitizer() { + Type returnType = Type.getReturnType(desc); + if (isPrimitive(returnType)) { + // no need to check primitives as we are not tainting them + LOGGER.warn( + "Sanitizers should not be used on non-primitive return types. Return type {}. Security control: {}", + returnType.getClassName(), + securityControl); + return; + } + // Duplicate the return value on the stack + mv.visitInsn(Opcodes.DUP); + // Load the marks from securityControl onto the stack + mv.visitLdcInsn(securityControl.getMarks()); + // Insert the call to setSecureMarks with the return value and marks as parameters + mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); + } + + private void processInputValidator() { + boolean allParameters = securityControl.getParametersToMark() == null; + Type[] types = Type.getArgumentTypes(desc); + int parametersCount = 0; + for (int i = 0; i < types.length; i++) { + Type type = types[i]; + boolean isPrimitive = isPrimitive(type); + if (allParameters) { + if (!isPrimitive) { + callInputValidation(parametersCount); + } + } else if (securityControl.getParametersToMark().contains(i)) { + if (isPrimitive) { + LOGGER.warn( + "Input validators should not be used on primitive types. Parameter {} with type {} .Security control: {}", + i, + type.getClassName(), + securityControl); + } else { + callInputValidation(parametersCount); + } + } + parametersCount += types[i].getSize(); + } + } + private void callInputValidation(int i) { // Duplicate the parameter value on the stack mv.visitVarInsn(Opcodes.ALOAD, i); @@ -57,4 +94,10 @@ private void callInputValidation(int i) { // Insert the call to setSecureMarks with the parameter value and marks as parameters mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); } + + private static boolean isPrimitive(Type type) { + // Check if is a primitive type + int sort = type.getSort(); + return sort >= Type.BOOLEAN && sort <= Type.DOUBLE; + } } diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy index bf3dee019be..868e0aa74d0 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy @@ -17,14 +17,19 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ private static final String SANITIZE_VALIDATE_OBJECT = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeObject' private static final String SANITIZE_INPUTS= 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeInputs' private static final String SANITIZE_MANY_INPUTS= 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeManyInputs' + private static final String SANITIZE_INT = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeInt' + private static final String SANITIZE_LONG = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeLong' private static final String INPUT_VALIDATOR_VALIDATE_ALL = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateAll' private static final String INPUT_VALIDATOR_VALIDATE_OVERLOADED = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validate:java.lang.Object,java.lang.String,java.lang.String:1,2' private static final String INPUT_VALIDATOR_VALIDATE_RETURNING_INT = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateReturningInt' private static final String INPUT_VALIDATOR_VALIDATE_OBJECT = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateObject' + private static final String INPUT_VALIDATOR_VALIDATE_LONG = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateLong' + private static final String INPUT_VALIDATOR_VALIDATE_SELECTED_LONG = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:validateSelectedLong:0' + def setupSpec() { - final config = "${SANITIZER};${SANITIZE_VALIDATE_OBJECT};${SANITIZE_INPUTS};${SANITIZE_MANY_INPUTS};${INPUT_VALIDATOR_VALIDATE_ALL};${INPUT_VALIDATOR_VALIDATE_OVERLOADED};${INPUT_VALIDATOR_VALIDATE_RETURNING_INT};${INPUT_VALIDATOR_VALIDATE_OBJECT}" + final config = "${SANITIZER};${SANITIZE_VALIDATE_OBJECT};${SANITIZE_INPUTS};${SANITIZE_MANY_INPUTS};${SANITIZE_INT};${SANITIZE_LONG};${INPUT_VALIDATOR_VALIDATE_ALL};${INPUT_VALIDATOR_VALIDATE_OVERLOADED};${INPUT_VALIDATOR_VALIDATE_RETURNING_INT};${INPUT_VALIDATOR_VALIDATE_OBJECT};${INPUT_VALIDATOR_VALIDATE_LONG};${INPUT_VALIDATOR_VALIDATE_SELECTED_LONG}" Instrumentation instrumentation = ByteBuddyAgent.install() List securityControls = SecurityControlFormatter.format(config) @@ -43,15 +48,17 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ SecurityControlTestSuite.&"$method".call(*args) then: - 1 * iastModule.markIfTainted( 'Sanitized', marks) + expected * iastModule.markIfTainted( toSanitize, marks) 0 * _ where: - method | args - 'sanitize' | ['test'] - 'sanitizeObject' | ['test'] - 'sanitizeInputs' | ['test1', new Object(), 27i] - 'sanitizeManyInputs' | ['test', 'test2', 'test3', 'test4', 'test5', 'test6', 'test7', 'test8', 'test9', 'test10'] + method | args | toSanitize | expected + 'sanitize' | ['test'] | 'Sanitized' | 1 + 'sanitizeObject' | ['test'] | 'Sanitized' | 1 + 'sanitizeInputs' | ['test1', new Object(), 27i] | 'Sanitized' | 1 + 'sanitizeManyInputs' | ['test', 'test2', 'test3', 'test4', 'test5', 'test6', 'test7', 'test8', 'test9', 'test10'] | 'Sanitized' | 1 + 'sanitizeInt' | [1i] | args | 0 + 'sanitizeLong' | [1L] | args | 0 } void 'test validate'(){ @@ -70,13 +77,19 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ 0 * _ where: - method | args | toValidate | expected - 'validateAll' | ['test'] | args | 1 - 'validateAll' | ['test1', 'test2'] | args | 1 - 'validateAll' | ['test', 'test2', 'test3', 'test4', 'test5', 'test6', 'test7', 'test8', 'test9', 'test10'] | args | 1 - 'validate' | ['test'] | args | 0 - 'validate' | [new Object(), 'test1', 'test2'] | [args[1], args[2]] | 1 - 'validateReturningInt' | ['test'] | args | 1 - 'validateObject' | [new Object()] | args | 1 + method | args | toValidate | expected + 'validateAll' | ['test'] | args | 1 + 'validateAll' | ['test1', 'test2'] | args | 1 + 'validateAll' | [1L, 'test2'] | [args[1]] | 1 + 'validateAll' | ['test', 'test2', 'test3', 'test4', 'test5', 'test6', 'test7', 'test8', 'test9', 'test10'] | args | 1 + 'validate' | ['test'] | args | 0 + 'validate' | [new Object(), 'test1', 'test2'] | [args[1], args[2]] | 1 + 'validateReturningInt' | ['test'] | args | 1 + 'validateObject' | [new Object()] | args | 1 + 'validateLong' | [1L, 'test2'] | [args[1]] | 1 + 'validateLong' | ['test2', 2L] | [args[0]] | 1 + 'validateLong' | [1L, 'test2', 2L] | [args[1]] | 1 + 'validateSelectedLong' | [1L] | args | 0 + 'validateSelectedLong' | [1L, 2L] | [args[0]] | 0 } } diff --git a/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java index ff6d0565d57..d53a8e19de0 100644 --- a/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java +++ b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java @@ -2,6 +2,10 @@ public class SecurityControlTestSuite { + public static boolean validateAll(long input, String input2) { + return true; // dummy implementation + } + public static boolean validateAll(String input) { return true; // dummy implementation } @@ -24,6 +28,26 @@ public static boolean validateAll( return true; // dummy implementation } + public static boolean validateLong(long input, String input2) { + return true; // dummy implementation + } + + public static boolean validateLong(String input, long input2) { + return true; // dummy implementation + } + + public static boolean validateLong(long intput1, String input2, long input3) { + return true; // dummy implementation + } + + public static boolean validateSelectedLong(long intput1) { + return true; // dummy implementation + } + + public static boolean validateSelectedLong(long input1, long intput2) { + return true; // dummy implementation + } + public static boolean validate(String input) { return true; // dummy implementation } @@ -65,4 +89,12 @@ public static String sanitizeManyInputs( String input10) { return "Sanitized"; } + + public static int sanitizeInt(int input) { + return input; + } + + public static long sanitizeLong(long input) { + return input; + } } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java index f991baaa25a..24ecd4dc654 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java @@ -63,4 +63,24 @@ public List getParameterTypes() { public Set getParametersToMark() { return parametersToMark; } + + @Override + public String toString() { + return "SecurityControl{" + + "type=" + + type + + ", marks=" + + marks + + ", className='" + + className + + '\'' + + ", method='" + + method + + '\'' + + ", parameterTypes=" + + parameterTypes + + ", parametersToMark=" + + parametersToMark + + '}'; + } } From dee1faf71b2761f082d3ec1a5af9159f5f00f078 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 18 Dec 2024 10:22:29 +0100 Subject: [PATCH 32/38] Fix jacoco --- internal-api/build.gradle | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal-api/build.gradle b/internal-api/build.gradle index 55528d74d39..96c7912e542 100644 --- a/internal-api/build.gradle +++ b/internal-api/build.gradle @@ -191,7 +191,8 @@ excludedClassesCoverage += [ 'datadog.trace.util.stacktrace.StackTraceEvent', 'datadog.trace.util.stacktrace.StackTraceFrame', 'datadog.trace.api.iast.VulnerabilityMarks', - 'datadog.trace.api.iast.securitycontrol.SecurityControlHelper' + 'datadog.trace.api.iast.securitycontrol.SecurityControlHelper', + 'datadog.trace.api.iast.securitycontrol.SecurityControl' ] excludedClassesBranchCoverage = [ 'datadog.trace.api.ProductActivationConfig', From 7411d9be2016c1f99daae94e28cb29cb68954a46 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 19 Dec 2024 13:00:27 +0100 Subject: [PATCH 33/38] changes suggested by manu --- .../java/com/datadog/iast/IastSystem.java | 3 +- .../IastSecurityControlTransformer.java | 25 ++--- .../SecurityControlMethodAdapter.java | 46 ++++---- .../SecurityControlMethodClassVisitor.java | 4 +- ...ecurityControlTransformerForkedTest.groovy | 46 +++++++- .../SecurityControlStaticTestSuite.java | 100 ++++++++++++++++++ .../SecurityControlTestSuite.java | 38 +++---- .../iast/securitycontrol/SecurityControl.java | 8 +- .../SecurityControlFormatter.java | 24 +++-- .../SecurityControlFormatterTest.groovy | 32 +++--- 10 files changed, 239 insertions(+), 87 deletions(-) create mode 100644 dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlStaticTestSuite.java diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java index 69842235d3d..118be0e231b 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/IastSystem.java @@ -58,6 +58,7 @@ import java.lang.reflect.Constructor; import java.lang.reflect.UndeclaredThrowableException; import java.util.List; +import java.util.Map; import java.util.function.BiFunction; import java.util.function.Supplier; import java.util.stream.Stream; @@ -129,7 +130,7 @@ private static void maybeApplySecurityControls(@Nullable Instrumentation instrum if (Config.get().getIastSecurityControlsConfiguration() == null || instrumentation == null) { return; } - List securityControls = + Map> securityControls = SecurityControlFormatter.format(Config.get().getIastSecurityControlsConfiguration()); if (securityControls == null) { LOGGER.warn("No security controls to apply"); diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java index f75156e42d4..d3d0b8ef247 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java @@ -1,10 +1,12 @@ package com.datadog.iast.securitycontrol; +import static org.objectweb.asm.ClassReader.SKIP_DEBUG; +import static org.objectweb.asm.ClassReader.SKIP_FRAMES; + import datadog.trace.api.iast.securitycontrol.SecurityControl; import java.lang.instrument.ClassFileTransformer; import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; +import java.util.Map; import javax.annotation.Nullable; import org.objectweb.asm.ClassReader; import org.objectweb.asm.ClassVisitor; @@ -17,13 +19,10 @@ public class IastSecurityControlTransformer implements ClassFileTransformer { private static final Logger LOGGER = LoggerFactory.getLogger(IastSecurityControlTransformer.class); - private final List securityControls; - private final Set classFilter; + private final Map> securityControls; - public IastSecurityControlTransformer(List securityControls) { + public IastSecurityControlTransformer(Map> securityControls) { this.securityControls = securityControls; - this.classFilter = - securityControls.stream().map(SecurityControl::getClassName).collect(Collectors.toSet()); } @Override @@ -34,10 +33,10 @@ public byte[] transform( Class classBeingRedefined, java.security.ProtectionDomain protectionDomain, byte[] classfileBuffer) { - if (!classFilter.contains(className)) { + if (!securityControls.containsKey(className)) { return null; // Do not transform classes that are not in the classFilter } - List match = getSecurityControl(className); + List match = securityControls.get(className); if (match == null || match.isEmpty()) { return null; // Do not transform classes that do not have a security control } @@ -45,17 +44,11 @@ public byte[] transform( ClassReader cr = new ClassReader(classfileBuffer); ClassWriter cw = new ClassWriter(cr, ClassWriter.COMPUTE_FRAMES); ClassVisitor cv = new SecurityControlMethodClassVisitor(cw, match); - cr.accept(cv, 0); + cr.accept(cv, SKIP_DEBUG | SKIP_FRAMES); return cw.toByteArray(); } catch (Throwable e) { LOGGER.warn("Failed to transform class: {}", className, e); return null; } } - - private List getSecurityControl(final String className) { - return securityControls.stream() - .filter(sc -> sc.getClassName().equals(className)) - .collect(Collectors.toList()); - } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java index 0b926e8dad1..09a78a39469 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java @@ -19,39 +19,47 @@ public class SecurityControlMethodAdapter extends MethodVisitor { private final MethodVisitor mv; private final SecurityControl securityControl; private final String desc; + boolean isStatic; public SecurityControlMethodAdapter( - final MethodVisitor mv, final SecurityControl securityControl, final String desc) { + final MethodVisitor mv, + final SecurityControl securityControl, + final String desc, + boolean isStatic) { super(Opcodes.ASM9, mv); this.mv = mv; this.securityControl = securityControl; this.desc = desc; + this.isStatic = isStatic; + } + + @Override + public void visitCode() { + super.visitCode(); + if (securityControl.getType() == SecurityControlType.INPUT_VALIDATOR) { + processInputValidator(); + } } @Override public void visitInsn(int opcode) { - // Check if the opcode is a return instruction - if (opcode >= Opcodes.IRETURN && opcode <= Opcodes.RETURN) { - if (securityControl.getType() == SecurityControlType.INPUT_VALIDATOR) { - processInputValidator(); - } else { // SecurityControlType.SANITIZER + if (securityControl.getType() == SecurityControlType.SANITIZER) { + // Only process the return value if is an Object as we are not tainting primitives + if (opcode == Opcodes.ARETURN) { processSanitizer(); + } else { + Type returnType = Type.getReturnType(desc); + // no need to check primitives as we are not tainting them + LOGGER.warn( + "Sanitizers should not be used on primitive return types. Return type {}. Security control: {}", + returnType.getClassName(), + securityControl); } } - super.visitInsn(opcode); } private void processSanitizer() { - Type returnType = Type.getReturnType(desc); - if (isPrimitive(returnType)) { - // no need to check primitives as we are not tainting them - LOGGER.warn( - "Sanitizers should not be used on non-primitive return types. Return type {}. Security control: {}", - returnType.getClassName(), - securityControl); - return; - } // Duplicate the return value on the stack mv.visitInsn(Opcodes.DUP); // Load the marks from securityControl onto the stack @@ -71,7 +79,7 @@ private void processInputValidator() { if (!isPrimitive) { callInputValidation(parametersCount); } - } else if (securityControl.getParametersToMark().contains(i)) { + } else if (securityControl.getParametersToMark().get(i)) { if (isPrimitive) { LOGGER.warn( "Input validators should not be used on primitive types. Parameter {} with type {} .Security control: {}", @@ -88,7 +96,9 @@ private void processInputValidator() { private void callInputValidation(int i) { // Duplicate the parameter value on the stack - mv.visitVarInsn(Opcodes.ALOAD, i); + mv.visitVarInsn( + Opcodes.ALOAD, + isStatic ? i : i + 1); // instance methods have this as first element in the stack // Load the marks from securityControl onto the stack mv.visitLdcInsn(securityControl.getMarks()); // Insert the call to setSecureMarks with the parameter value and marks as parameters diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java index ba8d07cf477..e2b33408c84 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java @@ -1,5 +1,6 @@ package com.datadog.iast.securitycontrol; +import static org.objectweb.asm.Opcodes.ACC_STATIC; import static org.objectweb.asm.Opcodes.ASM8; import datadog.trace.api.iast.securitycontrol.SecurityControl; @@ -36,7 +37,8 @@ public MethodVisitor visitMethod( } } if (match != null) { - return new SecurityControlMethodAdapter(mv, match, desc); + final boolean isStatic = (access & ACC_STATIC) > 0; + return new SecurityControlMethodAdapter(mv, match, desc, isStatic); } return mv; } diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy index 868e0aa74d0..d87583eabd4 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/securitycontrol/IastSecurityControlTransformerForkedTest.groovy @@ -6,6 +6,7 @@ import datadog.trace.api.iast.propagation.PropagationModule import datadog.trace.api.iast.securitycontrol.SecurityControl import datadog.trace.api.iast.securitycontrol.SecurityControlFormatter import datadog.trace.test.util.DDSpecification +import foo.bar.securitycontrol.SecurityControlStaticTestSuite import foo.bar.securitycontrol.SecurityControlTestSuite import net.bytebuddy.agent.ByteBuddyAgent @@ -13,6 +14,21 @@ import java.lang.instrument.Instrumentation class IastSecurityControlTransformerForkedTest extends DDSpecification{ + //static methods + private static final String STATIC_SANITIZER = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:sanitize' + private static final String STATIC_SANITIZE_VALIDATE_OBJECT = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:sanitizeObject' + private static final String STATIC_SANITIZE_INPUTS= 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:sanitizeInputs' + private static final String STATIC_SANITIZE_MANY_INPUTS= 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:sanitizeManyInputs' + private static final String STATIC_SANITIZE_INT = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:sanitizeInt' + private static final String STATIC_SANITIZE_LONG = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:sanitizeLong' + private static final String STATIC_INPUT_VALIDATOR_VALIDATE_ALL = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:validateAll' + private static final String STATIC_INPUT_VALIDATOR_VALIDATE_OVERLOADED = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:validate:java.lang.Object,java.lang.String,java.lang.String:1,2' + private static final String STATIC_INPUT_VALIDATOR_VALIDATE_RETURNING_INT = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:validateReturningInt' + private static final String STATIC_INPUT_VALIDATOR_VALIDATE_OBJECT = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:validateObject' + private static final String STATIC_INPUT_VALIDATOR_VALIDATE_LONG = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:validateLong' + private static final String STATIC_INPUT_VALIDATOR_VALIDATE_SELECTED_LONG = 'INPUT_VALIDATOR:XSS:foo.bar.securitycontrol.SecurityControlStaticTestSuite:validateSelectedLong:0' + + //not static methods private static final String SANITIZER = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitize' private static final String SANITIZE_VALIDATE_OBJECT = 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeObject' private static final String SANITIZE_INPUTS= 'SANITIZER:XSS:foo.bar.securitycontrol.SecurityControlTestSuite:sanitizeInputs' @@ -29,10 +45,12 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ def setupSpec() { - final config = "${SANITIZER};${SANITIZE_VALIDATE_OBJECT};${SANITIZE_INPUTS};${SANITIZE_MANY_INPUTS};${SANITIZE_INT};${SANITIZE_LONG};${INPUT_VALIDATOR_VALIDATE_ALL};${INPUT_VALIDATOR_VALIDATE_OVERLOADED};${INPUT_VALIDATOR_VALIDATE_RETURNING_INT};${INPUT_VALIDATOR_VALIDATE_OBJECT};${INPUT_VALIDATOR_VALIDATE_LONG};${INPUT_VALIDATOR_VALIDATE_SELECTED_LONG}" + final staticConfig = "${STATIC_SANITIZER};${STATIC_SANITIZE_VALIDATE_OBJECT};${STATIC_SANITIZE_INPUTS};${STATIC_SANITIZE_MANY_INPUTS};${STATIC_SANITIZE_INT};${STATIC_SANITIZE_LONG};${STATIC_INPUT_VALIDATOR_VALIDATE_ALL};${STATIC_INPUT_VALIDATOR_VALIDATE_OVERLOADED};${STATIC_INPUT_VALIDATOR_VALIDATE_RETURNING_INT};${STATIC_INPUT_VALIDATOR_VALIDATE_OBJECT};${STATIC_INPUT_VALIDATOR_VALIDATE_LONG};${STATIC_INPUT_VALIDATOR_VALIDATE_SELECTED_LONG}" + final config = "${SANITIZER};${SANITIZE_VALIDATE_OBJECT};${SANITIZE_INPUTS};${SANITIZE_MANY_INPUTS};${SANITIZE_INT};${SANITIZE_LONG};${INPUT_VALIDATOR_VALIDATE_ALL};${INPUT_VALIDATOR_VALIDATE_OVERLOADED};${INPUT_VALIDATOR_VALIDATE_RETURNING_INT};${INPUT_VALIDATOR_VALIDATE_OBJECT};${INPUT_VALIDATOR_VALIDATE_LONG};${INPUT_VALIDATOR_VALIDATE_SELECTED_LONG}" + final fullConfig = "${staticConfig};${config}" Instrumentation instrumentation = ByteBuddyAgent.install() - List securityControls = - SecurityControlFormatter.format(config) + Map> securityControls = + SecurityControlFormatter.format(fullConfig) assert securityControls != null instrumentation.addTransformer(new IastSecurityControlTransformer(securityControls), true) } @@ -45,7 +63,15 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ final marks = (VulnerabilityMarks.XSS_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) when: - SecurityControlTestSuite.&"$method".call(*args) + SecurityControlStaticTestSuite.&"$method".call(*args) + + then: + expected * iastModule.markIfTainted( toSanitize, marks) + 0 * _ + + when: + final suite = new SecurityControlTestSuite() + suite.&"$method".call(*args) then: expected * iastModule.markIfTainted( toSanitize, marks) @@ -68,7 +94,17 @@ class IastSecurityControlTransformerForkedTest extends DDSpecification{ final marks = (VulnerabilityMarks.XSS_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) when: - SecurityControlTestSuite.&"$method".call(*args) + SecurityControlStaticTestSuite.&"$method".call(*args) + + then: + for (final validate : toValidate){ + expected * iastModule.markIfTainted(validate, marks) + } + 0 * _ + + when: + final suite = new SecurityControlTestSuite() + suite.&"$method".call(*args) then: for (final validate : toValidate){ diff --git a/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlStaticTestSuite.java b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlStaticTestSuite.java new file mode 100644 index 00000000000..d0169c06773 --- /dev/null +++ b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlStaticTestSuite.java @@ -0,0 +1,100 @@ +package foo.bar.securitycontrol; + +public class SecurityControlStaticTestSuite { + + public static boolean validateAll(long input, String input2) { + return true; // dummy implementation + } + + public static boolean validateAll(String input) { + return true; // dummy implementation + } + + public static boolean validateAll(String input, String input2) { + return true; // dummy implementation + } + + public static boolean validateAll( + String input, + String input2, + String input3, + String input4, + String input5, + String input6, + String input7, + String input8, + String input9, + String input10) { + return true; // dummy implementation + } + + public static boolean validateLong(long input, String input2) { + return true; // dummy implementation + } + + public static boolean validateLong(String input, long input2) { + return true; // dummy implementation + } + + public static boolean validateLong(long intput1, String input2, long input3) { + return true; // dummy implementation + } + + public static boolean validateSelectedLong(long intput1) { + return true; // dummy implementation + } + + public static boolean validateSelectedLong(long input1, long intput2) { + return true; // dummy implementation + } + + public static boolean validate(String input) { + return true; // dummy implementation + } + + public static boolean validate(Object o, String input, String input2) { + return true; // dummy implementation + } + + public static int validateReturningInt(String input) { + return 1; // dummy implementation + } + + public static int validateObject(Object input) { + return 1; // dummy implementation + } + + public static String sanitize(String input) { + return "Sanitized"; + } + + public static Object sanitizeObject(String input) { + return "Sanitized"; + } + + public static String sanitizeInputs(String input, Object input2, int input3) { + return "Sanitized"; + } + + public static String sanitizeManyInputs( + String input, + String input2, + String input3, + String input4, + String input5, + String input6, + String input7, + String input8, + String input9, + String input10) { + return "Sanitized"; + } + + public static int sanitizeInt(int input) { + return input; + } + + public static long sanitizeLong(long input) { + return input; + } +} diff --git a/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java index d53a8e19de0..99fae518950 100644 --- a/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java +++ b/dd-java-agent/agent-iast/src/test/java/foo/bar/securitycontrol/SecurityControlTestSuite.java @@ -2,19 +2,19 @@ public class SecurityControlTestSuite { - public static boolean validateAll(long input, String input2) { + public boolean validateAll(long input, String input2) { return true; // dummy implementation } - public static boolean validateAll(String input) { + public boolean validateAll(String input) { return true; // dummy implementation } - public static boolean validateAll(String input, String input2) { + public boolean validateAll(String input, String input2) { return true; // dummy implementation } - public static boolean validateAll( + public boolean validateAll( String input, String input2, String input3, @@ -28,55 +28,55 @@ public static boolean validateAll( return true; // dummy implementation } - public static boolean validateLong(long input, String input2) { + public boolean validateLong(long input, String input2) { return true; // dummy implementation } - public static boolean validateLong(String input, long input2) { + public boolean validateLong(String input, long input2) { return true; // dummy implementation } - public static boolean validateLong(long intput1, String input2, long input3) { + public boolean validateLong(long intput1, String input2, long input3) { return true; // dummy implementation } - public static boolean validateSelectedLong(long intput1) { + public boolean validateSelectedLong(long intput1) { return true; // dummy implementation } - public static boolean validateSelectedLong(long input1, long intput2) { + public boolean validateSelectedLong(long input1, long intput2) { return true; // dummy implementation } - public static boolean validate(String input) { + public boolean validate(String input) { return true; // dummy implementation } - public static boolean validate(Object o, String input, String input2) { + public boolean validate(Object o, String input, String input2) { return true; // dummy implementation } - public static int validateReturningInt(String input) { + public int validateReturningInt(String input) { return 1; // dummy implementation } - public static int validateObject(Object input) { + public int validateObject(Object input) { return 1; // dummy implementation } - public static String sanitize(String input) { + public String sanitize(String input) { return "Sanitized"; } - public static Object sanitizeObject(String input) { + public Object sanitizeObject(String input) { return "Sanitized"; } - public static String sanitizeInputs(String input, Object input2, int input3) { + public String sanitizeInputs(String input, Object input2, int input3) { return "Sanitized"; } - public static String sanitizeManyInputs( + public String sanitizeManyInputs( String input, String input2, String input3, @@ -90,11 +90,11 @@ public static String sanitizeManyInputs( return "Sanitized"; } - public static int sanitizeInt(int input) { + public int sanitizeInt(int input) { return input; } - public static long sanitizeLong(long input) { + public long sanitizeLong(long input) { return input; } } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java index 24ecd4dc654..5126d323811 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControl.java @@ -1,7 +1,7 @@ package datadog.trace.api.iast.securitycontrol; +import java.util.BitSet; import java.util.List; -import java.util.Set; import javax.annotation.Nonnull; import javax.annotation.Nullable; @@ -17,7 +17,7 @@ public class SecurityControl { @Nullable private List parameterTypes; - @Nullable private Set parametersToMark; + @Nullable private BitSet parametersToMark; public SecurityControl( @Nonnull SecurityControlType type, @@ -25,7 +25,7 @@ public SecurityControl( @Nonnull String className, @Nonnull String method, @Nullable List parameterTypes, - @Nullable Set parametersToMark) { + @Nullable BitSet parametersToMark) { this.type = type; this.marks = marks; this.className = className; @@ -60,7 +60,7 @@ public List getParameterTypes() { } @Nullable - public Set getParametersToMark() { + public BitSet getParametersToMark() { return parametersToMark; } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java index 2c13bcddd07..2d6894c5c8b 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/securitycontrol/SecurityControlFormatter.java @@ -6,8 +6,10 @@ import de.thetaphi.forbiddenapis.SuppressForbidden; import java.util.ArrayList; import java.util.Arrays; +import java.util.BitSet; +import java.util.HashMap; import java.util.List; -import java.util.Set; +import java.util.Map; import javax.annotation.Nonnull; import javax.annotation.Nullable; import org.slf4j.Logger; @@ -25,7 +27,8 @@ public class SecurityControlFormatter { private static final String ALL = "*"; @Nullable - public static List format(final @Nonnull String securityControlString) { + public static Map> format( + final @Nonnull String securityControlString) { if (securityControlString.isEmpty()) { log.warn("Security control configuration is empty"); @@ -36,13 +39,14 @@ public static List format(final @Nonnull String securityControl String[] list = config.split(SECURITY_CONTROL_DELIMITER); - List securityControls = new ArrayList<>(list.length); + Map> securityControls = new HashMap<>(); for (String s : list) { try { SecurityControl securityControl = getSecurityControl(s); if (securityControl != null) { - securityControls.add(securityControl); + securityControls.putIfAbsent(securityControl.getClassName(), new ArrayList<>()); + securityControls.get(securityControl.getClassName()).add(securityControl); } } catch (Exception e) { log.warn("Security control configuration is invalid: {}", s); @@ -71,7 +75,7 @@ private static SecurityControl getSecurityControl(@Nonnull final String config) String method = split[3]; List parameterTypes = null; - Set parametersToMark = null; + BitSet parametersToMark = null; if (split.length > 4) { String[] elements = split[4].split(SECURITY_CONTROL_ELEMENT_DELIMITER); @@ -107,10 +111,12 @@ private static int getMarks(String s) { return marks; } - private static Set getParametersToMark(String[] elements) { - return Arrays.stream(elements) - .map(Integer::parseInt) - .collect(java.util.stream.Collectors.toSet()); + private static BitSet getParametersToMark(String[] elements) { + BitSet bitSet = new BitSet(); + Arrays.stream(elements) + .map(Integer::parseInt) // Convert each element to an Integer + .forEach(bitSet::set); + return bitSet; } private static boolean isNumeric(String str) { diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy index 4a6a07664e2..d094eefdd9b 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/securitycontrol/SecurityControlFormatterTest.groovy @@ -13,7 +13,9 @@ class SecurityControlFormatterTest extends DDSpecification{ expect: result.size() == 1 - def securityControl = result.get(0) + def securityControls = result.get('bar/foo/CustomInputValidator') + securityControls.size() == 1 + def securityControl = securityControls.get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" @@ -30,7 +32,9 @@ class SecurityControlFormatterTest extends DDSpecification{ expect: result.size() == 1 - def securityControl = result.get(0) + def securityControls = result.get('bar/foo/CustomSanitizer') + securityControls.size() == 1 + def securityControl = securityControls.get(0) securityControl.getType() == SecurityControlType.SANITIZER securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomSanitizer" @@ -47,7 +51,7 @@ class SecurityControlFormatterTest extends DDSpecification{ expect: result.size() == 2 - def inputValidator = result.get(0) + def inputValidator = result.get('bar/foo/CustomInputValidator').get(0) inputValidator.getType() == SecurityControlType.INPUT_VALIDATOR inputValidator.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) inputValidator.getClassName() == "bar/foo/CustomInputValidator" @@ -55,7 +59,7 @@ class SecurityControlFormatterTest extends DDSpecification{ inputValidator.getParameterTypes() == null inputValidator.getParametersToMark() == null - def sanitizer = result.get(1) + def sanitizer = result.get('bar/foo/CustomSanitizer').get(0) sanitizer.getType() == SecurityControlType.SANITIZER sanitizer.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) sanitizer.getClassName() == "bar/foo/CustomSanitizer" @@ -72,7 +76,7 @@ class SecurityControlFormatterTest extends DDSpecification{ expect: result.size() == 1 - def securityControl = result.get(0) + def securityControl = result.get('bar/foo/CustomInputValidator').get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.SQL_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" @@ -89,7 +93,7 @@ class SecurityControlFormatterTest extends DDSpecification{ expect: result.size() == 1 - def securityControl = result.get(0) + def securityControl = result.get('bar/foo/CustomInputValidator').get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" @@ -106,15 +110,15 @@ class SecurityControlFormatterTest extends DDSpecification{ expect: result.size() == 1 - def securityControl = result.get(0) + def securityControl = result.get('bar/foo/CustomInputValidator').get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == null - securityControl.getParametersToMark().size() == 2 - securityControl.getParametersToMark().contains(1) - securityControl.getParametersToMark().contains(2) + securityControl.getParametersToMark().cardinality() == 2 + securityControl.getParametersToMark().get(1) + securityControl.getParametersToMark().get(2) } void 'test overcharged methods with parameters to mark'() { @@ -125,15 +129,15 @@ class SecurityControlFormatterTest extends DDSpecification{ expect: result.size() == 1 - def securityControl = result.get(0) + def securityControl = result.get('bar/foo/CustomInputValidator').get(0) securityControl.getType() == SecurityControlType.INPUT_VALIDATOR securityControl.getMarks() == (VulnerabilityMarks.COMMAND_INJECTION_MARK | VulnerabilityMarks.CUSTOM_SECURITY_CONTROL_MARK) securityControl.getClassName() == "bar/foo/CustomInputValidator" securityControl.getMethod() == "validate" securityControl.getParameterTypes() == ["java.lang.Object", "java.lang.String", "java.lang.String"] - securityControl.getParametersToMark().size() == 2 - securityControl.getParametersToMark().contains(1) - securityControl.getParametersToMark().contains(2) + securityControl.getParametersToMark().cardinality() == 2 + securityControl.getParametersToMark().get(1) + securityControl.getParametersToMark().get(2) } void 'test error control'() { From 045b37619b5f56bd83f81e13bac063853c7ac01e Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 19 Dec 2024 13:00:27 +0100 Subject: [PATCH 34/38] changes suggested by manu --- .../iast/securitycontrol/SecurityControlMethodAdapter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java index 09a78a39469..20fadc8a37c 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java @@ -19,7 +19,7 @@ public class SecurityControlMethodAdapter extends MethodVisitor { private final MethodVisitor mv; private final SecurityControl securityControl; private final String desc; - boolean isStatic; + private final boolean isStatic; public SecurityControlMethodAdapter( final MethodVisitor mv, From 49ad3d61be37f70c6a148353b18bc776d0725a74 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 19 Dec 2024 15:56:34 +0100 Subject: [PATCH 35/38] remove unnecessary check --- .../iast/securitycontrol/IastSecurityControlTransformer.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java index d3d0b8ef247..b15e7abeeb6 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/IastSecurityControlTransformer.java @@ -33,9 +33,6 @@ public byte[] transform( Class classBeingRedefined, java.security.ProtectionDomain protectionDomain, byte[] classfileBuffer) { - if (!securityControls.containsKey(className)) { - return null; // Do not transform classes that are not in the classFilter - } List match = securityControls.get(className); if (match == null || match.isEmpty()) { return null; // Do not transform classes that do not have a security control From 0ae0bdde6f73b583041ae2b70fc1dd273a50906c Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 19 Dec 2024 16:13:02 +0100 Subject: [PATCH 36/38] separate method visitors for adapters and input validators --- .../AbstractMethodAdapter.java | 45 +++++++ .../InputValidatorMethodAdapter.java | 63 ++++++++++ .../SanitizerMethodAdapter.java | 31 +++++ .../SecurityControlMethodAdapter.java | 113 ------------------ .../SecurityControlMethodClassVisitor.java | 45 ++++++- 5 files changed, 181 insertions(+), 116 deletions(-) create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/AbstractMethodAdapter.java create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/InputValidatorMethodAdapter.java create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SanitizerMethodAdapter.java delete mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/AbstractMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/AbstractMethodAdapter.java new file mode 100644 index 00000000000..0cd8663b7e5 --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/AbstractMethodAdapter.java @@ -0,0 +1,45 @@ +package com.datadog.iast.securitycontrol; + +import datadog.trace.api.iast.securitycontrol.SecurityControl; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; + +public class AbstractMethodAdapter extends MethodVisitor { + + private static final String HELPER = + "datadog/trace/api/iast/securitycontrol/SecurityControlHelper"; + private static final String METHOD = "setSecureMarks"; + private static final String DESCRIPTOR = "(Ljava/lang/Object;I)V"; + protected final MethodVisitor mv; + protected final SecurityControl securityControl; + protected final int accessFlags; + protected final Type method; + + public AbstractMethodAdapter( + final MethodVisitor mv, + final SecurityControl securityControl, + final int accessFlags, + final Type method) { + super(Opcodes.ASM9, mv); + this.mv = mv; + this.securityControl = securityControl; + this.accessFlags = accessFlags; + this.method = method; + } + + protected boolean isStatic() { + return (accessFlags & Opcodes.ACC_STATIC) > 0; + } + + /** + * This method loads the current secure marks defined in the control and then calls the helper + * method + */ + protected void loadMarksAndCallHelper() { + // Load the marks from securityControl onto the stack + mv.visitLdcInsn(securityControl.getMarks()); + // Insert the call to setSecureMarks with the return value and marks as parameters + mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); + } +} diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/InputValidatorMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/InputValidatorMethodAdapter.java new file mode 100644 index 00000000000..def0bde8ba6 --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/InputValidatorMethodAdapter.java @@ -0,0 +1,63 @@ +package com.datadog.iast.securitycontrol; + +import static com.datadog.iast.securitycontrol.SecurityControlMethodClassVisitor.LOGGER; +import static com.datadog.iast.securitycontrol.SecurityControlMethodClassVisitor.isPrimitive; + +import datadog.trace.api.iast.securitycontrol.SecurityControl; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; + +public class InputValidatorMethodAdapter extends AbstractMethodAdapter { + + private final boolean isStatic; + + public InputValidatorMethodAdapter( + final MethodVisitor mv, + final SecurityControl securityControl, + final int accessFlags, + final Type method) { + super(mv, securityControl, accessFlags, method); + isStatic = isStatic(); + } + + @Override + public void visitCode() { + super.visitCode(); + processInputValidator(); + } + + private void processInputValidator() { + boolean allParameters = securityControl.getParametersToMark() == null; + Type[] types = method.getArgumentTypes(); + int parametersCount = 0; + for (int i = 0; i < types.length; i++) { + Type type = types[i]; + boolean isPrimitive = isPrimitive(type); + if (allParameters) { + if (!isPrimitive) { + callInputValidation(parametersCount); + } + } else if (securityControl.getParametersToMark().get(i)) { + if (isPrimitive) { + LOGGER.warn( + "Input validators should not be used on primitive types. Parameter {} with type {} .Security control: {}", + i, + type.getClassName(), + securityControl); + } else { + callInputValidation(parametersCount); + } + } + parametersCount += type.getSize(); + } + } + + private void callInputValidation(int i) { + // Load the parameter onto the stack + mv.visitVarInsn( + Opcodes.ALOAD, + isStatic ? i : i + 1); // instance methods have this as first element in the stack + loadMarksAndCallHelper(); + } +} diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SanitizerMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SanitizerMethodAdapter.java new file mode 100644 index 00000000000..26502759f42 --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SanitizerMethodAdapter.java @@ -0,0 +1,31 @@ +package com.datadog.iast.securitycontrol; + +import datadog.trace.api.iast.securitycontrol.SecurityControl; +import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; +import org.objectweb.asm.Type; + +public class SanitizerMethodAdapter extends AbstractMethodAdapter { + + public SanitizerMethodAdapter( + final MethodVisitor mv, + final SecurityControl securityControl, + final int accessFlags, + final Type method) { + super(mv, securityControl, accessFlags, method); + } + + @Override + public void visitInsn(int opcode) { + if (opcode == Opcodes.ARETURN) { + processSanitizer(); + } + super.visitInsn(opcode); + } + + private void processSanitizer() { + // Duplicate the return value on the stack + mv.visitInsn(Opcodes.DUP); + loadMarksAndCallHelper(); + } +} diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java deleted file mode 100644 index 20fadc8a37c..00000000000 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodAdapter.java +++ /dev/null @@ -1,113 +0,0 @@ -package com.datadog.iast.securitycontrol; - -import datadog.trace.api.iast.securitycontrol.SecurityControl; -import datadog.trace.api.iast.securitycontrol.SecurityControlType; -import org.objectweb.asm.MethodVisitor; -import org.objectweb.asm.Opcodes; -import org.objectweb.asm.Type; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -public class SecurityControlMethodAdapter extends MethodVisitor { - - private static final Logger LOGGER = LoggerFactory.getLogger(SecurityControlMethodAdapter.class); - - public static final String HELPER = - "datadog/trace/api/iast/securitycontrol/SecurityControlHelper"; - public static final String METHOD = "setSecureMarks"; - public static final String DESCRIPTOR = "(Ljava/lang/Object;I)V"; - private final MethodVisitor mv; - private final SecurityControl securityControl; - private final String desc; - private final boolean isStatic; - - public SecurityControlMethodAdapter( - final MethodVisitor mv, - final SecurityControl securityControl, - final String desc, - boolean isStatic) { - super(Opcodes.ASM9, mv); - this.mv = mv; - this.securityControl = securityControl; - this.desc = desc; - this.isStatic = isStatic; - } - - @Override - public void visitCode() { - super.visitCode(); - if (securityControl.getType() == SecurityControlType.INPUT_VALIDATOR) { - processInputValidator(); - } - } - - @Override - public void visitInsn(int opcode) { - if (securityControl.getType() == SecurityControlType.SANITIZER) { - // Only process the return value if is an Object as we are not tainting primitives - if (opcode == Opcodes.ARETURN) { - processSanitizer(); - } else { - Type returnType = Type.getReturnType(desc); - // no need to check primitives as we are not tainting them - LOGGER.warn( - "Sanitizers should not be used on primitive return types. Return type {}. Security control: {}", - returnType.getClassName(), - securityControl); - } - } - super.visitInsn(opcode); - } - - private void processSanitizer() { - // Duplicate the return value on the stack - mv.visitInsn(Opcodes.DUP); - // Load the marks from securityControl onto the stack - mv.visitLdcInsn(securityControl.getMarks()); - // Insert the call to setSecureMarks with the return value and marks as parameters - mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); - } - - private void processInputValidator() { - boolean allParameters = securityControl.getParametersToMark() == null; - Type[] types = Type.getArgumentTypes(desc); - int parametersCount = 0; - for (int i = 0; i < types.length; i++) { - Type type = types[i]; - boolean isPrimitive = isPrimitive(type); - if (allParameters) { - if (!isPrimitive) { - callInputValidation(parametersCount); - } - } else if (securityControl.getParametersToMark().get(i)) { - if (isPrimitive) { - LOGGER.warn( - "Input validators should not be used on primitive types. Parameter {} with type {} .Security control: {}", - i, - type.getClassName(), - securityControl); - } else { - callInputValidation(parametersCount); - } - } - parametersCount += types[i].getSize(); - } - } - - private void callInputValidation(int i) { - // Duplicate the parameter value on the stack - mv.visitVarInsn( - Opcodes.ALOAD, - isStatic ? i : i + 1); // instance methods have this as first element in the stack - // Load the marks from securityControl onto the stack - mv.visitLdcInsn(securityControl.getMarks()); - // Insert the call to setSecureMarks with the parameter value and marks as parameters - mv.visitMethodInsn(Opcodes.INVOKESTATIC, HELPER, METHOD, DESCRIPTOR, false); - } - - private static boolean isPrimitive(Type type) { - // Check if is a primitive type - int sort = type.getSort(); - return sort >= Type.BOOLEAN && sort <= Type.DOUBLE; - } -} diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java index e2b33408c84..60003803612 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java @@ -1,18 +1,23 @@ package com.datadog.iast.securitycontrol; -import static org.objectweb.asm.Opcodes.ACC_STATIC; import static org.objectweb.asm.Opcodes.ASM8; import datadog.trace.api.iast.securitycontrol.SecurityControl; +import datadog.trace.api.iast.securitycontrol.SecurityControlType; import java.util.List; import javax.annotation.Nullable; import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; import org.objectweb.asm.MethodVisitor; +import org.objectweb.asm.Opcodes; import org.objectweb.asm.Type; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class SecurityControlMethodClassVisitor extends ClassVisitor { + static final Logger LOGGER = LoggerFactory.getLogger(SecurityControlMethodClassVisitor.class); + private final List securityControls; public SecurityControlMethodClassVisitor( @@ -37,8 +42,12 @@ public MethodVisitor visitMethod( } } if (match != null) { - final boolean isStatic = (access & ACC_STATIC) > 0; - return new SecurityControlMethodAdapter(mv, match, desc, isStatic); + final Type method = Type.getMethodType(desc); + if (match.getType() == SecurityControlType.SANITIZER) { + mv = adaptSanitizer(mv, match, access, method); + } else { + mv = adaptInputValidator(mv, match, access, method); + } } return mv; } @@ -70,4 +79,34 @@ private boolean shouldBeAdapted(SecurityControl securityControl, String name, St return true; } + + private MethodVisitor adaptSanitizer( + final MethodVisitor mv, + final SecurityControl control, + final int accessFlags, + final Type method) { + if (isPrimitive(method.getReturnType())) { + // no need to check primitives as we are not tainting them + LOGGER.warn( + "Sanitizers should not be used on non-primitive return types. Return type {}. Security control: {}", + method.getReturnType().getClassName(), + control); + return mv; + } + return new SanitizerMethodAdapter(mv, control, accessFlags, method); + } + + private MethodVisitor adaptInputValidator( + final MethodVisitor mv, + final SecurityControl control, + final int accessFlags, + final Type method) { + return new InputValidatorMethodAdapter(mv, control, accessFlags, method); + } + + public static boolean isPrimitive(final Type type) { + // Check if is a primitive type + int sort = type.getSort(); + return sort >= Type.BOOLEAN && sort <= Type.DOUBLE; + } } From 78e6d43e2bf2fdcc9c2a7dbdc76f841a1e7e47ab Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 19 Dec 2024 16:28:26 +0100 Subject: [PATCH 37/38] fix spotless --- .../iast/securitycontrol/SecurityControlMethodClassVisitor.java | 1 - 1 file changed, 1 deletion(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java index 60003803612..5b800946abe 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java @@ -9,7 +9,6 @@ import org.objectweb.asm.ClassVisitor; import org.objectweb.asm.ClassWriter; import org.objectweb.asm.MethodVisitor; -import org.objectweb.asm.Opcodes; import org.objectweb.asm.Type; import org.slf4j.Logger; import org.slf4j.LoggerFactory; From de8c19855d41288aeef7bbebaf2574f0061268d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Gonz=C3=A1lez=20Garc=C3=ADa?= Date: Fri, 20 Dec 2024 11:41:42 +0100 Subject: [PATCH 38/38] Update dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Manuel Álvarez Álvarez --- .../iast/securitycontrol/SecurityControlMethodClassVisitor.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java index 5b800946abe..0a1c825fd55 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/securitycontrol/SecurityControlMethodClassVisitor.java @@ -87,7 +87,7 @@ private MethodVisitor adaptSanitizer( if (isPrimitive(method.getReturnType())) { // no need to check primitives as we are not tainting them LOGGER.warn( - "Sanitizers should not be used on non-primitive return types. Return type {}. Security control: {}", + "Sanitizers should not be used on primitive return types. Return type {}. Security control: {}", method.getReturnType().getClassName(), control); return mv;