From 4107d92a3d55cf8ffe4eaebe05b5753d1c48b37b Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Sat, 1 Jul 2023 23:01:02 +0200 Subject: [PATCH 01/15] Added Tomcat 7+ instrumentation for exception reporting --- .../tomcat7/ErrorReportValueAdvice.java | 60 +++++++++++++++++++ .../ErrorReportValueInstrumentation.java | 45 ++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java create mode 100644 dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java new file mode 100644 index 00000000000..feeff917c7c --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -0,0 +1,60 @@ +package datadog.trace.instrumentation.tomcat7; + +import net.bytebuddy.asm.Advice; +import org.apache.catalina.connector.Request; +import org.apache.catalina.connector.Response; + +import java.io.IOException; +import java.io.Writer; + +public class ErrorReportValueAdvice { + + @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) + public static boolean onEnter( + @Advice.Argument(value = 0) Request request, + @Advice.Argument(value = 1) Response response, + @Advice.Argument(value = 2) Throwable throwable) { + System.out.println(">>> ErrorReportValue.report(" + throwable + ") -> " + response.getBytesWritten(false)); + + int statusCode = response.getStatus(); + + // Do nothing on a 1xx, 2xx and 3xx status + // Do nothing if anything has been written already + // Do nothing if the response hasn't been explicitly marked as in error + // and that error has not been reported. + if (statusCode < 400 || response.getContentWritten() > 0 || !response.isError()) { + return true; // skip original method + } + + StringBuilder sb = new StringBuilder(); + sb.append("This is fake page displayed to suppress stack trace leak"); + + try { + try { + response.setContentType("text/html"); + response.setCharacterEncoding("utf-8"); + } catch (Throwable t) { + // Ignore + } + Writer writer = response.getReporter(); + if (writer != null) { + // If writer is null, it's an indication that the response has + // been hard committed already, which should never happen + writer.write(sb.toString()); + response.finishResponse(); + } + } catch (IOException | IllegalStateException e) { + // Ignore + } + + return false; + } + +// @Advice.OnMethodExit +// public static void onExit( +// @Advice.Argument(value = 0) Request request, +// @Advice.Argument(value = 1) Response response, +// @Advice.Argument(value = 2) Throwable throwable) { +// System.out.println("<<< ErrorReportValue.report(" + throwable + ") -> " + response.getBytesWritten(false)); +// } +} diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java new file mode 100644 index 00000000000..9dd72ba37df --- /dev/null +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java @@ -0,0 +1,45 @@ +package datadog.trace.instrumentation.tomcat7; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; + +import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isProtected; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; + +@AutoService(Instrumenter.class) +public class ErrorReportValueInstrumentation extends Instrumenter.AppSec implements Instrumenter.ForSingleType { + + public ErrorReportValueInstrumentation() { + super("tomcat"); + } + + @Override + public String instrumentedType() { + return "org.apache.catalina.valves.ErrorReportValve"; + } + + @Override + public String[] helperClassNames() { + return new String[] { + "java.lang.StringBuilder", + "java.io.Writer", + "java.io.IOException", + "java.lang.IllegalStateException", + "datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator" + }; + } + + @Override + public void adviceTransformations(AdviceTransformation transformation) { + transformation.applyAdvice( + isMethod() + .and(named("report")) + .and(takesArgument(0, named("org.apache.catalina.connector.Request"))) + .and(takesArgument(1, named("org.apache.catalina.connector.Response"))) + .and(takesArgument(2, Throwable.class)) + .and(isProtected()), + packageName + ".ErrorReportValueAdvice"); + } +} From d5433fa4ebe66041d35cc60d65953d98d8424c75 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Mon, 3 Jul 2023 12:44:43 +0200 Subject: [PATCH 02/15] Added AppSec reporting for the stacktrace leak --- .../decorator/StacktraceLeakDecorator.java | 29 ++++++++++ .../datadog/appsec/gateway/GatewayBridge.java | 57 +++++++++++++++++++ .../tomcat7/ErrorReportValueAdvice.java | 9 +++ .../datadog/trace/api/gateway/Events.java | 12 ++++ 4 files changed, 107 insertions(+) create mode 100644 dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java new file mode 100644 index 00000000000..20347054c30 --- /dev/null +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java @@ -0,0 +1,29 @@ +package datadog.trace.bootstrap.instrumentation.decorator; + +import datadog.trace.api.function.TriFunction; +import datadog.trace.api.gateway.CallbackProvider; +import datadog.trace.api.gateway.Flow; +import datadog.trace.api.gateway.RequestContext; +import datadog.trace.api.gateway.RequestContextSlot; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; + +import static datadog.trace.api.gateway.Events.EVENTS; + +public class StacktraceLeakDecorator { + + public static final StacktraceLeakDecorator DECORATE = new StacktraceLeakDecorator(); + + public void onStacktraceLeak(final AgentSpan span, Throwable throwable, boolean leaked) { + CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); + RequestContext requestContext = span.getRequestContext(); + if (cbp == null || requestContext == null) { + return; + } + TriFunction> addrCallback = + cbp.getCallback(EVENTS.responseStacktrace()); + if (null != addrCallback) { + addrCallback.apply(requestContext, throwable, leaked); + } + } +} diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java index e89927ebb44..80924bee50b 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java @@ -383,6 +383,63 @@ public void init() { } } }); + + subscriptionService.registerCallback( + EVENTS.responseStacktrace(), + (ctx_, throwable, leaked) -> { + AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC); + if (ctx == null || throwable == null) { + return NoopFlow.INSTANCE; + } + + + List parameterList = new ArrayList<>(); + + // Recursive variant +// while (throwable != null) { +// parameterList.add( +// new Parameter.ParameterBuilder() +// .withAddress("Response body") +// //.withKeyPath(Arrays.asList("user-agent2",0)) +// .withValue(throwable.toString()) +// //.withHighlight(Collections.singletonList(throwable.getCause().toString())) +// .build()); +// throwable = throwable.getCause(); +// } + + parameterList.add( + new Parameter.ParameterBuilder() + .withAddress("Response body") + //.withKeyPath(Arrays.asList("user-agent2",0)) + .withValue(throwable.toString()) + .withHighlight(Collections.singletonList(throwable.getCause().toString())) + .build()); + + RuleMatch ruleMatch = + new RuleMatch.RuleMatchBuilder() + //.withOperator("exact_match") + //.withOperatorValue("Exception") + .withParameters(parameterList) + .build(); + + AppSecEvent100 event = new AppSecEvent100.AppSecEvent100Builder() + .withRule( + new Rule.RuleBuilder() + .withId("Stacktrace leak") + //.withName("Stacktrace leak") + .withTags( + new com.datadog.appsec.report.raw.events.Tags.TagsBuilder() + .withType("Exception") + .withCategory("attack_attempt") + .build()) + .build()) + .withRuleMatches(Collections.singletonList(ruleMatch)) + .build(); + ctx.reportEvents(Collections.singletonList(event), null); + + // TODO need to be Flow that suppress stacktrace + return NoopFlow.INSTANCE; + }); } public void stop() { diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java index feeff917c7c..364a4f90f4a 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -1,12 +1,16 @@ package datadog.trace.instrumentation.tomcat7; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import net.bytebuddy.asm.Advice; import org.apache.catalina.connector.Request; import org.apache.catalina.connector.Response; +import datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator; import java.io.IOException; import java.io.Writer; +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; + public class ErrorReportValueAdvice { @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) @@ -26,6 +30,11 @@ public static boolean onEnter( return true; // skip original method } + final AgentSpan span = activeSpan(); + if (span != null && throwable != null) { + StacktraceLeakDecorator.DECORATE.onStacktraceLeak(span, throwable, false); + } + StringBuilder sb = new StringBuilder(); sb.append("This is fake page displayed to suppress stack trace leak"); diff --git a/internal-api/src/main/java/datadog/trace/api/gateway/Events.java b/internal-api/src/main/java/datadog/trace/api/gateway/Events.java index 693a5ca7b34..47103ed1b5c 100644 --- a/internal-api/src/main/java/datadog/trace/api/gateway/Events.java +++ b/internal-api/src/main/java/datadog/trace/api/gateway/Events.java @@ -189,6 +189,18 @@ public EventType>> grpcServerReque return (EventType>>) GRPC_SERVER_REQUEST_MESSAGE; } + static final int RESPONSE_STACKTRACE_ID = 15; + + @SuppressWarnings("rawtypes") + private static final EventType RESPONSE_STACKTRACE = + new ET<>("response.stacktrace_leak", RESPONSE_STACKTRACE_ID); + /** Response contains stacktrace */ + @SuppressWarnings("unchecked") + public EventType>> responseStacktrace() { + return (EventType>>) RESPONSE_STACKTRACE; + } + + static final int MAX_EVENTS = nextId.get(); private static final class ET extends EventType { From f6495f3ad45a01154298d83d67dd6122532c70cc Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Thu, 20 Jul 2023 16:34:04 +0200 Subject: [PATCH 03/15] Added IAST reporting for the stacktrace leak --- .../java/com/datadog/iast/IastSystem.java | 4 +- .../datadog/iast/model/VulnerabilityType.java | 3 + .../iast/sink/StacktraceLeakModuleImpl.java | 18 +++ .../iast/sink/StacktraceLeakModuleTest.groovy | 51 ++++++++ .../tomcat7/ErrorReportValueAdvice.java | 114 ++++++++++-------- .../ErrorReportValueInstrumentation.java | 66 +++++----- .../trace/api/iast/InstrumentationBridge.java | 9 ++ .../trace/api/iast/VulnerabilityTypes.java | 7 +- .../api/iast/sink/StacktraceLeakModule.java | 8 ++ 9 files changed, 196 insertions(+), 84 deletions(-) create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java create mode 100644 dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/StacktraceLeakModuleTest.groovy create mode 100644 internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.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 589b2dfc80e..971aaae0106 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 @@ -14,6 +14,7 @@ import com.datadog.iast.sink.PathTraversalModuleImpl; import com.datadog.iast.sink.SqlInjectionModuleImpl; import com.datadog.iast.sink.SsrfModuleImpl; +import com.datadog.iast.sink.StacktraceLeakModuleImpl; import com.datadog.iast.sink.TrustBoundaryViolationModuleImpl; import com.datadog.iast.sink.UnvalidatedRedirectModuleImpl; import com.datadog.iast.sink.WeakCipherModuleImpl; @@ -100,7 +101,8 @@ private static Stream iastModules(final Dependencies dependencies) { new WeakRandomnessModuleImpl(dependencies), new XPathInjectionModuleImpl(dependencies), new TrustBoundaryViolationModuleImpl(dependencies), - new XssModuleImpl(dependencies)); + new XssModuleImpl(dependencies), + new StacktraceLeakModuleImpl()); } private static void registerRequestStartedCallback( diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/VulnerabilityType.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/VulnerabilityType.java index ae1d25ddfb3..62fdc6db56e 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/VulnerabilityType.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/VulnerabilityType.java @@ -65,6 +65,9 @@ public interface VulnerabilityType { InjectionType XSS = new InjectionTypeImpl(VulnerabilityTypes.XSS_STRING, VulnerabilityMarks.XSS_MARK, ' '); + VulnerabilityType STACKTRACE_LEAK = + new VulnerabilityTypeImpl(VulnerabilityTypes.STACKTRACE_LEAK_STRING, NOT_MARKED); + String name(); /** A bit flag to ignore tainted ranges for this vulnerability. Set to 0 if none. */ diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java new file mode 100644 index 00000000000..e22e227d33a --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java @@ -0,0 +1,18 @@ +package com.datadog.iast.sink; + +import com.datadog.iast.model.Evidence; +import com.datadog.iast.model.Vulnerability; +import com.datadog.iast.model.VulnerabilityType; +import datadog.trace.api.iast.sink.StacktraceLeakModule; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; +import javax.annotation.Nullable; + +public class StacktraceLeakModuleImpl extends SinkModuleBase implements StacktraceLeakModule { + @Override + public void onResponseException(@Nullable String exception) { + final AgentSpan span = AgentTracer.activeSpan(); + reporter.report( + span, new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, null, new Evidence(exception))); + } +} diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/StacktraceLeakModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/StacktraceLeakModuleTest.groovy new file mode 100644 index 00000000000..323554dfbe9 --- /dev/null +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/StacktraceLeakModuleTest.groovy @@ -0,0 +1,51 @@ +package com.datadog.iast.sink + +import com.datadog.iast.IastModuleImplTestBase +import com.datadog.iast.model.Evidence +import com.datadog.iast.model.Vulnerability +import com.datadog.iast.model.VulnerabilityType +import datadog.trace.api.iast.sink.StacktraceLeakModule +import datadog.trace.bootstrap.instrumentation.api.AgentSpan + +class StacktraceLeakModuleTest extends IastModuleImplTestBase { + private StacktraceLeakModule module + + def setup() { + module = registerDependencies(new StacktraceLeakModuleImpl()) + } + + void 'iast stacktrace leak module'() { + given: + final spanId = 123456 + final span = Mock(AgentSpan) + + def throwable = new Exception('some exception') + def moduleName = 'moduleName' + def className = 'className' + def methodName = 'methodName' + + when: + module.onStacktraceLeak(throwable, moduleName, className, methodName) + + then: + 1 * tracer.activeSpan() >> span + 1 * span.getSpanId() >> spanId + 1 * span.getServiceName() + 1 * reporter.report(_, _) >> { args -> + Vulnerability vuln = args[1] as Vulnerability + assert vuln != null + assert vuln.getType() == VulnerabilityType.STACKTRACE_LEAK + assert vuln.getEvidence() == new Evidence('ExceptionHandler in moduleName \r\nthrown java.lang.Exception') + assert vuln.getLocation() != null + } + 0 * _ + } + + void 'iast stacktrace leak no exception'() { + when: + module.onStacktraceLeak(null, null, null, null) + + then: + 0 * _ + } +} diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java index 364a4f90f4a..a0104126d76 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -1,69 +1,83 @@ package datadog.trace.instrumentation.tomcat7; +import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; + +import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.sink.StacktraceLeakModule; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; -import net.bytebuddy.asm.Advice; -import org.apache.catalina.connector.Request; -import org.apache.catalina.connector.Response; import datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator; - import java.io.IOException; import java.io.Writer; - -import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; +import net.bytebuddy.asm.Advice; +import org.apache.catalina.connector.Request; +import org.apache.catalina.connector.Response; public class ErrorReportValueAdvice { - @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) - public static boolean onEnter( - @Advice.Argument(value = 0) Request request, - @Advice.Argument(value = 1) Response response, - @Advice.Argument(value = 2) Throwable throwable) { - System.out.println(">>> ErrorReportValue.report(" + throwable + ") -> " + response.getBytesWritten(false)); + @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) + public static boolean onEnter( + @Advice.Argument(value = 0) Request request, + @Advice.Argument(value = 1) Response response, + @Advice.Argument(value = 2) Throwable throwable) { + System.out.println( + ">>> ErrorReportValue.report(" + throwable + ") -> " + response.getBytesWritten(false)); - int statusCode = response.getStatus(); + int statusCode = response.getStatus(); - // Do nothing on a 1xx, 2xx and 3xx status - // Do nothing if anything has been written already - // Do nothing if the response hasn't been explicitly marked as in error - // and that error has not been reported. - if (statusCode < 400 || response.getContentWritten() > 0 || !response.isError()) { - return true; // skip original method - } - - final AgentSpan span = activeSpan(); - if (span != null && throwable != null) { - StacktraceLeakDecorator.DECORATE.onStacktraceLeak(span, throwable, false); - } + // Do nothing on a 1xx, 2xx and 3xx status + // Do nothing if anything has been written already + // Do nothing if the response hasn't been explicitly marked as in error + // and that error has not been reported. + if (statusCode < 400 || response.getContentWritten() > 0 || !response.isError()) { + return true; // skip original method + } - StringBuilder sb = new StringBuilder(); - sb.append("This is fake page displayed to suppress stack trace leak"); + final AgentSpan span = activeSpan(); + if (span != null && throwable != null) { + // Report AppSec + StacktraceLeakDecorator.DECORATE.onStacktraceLeak(span, throwable, false); + // Report IAST + final StacktraceLeakModule module = InstrumentationBridge.STACKTRACE_LEAK_MODULE; + if (module != null) { try { - try { - response.setContentType("text/html"); - response.setCharacterEncoding("utf-8"); - } catch (Throwable t) { - // Ignore - } - Writer writer = response.getReporter(); - if (writer != null) { - // If writer is null, it's an indication that the response has - // been hard committed already, which should never happen - writer.write(sb.toString()); - response.finishResponse(); - } - } catch (IOException | IllegalStateException e) { - // Ignore + module.onResponseException(throwable.getMessage()); + } catch (final Throwable e) { + module.onUnexpectedException("onResponseException threw", e); } + } + } - return false; + StringBuilder sb = new StringBuilder(); + sb.append("This is fake page displayed to suppress stack trace leak"); + + try { + try { + response.setContentType("text/html"); + response.setCharacterEncoding("utf-8"); + } catch (Throwable t) { + // Ignore + } + Writer writer = response.getReporter(); + if (writer != null) { + // If writer is null, it's an indication that the response has + // been hard committed already, which should never happen + writer.write(sb.toString()); + response.finishResponse(); + } + } catch (IOException | IllegalStateException e) { + // Ignore } -// @Advice.OnMethodExit -// public static void onExit( -// @Advice.Argument(value = 0) Request request, -// @Advice.Argument(value = 1) Response response, -// @Advice.Argument(value = 2) Throwable throwable) { -// System.out.println("<<< ErrorReportValue.report(" + throwable + ") -> " + response.getBytesWritten(false)); -// } + return false; + } + + // @Advice.OnMethodExit + // public static void onExit( + // @Advice.Argument(value = 0) Request request, + // @Advice.Argument(value = 1) Response response, + // @Advice.Argument(value = 2) Throwable throwable) { + // System.out.println("<<< ErrorReportValue.report(" + throwable + ") -> " + + // response.getBytesWritten(false)); + // } } diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java index 9dd72ba37df..f34cd5e1716 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java @@ -1,45 +1,47 @@ package datadog.trace.instrumentation.tomcat7; -import com.google.auto.service.AutoService; -import datadog.trace.agent.tooling.Instrumenter; - import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isProtected; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; + @AutoService(Instrumenter.class) -public class ErrorReportValueInstrumentation extends Instrumenter.AppSec implements Instrumenter.ForSingleType { +public class ErrorReportValueInstrumentation extends Instrumenter.AppSec + implements Instrumenter.ForSingleType { - public ErrorReportValueInstrumentation() { - super("tomcat"); - } + public ErrorReportValueInstrumentation() { + super("tomcat"); + } - @Override - public String instrumentedType() { - return "org.apache.catalina.valves.ErrorReportValve"; - } + @Override + public String instrumentedType() { + return "org.apache.catalina.valves.ErrorReportValve"; + } - @Override - public String[] helperClassNames() { - return new String[] { - "java.lang.StringBuilder", - "java.io.Writer", - "java.io.IOException", - "java.lang.IllegalStateException", - "datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator" - }; - } + @Override + public String[] helperClassNames() { + return new String[] { + "java.lang.StringBuilder", + "java.io.Writer", + "java.io.IOException", + "java.lang.IllegalStateException", + "datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator", + "datadog.trace.instrumentation.tomcat7.TestIast" + }; + } - @Override - public void adviceTransformations(AdviceTransformation transformation) { - transformation.applyAdvice( - isMethod() - .and(named("report")) - .and(takesArgument(0, named("org.apache.catalina.connector.Request"))) - .and(takesArgument(1, named("org.apache.catalina.connector.Response"))) - .and(takesArgument(2, Throwable.class)) - .and(isProtected()), - packageName + ".ErrorReportValueAdvice"); - } + @Override + public void adviceTransformations(AdviceTransformation transformation) { + transformation.applyAdvice( + isMethod() + .and(named("report")) + .and(takesArgument(0, named("org.apache.catalina.connector.Request"))) + .and(takesArgument(1, named("org.apache.catalina.connector.Response"))) + .and(takesArgument(2, Throwable.class)) + .and(isProtected()), + packageName + ".ErrorReportValueAdvice"); + } } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/InstrumentationBridge.java b/internal-api/src/main/java/datadog/trace/api/iast/InstrumentationBridge.java index d00e5473f2e..459ba10196a 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/InstrumentationBridge.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/InstrumentationBridge.java @@ -13,6 +13,7 @@ import datadog.trace.api.iast.sink.PathTraversalModule; import datadog.trace.api.iast.sink.SqlInjectionModule; import datadog.trace.api.iast.sink.SsrfModule; +import datadog.trace.api.iast.sink.StacktraceLeakModule; import datadog.trace.api.iast.sink.TrustBoundaryViolationModule; import datadog.trace.api.iast.sink.UnvalidatedRedirectModule; import datadog.trace.api.iast.sink.WeakCipherModule; @@ -51,6 +52,8 @@ public abstract class InstrumentationBridge { public static volatile XssModule XSS; + public static volatile StacktraceLeakModule STACKTRACE_LEAK_MODULE; + private InstrumentationBridge() {} public static void registerIastModule(final IastModule module) { @@ -96,6 +99,8 @@ public static void registerIastModule(final IastModule module) { TRUST_BOUNDARY_VIOLATION = (TrustBoundaryViolationModule) module; } else if (module instanceof XssModule) { XSS = (XssModule) module; + } else if (module instanceof StacktraceLeakModule) { + STACKTRACE_LEAK_MODULE = (StacktraceLeakModule) module; } else { throw new UnsupportedOperationException("Module not yet supported: " + module); } @@ -167,6 +172,9 @@ public static E getIastModule(final Class type) { if (type == XssModule.class) { return (E) XSS; } + if (type == StacktraceLeakModule.class) { + return (E) STACKTRACE_LEAK_MODULE; + } throw new UnsupportedOperationException("Module not yet supported: " + type); } @@ -193,5 +201,6 @@ public static void clearIastModules() { XPATH_INJECTION = null; TRUST_BOUNDARY_VIOLATION = null; XSS = null; + STACKTRACE_LEAK_MODULE = null; } } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityTypes.java b/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityTypes.java index 74e5a8ce22b..1a21ce795e5 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityTypes.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityTypes.java @@ -38,6 +38,8 @@ private VulnerabilityTypes() {} public static final String TRUST_BOUNDARY_VIOLATION_STRING = "TRUST_BOUNDARY_VIOLATION"; public static final byte XSS = 16; public static final String XSS_STRING = "XSS"; + public static final byte STACKTRACE_LEAK = 17; + public static final String STACKTRACE_LEAK_STRING = "STACKTRACE_LEAK"; /** * Use for telemetry only, this is a special vulnerability type that is not reported, reported @@ -78,7 +80,8 @@ private VulnerabilityTypes() {} HSTS_HEADER_MISSING, XCONTENTTYPE_HEADER_MISSING, NO_SAMESITE_COOKIE, - XSS + XSS, + STACKTRACE_LEAK }; public static byte[] values() { @@ -121,6 +124,8 @@ public static String toString(final byte sourceType) { return VulnerabilityTypes.NO_SAMESITE_COOKIE_STRING; case VulnerabilityTypes.XSS: return VulnerabilityTypes.XSS_STRING; + case VulnerabilityTypes.STACKTRACE_LEAK: + return VulnerabilityTypes.STACKTRACE_LEAK_STRING; default: return null; } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java b/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java new file mode 100644 index 00000000000..50d2a35ca08 --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java @@ -0,0 +1,8 @@ +package datadog.trace.api.iast.sink; + +import datadog.trace.api.iast.IastModule; +import javax.annotation.Nullable; + +public interface StacktraceLeakModule extends IastModule { + void onResponseException(@Nullable final String expression); +} From b0a326f8ef6637f71a8ee445e777532908e0ad30 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Fri, 21 Jul 2023 11:51:15 +0200 Subject: [PATCH 04/15] Report Vulnerability per service --- .../iast/sink/StacktraceLeakModuleImpl.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java index e22e227d33a..004e2603543 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java @@ -3,16 +3,23 @@ import com.datadog.iast.model.Evidence; import com.datadog.iast.model.Vulnerability; import com.datadog.iast.model.VulnerabilityType; +import datadog.trace.api.Config; import datadog.trace.api.iast.sink.StacktraceLeakModule; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; + import javax.annotation.Nullable; public class StacktraceLeakModuleImpl extends SinkModuleBase implements StacktraceLeakModule { - @Override - public void onResponseException(@Nullable String exception) { - final AgentSpan span = AgentTracer.activeSpan(); - reporter.report( - span, new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, null, new Evidence(exception))); - } + @Override + public void onResponseException(@Nullable String exception) { + final AgentSpan span = AgentTracer.activeSpan(); + String serviceName = Config.get().getServiceName(); + reporter.report( + span, + new Vulnerability( + VulnerabilityType.STACKTRACE_LEAK, + null, + new Evidence(serviceName))); + } } From d40856e6ccb58c90b397876cd5911e6b8c5ba0f5 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Wed, 26 Jul 2023 11:30:02 +0200 Subject: [PATCH 05/15] Spotless --- .../decorator/StacktraceLeakDecorator.java | 28 ++--- .../iast/sink/StacktraceLeakModuleImpl.java | 20 ++-- .../datadog/appsec/gateway/GatewayBridge.java | 103 +++++++++--------- .../ErrorReportValueInstrumentation.java | 3 +- .../datadog/trace/api/gateway/Events.java | 9 +- 5 files changed, 80 insertions(+), 83 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java index 20347054c30..1ac1cc0ed67 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java @@ -1,5 +1,7 @@ package datadog.trace.bootstrap.instrumentation.decorator; +import static datadog.trace.api.gateway.Events.EVENTS; + import datadog.trace.api.function.TriFunction; import datadog.trace.api.gateway.CallbackProvider; import datadog.trace.api.gateway.Flow; @@ -8,22 +10,20 @@ import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import static datadog.trace.api.gateway.Events.EVENTS; - public class StacktraceLeakDecorator { - public static final StacktraceLeakDecorator DECORATE = new StacktraceLeakDecorator(); + public static final StacktraceLeakDecorator DECORATE = new StacktraceLeakDecorator(); - public void onStacktraceLeak(final AgentSpan span, Throwable throwable, boolean leaked) { - CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); - RequestContext requestContext = span.getRequestContext(); - if (cbp == null || requestContext == null) { - return; - } - TriFunction> addrCallback = - cbp.getCallback(EVENTS.responseStacktrace()); - if (null != addrCallback) { - addrCallback.apply(requestContext, throwable, leaked); - } + public void onStacktraceLeak(final AgentSpan span, Throwable throwable, boolean leaked) { + CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); + RequestContext requestContext = span.getRequestContext(); + if (cbp == null || requestContext == null) { + return; + } + TriFunction> addrCallback = + cbp.getCallback(EVENTS.responseStacktrace()); + if (null != addrCallback) { + addrCallback.apply(requestContext, throwable, leaked); } + } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java index 004e2603543..1629d93c65a 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java @@ -7,19 +7,15 @@ import datadog.trace.api.iast.sink.StacktraceLeakModule; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; - import javax.annotation.Nullable; public class StacktraceLeakModuleImpl extends SinkModuleBase implements StacktraceLeakModule { - @Override - public void onResponseException(@Nullable String exception) { - final AgentSpan span = AgentTracer.activeSpan(); - String serviceName = Config.get().getServiceName(); - reporter.report( - span, - new Vulnerability( - VulnerabilityType.STACKTRACE_LEAK, - null, - new Evidence(serviceName))); - } + @Override + public void onResponseException(@Nullable String exception) { + final AgentSpan span = AgentTracer.activeSpan(); + String serviceName = Config.get().getServiceName(); + reporter.report( + span, + new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, null, new Evidence(serviceName))); + } } diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java index 80924bee50b..5e98b92798e 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java @@ -387,58 +387,59 @@ public void init() { subscriptionService.registerCallback( EVENTS.responseStacktrace(), (ctx_, throwable, leaked) -> { - AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC); - if (ctx == null || throwable == null) { - return NoopFlow.INSTANCE; - } - - - List parameterList = new ArrayList<>(); - - // Recursive variant -// while (throwable != null) { -// parameterList.add( -// new Parameter.ParameterBuilder() -// .withAddress("Response body") -// //.withKeyPath(Arrays.asList("user-agent2",0)) -// .withValue(throwable.toString()) -// //.withHighlight(Collections.singletonList(throwable.getCause().toString())) -// .build()); -// throwable = throwable.getCause(); -// } - - parameterList.add( - new Parameter.ParameterBuilder() - .withAddress("Response body") - //.withKeyPath(Arrays.asList("user-agent2",0)) - .withValue(throwable.toString()) - .withHighlight(Collections.singletonList(throwable.getCause().toString())) - .build()); - - RuleMatch ruleMatch = - new RuleMatch.RuleMatchBuilder() - //.withOperator("exact_match") - //.withOperatorValue("Exception") - .withParameters(parameterList) - .build(); - - AppSecEvent100 event = new AppSecEvent100.AppSecEvent100Builder() - .withRule( - new Rule.RuleBuilder() - .withId("Stacktrace leak") - //.withName("Stacktrace leak") - .withTags( - new com.datadog.appsec.report.raw.events.Tags.TagsBuilder() - .withType("Exception") - .withCategory("attack_attempt") - .build()) - .build()) - .withRuleMatches(Collections.singletonList(ruleMatch)) - .build(); - ctx.reportEvents(Collections.singletonList(event), null); - - // TODO need to be Flow that suppress stacktrace + AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC); + if (ctx == null || throwable == null) { return NoopFlow.INSTANCE; + } + + List parameterList = new ArrayList<>(); + + // Recursive variant + // while (throwable != null) { + // parameterList.add( + // new Parameter.ParameterBuilder() + // .withAddress("Response body") + // //.withKeyPath(Arrays.asList("user-agent2",0)) + // .withValue(throwable.toString()) + // + // //.withHighlight(Collections.singletonList(throwable.getCause().toString())) + // .build()); + // throwable = throwable.getCause(); + // } + + parameterList.add( + new Parameter.ParameterBuilder() + .withAddress("Response body") + // .withKeyPath(Arrays.asList("user-agent2",0)) + .withValue(throwable.toString()) + .withHighlight(Collections.singletonList(throwable.getCause().toString())) + .build()); + + RuleMatch ruleMatch = + new RuleMatch.RuleMatchBuilder() + // .withOperator("exact_match") + // .withOperatorValue("Exception") + .withParameters(parameterList) + .build(); + + AppSecEvent100 event = + new AppSecEvent100.AppSecEvent100Builder() + .withRule( + new Rule.RuleBuilder() + .withId("Stacktrace leak") + // .withName("Stacktrace leak") + .withTags( + new com.datadog.appsec.report.raw.events.Tags.TagsBuilder() + .withType("Exception") + .withCategory("attack_attempt") + .build()) + .build()) + .withRuleMatches(Collections.singletonList(ruleMatch)) + .build(); + ctx.reportEvents(Collections.singletonList(event), null); + + // TODO need to be Flow that suppress stacktrace + return NoopFlow.INSTANCE; }); } diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java index f34cd5e1716..a2110297b31 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java @@ -28,8 +28,7 @@ public String[] helperClassNames() { "java.io.Writer", "java.io.IOException", "java.lang.IllegalStateException", - "datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator", - "datadog.trace.instrumentation.tomcat7.TestIast" + "datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator" }; } diff --git a/internal-api/src/main/java/datadog/trace/api/gateway/Events.java b/internal-api/src/main/java/datadog/trace/api/gateway/Events.java index 47103ed1b5c..565ded92041 100644 --- a/internal-api/src/main/java/datadog/trace/api/gateway/Events.java +++ b/internal-api/src/main/java/datadog/trace/api/gateway/Events.java @@ -193,14 +193,15 @@ public EventType>> grpcServerReque @SuppressWarnings("rawtypes") private static final EventType RESPONSE_STACKTRACE = - new ET<>("response.stacktrace_leak", RESPONSE_STACKTRACE_ID); + new ET<>("response.stacktrace_leak", RESPONSE_STACKTRACE_ID); /** Response contains stacktrace */ @SuppressWarnings("unchecked") - public EventType>> responseStacktrace() { - return (EventType>>) RESPONSE_STACKTRACE; + public EventType>> + responseStacktrace() { + return (EventType>>) + RESPONSE_STACKTRACE; } - static final int MAX_EVENTS = nextId.get(); private static final class ET extends EventType { From 918ba92c6f546a004efb7dfe113253fcfea222b6 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Mon, 31 Jul 2023 17:44:31 +0200 Subject: [PATCH 06/15] Revert commit ed24a0a7 --- .../decorator/StacktraceLeakDecorator.java | 29 ---------- .../datadog/appsec/gateway/GatewayBridge.java | 58 ------------------- .../tomcat7/ErrorReportValueAdvice.java | 4 -- .../ErrorReportValueInstrumentation.java | 3 +- .../datadog/trace/api/gateway/Events.java | 13 ----- 5 files changed, 1 insertion(+), 106 deletions(-) diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java index 1ac1cc0ed67..e69de29bb2d 100644 --- a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java +++ b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java @@ -1,29 +0,0 @@ -package datadog.trace.bootstrap.instrumentation.decorator; - -import static datadog.trace.api.gateway.Events.EVENTS; - -import datadog.trace.api.function.TriFunction; -import datadog.trace.api.gateway.CallbackProvider; -import datadog.trace.api.gateway.Flow; -import datadog.trace.api.gateway.RequestContext; -import datadog.trace.api.gateway.RequestContextSlot; -import datadog.trace.bootstrap.instrumentation.api.AgentSpan; -import datadog.trace.bootstrap.instrumentation.api.AgentTracer; - -public class StacktraceLeakDecorator { - - public static final StacktraceLeakDecorator DECORATE = new StacktraceLeakDecorator(); - - public void onStacktraceLeak(final AgentSpan span, Throwable throwable, boolean leaked) { - CallbackProvider cbp = AgentTracer.get().getCallbackProvider(RequestContextSlot.APPSEC); - RequestContext requestContext = span.getRequestContext(); - if (cbp == null || requestContext == null) { - return; - } - TriFunction> addrCallback = - cbp.getCallback(EVENTS.responseStacktrace()); - if (null != addrCallback) { - addrCallback.apply(requestContext, throwable, leaked); - } - } -} diff --git a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java index 5e98b92798e..e89927ebb44 100644 --- a/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java +++ b/dd-java-agent/appsec/src/main/java/com/datadog/appsec/gateway/GatewayBridge.java @@ -383,64 +383,6 @@ public void init() { } } }); - - subscriptionService.registerCallback( - EVENTS.responseStacktrace(), - (ctx_, throwable, leaked) -> { - AppSecRequestContext ctx = ctx_.getData(RequestContextSlot.APPSEC); - if (ctx == null || throwable == null) { - return NoopFlow.INSTANCE; - } - - List parameterList = new ArrayList<>(); - - // Recursive variant - // while (throwable != null) { - // parameterList.add( - // new Parameter.ParameterBuilder() - // .withAddress("Response body") - // //.withKeyPath(Arrays.asList("user-agent2",0)) - // .withValue(throwable.toString()) - // - // //.withHighlight(Collections.singletonList(throwable.getCause().toString())) - // .build()); - // throwable = throwable.getCause(); - // } - - parameterList.add( - new Parameter.ParameterBuilder() - .withAddress("Response body") - // .withKeyPath(Arrays.asList("user-agent2",0)) - .withValue(throwable.toString()) - .withHighlight(Collections.singletonList(throwable.getCause().toString())) - .build()); - - RuleMatch ruleMatch = - new RuleMatch.RuleMatchBuilder() - // .withOperator("exact_match") - // .withOperatorValue("Exception") - .withParameters(parameterList) - .build(); - - AppSecEvent100 event = - new AppSecEvent100.AppSecEvent100Builder() - .withRule( - new Rule.RuleBuilder() - .withId("Stacktrace leak") - // .withName("Stacktrace leak") - .withTags( - new com.datadog.appsec.report.raw.events.Tags.TagsBuilder() - .withType("Exception") - .withCategory("attack_attempt") - .build()) - .build()) - .withRuleMatches(Collections.singletonList(ruleMatch)) - .build(); - ctx.reportEvents(Collections.singletonList(event), null); - - // TODO need to be Flow that suppress stacktrace - return NoopFlow.INSTANCE; - }); } public void stop() { diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java index a0104126d76..433b3d45b36 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -5,7 +5,6 @@ import datadog.trace.api.iast.InstrumentationBridge; import datadog.trace.api.iast.sink.StacktraceLeakModule; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; -import datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator; import java.io.IOException; import java.io.Writer; import net.bytebuddy.asm.Advice; @@ -34,9 +33,6 @@ public static boolean onEnter( final AgentSpan span = activeSpan(); if (span != null && throwable != null) { - // Report AppSec - StacktraceLeakDecorator.DECORATE.onStacktraceLeak(span, throwable, false); - // Report IAST final StacktraceLeakModule module = InstrumentationBridge.STACKTRACE_LEAK_MODULE; if (module != null) { diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java index a2110297b31..73be254ef1d 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java @@ -27,8 +27,7 @@ public String[] helperClassNames() { "java.lang.StringBuilder", "java.io.Writer", "java.io.IOException", - "java.lang.IllegalStateException", - "datadog.trace.bootstrap.instrumentation.decorator.StacktraceLeakDecorator" + "java.lang.IllegalStateException" }; } diff --git a/internal-api/src/main/java/datadog/trace/api/gateway/Events.java b/internal-api/src/main/java/datadog/trace/api/gateway/Events.java index 565ded92041..693a5ca7b34 100644 --- a/internal-api/src/main/java/datadog/trace/api/gateway/Events.java +++ b/internal-api/src/main/java/datadog/trace/api/gateway/Events.java @@ -189,19 +189,6 @@ public EventType>> grpcServerReque return (EventType>>) GRPC_SERVER_REQUEST_MESSAGE; } - static final int RESPONSE_STACKTRACE_ID = 15; - - @SuppressWarnings("rawtypes") - private static final EventType RESPONSE_STACKTRACE = - new ET<>("response.stacktrace_leak", RESPONSE_STACKTRACE_ID); - /** Response contains stacktrace */ - @SuppressWarnings("unchecked") - public EventType>> - responseStacktrace() { - return (EventType>>) - RESPONSE_STACKTRACE; - } - static final int MAX_EVENTS = nextId.get(); private static final class ET extends EventType { From 02dc5b57d7d3ec558019496ca52745cd99ed11a7 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Tue, 1 Aug 2023 13:57:43 +0200 Subject: [PATCH 07/15] Minor fixes --- .../decorator/StacktraceLeakDecorator.java | 0 .../iast/sink/StacktraceLeakModuleImpl.java | 15 ++++++++------- .../tomcat7/ErrorReportValueAdvice.java | 2 +- .../trace/api/iast/sink/StacktraceLeakModule.java | 2 +- 4 files changed, 10 insertions(+), 9 deletions(-) delete mode 100644 dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java diff --git a/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java b/dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/StacktraceLeakDecorator.java deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java index 1629d93c65a..45051da060d 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java @@ -7,15 +7,16 @@ import datadog.trace.api.iast.sink.StacktraceLeakModule; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import javax.annotation.Nullable; public class StacktraceLeakModuleImpl extends SinkModuleBase implements StacktraceLeakModule { @Override - public void onResponseException(@Nullable String exception) { - final AgentSpan span = AgentTracer.activeSpan(); - String serviceName = Config.get().getServiceName(); - reporter.report( - span, - new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, null, new Evidence(serviceName))); + public void onStacktraceLeak(Throwable throwable) { + if (throwable != null) { + final AgentSpan span = AgentTracer.activeSpan(); + String serviceName = Config.get().getServiceName(); + reporter.report( + span, + new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, null, new Evidence(serviceName))); + } } } diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java index 433b3d45b36..0dc9ffd26f5 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -37,7 +37,7 @@ public static boolean onEnter( final StacktraceLeakModule module = InstrumentationBridge.STACKTRACE_LEAK_MODULE; if (module != null) { try { - module.onResponseException(throwable.getMessage()); + module.onStacktraceLeak(throwable); } catch (final Throwable e) { module.onUnexpectedException("onResponseException threw", e); } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java b/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java index 50d2a35ca08..05f11f75164 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java @@ -4,5 +4,5 @@ import javax.annotation.Nullable; public interface StacktraceLeakModule extends IastModule { - void onResponseException(@Nullable final String expression); + void onStacktraceLeak(@Nullable final Throwable expression); } From 6e2104f73e188ed56db4b704ce5af2df67bca743 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Tue, 1 Aug 2023 15:49:51 +0200 Subject: [PATCH 08/15] Use default built-in blocking page --- .../iast/sink/StacktraceLeakModuleImpl.java | 4 +-- .../tomcat7/ErrorReportValueAdvice.java | 33 +++++++------------ .../ErrorReportValueInstrumentation.java | 3 +- 3 files changed, 16 insertions(+), 24 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java index 45051da060d..0dc85297bf5 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java @@ -15,8 +15,8 @@ public void onStacktraceLeak(Throwable throwable) { final AgentSpan span = AgentTracer.activeSpan(); String serviceName = Config.get().getServiceName(); reporter.report( - span, - new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, null, new Evidence(serviceName))); + span, + new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, null, new Evidence(serviceName))); } } } diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java index 0dc9ffd26f5..34b96d6aa4c 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -1,33 +1,30 @@ package datadog.trace.instrumentation.tomcat7; +import static datadog.trace.bootstrap.blocking.BlockingActionHelper.TemplateType.HTML; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; import datadog.trace.api.iast.InstrumentationBridge; import datadog.trace.api.iast.sink.StacktraceLeakModule; +import datadog.trace.bootstrap.blocking.BlockingActionHelper; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import java.io.IOException; import java.io.Writer; +import java.nio.charset.StandardCharsets; import net.bytebuddy.asm.Advice; -import org.apache.catalina.connector.Request; import org.apache.catalina.connector.Response; public class ErrorReportValueAdvice { @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) public static boolean onEnter( - @Advice.Argument(value = 0) Request request, @Advice.Argument(value = 1) Response response, @Advice.Argument(value = 2) Throwable throwable) { - System.out.println( - ">>> ErrorReportValue.report(" + throwable + ") -> " + response.getBytesWritten(false)); - int statusCode = response.getStatus(); // Do nothing on a 1xx, 2xx and 3xx status - // Do nothing if anything has been written already // Do nothing if the response hasn't been explicitly marked as in error // and that error has not been reported. - if (statusCode < 400 || response.getContentWritten() > 0 || !response.isError()) { + if (statusCode < 400 || !response.isError()) { return true; // skip original method } @@ -44,13 +41,15 @@ public static boolean onEnter( } } - StringBuilder sb = new StringBuilder(); - sb.append("This is fake page displayed to suppress stack trace leak"); + byte[] template = BlockingActionHelper.getTemplate(HTML); + if (template == null) { + return false; + } try { try { - response.setContentType("text/html"); - response.setCharacterEncoding("utf-8"); + String contentType = BlockingActionHelper.getContentType(HTML); + response.setContentType(contentType); } catch (Throwable t) { // Ignore } @@ -58,7 +57,8 @@ public static boolean onEnter( if (writer != null) { // If writer is null, it's an indication that the response has // been hard committed already, which should never happen - writer.write(sb.toString()); + String html = new String(template, StandardCharsets.UTF_8); + writer.write(html); response.finishResponse(); } } catch (IOException | IllegalStateException e) { @@ -67,13 +67,4 @@ public static boolean onEnter( return false; } - - // @Advice.OnMethodExit - // public static void onExit( - // @Advice.Argument(value = 0) Request request, - // @Advice.Argument(value = 1) Response response, - // @Advice.Argument(value = 2) Throwable throwable) { - // System.out.println("<<< ErrorReportValue.report(" + throwable + ") -> " + - // response.getBytesWritten(false)); - // } } diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java index 73be254ef1d..c54580970d7 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java @@ -27,7 +27,8 @@ public String[] helperClassNames() { "java.lang.StringBuilder", "java.io.Writer", "java.io.IOException", - "java.lang.IllegalStateException" + "java.lang.IllegalStateException", + "datadog.trace.bootstrap.blocking.BlockingActionHelper" }; } From 2b0c454530dbdd245b94476b3432bae4520b660b Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Wed, 2 Aug 2023 23:51:58 +0200 Subject: [PATCH 09/15] Instrumentation switched to IAST --- .../tomcat7/ErrorReportValueInstrumentation.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java index c54580970d7..62d736824eb 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java @@ -9,7 +9,7 @@ import datadog.trace.agent.tooling.Instrumenter; @AutoService(Instrumenter.class) -public class ErrorReportValueInstrumentation extends Instrumenter.AppSec +public class ErrorReportValueInstrumentation extends Instrumenter.Iast implements Instrumenter.ForSingleType { public ErrorReportValueInstrumentation() { From dfc201e6a0ffd45aeb8b3ad1c5423c33ae665aac Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Wed, 9 Aug 2023 23:15:29 +0200 Subject: [PATCH 10/15] Skip error 404 --- .../trace/instrumentation/tomcat7/ErrorReportValueAdvice.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java index 34b96d6aa4c..8d2d6832974 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -21,10 +21,10 @@ public static boolean onEnter( @Advice.Argument(value = 2) Throwable throwable) { int statusCode = response.getStatus(); - // Do nothing on a 1xx, 2xx and 3xx status + // Do nothing on a 1xx, 2xx, 3xx and 404 status // Do nothing if the response hasn't been explicitly marked as in error // and that error has not been reported. - if (statusCode < 400 || !response.isError()) { + if (statusCode < 400 || statusCode == 404 || !response.isError()) { return true; // skip original method } From bc761edbfc5ff294ae44454cd7db98ef18474971 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Fri, 18 Aug 2023 23:16:26 +0200 Subject: [PATCH 11/15] Added option to disable stacktrace suppression --- .../tomcat7/ErrorReportValueAdvice.java | 6 ++++++ .../main/java/datadog/trace/api/ConfigDefaults.java | 1 + .../main/java/datadog/trace/api/config/IastConfig.java | 1 + .../src/main/java/datadog/trace/api/Config.java | 10 ++++++++++ 4 files changed, 18 insertions(+) diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java index 8d2d6832974..c0fec063fa6 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -3,6 +3,7 @@ import static datadog.trace.bootstrap.blocking.BlockingActionHelper.TemplateType.HTML; import static datadog.trace.bootstrap.instrumentation.api.AgentTracer.activeSpan; +import datadog.trace.api.Config; import datadog.trace.api.iast.InstrumentationBridge; import datadog.trace.api.iast.sink.StacktraceLeakModule; import datadog.trace.bootstrap.blocking.BlockingActionHelper; @@ -41,6 +42,11 @@ public static boolean onEnter( } } + // If we don't need to suppress stacktrace leak + if (!Config.get().isIastStacktraceLeakSuppress()) { + return false; + } + byte[] template = BlockingActionHelper.getTemplate(HTML); if (template == null) { return false; diff --git a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java index 24cd05cacdf..daf57bd9c5f 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java @@ -105,6 +105,7 @@ public final class ConfigDefaults { static final String DEFAULT_IAST_REDACTION_VALUE_PATTERN = "(?:bearer\\s+[a-z0-9\\._\\-]+|glpat-[\\w\\-]{20}|gh[opsu]_[0-9a-zA-Z]{36}|ey[I-L][\\w=\\-]+\\.ey[I-L][\\w=\\-]+(?:\\.[\\w.+/=\\-]+)?|(?:[\\-]{5}BEGIN[a-z\\s]+PRIVATE\\sKEY[\\-]{5}[^\\-]+[\\-]{5}END[a-z\\s]+PRIVATE\\sKEY[\\-]{5}|ssh-rsa\\s*[a-z0-9/\\.+]{100,}))"; public static final int DEFAULT_IAST_MAX_RANGE_COUNT = 10; + static final boolean DEFAULT_IAST_STACKTRACE_LEAK_SUPPRESS = true; static final int DEFAULT_IAST_TRUNCATION_MAX_VALUE_LENGTH = 250; public static final boolean DEFAULT_IAST_DEDUPLICATION_ENABLED = true; 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 3b6aa5c284c..286533c8230 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 @@ -16,6 +16,7 @@ public final class IastConfig { public static final String IAST_REDACTION_ENABLED = "iast.redaction.enabled"; 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_MAX_RANGE_COUNT = "iast.max-range-count"; public static final String IAST_TRUNCATION_MAX_VALUE_LENGTH = "iast.trunctation.max.value.length"; 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 5f0aceaef72..3f252209497 100644 --- a/internal-api/src/main/java/datadog/trace/api/Config.java +++ b/internal-api/src/main/java/datadog/trace/api/Config.java @@ -68,6 +68,7 @@ import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_REDACTION_ENABLED; import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_REDACTION_NAME_PATTERN; import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_REDACTION_VALUE_PATTERN; +import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_STACKTRACE_LEAK_SUPPRESS; import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_TRUNCATION_MAX_VALUE_LENGTH; import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_WEAK_CIPHER_ALGORITHMS; import static datadog.trace.api.ConfigDefaults.DEFAULT_IAST_WEAK_HASH_ALGORITHMS; @@ -237,6 +238,7 @@ import static datadog.trace.api.config.IastConfig.IAST_REDACTION_ENABLED; import static datadog.trace.api.config.IastConfig.IAST_REDACTION_NAME_PATTERN; import static datadog.trace.api.config.IastConfig.IAST_REDACTION_VALUE_PATTERN; +import static datadog.trace.api.config.IastConfig.IAST_STACKTRACE_LEAK_SUPPRESS; import static datadog.trace.api.config.IastConfig.IAST_TELEMETRY_VERBOSITY; import static datadog.trace.api.config.IastConfig.IAST_TRUNCATION_MAX_VALUE_LENGTH; import static datadog.trace.api.config.IastConfig.IAST_WEAK_CIPHER_ALGORITHMS; @@ -677,6 +679,7 @@ static class HostNameHolder { private final String iastRedactionValuePattern; private final int iastMaxRangeCount; private final int iastTruncationMaxValueLength; + private final boolean iastStacktraceLeakSuppress; private final boolean ciVisibilityTraceSanitationEnabled; private final boolean ciVisibilityAgentlessEnabled; @@ -1532,6 +1535,9 @@ private Config(final ConfigProvider configProvider, final InstrumenterConfig ins configProvider.getInteger( IAST_TRUNCATION_MAX_VALUE_LENGTH, DEFAULT_IAST_TRUNCATION_MAX_VALUE_LENGTH); iastMaxRangeCount = iastDetectionMode.getIastMaxRangeCount(configProvider); + iastStacktraceLeakSuppress = + configProvider.getBoolean( + IAST_STACKTRACE_LEAK_SUPPRESS, DEFAULT_IAST_STACKTRACE_LEAK_SUPPRESS); ciVisibilityTraceSanitationEnabled = configProvider.getBoolean(CIVISIBILITY_TRACE_SANITATION_ENABLED, true); @@ -2563,6 +2569,10 @@ public int getIastMaxRangeCount() { return iastMaxRangeCount; } + public boolean isIastStacktraceLeakSuppress() { + return iastStacktraceLeakSuppress; + } + public boolean isCiVisibilityEnabled() { return instrumenterConfig.isCiVisibilityEnabled(); } From b88adafdf29d8073a233b80a1b1da474bee51b0e Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Thu, 24 Aug 2023 17:34:59 +0200 Subject: [PATCH 12/15] Removed redundant helpers in the Tomcat instrumentation --- .../tomcat7/ErrorReportValueInstrumentation.java | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java index 62d736824eb..864c132da58 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java @@ -21,17 +21,6 @@ public String instrumentedType() { return "org.apache.catalina.valves.ErrorReportValve"; } - @Override - public String[] helperClassNames() { - return new String[] { - "java.lang.StringBuilder", - "java.io.Writer", - "java.io.IOException", - "java.lang.IllegalStateException", - "datadog.trace.bootstrap.blocking.BlockingActionHelper" - }; - } - @Override public void adviceTransformations(AdviceTransformation transformation) { transformation.applyAdvice( From 8ff8f5fa2f8f47a078be82e635308ae3e82f139f Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Thu, 24 Aug 2023 17:37:37 +0200 Subject: [PATCH 13/15] Improved vulnerability reporting data --- .../java/com/datadog/iast/IastSystem.java | 2 +- .../iast/sink/StacktraceLeakModuleImpl.java | 25 +++++++++++++++---- .../iast/sink/StacktraceLeakModuleTest.groovy | 2 +- .../tomcat7/ErrorReportValueAdvice.java | 6 +++-- .../ErrorReportValueInstrumentation.java | 5 ++++ .../api/iast/sink/StacktraceLeakModule.java | 3 ++- 6 files changed, 33 insertions(+), 10 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 971aaae0106..5da60ff4d30 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 @@ -102,7 +102,7 @@ private static Stream iastModules(final Dependencies dependencies) { new XPathInjectionModuleImpl(dependencies), new TrustBoundaryViolationModuleImpl(dependencies), new XssModuleImpl(dependencies), - new StacktraceLeakModuleImpl()); + new StacktraceLeakModuleImpl(dependencies)); } private static void registerRequestStartedCallback( diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java index 0dc85297bf5..2cfb07e5f49 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/StacktraceLeakModuleImpl.java @@ -1,22 +1,37 @@ package com.datadog.iast.sink; +import com.datadog.iast.Dependencies; import com.datadog.iast.model.Evidence; +import com.datadog.iast.model.Location; import com.datadog.iast.model.Vulnerability; import com.datadog.iast.model.VulnerabilityType; -import datadog.trace.api.Config; import datadog.trace.api.iast.sink.StacktraceLeakModule; import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.api.AgentTracer; +import org.jetbrains.annotations.NotNull; public class StacktraceLeakModuleImpl extends SinkModuleBase implements StacktraceLeakModule { + + public StacktraceLeakModuleImpl(@NotNull Dependencies dependencies) { + super(dependencies); + } + @Override - public void onStacktraceLeak(Throwable throwable) { + public void onStacktraceLeak( + Throwable throwable, String moduleName, String className, String methodName) { if (throwable != null) { final AgentSpan span = AgentTracer.activeSpan(); - String serviceName = Config.get().getServiceName(); + + Evidence evidence = + new Evidence( + "ExceptionHandler in " + + moduleName + + " \r\nthrown " + + throwable.getClass().getName()); + Location location = Location.forSpanAndClassAndMethod(span, className, methodName); + reporter.report( - span, - new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, null, new Evidence(serviceName))); + span, new Vulnerability(VulnerabilityType.STACKTRACE_LEAK, location, evidence)); } } } diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/StacktraceLeakModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/StacktraceLeakModuleTest.groovy index 323554dfbe9..ecf8aaa3bd0 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/StacktraceLeakModuleTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/StacktraceLeakModuleTest.groovy @@ -11,7 +11,7 @@ class StacktraceLeakModuleTest extends IastModuleImplTestBase { private StacktraceLeakModule module def setup() { - module = registerDependencies(new StacktraceLeakModuleImpl()) + module = new StacktraceLeakModuleImpl(dependencies) } void 'iast stacktrace leak module'() { diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java index c0fec063fa6..6d5cf633435 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueAdvice.java @@ -19,7 +19,9 @@ public class ErrorReportValueAdvice { @Advice.OnMethodEnter(skipOn = Advice.OnNonDefaultValue.class) public static boolean onEnter( @Advice.Argument(value = 1) Response response, - @Advice.Argument(value = 2) Throwable throwable) { + @Advice.Argument(value = 2) Throwable throwable, + @Advice.Origin("#t") String className, + @Advice.Origin("#m") String methodName) { int statusCode = response.getStatus(); // Do nothing on a 1xx, 2xx, 3xx and 404 status @@ -35,7 +37,7 @@ public static boolean onEnter( final StacktraceLeakModule module = InstrumentationBridge.STACKTRACE_LEAK_MODULE; if (module != null) { try { - module.onStacktraceLeak(throwable); + module.onStacktraceLeak(throwable, "Tomcat 7+", className, methodName); } catch (final Throwable e) { module.onUnexpectedException("onResponseException threw", e); } diff --git a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java index 864c132da58..117ce32f082 100644 --- a/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java +++ b/dd-java-agent/instrumentation/tomcat-appsec-7/src/main/java/datadog/trace/instrumentation/tomcat7/ErrorReportValueInstrumentation.java @@ -16,6 +16,11 @@ public ErrorReportValueInstrumentation() { super("tomcat"); } + @Override + public String muzzleDirective() { + return "from7"; + } + @Override public String instrumentedType() { return "org.apache.catalina.valves.ErrorReportValve"; diff --git a/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java b/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java index 05f11f75164..2a6a668303c 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/sink/StacktraceLeakModule.java @@ -4,5 +4,6 @@ import javax.annotation.Nullable; public interface StacktraceLeakModule extends IastModule { - void onStacktraceLeak(@Nullable final Throwable expression); + void onStacktraceLeak( + @Nullable final Throwable expression, String moduleName, String className, String methodName); } From a7dd60866cf06755783d8fa6d786618b20b6fcc2 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Mon, 16 Oct 2023 14:18:53 +0200 Subject: [PATCH 14/15] Stacktrace leak suppress - disabled by default --- .../src/main/java/datadog/trace/api/ConfigDefaults.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java index daf57bd9c5f..940d44fb5da 100644 --- a/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java +++ b/dd-trace-api/src/main/java/datadog/trace/api/ConfigDefaults.java @@ -105,7 +105,7 @@ public final class ConfigDefaults { static final String DEFAULT_IAST_REDACTION_VALUE_PATTERN = "(?:bearer\\s+[a-z0-9\\._\\-]+|glpat-[\\w\\-]{20}|gh[opsu]_[0-9a-zA-Z]{36}|ey[I-L][\\w=\\-]+\\.ey[I-L][\\w=\\-]+(?:\\.[\\w.+/=\\-]+)?|(?:[\\-]{5}BEGIN[a-z\\s]+PRIVATE\\sKEY[\\-]{5}[^\\-]+[\\-]{5}END[a-z\\s]+PRIVATE\\sKEY[\\-]{5}|ssh-rsa\\s*[a-z0-9/\\.+]{100,}))"; public static final int DEFAULT_IAST_MAX_RANGE_COUNT = 10; - static final boolean DEFAULT_IAST_STACKTRACE_LEAK_SUPPRESS = true; + static final boolean DEFAULT_IAST_STACKTRACE_LEAK_SUPPRESS = false; static final int DEFAULT_IAST_TRUNCATION_MAX_VALUE_LENGTH = 250; public static final boolean DEFAULT_IAST_DEDUPLICATION_ENABLED = true; From 3db28f39f77a4e37be1dfad6cf3a3d42c40a7588 Mon Sep 17 00:00:00 2001 From: Valentin Zakharov Date: Wed, 1 Nov 2023 23:01:31 +0100 Subject: [PATCH 15/15] Added spring-tomcat7 smoke test application --- .../appsec/spring-tomcat7/build.gradle | 33 +++++++++++ .../appsec/springtomcat7/AppConfigurer.java | 11 ++++ .../appsec/springtomcat7/Controller.java | 18 ++++++ .../smoketest/appsec/springtomcat7/Main.java | 56 +++++++++++++++++++ .../appsec/SpringTomcatSmokeTest.groovy | 36 ++++++++++++ settings.gradle | 1 + 6 files changed, 155 insertions(+) create mode 100644 dd-smoke-tests/appsec/spring-tomcat7/build.gradle create mode 100644 dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/AppConfigurer.java create mode 100644 dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/Controller.java create mode 100644 dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/Main.java create mode 100644 dd-smoke-tests/appsec/spring-tomcat7/src/test/groovy/datadog/smoketest/appsec/SpringTomcatSmokeTest.groovy diff --git a/dd-smoke-tests/appsec/spring-tomcat7/build.gradle b/dd-smoke-tests/appsec/spring-tomcat7/build.gradle new file mode 100644 index 00000000000..bb3b9ddf5e9 --- /dev/null +++ b/dd-smoke-tests/appsec/spring-tomcat7/build.gradle @@ -0,0 +1,33 @@ +plugins { + id "com.github.johnrengelman.shadow" +} + +apply from: "$rootDir/gradle/java.gradle" +description = 'Spring Tomcat7 Smoke Tests.' + +jar { + manifest { + attributes('Main-Class': 'datadog.smoketest.appsec.springtomcat7.Main') + } +} + +dependencies { + implementation group: 'org.apache.tomcat.embed', name: 'tomcat-embed-jasper', version: '7.0.47' + implementation group: 'org.apache.tomcat.embed', name: 'tomcat-embed-core', version: '7.0.47' + implementation group: 'org.apache.tomcat', name: 'tomcat-juli', version: '7.0.47' + implementation group: 'org.springframework', name: 'spring-webmvc', version: '4.0.0.RELEASE' + + testImplementation project(':dd-smoke-tests:appsec') +} + +tasks.withType(Test).configureEach { + dependsOn "shadowJar" + + jvmArgs "-Ddatadog.smoketest.appsec.springtomcat7.shadowJar.path=${tasks.shadowJar.archiveFile.get()}" +} + +task testRuntimeActivation(type: Test) { + jvmArgs '-Dsmoke_test.appsec.enabled=inactive', + "-Ddatadog.smoketest.appsec.springtomcat7.shadowJar.path=${tasks.shadowJar.archiveFile.get()}" +} +tasks['check'].dependsOn(testRuntimeActivation) diff --git a/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/AppConfigurer.java b/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/AppConfigurer.java new file mode 100644 index 00000000000..f32a9d67b82 --- /dev/null +++ b/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/AppConfigurer.java @@ -0,0 +1,11 @@ +package datadog.smoketest.appsec.springtomcat7; + +import org.springframework.context.annotation.ComponentScan; +import org.springframework.context.annotation.Configuration; +import org.springframework.web.servlet.config.annotation.EnableWebMvc; +import org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter; + +@Configuration +@EnableWebMvc +@ComponentScan(basePackages = {"datadog.smoketest.appsec.springtomcat7"}) +public class AppConfigurer extends WebMvcConfigurerAdapter {} diff --git a/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/Controller.java b/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/Controller.java new file mode 100644 index 00000000000..54e64878cf6 --- /dev/null +++ b/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/Controller.java @@ -0,0 +1,18 @@ +package datadog.smoketest.appsec.springtomcat7; + +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RestController; + +@RestController +public class Controller { + + @RequestMapping("/") + public String htmlString() { + return "Hello world!"; + } + + @RequestMapping("/exception") + public void exceptionMethod() throws Throwable { + throw new Throwable("hello"); + } +} diff --git a/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/Main.java b/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/Main.java new file mode 100644 index 00000000000..629bd4e8ef5 --- /dev/null +++ b/dd-smoke-tests/appsec/spring-tomcat7/src/main/java/datadog/smoketest/appsec/springtomcat7/Main.java @@ -0,0 +1,56 @@ +package datadog.smoketest.appsec.springtomcat7; + +import de.thetaphi.forbiddenapis.SuppressForbidden; +import java.io.File; +import org.apache.catalina.Context; +import org.apache.catalina.startup.Tomcat; +import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; +import org.springframework.web.servlet.DispatcherServlet; + +public class Main { + + private static final String ROOT = "/"; + private static final String SERVLET = "dispatcherServlet"; + + @SuppressForbidden + public static void main(String[] args) throws Exception { + int port = 8080; + for (String arg : args) { + if (arg.contains("=")) { + String[] kv = arg.split("="); + if (kv.length == 2) { + if ("--server.port".equalsIgnoreCase(kv[0])) { + try { + port = Integer.parseInt(kv[1]); + } catch (NumberFormatException e) { + System.out.println( + "--server.port '" + + kv[1] + + "' is not valid port. Will be used default port " + + port); + } + } + } + } + } + + Tomcat tomcat = new Tomcat(); + tomcat.setPort(port); + + Context context = tomcat.addContext(ROOT, new File(".").getAbsolutePath()); + + Tomcat.addServlet( + context, + SERVLET, + new DispatcherServlet( + new AnnotationConfigWebApplicationContext() { + { + register(AppConfigurer.class); + } + })); + context.addServletMapping(ROOT, SERVLET); + + tomcat.start(); + tomcat.getServer().await(); + } +} diff --git a/dd-smoke-tests/appsec/spring-tomcat7/src/test/groovy/datadog/smoketest/appsec/SpringTomcatSmokeTest.groovy b/dd-smoke-tests/appsec/spring-tomcat7/src/test/groovy/datadog/smoketest/appsec/SpringTomcatSmokeTest.groovy new file mode 100644 index 00000000000..75e7168cd3f --- /dev/null +++ b/dd-smoke-tests/appsec/spring-tomcat7/src/test/groovy/datadog/smoketest/appsec/SpringTomcatSmokeTest.groovy @@ -0,0 +1,36 @@ +package datadog.smoketest.appsec + +import okhttp3.Request + +class SpringTomcatSmokeTest extends AbstractAppSecServerSmokeTest { + + @Override + ProcessBuilder createProcessBuilder() { + String springBootShadowJar = System.getProperty("datadog.smoketest.appsec.springtomcat7.shadowJar.path") + + List command = new ArrayList<>() + command.add(javaPath()) + command.addAll(defaultJavaProperties) + command.add("-Ddd.iast.enabled=true") + command.add("-Ddd.iast.stacktrace-leak.suppress=true") + command.addAll((String[]) ["-jar", springBootShadowJar, "--server.port=${httpPort}"]) + + ProcessBuilder processBuilder = new ProcessBuilder(command) + processBuilder.directory(new File(buildDirectory)) + } + + def "suppress exception stacktrace"() { + when: + String url = "http://localhost:${httpPort}/exception" + def request = new Request.Builder() + .url(url) + .build() + def response = client.newCall(request).execute() + def responseBodyStr = response.body().string() + waitForTraceCount 1 + + then: + responseBodyStr.contains('Sorry, you cannot access this page. Please contact the customer service team.') + response.code() == 500 + } +} \ No newline at end of file diff --git a/settings.gradle b/settings.gradle index d5917b55a68..80f4db89e19 100644 --- a/settings.gradle +++ b/settings.gradle @@ -141,6 +141,7 @@ include ':dd-smoke-tests:vertx-3.9-resteasy' include ':dd-smoke-tests:vertx-4.2' include ':dd-smoke-tests:wildfly' include ':dd-smoke-tests:appsec' +include ':dd-smoke-tests:appsec:spring-tomcat7' include ':dd-smoke-tests:appsec:springboot' include ':dd-smoke-tests:appsec:springboot-grpc' include ':dd-smoke-tests:appsec:springboot-security'