From ea1ae4a6691a2776c1311b3232a26d50dcf4cc51 Mon Sep 17 00:00:00 2001 From: Alejandro Gonzalez Date: Wed, 14 Feb 2024 11:23:54 +0100 Subject: [PATCH 01/18] Add session rewriting support --- .../java/com/datadog/iast/IastSystem.java | 4 +- .../datadog/iast/model/VulnerabilityType.java | 3 + .../iast/sink/SessionRewritingModuleImpl.java | 43 +++ .../sink/SessionRewritingModuleTest.groovy | 51 +++ .../test/groovy/RequestDispatcher2Utils.java | 152 +++++++++ .../IastServlet3ContextInstrumentation.java | 82 +++++ ...tServlet3ContextInstrumentationTest.groovy | 62 ++++ .../smoketest/RequestDispatcher3Utils.java | 283 ++++++++++++++++ ...tJakartaServletContextInstrumentation.java | 78 +++++ ...taServletContextInstrumentationTest.groovy | 62 ++++ .../groovy/JakartaRequestDispatcherUtils.java | 304 ++++++++++++++++++ .../trace/api/iast/InstrumentationBridge.java | 2 + .../trace/api/iast/VulnerabilityTypes.java | 7 +- .../api/iast/sink/SessionRewritingModule.java | 9 + .../api/iast/VulnerabilityTypesTest.groovy | 1 + 15 files changed, 1140 insertions(+), 3 deletions(-) create mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java create mode 100644 dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/SessionRewritingModuleTest.groovy create mode 100644 dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/RequestDispatcher2Utils.java create mode 100644 dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java create mode 100644 dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationTest.groovy create mode 100644 dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java create mode 100644 dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java create mode 100644 dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletContextInstrumentationTest.groovy create mode 100644 dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaRequestDispatcherUtils.java create mode 100644 internal-api/src/main/java/datadog/trace/api/iast/sink/SessionRewritingModule.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 8e21b068d59..b56024199ba 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 @@ -21,6 +21,7 @@ import com.datadog.iast.sink.NoSameSiteCookieModuleImpl; import com.datadog.iast.sink.PathTraversalModuleImpl; import com.datadog.iast.sink.ReflectionInjectionModuleImpl; +import com.datadog.iast.sink.SessionRewritingModuleImpl; import com.datadog.iast.sink.SqlInjectionModuleImpl; import com.datadog.iast.sink.SsrfModuleImpl; import com.datadog.iast.sink.StacktraceLeakModuleImpl; @@ -141,7 +142,8 @@ private static Stream iastModules( ApplicationModuleImpl.class, HardcodedSecretModuleImpl.class, InsecureAuthProtocolModuleImpl.class, - ReflectionInjectionModuleImpl.class); + ReflectionInjectionModuleImpl.class, + SessionRewritingModuleImpl.class); if (iast != FULLY_ENABLED) { modules = modules.filter(IastSystem::isOptOut); } 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 a5a294456e1..676c083316b 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 @@ -86,6 +86,9 @@ public interface VulnerabilityType { new VulnerabilityTypeImpl( VulnerabilityTypes.REFLECTION_INJECTION, VulnerabilityMarks.REFLECTION_INJECTION_MARK); + VulnerabilityType SESSION_REWRITING = + new VulnerabilityTypeImpl(VulnerabilityTypes.SESSION_REWRITING); + 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/SessionRewritingModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java new file mode 100644 index 00000000000..ac953c31fd5 --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java @@ -0,0 +1,43 @@ +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.iast.IastContext; +import datadog.trace.api.iast.sink.SessionRewritingModule; +import datadog.trace.bootstrap.instrumentation.api.AgentSpan; +import datadog.trace.bootstrap.instrumentation.api.AgentTracer; +import java.util.Set; +import org.jetbrains.annotations.NotNull; + +public class SessionRewritingModuleImpl extends SinkModuleBase implements SessionRewritingModule { + + static final String EVIDENCE_VALUE = + "URL rewriting may be used by the container for session tracking"; + + public SessionRewritingModuleImpl(final Dependencies dependencies) { + super(dependencies); + } + + @Override + public void checkSessionTrackingModes(@NotNull Set sessionTrackingModes) { + if (!sessionTrackingModes.contains("URL")) { + return; + } + + final IastContext ctx = IastContext.Provider.get(); + if (ctx == null) { + return; + } + final AgentSpan span = AgentTracer.activeSpan(); + // overhead is not checked here as it's called once per application context + reporter.report( + span, + new Vulnerability( + VulnerabilityType.SESSION_REWRITING, + Location.forSpanAndStack(span, getCurrentStackTrace()), + new Evidence(EVIDENCE_VALUE))); + } +} diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/SessionRewritingModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/SessionRewritingModuleTest.groovy new file mode 100644 index 00000000000..d21d0b88478 --- /dev/null +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/SessionRewritingModuleTest.groovy @@ -0,0 +1,51 @@ +package com.datadog.iast.sink + +import com.datadog.iast.IastModuleImplTestBase +import com.datadog.iast.Reporter +import com.datadog.iast.model.Vulnerability +import com.datadog.iast.model.VulnerabilityType +import datadog.trace.api.iast.sink.SessionRewritingModule + +import static com.datadog.iast.sink.SessionRewritingModuleImpl.EVIDENCE_VALUE + +class SessionRewritingModuleTest extends IastModuleImplTestBase { + + + private SessionRewritingModule module + + def setup() { + module = new SessionRewritingModuleImpl(dependencies) + } + + @Override + protected Reporter buildReporter() { + return Mock(Reporter) + } + + void 'iast module detects session rewriting on sessionTrackingModes'() { + when: + module.checkSessionTrackingModes(sessionTrackingModes as Set) + + then: + if (expected != null) { + 1 * reporter.report(_, _) >> { args -> assertVulnerability(args[1] as Vulnerability, expected) } + } else { + 0 * reporter.report(_, _) + } + + where: + sessionTrackingModes | expected + [] | null + ['COOKIE'] | null + ['URL'] | EVIDENCE_VALUE + ['COOKIE', 'URL'] | EVIDENCE_VALUE + } + + private static void assertVulnerability(final Vulnerability vuln, final String expected) { + assert vuln != null + assert vuln.getType() == VulnerabilityType.SESSION_REWRITING + assert vuln.getLocation() != null + final evidence = vuln.getEvidence() + assert evidence.value == expected + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/RequestDispatcher2Utils.java b/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/RequestDispatcher2Utils.java new file mode 100644 index 00000000000..36ebb8b9e75 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/RequestDispatcher2Utils.java @@ -0,0 +1,152 @@ +import java.io.IOException; +import java.io.InputStream; +import java.net.MalformedURLException; +import java.net.URL; +import java.util.Enumeration; +import java.util.Set; +import javax.servlet.RequestDispatcher; +import javax.servlet.Servlet; +import javax.servlet.ServletContext; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; + +public class RequestDispatcher2Utils { + + /* RequestDispatcher can't be visible to groovy otherwise things break, so everything is + * encapsulated in here where groovy doesn't need to access it. + */ + + void getRealPath(final String target) { + new TestContext().getRealPath(target); + } + + class TestContext implements ServletContext { + + @Override + public String getContextPath() { + return null; + } + + @Override + public ServletContext getContext(String s) { + return null; + } + + @Override + public int getMajorVersion() { + return 0; + } + + @Override + public int getMinorVersion() { + return 0; + } + + @Override + public String getMimeType(String s) { + return null; + } + + @Override + public Set getResourcePaths(String s) { + return null; + } + + @Override + public URL getResource(String s) throws MalformedURLException { + return null; + } + + @Override + public InputStream getResourceAsStream(String s) { + return null; + } + + @Override + public RequestDispatcher getRequestDispatcher(String s) { + return null; + } + + @Override + public RequestDispatcher getNamedDispatcher(String s) { + return null; + } + + @Override + public Servlet getServlet(String s) throws ServletException { + return null; + } + + @Override + public Enumeration getServlets() { + return null; + } + + @Override + public Enumeration getServletNames() { + return null; + } + + @Override + public void log(String s) {} + + @Override + public void log(Exception e, String s) {} + + @Override + public void log(String s, Throwable throwable) {} + + @Override + public String getRealPath(String s) { + return null; + } + + @Override + public String getServerInfo() { + return null; + } + + @Override + public String getInitParameter(String s) { + return null; + } + + @Override + public Enumeration getInitParameterNames() { + return null; + } + + @Override + public Object getAttribute(String s) { + return null; + } + + @Override + public Enumeration getAttributeNames() { + return null; + } + + @Override + public void setAttribute(String s, Object o) {} + + @Override + public void removeAttribute(String s) {} + + @Override + public String getServletContextName() { + return null; + } + } + + class TestDispatcher implements RequestDispatcher { + + @Override + public void forward(ServletRequest servletRequest, ServletResponse servletResponse) + throws ServletException, IOException {} + + @Override + public void include(ServletRequest servletRequest, ServletResponse servletResponse) + throws ServletException, IOException {} + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java new file mode 100644 index 00000000000..2c0cc71dcb1 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java @@ -0,0 +1,82 @@ +package datadog.trace.instrumentation.servlet3; + +import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; +import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedNoneOf; +import static java.util.stream.Collectors.*; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.Sink; +import datadog.trace.api.iast.VulnerabilityTypes; +import datadog.trace.api.iast.sink.ApplicationModule; +import datadog.trace.api.iast.sink.SessionRewritingModule; +import datadog.trace.bootstrap.InstrumentationContext; +import java.util.Collections; +import java.util.Map; +import javax.servlet.ServletContext; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public final class IastServlet3ContextInstrumentation extends InstrumenterModule.Iast + implements Instrumenter.ForTypeHierarchy { + + private static final String TYPE = "javax.servlet.ServletContext"; + + public IastServlet3ContextInstrumentation() { + super("servlet", "servlet-3", "servlet-context"); + } + + @Override + public String hierarchyMarkerType() { + return TYPE; + } + + @Override + public ElementMatcher hierarchyMatcher() { + return implementsInterface(named(hierarchyMarkerType())) + .and(namedNoneOf("org.apache.catalina.core.ApplicationContext")); // Tomcat has + // org.apache.catalina.core.ApplicationContextFacade which implements ServletContext and calls + // internally org.apache.catalina.core.ApplicationContext + } + + @Override + public Map contextStore() { + return Collections.singletonMap(TYPE, String.class.getName()); + } + + @Override + public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvice( + isMethod().and(isPublic()).and(named("getRealPath")).and(takesArguments(String.class)), + IastServlet3ContextInstrumentation.class.getName() + "$IastContextAdvice"); + } + + public static class IastContextAdvice { + @Sink(VulnerabilityTypes.APPLICATION) + @Advice.OnMethodExit(suppress = Throwable.class) + public static void getRealPath( + @Advice.This final ServletContext context, @Advice.Return final String realPath) { + final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION; + final SessionRewritingModule sessionRewritingModule = InstrumentationBridge.SESSION_REWRITING; + if ((applicationModule != null || sessionRewritingModule != null) + && InstrumentationContext.get(ServletContext.class, String.class).get(context) == null) { + InstrumentationContext.get(ServletContext.class, String.class).put(context, realPath); + if (applicationModule != null) { + applicationModule.onRealPath(realPath); + } + if (sessionRewritingModule != null && context.getEffectiveSessionTrackingModes() != null) { + sessionRewritingModule.checkSessionTrackingModes( + context.getEffectiveSessionTrackingModes().stream().map(Enum::name).collect(toSet())); + } + } + } + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationTest.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationTest.groovy new file mode 100644 index 00000000000..8536e2ef3e2 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationTest.groovy @@ -0,0 +1,62 @@ +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.api.iast.InstrumentationBridge +import datadog.trace.api.iast.sink.ApplicationModule +import datadog.trace.api.iast.sink.SessionRewritingModule +import foo.bar.smoketest.RequestDispatcher3Utils + +import javax.servlet.SessionTrackingMode + +class IastServlet3ContextInstrumentationTest extends AgentTestRunner{ + + @Override + protected void configurePreAgent() { + injectSysConfig("dd.iast.enabled", "true") + } + + void 'test ApplicationModule onRealPath'() { + given: + final module = Mock(ApplicationModule) + InstrumentationBridge.registerIastModule(module) + final utils = new RequestDispatcher3Utils() + + when: + utils.getRealPath("/") + + then: + 1 * module.onRealPath(_) + 0 * _ + } + + void 'test SessionRewriting onRealPath'() { + given: + final module = Mock(SessionRewritingModule) + InstrumentationBridge.registerIastModule(module) + final sessionTrackingModes = [SessionTrackingMode.COOKIE, SessionTrackingMode.URL] as Set + final utils = new RequestDispatcher3Utils(sessionTrackingModes) + + when: + utils.getRealPath("/") + + then: + 1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) + 0 * _ + } + + void 'test onRealPath'() { + given: + final appModule = Mock(ApplicationModule) + InstrumentationBridge.registerIastModule(appModule) + final sessionModule = Mock(SessionRewritingModule) + InstrumentationBridge.registerIastModule(sessionModule) + final sessionTrackingModes = [SessionTrackingMode.COOKIE, SessionTrackingMode.URL] as Set + final utils = new RequestDispatcher3Utils(sessionTrackingModes) + + when: + utils.getRealPath("/") + + then: + 1 * appModule.onRealPath(_) + 1 * sessionModule.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) + 0 * _ + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java b/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java new file mode 100644 index 00000000000..c57f71e1a4b --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java @@ -0,0 +1,283 @@ +package foo.bar.smoketest; + +import java.io.InputStream; +import java.net.MalformedURLException; +import java.net.URL; +import java.util.Enumeration; +import java.util.EventListener; +import java.util.Map; +import java.util.Set; +import javax.servlet.Filter; +import javax.servlet.FilterRegistration; +import javax.servlet.RequestDispatcher; +import javax.servlet.Servlet; +import javax.servlet.ServletContext; +import javax.servlet.ServletException; +import javax.servlet.ServletRegistration; +import javax.servlet.SessionCookieConfig; +import javax.servlet.SessionTrackingMode; +import javax.servlet.descriptor.JspConfigDescriptor; + +/* RequestDispatcher can't be visible to groovy otherwise things break, so everything is + * encapsulated in here where groovy doesn't need to access it. + */ + +public class RequestDispatcher3Utils { + + private Set sessionTrackingModes; + + RequestDispatcher3Utils() {} + + RequestDispatcher3Utils(Set sessionTrackingModes) { + this.sessionTrackingModes = sessionTrackingModes; + } + + void getRealPath(final String target) { + new MockServletContext().getRealPath(target); + } + + class MockServletContext implements ServletContext { + + @Override + public String getContextPath() { + return null; + } + + @Override + public ServletContext getContext(String s) { + return null; + } + + @Override + public int getMajorVersion() { + return 0; + } + + @Override + public int getMinorVersion() { + return 0; + } + + @Override + public int getEffectiveMajorVersion() { + return 0; + } + + @Override + public int getEffectiveMinorVersion() { + return 0; + } + + @Override + public String getMimeType(String s) { + return null; + } + + @Override + public Set getResourcePaths(String s) { + return null; + } + + @Override + public URL getResource(String s) throws MalformedURLException { + return null; + } + + @Override + public InputStream getResourceAsStream(String s) { + return null; + } + + @Override + public RequestDispatcher getRequestDispatcher(String s) { + return null; + } + + @Override + public RequestDispatcher getNamedDispatcher(String s) { + return null; + } + + @Override + public Servlet getServlet(String s) throws ServletException { + return null; + } + + @Override + public Enumeration getServlets() { + return null; + } + + @Override + public Enumeration getServletNames() { + return null; + } + + @Override + public void log(String s) {} + + @Override + public void log(Exception e, String s) {} + + @Override + public void log(String s, Throwable throwable) {} + + @Override + public String getRealPath(String s) { + return null; + } + + @Override + public String getServerInfo() { + return null; + } + + @Override + public String getInitParameter(String s) { + return null; + } + + @Override + public Enumeration getInitParameterNames() { + return null; + } + + @Override + public boolean setInitParameter(String s, String s1) { + return false; + } + + @Override + public Object getAttribute(String s) { + return null; + } + + @Override + public Enumeration getAttributeNames() { + return null; + } + + @Override + public void setAttribute(String s, Object o) {} + + @Override + public void removeAttribute(String s) {} + + @Override + public String getServletContextName() { + return null; + } + + @Override + public ServletRegistration.Dynamic addServlet(String s, String s1) { + return null; + } + + @Override + public ServletRegistration.Dynamic addServlet(String s, Servlet servlet) { + return null; + } + + @Override + public ServletRegistration.Dynamic addServlet(String s, Class aClass) { + return null; + } + + @Override + public T createServlet(Class aClass) throws ServletException { + return null; + } + + @Override + public ServletRegistration getServletRegistration(String s) { + return null; + } + + @Override + public Map getServletRegistrations() { + return null; + } + + @Override + public FilterRegistration.Dynamic addFilter(String s, String s1) { + return null; + } + + @Override + public FilterRegistration.Dynamic addFilter(String s, Filter filter) { + return null; + } + + @Override + public FilterRegistration.Dynamic addFilter(String s, Class aClass) { + return null; + } + + @Override + public T createFilter(Class aClass) throws ServletException { + return null; + } + + @Override + public FilterRegistration getFilterRegistration(String s) { + return null; + } + + @Override + public Map getFilterRegistrations() { + return null; + } + + @Override + public SessionCookieConfig getSessionCookieConfig() { + return null; + } + + @Override + public void setSessionTrackingModes(Set set) { + sessionTrackingModes = set; + } + + @Override + public Set getDefaultSessionTrackingModes() { + return null; + } + + @Override + public Set getEffectiveSessionTrackingModes() { + return sessionTrackingModes; + } + + @Override + public void addListener(String s) {} + + @Override + public void addListener(T t) {} + + @Override + public void addListener(Class aClass) {} + + @Override + public T createListener(Class aClass) throws ServletException { + return null; + } + + @Override + public JspConfigDescriptor getJspConfigDescriptor() { + return null; + } + + @Override + public ClassLoader getClassLoader() { + return null; + } + + @Override + public void declareRoles(String... strings) {} + + @Override + public String getVirtualServerName() { + return null; + } + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java new file mode 100644 index 00000000000..4bc2754182b --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java @@ -0,0 +1,78 @@ +package datadog.trace.instrumentation.servlet5; + +import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; +import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; +import static java.util.stream.Collectors.toSet; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.api.iast.InstrumentationBridge; +import datadog.trace.api.iast.Sink; +import datadog.trace.api.iast.VulnerabilityTypes; +import datadog.trace.api.iast.sink.ApplicationModule; +import datadog.trace.api.iast.sink.SessionRewritingModule; +import datadog.trace.bootstrap.InstrumentationContext; +import jakarta.servlet.ServletContext; +import java.util.Collections; +import java.util.Map; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public final class IastJakartaServletContextInstrumentation extends InstrumenterModule.Iast + implements Instrumenter.ForTypeHierarchy { + + private static final String TYPE = "jakarta.servlet.ServletContext"; + + public IastJakartaServletContextInstrumentation() { + super("servlet", "servlet-5", "servlet-context"); + } + + @Override + public String hierarchyMarkerType() { + return TYPE; + } + + @Override + public ElementMatcher hierarchyMatcher() { + return implementsInterface(named(hierarchyMarkerType())); + } + + @Override + public Map contextStore() { + return Collections.singletonMap(TYPE, String.class.getName()); + } + + @Override + public void methodAdvice(MethodTransformer transformer) { + transformer.applyAdvice( + isMethod().and(isPublic()).and(named("getRealPath")).and(takesArguments(String.class)), + IastJakartaServletContextInstrumentation.class.getName() + "$IastContextAdvice"); + } + + public static class IastContextAdvice { + @Sink(VulnerabilityTypes.APPLICATION) + @Advice.OnMethodExit(suppress = Throwable.class) + public static void getRealPath( + @Advice.This final ServletContext context, @Advice.Return final String realPath) { + final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION; + final SessionRewritingModule sessionRewritingModule = InstrumentationBridge.SESSION_REWRITING; + if ((applicationModule != null || sessionRewritingModule != null) + && InstrumentationContext.get(ServletContext.class, String.class).get(context) == null) { + InstrumentationContext.get(ServletContext.class, String.class).put(context, realPath); + if (applicationModule != null) { + applicationModule.onRealPath(realPath); + } + if (sessionRewritingModule != null && context.getEffectiveSessionTrackingModes() != null) { + sessionRewritingModule.checkSessionTrackingModes( + context.getEffectiveSessionTrackingModes().stream().map(Enum::name).collect(toSet())); + } + } + } + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletContextInstrumentationTest.groovy b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletContextInstrumentationTest.groovy new file mode 100644 index 00000000000..f4780cc6004 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletContextInstrumentationTest.groovy @@ -0,0 +1,62 @@ +import datadog.trace.agent.test.AgentTestRunner +import datadog.trace.api.iast.InstrumentationBridge +import datadog.trace.api.iast.sink.ApplicationModule +import datadog.trace.api.iast.sink.SessionRewritingModule + +import jakarta.servlet.SessionTrackingMode + + +class IastJakartaServletContextInstrumentationTest extends AgentTestRunner{ + + @Override + protected void configurePreAgent() { + injectSysConfig("dd.iast.enabled", "true") + } + + void 'test ApplicationModule onRealPath'() { + given: + final module = Mock(ApplicationModule) + InstrumentationBridge.registerIastModule(module) + final utils = new JakartaRequestDispatcherUtils() + + when: + utils.getRealPath("/") + + then: + 1 * module.onRealPath(_) + 0 * _ + } + + void 'test SessionRewriting onRealPath'() { + given: + final module = Mock(SessionRewritingModule) + InstrumentationBridge.registerIastModule(module) + final sessionTrackingModes = [SessionTrackingMode.COOKIE, SessionTrackingMode.URL] as Set + final utils = new JakartaRequestDispatcherUtils(sessionTrackingModes) + + when: + utils.getRealPath("/") + + then: + 1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) + 0 * _ + } + + void 'test onRealPath'() { + given: + final appModule = Mock(ApplicationModule) + InstrumentationBridge.registerIastModule(appModule) + final sessionModule = Mock(SessionRewritingModule) + InstrumentationBridge.registerIastModule(sessionModule) + final sessionTrackingModes = [SessionTrackingMode.COOKIE, SessionTrackingMode.URL] as Set + final utils = new JakartaRequestDispatcherUtils(sessionTrackingModes) + + when: + utils.getRealPath("/") + + then: + 1 * appModule.onRealPath(_) + 1 * sessionModule.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) + 0 * _ + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaRequestDispatcherUtils.java b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaRequestDispatcherUtils.java new file mode 100644 index 00000000000..8c6bc48b97d --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaRequestDispatcherUtils.java @@ -0,0 +1,304 @@ +import jakarta.servlet.Filter; +import jakarta.servlet.FilterRegistration; +import jakarta.servlet.RequestDispatcher; +import jakarta.servlet.Servlet; +import jakarta.servlet.ServletContext; +import jakarta.servlet.ServletException; +import jakarta.servlet.ServletRegistration; +import jakarta.servlet.SessionCookieConfig; +import jakarta.servlet.SessionTrackingMode; +import jakarta.servlet.descriptor.JspConfigDescriptor; +import java.io.InputStream; +import java.net.MalformedURLException; +import java.net.URL; +import java.util.Enumeration; +import java.util.EventListener; +import java.util.Map; +import java.util.Set; + +public class JakartaRequestDispatcherUtils { + + private Set sessionTrackingModes; + + JakartaRequestDispatcherUtils() {} + + JakartaRequestDispatcherUtils(Set sessionTrackingModes) { + this.sessionTrackingModes = sessionTrackingModes; + } + + void getRealPath(final String target) { + new TestContext().getRealPath(target); + } + + class TestContext implements ServletContext { + + @Override + public String getContextPath() { + return null; + } + + @Override + public ServletContext getContext(String s) { + return null; + } + + @Override + public int getMajorVersion() { + return 0; + } + + @Override + public int getMinorVersion() { + return 0; + } + + @Override + public int getEffectiveMajorVersion() { + return 0; + } + + @Override + public int getEffectiveMinorVersion() { + return 0; + } + + @Override + public String getMimeType(String s) { + return null; + } + + @Override + public Set getResourcePaths(String s) { + return null; + } + + @Override + public URL getResource(String s) throws MalformedURLException { + return null; + } + + @Override + public InputStream getResourceAsStream(String s) { + return null; + } + + @Override + public RequestDispatcher getRequestDispatcher(String s) { + return null; + } + + @Override + public RequestDispatcher getNamedDispatcher(String s) { + return null; + } + + @Override + public Servlet getServlet(String s) throws ServletException { + return null; + } + + @Override + public Enumeration getServlets() { + return null; + } + + @Override + public Enumeration getServletNames() { + return null; + } + + @Override + public void log(String s) {} + + @Override + public void log(Exception e, String s) {} + + @Override + public void log(String s, Throwable throwable) {} + + @Override + public String getRealPath(String s) { + return null; + } + + @Override + public String getServerInfo() { + return null; + } + + @Override + public String getInitParameter(String s) { + return null; + } + + @Override + public Enumeration getInitParameterNames() { + return null; + } + + @Override + public boolean setInitParameter(String s, String s1) { + return false; + } + + @Override + public Object getAttribute(String s) { + return null; + } + + @Override + public Enumeration getAttributeNames() { + return null; + } + + @Override + public void setAttribute(String s, Object o) {} + + @Override + public void removeAttribute(String s) {} + + @Override + public String getServletContextName() { + return null; + } + + @Override + public ServletRegistration.Dynamic addServlet(String s, String s1) { + return null; + } + + @Override + public ServletRegistration.Dynamic addServlet(String s, Servlet servlet) { + return null; + } + + @Override + public ServletRegistration.Dynamic addServlet(String s, Class aClass) { + return null; + } + + @Override + public ServletRegistration.Dynamic addJspFile(String s, String s1) { + return null; + } + + @Override + public T createServlet(Class aClass) throws ServletException { + return null; + } + + @Override + public ServletRegistration getServletRegistration(String s) { + return null; + } + + @Override + public Map getServletRegistrations() { + return null; + } + + @Override + public FilterRegistration.Dynamic addFilter(String s, String s1) { + return null; + } + + @Override + public FilterRegistration.Dynamic addFilter(String s, Filter filter) { + return null; + } + + @Override + public FilterRegistration.Dynamic addFilter(String s, Class aClass) { + return null; + } + + @Override + public T createFilter(Class aClass) throws ServletException { + return null; + } + + @Override + public FilterRegistration getFilterRegistration(String s) { + return null; + } + + @Override + public Map getFilterRegistrations() { + return null; + } + + @Override + public SessionCookieConfig getSessionCookieConfig() { + return null; + } + + @Override + public void setSessionTrackingModes(Set set) {} + + @Override + public Set getDefaultSessionTrackingModes() { + return null; + } + + @Override + public Set getEffectiveSessionTrackingModes() { + return sessionTrackingModes; + } + + @Override + public void addListener(String s) {} + + @Override + public void addListener(T t) {} + + @Override + public void addListener(Class aClass) {} + + @Override + public T createListener(Class aClass) throws ServletException { + return null; + } + + @Override + public JspConfigDescriptor getJspConfigDescriptor() { + return null; + } + + @Override + public ClassLoader getClassLoader() { + return null; + } + + @Override + public void declareRoles(String... strings) {} + + @Override + public String getVirtualServerName() { + return null; + } + + @Override + public int getSessionTimeout() { + return 0; + } + + @Override + public void setSessionTimeout(int i) {} + + @Override + public String getRequestCharacterEncoding() { + return null; + } + + @Override + public void setRequestCharacterEncoding(String s) {} + + @Override + public String getResponseCharacterEncoding() { + return null; + } + + @Override + public void setResponseCharacterEncoding(String s) {} + } +} 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 cb89379ce91..3501495cfe7 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 @@ -16,6 +16,7 @@ import datadog.trace.api.iast.sink.NoSameSiteCookieModule; import datadog.trace.api.iast.sink.PathTraversalModule; import datadog.trace.api.iast.sink.ReflectionInjectionModule; +import datadog.trace.api.iast.sink.SessionRewritingModule; import datadog.trace.api.iast.sink.SqlInjectionModule; import datadog.trace.api.iast.sink.SsrfModule; import datadog.trace.api.iast.sink.StacktraceLeakModule; @@ -65,6 +66,7 @@ public abstract class InstrumentationBridge { public static HardcodedSecretModule HARDCODED_SECRET; public static InsecureAuthProtocolModule INSECURE_AUTH_PROTOCOL; public static ReflectionInjectionModule REFLECTION_INJECTION; + public static SessionRewritingModule SESSION_REWRITING; private static final Map, Field> MODULE_MAP = buildModuleMap(); 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 b497dea01ea..ab09cd0b679 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 @@ -34,6 +34,7 @@ private VulnerabilityTypes() {} public static final byte HARDCODED_SECRET = 25; public static final byte INSECURE_AUTH_PROTOCOL = 26; public static final byte REFLECTION_INJECTION = 27; + public static final byte SESSION_REWRITING = 28; /** * Use for telemetry only, this is a special vulnerability type that is not reported, reported @@ -73,7 +74,8 @@ private VulnerabilityTypes() {} DEFAULT_HTML_ESCAPE_INVALID, SESSION_TIMEOUT, DIRECTORY_LISTING_LEAK, - INSECURE_JSP_LAYOUT + INSECURE_JSP_LAYOUT, + SESSION_REWRITING }; /** @@ -108,7 +110,8 @@ private VulnerabilityTypes() {} "ADMIN_CONSOLE_ACTIVE", "HARDCODED_SECRET", "INSECURE_AUTH_PROTOCOL", - "REFLECTION_INJECTION" + "REFLECTION_INJECTION", + "SESSION_REWRITING" }; public static String toString(final byte vulnerability) { diff --git a/internal-api/src/main/java/datadog/trace/api/iast/sink/SessionRewritingModule.java b/internal-api/src/main/java/datadog/trace/api/iast/sink/SessionRewritingModule.java new file mode 100644 index 00000000000..a98fe53b31e --- /dev/null +++ b/internal-api/src/main/java/datadog/trace/api/iast/sink/SessionRewritingModule.java @@ -0,0 +1,9 @@ +package datadog.trace.api.iast.sink; + +import datadog.trace.api.iast.IastModule; +import java.util.Set; +import javax.annotation.Nonnull; + +public interface SessionRewritingModule extends IastModule { + void checkSessionTrackingModes(@Nonnull Set sessionTrackingModes); +} diff --git a/internal-api/src/test/groovy/datadog/trace/api/iast/VulnerabilityTypesTest.groovy b/internal-api/src/test/groovy/datadog/trace/api/iast/VulnerabilityTypesTest.groovy index 0ffcada012c..f7d37f4ec7f 100644 --- a/internal-api/src/test/groovy/datadog/trace/api/iast/VulnerabilityTypesTest.groovy +++ b/internal-api/src/test/groovy/datadog/trace/api/iast/VulnerabilityTypesTest.groovy @@ -42,5 +42,6 @@ class VulnerabilityTypesTest extends DDSpecification { VulnerabilityTypes.HARDCODED_SECRET | 'HARDCODED_SECRET' VulnerabilityTypes.INSECURE_AUTH_PROTOCOL | 'INSECURE_AUTH_PROTOCOL' VulnerabilityTypes.REFLECTION_INJECTION | 'REFLECTION_INJECTION' + VulnerabilityTypes.SESSION_REWRITING | 'SESSION_REWRITING' } } From 6d01e50b01c22efa549a8c8e61d7a17b668b36e7 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 29 Feb 2024 13:52:53 +0100 Subject: [PATCH 02/18] fix servlet2 muzzle --- .../servlet3/IastServlet3ContextInstrumentation.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java index 2c0cc71dcb1..0653b17d586 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java @@ -39,6 +39,11 @@ public String hierarchyMarkerType() { return TYPE; } + @Override + public String muzzleDirective() { + return "servlet-3.x"; + } + @Override public ElementMatcher hierarchyMatcher() { return implementsInterface(named(hierarchyMarkerType())) From 475cebf5faacbc00ac8c0a77186f075af8962b73 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 29 Feb 2024 17:24:21 +0100 Subject: [PATCH 03/18] fix latestDep for servlet3 --- .../smoketest/RequestDispatcher3Utils.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java b/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java index c57f71e1a4b..cf558e2a6db 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java @@ -279,5 +279,27 @@ public void declareRoles(String... strings) {} public String getVirtualServerName() { return null; } + + public void setResponseCharacterEncoding(String s) {} + + public String getResponseCharacterEncoding() { + return null; + } + + public void setRequestCharacterEncoding(String s) {} + + public String getRequestCharacterEncoding() { + return null; + } + + public void setSessionTimeout(int timeout) {} + + public int getSessionTimeout() { + return 0; + } + + public ServletRegistration.Dynamic addJspFile(String s, String j) { + return null; + } } } From 1218f66286af9239df4ca1462fcc06c987579d32 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 4 Mar 2024 11:16:33 +0100 Subject: [PATCH 04/18] move to forked test --- ...=> IastServlet3ContextInstrumentationForkedTest.groovy} | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) rename dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/{IastServlet3ContextInstrumentationTest.groovy => IastServlet3ContextInstrumentationForkedTest.groovy} (91%) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationTest.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationForkedTest.groovy similarity index 91% rename from dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationTest.groovy rename to dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationForkedTest.groovy index 8536e2ef3e2..2bf642decbb 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationForkedTest.groovy @@ -6,13 +6,18 @@ import foo.bar.smoketest.RequestDispatcher3Utils import javax.servlet.SessionTrackingMode -class IastServlet3ContextInstrumentationTest extends AgentTestRunner{ +class IastServlet3ContextInstrumentationForkedTest extends AgentTestRunner{ @Override protected void configurePreAgent() { injectSysConfig("dd.iast.enabled", "true") } + @Override + void cleanup() { + InstrumentationBridge.clearIastModules() + } + void 'test ApplicationModule onRealPath'() { given: final module = Mock(ApplicationModule) From a72cc738cc25e3e5c4659dd0570c1420f4e522d7 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 4 Mar 2024 13:28:29 +0100 Subject: [PATCH 05/18] exclude jdk6 org.springframework.mock.web.MockServletContext --- .../servlet3/IastServlet3ContextInstrumentation.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java index 0653b17d586..2a8891bf370 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java @@ -47,9 +47,13 @@ public String muzzleDirective() { @Override public ElementMatcher hierarchyMatcher() { return implementsInterface(named(hierarchyMarkerType())) - .and(namedNoneOf("org.apache.catalina.core.ApplicationContext")); // Tomcat has + .and( + namedNoneOf( + "org.apache.catalina.core.ApplicationContext", + "org.springframework.mock.web.MockServletContext")); // Tomcat has // org.apache.catalina.core.ApplicationContextFacade which implements ServletContext and calls // internally org.apache.catalina.core.ApplicationContext + // org.springframework.mock.web.MockServletContext is jdk6 and fails instrumentation } @Override From 3300ccc242986d39fb74da8548fca476ee415340 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 4 Mar 2024 13:42:04 +0100 Subject: [PATCH 06/18] remove jdk8 code from advices --- .../IastServlet3ContextInstrumentation.java | 19 ++++++++++++------- ...tJakartaServletContextInstrumentation.java | 15 +++++++++++---- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java index 2a8891bf370..366e1449baa 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java @@ -18,8 +18,11 @@ import datadog.trace.api.iast.sink.SessionRewritingModule; import datadog.trace.bootstrap.InstrumentationContext; import java.util.Collections; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import javax.servlet.ServletContext; +import javax.servlet.SessionTrackingMode; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -47,10 +50,7 @@ public String muzzleDirective() { @Override public ElementMatcher hierarchyMatcher() { return implementsInterface(named(hierarchyMarkerType())) - .and( - namedNoneOf( - "org.apache.catalina.core.ApplicationContext", - "org.springframework.mock.web.MockServletContext")); // Tomcat has + .and(namedNoneOf("org.apache.catalina.core.ApplicationContext")); // Tomcat has // org.apache.catalina.core.ApplicationContextFacade which implements ServletContext and calls // internally org.apache.catalina.core.ApplicationContext // org.springframework.mock.web.MockServletContext is jdk6 and fails instrumentation @@ -81,9 +81,14 @@ public static void getRealPath( if (applicationModule != null) { applicationModule.onRealPath(realPath); } - if (sessionRewritingModule != null && context.getEffectiveSessionTrackingModes() != null) { - sessionRewritingModule.checkSessionTrackingModes( - context.getEffectiveSessionTrackingModes().stream().map(Enum::name).collect(toSet())); + if (sessionRewritingModule != null + && context.getEffectiveSessionTrackingModes() != null + && !context.getEffectiveSessionTrackingModes().isEmpty()) { + Set sessionTrackingModes = new HashSet<>(); + for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { + sessionTrackingModes.add(mode.name()); + } + sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes); } } } diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java index 4bc2754182b..40f509b1b8b 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java @@ -2,7 +2,6 @@ import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; -import static java.util.stream.Collectors.toSet; import static net.bytebuddy.matcher.ElementMatchers.isMethod; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.takesArguments; @@ -17,8 +16,11 @@ import datadog.trace.api.iast.sink.SessionRewritingModule; import datadog.trace.bootstrap.InstrumentationContext; import jakarta.servlet.ServletContext; +import jakarta.servlet.SessionTrackingMode; import java.util.Collections; +import java.util.HashSet; import java.util.Map; +import java.util.Set; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -68,9 +70,14 @@ public static void getRealPath( if (applicationModule != null) { applicationModule.onRealPath(realPath); } - if (sessionRewritingModule != null && context.getEffectiveSessionTrackingModes() != null) { - sessionRewritingModule.checkSessionTrackingModes( - context.getEffectiveSessionTrackingModes().stream().map(Enum::name).collect(toSet())); + if (sessionRewritingModule != null + && context.getEffectiveSessionTrackingModes() != null + && !context.getEffectiveSessionTrackingModes().isEmpty()) { + Set sessionTrackingModes = new HashSet<>(); + for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { + sessionTrackingModes.add(mode.name()); + } + sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes); } } } From dc6b331686a286aab953d22c56e6f46fb6243d28 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Wed, 6 Mar 2024 09:21:25 +0100 Subject: [PATCH 07/18] Add smoke test --- .../springboot/controller/IastWebController.java | 6 ++++++ .../smoketest/AbstractIastSpringBootTest.groovy | 15 +++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/IastWebController.java b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/IastWebController.java index 32a9c8a1ba5..21e43772957 100644 --- a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/IastWebController.java +++ b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/IastWebController.java @@ -392,6 +392,12 @@ public String headerInjectionRedaction( return "Ok"; } + @GetMapping("/realPath") + public String realPath(HttpServletRequest request) { + request.getServletContext().getRealPath("/"); + return "Ok"; + } + private void withProcess(final Operation op) { Process process = null; 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 78ab89de9c1..78e1d9b2963 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 @@ -5,6 +5,7 @@ import okhttp3.MediaType import okhttp3.MultipartBody 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 @@ -968,6 +969,20 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { } } + void 'find session rewriting'() { + given: + String url = "http://localhost:${httpPort}/realPath" + + when: + Response response = client.newCall(new Request.Builder().url(url).get().build()).execute() + + then: + response.successful + hasVulnerability { vul -> + vul.type == 'SESSION_REWRITING' + } + } + } From 97641d50b7a591a755637a104bdcd000d84b26fe Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 11 Mar 2024 09:43:44 +0100 Subject: [PATCH 08/18] change approach to servlet on service --- .../servlet3/IastServlet3Advice.java | 23 ++++- .../IastServlet3ContextInstrumentation.java | 96 ------------------- .../src/test/groovy/JettyServlet3Test.groovy | 15 ++- .../src/test/groovy/TomcatServlet3Test.groovy | 11 ++- ...tJakartaServletContextInstrumentation.java | 85 ---------------- .../IastJakartaServletInstrumentation.java | 30 +++++- ...taServletContextInstrumentationTest.groovy | 62 ------------ ...stJakartaServletInstrumentationTest.groovy | 38 ++++++++ .../controller/IastWebController.java | 6 -- .../AbstractIastSpringBootTest.groovy | 4 +- 10 files changed, 109 insertions(+), 261 deletions(-) delete mode 100644 dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java delete mode 100644 dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletContextInstrumentation.java delete mode 100644 dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletContextInstrumentationTest.groovy diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java index a62ee94a5c0..313ef94c341 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java @@ -4,6 +4,13 @@ import datadog.trace.api.iast.Sink; import datadog.trace.api.iast.VulnerabilityTypes; import datadog.trace.api.iast.sink.ApplicationModule; +import datadog.trace.api.iast.sink.SessionRewritingModule; +import datadog.trace.bootstrap.InstrumentationContext; +import java.util.HashSet; +import java.util.Set; +import javax.servlet.ServletContext; +import javax.servlet.ServletRequest; +import javax.servlet.SessionTrackingMode; import datadog.trace.bootstrap.InstrumentationContext; import javax.servlet.ServletContext; import javax.servlet.ServletRequest; @@ -16,7 +23,8 @@ public class IastServlet3Advice { @Advice.OnMethodExit(suppress = Throwable.class) public static void onExit(@Advice.Argument(0) ServletRequest request) { final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION; - if (applicationModule == null) { + final SessionRewritingModule sessionRewritingModule = InstrumentationBridge.SESSION_REWRITING; + if (applicationModule == null && sessionRewritingModule == null) { return; } if (!(request instanceof HttpServletRequest)) { @@ -27,6 +35,17 @@ public static void onExit(@Advice.Argument(0) ServletRequest request) { return; } InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true); - applicationModule.onRealPath(context.getRealPath("/")); + if (applicationModule != null) { + applicationModule.onRealPath(context.getRealPath("/")); + } + if (sessionRewritingModule != null + && context.getEffectiveSessionTrackingModes() != null + && !context.getEffectiveSessionTrackingModes().isEmpty()) { + Set sessionTrackingModes = new HashSet<>(); + for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { + sessionTrackingModes.add(mode.name()); + } + sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes); + } } } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java deleted file mode 100644 index 366e1449baa..00000000000 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3ContextInstrumentation.java +++ /dev/null @@ -1,96 +0,0 @@ -package datadog.trace.instrumentation.servlet3; - -import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; -import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; -import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedNoneOf; -import static java.util.stream.Collectors.*; -import static net.bytebuddy.matcher.ElementMatchers.isMethod; -import static net.bytebuddy.matcher.ElementMatchers.isPublic; -import static net.bytebuddy.matcher.ElementMatchers.takesArguments; - -import com.google.auto.service.AutoService; -import datadog.trace.agent.tooling.Instrumenter; -import datadog.trace.agent.tooling.InstrumenterModule; -import datadog.trace.api.iast.InstrumentationBridge; -import datadog.trace.api.iast.Sink; -import datadog.trace.api.iast.VulnerabilityTypes; -import datadog.trace.api.iast.sink.ApplicationModule; -import datadog.trace.api.iast.sink.SessionRewritingModule; -import datadog.trace.bootstrap.InstrumentationContext; -import java.util.Collections; -import java.util.HashSet; -import java.util.Map; -import java.util.Set; -import javax.servlet.ServletContext; -import javax.servlet.SessionTrackingMode; -import net.bytebuddy.asm.Advice; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.matcher.ElementMatcher; - -@AutoService(Instrumenter.class) -public final class IastServlet3ContextInstrumentation extends InstrumenterModule.Iast - implements Instrumenter.ForTypeHierarchy { - - private static final String TYPE = "javax.servlet.ServletContext"; - - public IastServlet3ContextInstrumentation() { - super("servlet", "servlet-3", "servlet-context"); - } - - @Override - public String hierarchyMarkerType() { - return TYPE; - } - - @Override - public String muzzleDirective() { - return "servlet-3.x"; - } - - @Override - public ElementMatcher hierarchyMatcher() { - return implementsInterface(named(hierarchyMarkerType())) - .and(namedNoneOf("org.apache.catalina.core.ApplicationContext")); // Tomcat has - // org.apache.catalina.core.ApplicationContextFacade which implements ServletContext and calls - // internally org.apache.catalina.core.ApplicationContext - // org.springframework.mock.web.MockServletContext is jdk6 and fails instrumentation - } - - @Override - public Map contextStore() { - return Collections.singletonMap(TYPE, String.class.getName()); - } - - @Override - public void methodAdvice(MethodTransformer transformer) { - transformer.applyAdvice( - isMethod().and(isPublic()).and(named("getRealPath")).and(takesArguments(String.class)), - IastServlet3ContextInstrumentation.class.getName() + "$IastContextAdvice"); - } - - public static class IastContextAdvice { - @Sink(VulnerabilityTypes.APPLICATION) - @Advice.OnMethodExit(suppress = Throwable.class) - public static void getRealPath( - @Advice.This final ServletContext context, @Advice.Return final String realPath) { - final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION; - final SessionRewritingModule sessionRewritingModule = InstrumentationBridge.SESSION_REWRITING; - if ((applicationModule != null || sessionRewritingModule != null) - && InstrumentationContext.get(ServletContext.class, String.class).get(context) == null) { - InstrumentationContext.get(ServletContext.class, String.class).put(context, realPath); - if (applicationModule != null) { - applicationModule.onRealPath(realPath); - } - if (sessionRewritingModule != null - && context.getEffectiveSessionTrackingModes() != null - && !context.getEffectiveSessionTrackingModes().isEmpty()) { - Set sessionTrackingModes = new HashSet<>(); - for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { - sessionTrackingModes.add(mode.name()); - } - sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes); - } - } - } - } -} diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy index 608a54a3260..12bcc64bcd8 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy @@ -3,6 +3,9 @@ import datadog.trace.agent.test.base.HttpServer import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule +import datadog.trace.api.iast.sink.SessionRewritingModule +import datadog.trace.api.iast.InstrumentationBridge +import datadog.trace.api.iast.sink.ApplicationModule import datadog.trace.bootstrap.instrumentation.api.AgentSpan import datadog.trace.bootstrap.instrumentation.api.Tags import datadog.trace.instrumentation.servlet3.AsyncDispatcherDecorator @@ -17,6 +20,7 @@ import javax.servlet.AsyncEvent import javax.servlet.AsyncListener import javax.servlet.Servlet import javax.servlet.ServletException +import javax.servlet.SessionTrackingMode import javax.servlet.annotation.WebServlet import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse @@ -52,7 +56,7 @@ abstract class JettyServlet3Test extends AbstractServlet3Test hierarchyMatcher() { - return implementsInterface(named(hierarchyMarkerType())); - } - - @Override - public Map contextStore() { - return Collections.singletonMap(TYPE, String.class.getName()); - } - - @Override - public void methodAdvice(MethodTransformer transformer) { - transformer.applyAdvice( - isMethod().and(isPublic()).and(named("getRealPath")).and(takesArguments(String.class)), - IastJakartaServletContextInstrumentation.class.getName() + "$IastContextAdvice"); - } - - public static class IastContextAdvice { - @Sink(VulnerabilityTypes.APPLICATION) - @Advice.OnMethodExit(suppress = Throwable.class) - public static void getRealPath( - @Advice.This final ServletContext context, @Advice.Return final String realPath) { - final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION; - final SessionRewritingModule sessionRewritingModule = InstrumentationBridge.SESSION_REWRITING; - if ((applicationModule != null || sessionRewritingModule != null) - && InstrumentationContext.get(ServletContext.class, String.class).get(context) == null) { - InstrumentationContext.get(ServletContext.class, String.class).put(context, realPath); - if (applicationModule != null) { - applicationModule.onRealPath(realPath); - } - if (sessionRewritingModule != null - && context.getEffectiveSessionTrackingModes() != null - && !context.getEffectiveSessionTrackingModes().isEmpty()) { - Set sessionTrackingModes = new HashSet<>(); - for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { - sessionTrackingModes.add(mode.name()); - } - sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes); - } - } - } - } -} diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java index 6c18e3005fd..0fef506d011 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java @@ -12,6 +12,15 @@ import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.api.iast.InstrumentationBridge; import datadog.trace.api.iast.sink.ApplicationModule; +import datadog.trace.api.iast.sink.SessionRewritingModule; +import datadog.trace.bootstrap.InstrumentationContext; +import jakarta.servlet.ServletContext; +import jakarta.servlet.SessionTrackingMode; +import jakarta.servlet.http.HttpServlet; +import java.util.Collections; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; import datadog.trace.bootstrap.InstrumentationContext; import jakarta.servlet.ServletContext; import jakarta.servlet.http.HttpServlet; @@ -60,15 +69,28 @@ public static class IastAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void after(@Advice.This final HttpServlet servlet) { final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION; - if (applicationModule == null) { + final SessionRewritingModule sessionRewritingModule = InstrumentationBridge.SESSION_REWRITING; + if (applicationModule == null && sessionRewritingModule == null) { return; } final ServletContext context = servlet.getServletContext(); - if (InstrumentationContext.get(ServletContext.class, Boolean.class).get(context) != null) { + if (InstrumentationContext.get(ServletContext.class, String.class).get(context) != null) { return; } - InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true); - applicationModule.onRealPath(context.getRealPath("/")); + InstrumentationContext.get(ServletContext.class, String.class) + .put(context, true); + if (applicationModule != null) { + applicationModule.onRealPath(context.getRealPath("/")); + } + if (sessionRewritingModule != null + && context.getEffectiveSessionTrackingModes() != null + && !context.getEffectiveSessionTrackingModes().isEmpty()) { + Set sessionTrackingModes = new HashSet<>(); + for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { + sessionTrackingModes.add(mode.name()); + } + sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes); + } } } } diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletContextInstrumentationTest.groovy b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletContextInstrumentationTest.groovy deleted file mode 100644 index f4780cc6004..00000000000 --- a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletContextInstrumentationTest.groovy +++ /dev/null @@ -1,62 +0,0 @@ -import datadog.trace.agent.test.AgentTestRunner -import datadog.trace.api.iast.InstrumentationBridge -import datadog.trace.api.iast.sink.ApplicationModule -import datadog.trace.api.iast.sink.SessionRewritingModule - -import jakarta.servlet.SessionTrackingMode - - -class IastJakartaServletContextInstrumentationTest extends AgentTestRunner{ - - @Override - protected void configurePreAgent() { - injectSysConfig("dd.iast.enabled", "true") - } - - void 'test ApplicationModule onRealPath'() { - given: - final module = Mock(ApplicationModule) - InstrumentationBridge.registerIastModule(module) - final utils = new JakartaRequestDispatcherUtils() - - when: - utils.getRealPath("/") - - then: - 1 * module.onRealPath(_) - 0 * _ - } - - void 'test SessionRewriting onRealPath'() { - given: - final module = Mock(SessionRewritingModule) - InstrumentationBridge.registerIastModule(module) - final sessionTrackingModes = [SessionTrackingMode.COOKIE, SessionTrackingMode.URL] as Set - final utils = new JakartaRequestDispatcherUtils(sessionTrackingModes) - - when: - utils.getRealPath("/") - - then: - 1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) - 0 * _ - } - - void 'test onRealPath'() { - given: - final appModule = Mock(ApplicationModule) - InstrumentationBridge.registerIastModule(appModule) - final sessionModule = Mock(SessionRewritingModule) - InstrumentationBridge.registerIastModule(sessionModule) - final sessionTrackingModes = [SessionTrackingMode.COOKIE, SessionTrackingMode.URL] as Set - final utils = new JakartaRequestDispatcherUtils(sessionTrackingModes) - - when: - utils.getRealPath("/") - - then: - 1 * appModule.onRealPath(_) - 1 * sessionModule.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) - 0 * _ - } -} diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy index 3b574f5b223..831fc769809 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy @@ -1,6 +1,7 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule +import datadog.trace.api.iast.sink.SessionRewritingModule import foo.bar.smoketest.DummyHttpServlet import foo.bar.smoketest.DummyRequest import foo.bar.smoketest.DummyResponse @@ -19,6 +20,7 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{ void 'test no modules'() { final appModule = Mock(ApplicationModule) + final sessionModule = Mock(SessionRewritingModule) final Servlet servlet = new DummyHttpServlet() final ServletResponse response = new DummyResponse() final ServletRequest request = new DummyRequest() @@ -28,6 +30,7 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{ then: 0 * appModule.onRealPath(_) + 0 * sessionModule.checkSessionTrackingModes(_) 0 * _ } @@ -46,4 +49,39 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{ 1 * module.onRealPath(_) 0 * _ } + + void 'test SessionRewriting'() { + given: + final module = Mock(SessionRewritingModule) + InstrumentationBridge.registerIastModule(module) + final Servlet servlet = new DummyHttpServlet() + final ServletResponse response = new DummyResponse() + final ServletRequest request = new DummyRequest() + + when: + servlet.callPublicServiceMethod(request, response) + + then: + 1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) + 0 * _ + } + + void 'test all modules'() { + given: + final appModule = Mock(ApplicationModule) + InstrumentationBridge.registerIastModule(appModule) + final sessionModule = Mock(SessionRewritingModule) + InstrumentationBridge.registerIastModule(sessionModule) + final Servlet servlet = new DummyHttpServlet() + final ServletResponse response = new DummyResponse() + final ServletRequest request = new DummyRequest() + + when: + servlet.callPublicServiceMethod(request, response) + + then: + 1 * appModule.onRealPath(_) + 1 * sessionModule.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) + 0 * _ + } } diff --git a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/IastWebController.java b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/IastWebController.java index 21e43772957..32a9c8a1ba5 100644 --- a/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/IastWebController.java +++ b/dd-smoke-tests/iast-util/src/main/java/datadog/smoketest/springboot/controller/IastWebController.java @@ -392,12 +392,6 @@ public String headerInjectionRedaction( return "Ok"; } - @GetMapping("/realPath") - public String realPath(HttpServletRequest request) { - request.getServletContext().getRealPath("/"); - return "Ok"; - } - private void withProcess(final Operation op) { Process process = null; 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 78e1d9b2963..0369f4592fb 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 @@ -971,14 +971,14 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest { void 'find session rewriting'() { given: - String url = "http://localhost:${httpPort}/realPath" + String url = "http://localhost:${httpPort}/greeting" when: Response response = client.newCall(new Request.Builder().url(url).get().build()).execute() then: response.successful - hasVulnerability { vul -> + hasVulnerabilityInLogs { vul -> vul.type == 'SESSION_REWRITING' } } From 363bbd3490422676259dcc4cbb5b4d7ce52e2dda Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 11 Mar 2024 10:45:02 +0100 Subject: [PATCH 09/18] old approach test --- ...et3ContextInstrumentationForkedTest.groovy | 67 ------------------- 1 file changed, 67 deletions(-) delete mode 100644 dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationForkedTest.groovy diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationForkedTest.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationForkedTest.groovy deleted file mode 100644 index 2bf642decbb..00000000000 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/IastServlet3ContextInstrumentationForkedTest.groovy +++ /dev/null @@ -1,67 +0,0 @@ -import datadog.trace.agent.test.AgentTestRunner -import datadog.trace.api.iast.InstrumentationBridge -import datadog.trace.api.iast.sink.ApplicationModule -import datadog.trace.api.iast.sink.SessionRewritingModule -import foo.bar.smoketest.RequestDispatcher3Utils - -import javax.servlet.SessionTrackingMode - -class IastServlet3ContextInstrumentationForkedTest extends AgentTestRunner{ - - @Override - protected void configurePreAgent() { - injectSysConfig("dd.iast.enabled", "true") - } - - @Override - void cleanup() { - InstrumentationBridge.clearIastModules() - } - - void 'test ApplicationModule onRealPath'() { - given: - final module = Mock(ApplicationModule) - InstrumentationBridge.registerIastModule(module) - final utils = new RequestDispatcher3Utils() - - when: - utils.getRealPath("/") - - then: - 1 * module.onRealPath(_) - 0 * _ - } - - void 'test SessionRewriting onRealPath'() { - given: - final module = Mock(SessionRewritingModule) - InstrumentationBridge.registerIastModule(module) - final sessionTrackingModes = [SessionTrackingMode.COOKIE, SessionTrackingMode.URL] as Set - final utils = new RequestDispatcher3Utils(sessionTrackingModes) - - when: - utils.getRealPath("/") - - then: - 1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) - 0 * _ - } - - void 'test onRealPath'() { - given: - final appModule = Mock(ApplicationModule) - InstrumentationBridge.registerIastModule(appModule) - final sessionModule = Mock(SessionRewritingModule) - InstrumentationBridge.registerIastModule(sessionModule) - final sessionTrackingModes = [SessionTrackingMode.COOKIE, SessionTrackingMode.URL] as Set - final utils = new RequestDispatcher3Utils(sessionTrackingModes) - - when: - utils.getRealPath("/") - - then: - 1 * appModule.onRealPath(_) - 1 * sessionModule.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) - 0 * _ - } -} From 1b477a7a6de0809efa09e0e3599ab1972b303016 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 11 Mar 2024 12:21:56 +0100 Subject: [PATCH 10/18] fix codenarc --- .../servlet/request-3/src/test/groovy/JettyServlet3Test.groovy | 1 - 1 file changed, 1 deletion(-) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy index 12bcc64bcd8..6cbabfccbf1 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy @@ -20,7 +20,6 @@ import javax.servlet.AsyncEvent import javax.servlet.AsyncListener import javax.servlet.Servlet import javax.servlet.ServletException -import javax.servlet.SessionTrackingMode import javax.servlet.annotation.WebServlet import javax.servlet.http.HttpServletRequest import javax.servlet.http.HttpServletResponse From ea303b5c82f4fd1ea247ae92773e4f027879f91a Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Tue, 12 Mar 2024 08:58:36 +0100 Subject: [PATCH 11/18] Add HttpServletRequest check --- .../trace/instrumentation/servlet3/IastServlet3Advice.java | 1 + 1 file changed, 1 insertion(+) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java index 313ef94c341..deca48ca303 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java @@ -15,6 +15,7 @@ import javax.servlet.ServletContext; import javax.servlet.ServletRequest; import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletRequest; import net.bytebuddy.asm.Advice; public class IastServlet3Advice { From 9a4e90a772234df20b3db391c1e7ae886d9b7055 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 14 Mar 2024 12:25:01 +0100 Subject: [PATCH 12/18] fix spotless --- .../instrumentation/servlet3/IastServlet3Advice.java | 4 ---- .../servlet5/IastJakartaServletInstrumentation.java | 10 ++-------- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java index deca48ca303..1b8c8f896b8 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java @@ -11,10 +11,6 @@ import javax.servlet.ServletContext; import javax.servlet.ServletRequest; import javax.servlet.SessionTrackingMode; -import datadog.trace.bootstrap.InstrumentationContext; -import javax.servlet.ServletContext; -import javax.servlet.ServletRequest; -import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletRequest; import net.bytebuddy.asm.Advice; diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java index 0fef506d011..ae664b13cb2 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java @@ -21,11 +21,6 @@ import java.util.HashSet; import java.util.Map; import java.util.Set; -import datadog.trace.bootstrap.InstrumentationContext; -import jakarta.servlet.ServletContext; -import jakarta.servlet.http.HttpServlet; -import java.util.Collections; -import java.util.Map; import net.bytebuddy.asm.Advice; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -74,11 +69,10 @@ public static void after(@Advice.This final HttpServlet servlet) { return; } final ServletContext context = servlet.getServletContext(); - if (InstrumentationContext.get(ServletContext.class, String.class).get(context) != null) { + if (InstrumentationContext.get(ServletContext.class, Boolean.class).get(context) != null) { return; } - InstrumentationContext.get(ServletContext.class, String.class) - .put(context, true); + InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true); if (applicationModule != null) { applicationModule.onRealPath(context.getRealPath("/")); } From 6b2bf6b1e4ecf6946bff2fd18c12726c8df1f557 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 14 Mar 2024 12:39:20 +0100 Subject: [PATCH 13/18] self review --- .../test/groovy/RequestDispatcher2Utils.java | 152 --------- .../src/test/groovy/JettyServlet3Test.groovy | 4 +- .../smoketest/RequestDispatcher3Utils.java | 305 ------------------ .../groovy/JakartaRequestDispatcherUtils.java | 304 ----------------- 4 files changed, 1 insertion(+), 764 deletions(-) delete mode 100644 dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/RequestDispatcher2Utils.java delete mode 100644 dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java delete mode 100644 dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaRequestDispatcherUtils.java diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/RequestDispatcher2Utils.java b/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/RequestDispatcher2Utils.java deleted file mode 100644 index 36ebb8b9e75..00000000000 --- a/dd-java-agent/instrumentation/servlet/request-2/src/test/groovy/RequestDispatcher2Utils.java +++ /dev/null @@ -1,152 +0,0 @@ -import java.io.IOException; -import java.io.InputStream; -import java.net.MalformedURLException; -import java.net.URL; -import java.util.Enumeration; -import java.util.Set; -import javax.servlet.RequestDispatcher; -import javax.servlet.Servlet; -import javax.servlet.ServletContext; -import javax.servlet.ServletException; -import javax.servlet.ServletRequest; -import javax.servlet.ServletResponse; - -public class RequestDispatcher2Utils { - - /* RequestDispatcher can't be visible to groovy otherwise things break, so everything is - * encapsulated in here where groovy doesn't need to access it. - */ - - void getRealPath(final String target) { - new TestContext().getRealPath(target); - } - - class TestContext implements ServletContext { - - @Override - public String getContextPath() { - return null; - } - - @Override - public ServletContext getContext(String s) { - return null; - } - - @Override - public int getMajorVersion() { - return 0; - } - - @Override - public int getMinorVersion() { - return 0; - } - - @Override - public String getMimeType(String s) { - return null; - } - - @Override - public Set getResourcePaths(String s) { - return null; - } - - @Override - public URL getResource(String s) throws MalformedURLException { - return null; - } - - @Override - public InputStream getResourceAsStream(String s) { - return null; - } - - @Override - public RequestDispatcher getRequestDispatcher(String s) { - return null; - } - - @Override - public RequestDispatcher getNamedDispatcher(String s) { - return null; - } - - @Override - public Servlet getServlet(String s) throws ServletException { - return null; - } - - @Override - public Enumeration getServlets() { - return null; - } - - @Override - public Enumeration getServletNames() { - return null; - } - - @Override - public void log(String s) {} - - @Override - public void log(Exception e, String s) {} - - @Override - public void log(String s, Throwable throwable) {} - - @Override - public String getRealPath(String s) { - return null; - } - - @Override - public String getServerInfo() { - return null; - } - - @Override - public String getInitParameter(String s) { - return null; - } - - @Override - public Enumeration getInitParameterNames() { - return null; - } - - @Override - public Object getAttribute(String s) { - return null; - } - - @Override - public Enumeration getAttributeNames() { - return null; - } - - @Override - public void setAttribute(String s, Object o) {} - - @Override - public void removeAttribute(String s) {} - - @Override - public String getServletContextName() { - return null; - } - } - - class TestDispatcher implements RequestDispatcher { - - @Override - public void forward(ServletRequest servletRequest, ServletResponse servletResponse) - throws ServletException, IOException {} - - @Override - public void include(ServletRequest servletRequest, ServletResponse servletResponse) - throws ServletException, IOException {} - } -} diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy index 6cbabfccbf1..b2431a5dd55 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy @@ -1,8 +1,6 @@ import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServer import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions -import datadog.trace.api.iast.InstrumentationBridge -import datadog.trace.api.iast.sink.ApplicationModule import datadog.trace.api.iast.sink.SessionRewritingModule import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule @@ -544,7 +542,7 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync { then: 0 * appModule.onRealPath(_) 0 * sessionRewritingModule.checkSessionTrackingModes(_) - + 0 * _ } void 'test that iast modules are called'() { diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java b/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java deleted file mode 100644 index cf558e2a6db..00000000000 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/java/foo/bar/smoketest/RequestDispatcher3Utils.java +++ /dev/null @@ -1,305 +0,0 @@ -package foo.bar.smoketest; - -import java.io.InputStream; -import java.net.MalformedURLException; -import java.net.URL; -import java.util.Enumeration; -import java.util.EventListener; -import java.util.Map; -import java.util.Set; -import javax.servlet.Filter; -import javax.servlet.FilterRegistration; -import javax.servlet.RequestDispatcher; -import javax.servlet.Servlet; -import javax.servlet.ServletContext; -import javax.servlet.ServletException; -import javax.servlet.ServletRegistration; -import javax.servlet.SessionCookieConfig; -import javax.servlet.SessionTrackingMode; -import javax.servlet.descriptor.JspConfigDescriptor; - -/* RequestDispatcher can't be visible to groovy otherwise things break, so everything is - * encapsulated in here where groovy doesn't need to access it. - */ - -public class RequestDispatcher3Utils { - - private Set sessionTrackingModes; - - RequestDispatcher3Utils() {} - - RequestDispatcher3Utils(Set sessionTrackingModes) { - this.sessionTrackingModes = sessionTrackingModes; - } - - void getRealPath(final String target) { - new MockServletContext().getRealPath(target); - } - - class MockServletContext implements ServletContext { - - @Override - public String getContextPath() { - return null; - } - - @Override - public ServletContext getContext(String s) { - return null; - } - - @Override - public int getMajorVersion() { - return 0; - } - - @Override - public int getMinorVersion() { - return 0; - } - - @Override - public int getEffectiveMajorVersion() { - return 0; - } - - @Override - public int getEffectiveMinorVersion() { - return 0; - } - - @Override - public String getMimeType(String s) { - return null; - } - - @Override - public Set getResourcePaths(String s) { - return null; - } - - @Override - public URL getResource(String s) throws MalformedURLException { - return null; - } - - @Override - public InputStream getResourceAsStream(String s) { - return null; - } - - @Override - public RequestDispatcher getRequestDispatcher(String s) { - return null; - } - - @Override - public RequestDispatcher getNamedDispatcher(String s) { - return null; - } - - @Override - public Servlet getServlet(String s) throws ServletException { - return null; - } - - @Override - public Enumeration getServlets() { - return null; - } - - @Override - public Enumeration getServletNames() { - return null; - } - - @Override - public void log(String s) {} - - @Override - public void log(Exception e, String s) {} - - @Override - public void log(String s, Throwable throwable) {} - - @Override - public String getRealPath(String s) { - return null; - } - - @Override - public String getServerInfo() { - return null; - } - - @Override - public String getInitParameter(String s) { - return null; - } - - @Override - public Enumeration getInitParameterNames() { - return null; - } - - @Override - public boolean setInitParameter(String s, String s1) { - return false; - } - - @Override - public Object getAttribute(String s) { - return null; - } - - @Override - public Enumeration getAttributeNames() { - return null; - } - - @Override - public void setAttribute(String s, Object o) {} - - @Override - public void removeAttribute(String s) {} - - @Override - public String getServletContextName() { - return null; - } - - @Override - public ServletRegistration.Dynamic addServlet(String s, String s1) { - return null; - } - - @Override - public ServletRegistration.Dynamic addServlet(String s, Servlet servlet) { - return null; - } - - @Override - public ServletRegistration.Dynamic addServlet(String s, Class aClass) { - return null; - } - - @Override - public T createServlet(Class aClass) throws ServletException { - return null; - } - - @Override - public ServletRegistration getServletRegistration(String s) { - return null; - } - - @Override - public Map getServletRegistrations() { - return null; - } - - @Override - public FilterRegistration.Dynamic addFilter(String s, String s1) { - return null; - } - - @Override - public FilterRegistration.Dynamic addFilter(String s, Filter filter) { - return null; - } - - @Override - public FilterRegistration.Dynamic addFilter(String s, Class aClass) { - return null; - } - - @Override - public T createFilter(Class aClass) throws ServletException { - return null; - } - - @Override - public FilterRegistration getFilterRegistration(String s) { - return null; - } - - @Override - public Map getFilterRegistrations() { - return null; - } - - @Override - public SessionCookieConfig getSessionCookieConfig() { - return null; - } - - @Override - public void setSessionTrackingModes(Set set) { - sessionTrackingModes = set; - } - - @Override - public Set getDefaultSessionTrackingModes() { - return null; - } - - @Override - public Set getEffectiveSessionTrackingModes() { - return sessionTrackingModes; - } - - @Override - public void addListener(String s) {} - - @Override - public void addListener(T t) {} - - @Override - public void addListener(Class aClass) {} - - @Override - public T createListener(Class aClass) throws ServletException { - return null; - } - - @Override - public JspConfigDescriptor getJspConfigDescriptor() { - return null; - } - - @Override - public ClassLoader getClassLoader() { - return null; - } - - @Override - public void declareRoles(String... strings) {} - - @Override - public String getVirtualServerName() { - return null; - } - - public void setResponseCharacterEncoding(String s) {} - - public String getResponseCharacterEncoding() { - return null; - } - - public void setRequestCharacterEncoding(String s) {} - - public String getRequestCharacterEncoding() { - return null; - } - - public void setSessionTimeout(int timeout) {} - - public int getSessionTimeout() { - return 0; - } - - public ServletRegistration.Dynamic addJspFile(String s, String j) { - return null; - } - } -} diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaRequestDispatcherUtils.java b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaRequestDispatcherUtils.java deleted file mode 100644 index 8c6bc48b97d..00000000000 --- a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaRequestDispatcherUtils.java +++ /dev/null @@ -1,304 +0,0 @@ -import jakarta.servlet.Filter; -import jakarta.servlet.FilterRegistration; -import jakarta.servlet.RequestDispatcher; -import jakarta.servlet.Servlet; -import jakarta.servlet.ServletContext; -import jakarta.servlet.ServletException; -import jakarta.servlet.ServletRegistration; -import jakarta.servlet.SessionCookieConfig; -import jakarta.servlet.SessionTrackingMode; -import jakarta.servlet.descriptor.JspConfigDescriptor; -import java.io.InputStream; -import java.net.MalformedURLException; -import java.net.URL; -import java.util.Enumeration; -import java.util.EventListener; -import java.util.Map; -import java.util.Set; - -public class JakartaRequestDispatcherUtils { - - private Set sessionTrackingModes; - - JakartaRequestDispatcherUtils() {} - - JakartaRequestDispatcherUtils(Set sessionTrackingModes) { - this.sessionTrackingModes = sessionTrackingModes; - } - - void getRealPath(final String target) { - new TestContext().getRealPath(target); - } - - class TestContext implements ServletContext { - - @Override - public String getContextPath() { - return null; - } - - @Override - public ServletContext getContext(String s) { - return null; - } - - @Override - public int getMajorVersion() { - return 0; - } - - @Override - public int getMinorVersion() { - return 0; - } - - @Override - public int getEffectiveMajorVersion() { - return 0; - } - - @Override - public int getEffectiveMinorVersion() { - return 0; - } - - @Override - public String getMimeType(String s) { - return null; - } - - @Override - public Set getResourcePaths(String s) { - return null; - } - - @Override - public URL getResource(String s) throws MalformedURLException { - return null; - } - - @Override - public InputStream getResourceAsStream(String s) { - return null; - } - - @Override - public RequestDispatcher getRequestDispatcher(String s) { - return null; - } - - @Override - public RequestDispatcher getNamedDispatcher(String s) { - return null; - } - - @Override - public Servlet getServlet(String s) throws ServletException { - return null; - } - - @Override - public Enumeration getServlets() { - return null; - } - - @Override - public Enumeration getServletNames() { - return null; - } - - @Override - public void log(String s) {} - - @Override - public void log(Exception e, String s) {} - - @Override - public void log(String s, Throwable throwable) {} - - @Override - public String getRealPath(String s) { - return null; - } - - @Override - public String getServerInfo() { - return null; - } - - @Override - public String getInitParameter(String s) { - return null; - } - - @Override - public Enumeration getInitParameterNames() { - return null; - } - - @Override - public boolean setInitParameter(String s, String s1) { - return false; - } - - @Override - public Object getAttribute(String s) { - return null; - } - - @Override - public Enumeration getAttributeNames() { - return null; - } - - @Override - public void setAttribute(String s, Object o) {} - - @Override - public void removeAttribute(String s) {} - - @Override - public String getServletContextName() { - return null; - } - - @Override - public ServletRegistration.Dynamic addServlet(String s, String s1) { - return null; - } - - @Override - public ServletRegistration.Dynamic addServlet(String s, Servlet servlet) { - return null; - } - - @Override - public ServletRegistration.Dynamic addServlet(String s, Class aClass) { - return null; - } - - @Override - public ServletRegistration.Dynamic addJspFile(String s, String s1) { - return null; - } - - @Override - public T createServlet(Class aClass) throws ServletException { - return null; - } - - @Override - public ServletRegistration getServletRegistration(String s) { - return null; - } - - @Override - public Map getServletRegistrations() { - return null; - } - - @Override - public FilterRegistration.Dynamic addFilter(String s, String s1) { - return null; - } - - @Override - public FilterRegistration.Dynamic addFilter(String s, Filter filter) { - return null; - } - - @Override - public FilterRegistration.Dynamic addFilter(String s, Class aClass) { - return null; - } - - @Override - public T createFilter(Class aClass) throws ServletException { - return null; - } - - @Override - public FilterRegistration getFilterRegistration(String s) { - return null; - } - - @Override - public Map getFilterRegistrations() { - return null; - } - - @Override - public SessionCookieConfig getSessionCookieConfig() { - return null; - } - - @Override - public void setSessionTrackingModes(Set set) {} - - @Override - public Set getDefaultSessionTrackingModes() { - return null; - } - - @Override - public Set getEffectiveSessionTrackingModes() { - return sessionTrackingModes; - } - - @Override - public void addListener(String s) {} - - @Override - public void addListener(T t) {} - - @Override - public void addListener(Class aClass) {} - - @Override - public T createListener(Class aClass) throws ServletException { - return null; - } - - @Override - public JspConfigDescriptor getJspConfigDescriptor() { - return null; - } - - @Override - public ClassLoader getClassLoader() { - return null; - } - - @Override - public void declareRoles(String... strings) {} - - @Override - public String getVirtualServerName() { - return null; - } - - @Override - public int getSessionTimeout() { - return 0; - } - - @Override - public void setSessionTimeout(int i) {} - - @Override - public String getRequestCharacterEncoding() { - return null; - } - - @Override - public void setRequestCharacterEncoding(String s) {} - - @Override - public String getResponseCharacterEncoding() { - return null; - } - - @Override - public void setResponseCharacterEncoding(String s) {} - } -} From 7824d9b621f24571bb225546ca8a6a40c80152e3 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 14 Mar 2024 12:51:23 +0100 Subject: [PATCH 14/18] change session rewriting to serviceVulnerability --- .../datadog/iast/model/VulnerabilityType.java | 19 ++++++++++++++++++- .../iast/sink/SessionRewritingModuleImpl.java | 2 +- .../iast/model/VulnerabilityTypeTest.groovy | 4 ++++ 3 files changed, 23 insertions(+), 2 deletions(-) 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 676c083316b..8b7810f69c7 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 @@ -87,7 +87,7 @@ public interface VulnerabilityType { VulnerabilityTypes.REFLECTION_INJECTION, VulnerabilityMarks.REFLECTION_INJECTION_MARK); VulnerabilityType SESSION_REWRITING = - new VulnerabilityTypeImpl(VulnerabilityTypes.SESSION_REWRITING); + new ServiceVulnerabilityType(VulnerabilityTypes.SESSION_REWRITING); String name(); @@ -195,4 +195,21 @@ public long calculateHash(@Nonnull final Vulnerability vulnerability) { return crc.getValue(); } } + + class ServiceVulnerabilityType extends VulnerabilityTypeImpl { + public ServiceVulnerabilityType(byte type, int... marks) { + super(type, marks); + } + + @Override + public long calculateHash(@Nonnull final Vulnerability vulnerability) { + CRC32 crc = new CRC32(); + update(crc, name()); + String serviceName = vulnerability.getLocation().getServiceName(); + if (serviceName != null) { + update(crc, serviceName); + } + return crc.getValue(); + } + } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java index ac953c31fd5..3ef7e60e7b5 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java @@ -37,7 +37,7 @@ public void checkSessionTrackingModes(@NotNull Set sessionTrackingModes) span, new Vulnerability( VulnerabilityType.SESSION_REWRITING, - Location.forSpanAndStack(span, getCurrentStackTrace()), + Location.forSpan(span), new Evidence(EVIDENCE_VALUE))); } } diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/VulnerabilityTypeTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/VulnerabilityTypeTest.groovy index a73041bd45a..2d4f0d0dbd1 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/VulnerabilityTypeTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/VulnerabilityTypeTest.groovy @@ -6,6 +6,7 @@ import datadog.trace.test.util.DDSpecification import static com.datadog.iast.model.VulnerabilityType.INSECURE_COOKIE import static com.datadog.iast.model.VulnerabilityType.NO_HTTPONLY_COOKIE import static com.datadog.iast.model.VulnerabilityType.NO_SAMESITE_COOKIE +import static com.datadog.iast.model.VulnerabilityType.SESSION_REWRITING import static com.datadog.iast.model.VulnerabilityType.WEAK_CIPHER import static com.datadog.iast.model.VulnerabilityType.XCONTENTTYPE_HEADER_MISSING import static com.datadog.iast.model.VulnerabilityType.HSTS_HEADER_MISSING @@ -42,6 +43,9 @@ class VulnerabilityTypeTest extends DDSpecification { HSTS_HEADER_MISSING | getSpanLocation(123, null) | null | 121310697 HSTS_HEADER_MISSING | getSpanLocation(123, 'serviceName1') | null | 3533496951 HSTS_HEADER_MISSING | getSpanLocation(123, 'serviceName2') | null | 1268102093 + SESSION_REWRITING | getSpanLocation(123, null) | null | 2255304761 + SESSION_REWRITING | getSpanLocation(123, 'serviceName1') | null | 305779398 + SESSION_REWRITING | getSpanLocation(123, 'serviceName2') | null | 2335212412 } private Location getSpanAndStackLocation(final long spanId) { From c0932a2971000c433ce7b1a9faee91efb2e831b4 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Thu, 14 Mar 2024 14:22:07 +0100 Subject: [PATCH 15/18] fix codenarc --- .../servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy | 2 -- 1 file changed, 2 deletions(-) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy index 33b04cc8688..dc102a8b7e9 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy @@ -5,8 +5,6 @@ import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions import datadog.trace.api.CorrelationIdentifier import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule -import datadog.trace.api.iast.InstrumentationBridge -import datadog.trace.api.iast.sink.ApplicationModule import datadog.trace.api.iast.sink.SessionRewritingModule import datadog.trace.bootstrap.instrumentation.api.Tags import datadog.trace.instrumentation.servlet3.AsyncDispatcherDecorator From 4b3b6e4cd3e0ed3fae77fb56892c3ecd342e8580 Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Fri, 15 Mar 2024 08:22:08 +0100 Subject: [PATCH 16/18] change session rewriting evidence --- .../java/com/datadog/iast/sink/SessionRewritingModuleImpl.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java index 3ef7e60e7b5..e3dfad8cc8d 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java @@ -14,8 +14,7 @@ public class SessionRewritingModuleImpl extends SinkModuleBase implements SessionRewritingModule { - static final String EVIDENCE_VALUE = - "URL rewriting may be used by the container for session tracking"; + static final String EVIDENCE_VALUE = "Servlet URL Session Tracking Mode"; public SessionRewritingModuleImpl(final Dependencies dependencies) { super(dependencies); From e8a86de399cb81203201752e99a876ae07273d7e Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 1 Apr 2024 12:09:34 +0200 Subject: [PATCH 17/18] Add session rewriting to Application Module --- .../java/com/datadog/iast/IastSystem.java | 4 +- .../iast/sink/ApplicationModuleImpl.java | 18 +++++++ .../iast/sink/SessionRewritingModuleImpl.java | 42 --------------- .../iast/sink/ApplicationModuleTest.groovy | 30 +++++++++++ .../sink/SessionRewritingModuleTest.groovy | 51 ------------------- .../servlet3/IastServlet3Advice.java | 19 +++---- .../src/test/groovy/JettyServlet3Test.groovy | 10 ++-- .../src/test/groovy/TomcatServlet3Test.groovy | 11 ++-- .../IastJakartaServletInstrumentation.java | 19 +++---- ...stJakartaServletInstrumentationTest.groovy | 38 +------------- .../trace/api/iast/InstrumentationBridge.java | 2 - .../api/iast/sink/ApplicationModule.java | 4 ++ .../api/iast/sink/SessionRewritingModule.java | 9 ---- 13 files changed, 77 insertions(+), 180 deletions(-) delete mode 100644 dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java delete mode 100644 dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/SessionRewritingModuleTest.groovy delete mode 100644 internal-api/src/main/java/datadog/trace/api/iast/sink/SessionRewritingModule.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 f4ca98ea0f9..bd002f60416 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 @@ -21,7 +21,6 @@ import com.datadog.iast.sink.NoSameSiteCookieModuleImpl; import com.datadog.iast.sink.PathTraversalModuleImpl; import com.datadog.iast.sink.ReflectionInjectionModuleImpl; -import com.datadog.iast.sink.SessionRewritingModuleImpl; import com.datadog.iast.sink.SqlInjectionModuleImpl; import com.datadog.iast.sink.SsrfModuleImpl; import com.datadog.iast.sink.StacktraceLeakModuleImpl; @@ -148,8 +147,7 @@ private static Stream iastModules( ApplicationModuleImpl.class, HardcodedSecretModuleImpl.class, InsecureAuthProtocolModuleImpl.class, - ReflectionInjectionModuleImpl.class, - SessionRewritingModuleImpl.class); + ReflectionInjectionModuleImpl.class); if (iast != FULLY_ENABLED) { modules = modules.filter(IastSystem::isOptOut); } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java index 38ca75024ff..9c6b55f6559 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java @@ -32,6 +32,7 @@ import java.util.stream.Stream; import javax.annotation.Nonnull; import javax.annotation.Nullable; +import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -70,6 +71,8 @@ public class ApplicationModuleImpl extends SinkModuleBase implements Application public static final String WEB_XML = "web.xml"; + static final String SESSION_REWRITING_EVIDENCE_VALUE = "Servlet URL Session Tracking Mode"; + private static final Pattern PATTERN = Pattern.compile( Stream.of( @@ -103,6 +106,21 @@ public void onRealPath(final @Nullable String realPath) { checkWebXmlVulnerabilities(root, span); } + @Override + public void checkSessionTrackingModes(@NotNull Set sessionTrackingModes) { + if (!sessionTrackingModes.contains("URL")) { + return; + } + final AgentSpan span = AgentTracer.activeSpan(); + // overhead is not checked here as it's called once per application context + reporter.report( + span, + new Vulnerability( + VulnerabilityType.SESSION_REWRITING, + Location.forSpan(span), + new Evidence(SESSION_REWRITING_EVIDENCE_VALUE))); + } + private void checkWebXmlVulnerabilities(@Nonnull Path path, AgentSpan span) { String webXmlContent = webXmlContent(path); if (webXmlContent == null) { diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java deleted file mode 100644 index e3dfad8cc8d..00000000000 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/SessionRewritingModuleImpl.java +++ /dev/null @@ -1,42 +0,0 @@ -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.iast.IastContext; -import datadog.trace.api.iast.sink.SessionRewritingModule; -import datadog.trace.bootstrap.instrumentation.api.AgentSpan; -import datadog.trace.bootstrap.instrumentation.api.AgentTracer; -import java.util.Set; -import org.jetbrains.annotations.NotNull; - -public class SessionRewritingModuleImpl extends SinkModuleBase implements SessionRewritingModule { - - static final String EVIDENCE_VALUE = "Servlet URL Session Tracking Mode"; - - public SessionRewritingModuleImpl(final Dependencies dependencies) { - super(dependencies); - } - - @Override - public void checkSessionTrackingModes(@NotNull Set sessionTrackingModes) { - if (!sessionTrackingModes.contains("URL")) { - return; - } - - final IastContext ctx = IastContext.Provider.get(); - if (ctx == null) { - return; - } - final AgentSpan span = AgentTracer.activeSpan(); - // overhead is not checked here as it's called once per application context - reporter.report( - span, - new Vulnerability( - VulnerabilityType.SESSION_REWRITING, - Location.forSpan(span), - new Evidence(EVIDENCE_VALUE))); - } -} diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy index ab249758beb..3cdb4efaea1 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy @@ -2,6 +2,8 @@ package com.datadog.iast.sink import com.datadog.iast.IastModuleImplTestBase import com.datadog.iast.Reporter +import com.datadog.iast.model.Vulnerability +import com.datadog.iast.model.VulnerabilityType import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule @@ -11,6 +13,7 @@ import static com.datadog.iast.model.VulnerabilityType.DIRECTORY_LISTING_LEAK import static com.datadog.iast.model.VulnerabilityType.INSECURE_JSP_LAYOUT import static com.datadog.iast.model.VulnerabilityType.SESSION_TIMEOUT import static com.datadog.iast.model.VulnerabilityType.VERB_TAMPERING +import static com.datadog.iast.sink.ApplicationModuleImpl.SESSION_REWRITING_EVIDENCE_VALUE class ApplicationModuleTest extends IastModuleImplTestBase { @@ -71,6 +74,33 @@ class ApplicationModuleTest extends IastModuleImplTestBase { 'application/defaulthtmlescapeinvalid/no_tag_2' | DEFAULT_HTML_ESCAPE_INVALID | 'defaultHtmlEscape tag should be set' | NO_LINE } + void 'iast module detects session rewriting on sessionTrackingModes'() { + when: + module.checkSessionTrackingModes(sessionTrackingModes as Set) + + then: + if (expected != null) { + 1 * reporter.report(_, _) >> { args -> assertSessionRewriting(args[1] as Vulnerability, expected) } + } else { + 0 * reporter.report(_, _) + } + + where: + sessionTrackingModes | expected + [] | null + ['COOKIE'] | null + ['URL'] | SESSION_REWRITING_EVIDENCE_VALUE + ['COOKIE', 'URL'] | SESSION_REWRITING_EVIDENCE_VALUE + } + + private static void assertSessionRewriting(final Vulnerability vuln, final String expected) { + assert vuln != null + assert vuln.getType() == VulnerabilityType.SESSION_REWRITING + assert vuln.getLocation() != null + final evidence = vuln.getEvidence() + assert evidence.value == expected + } + private static void assertVulnerability(final vuln, final expectedVulnType) { assert vuln != null assert vuln.getType() == expectedVulnType diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/SessionRewritingModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/SessionRewritingModuleTest.groovy deleted file mode 100644 index d21d0b88478..00000000000 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/SessionRewritingModuleTest.groovy +++ /dev/null @@ -1,51 +0,0 @@ -package com.datadog.iast.sink - -import com.datadog.iast.IastModuleImplTestBase -import com.datadog.iast.Reporter -import com.datadog.iast.model.Vulnerability -import com.datadog.iast.model.VulnerabilityType -import datadog.trace.api.iast.sink.SessionRewritingModule - -import static com.datadog.iast.sink.SessionRewritingModuleImpl.EVIDENCE_VALUE - -class SessionRewritingModuleTest extends IastModuleImplTestBase { - - - private SessionRewritingModule module - - def setup() { - module = new SessionRewritingModuleImpl(dependencies) - } - - @Override - protected Reporter buildReporter() { - return Mock(Reporter) - } - - void 'iast module detects session rewriting on sessionTrackingModes'() { - when: - module.checkSessionTrackingModes(sessionTrackingModes as Set) - - then: - if (expected != null) { - 1 * reporter.report(_, _) >> { args -> assertVulnerability(args[1] as Vulnerability, expected) } - } else { - 0 * reporter.report(_, _) - } - - where: - sessionTrackingModes | expected - [] | null - ['COOKIE'] | null - ['URL'] | EVIDENCE_VALUE - ['COOKIE', 'URL'] | EVIDENCE_VALUE - } - - private static void assertVulnerability(final Vulnerability vuln, final String expected) { - assert vuln != null - assert vuln.getType() == VulnerabilityType.SESSION_REWRITING - assert vuln.getLocation() != null - final evidence = vuln.getEvidence() - assert evidence.value == expected - } -} diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java index 1b8c8f896b8..80de7211a60 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java @@ -4,7 +4,6 @@ import datadog.trace.api.iast.Sink; import datadog.trace.api.iast.VulnerabilityTypes; import datadog.trace.api.iast.sink.ApplicationModule; -import datadog.trace.api.iast.sink.SessionRewritingModule; import datadog.trace.bootstrap.InstrumentationContext; import java.util.HashSet; import java.util.Set; @@ -20,8 +19,7 @@ public class IastServlet3Advice { @Advice.OnMethodExit(suppress = Throwable.class) public static void onExit(@Advice.Argument(0) ServletRequest request) { final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION; - final SessionRewritingModule sessionRewritingModule = InstrumentationBridge.SESSION_REWRITING; - if (applicationModule == null && sessionRewritingModule == null) { + if (applicationModule == null) { return; } if (!(request instanceof HttpServletRequest)) { @@ -34,15 +32,14 @@ public static void onExit(@Advice.Argument(0) ServletRequest request) { InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true); if (applicationModule != null) { applicationModule.onRealPath(context.getRealPath("/")); - } - if (sessionRewritingModule != null - && context.getEffectiveSessionTrackingModes() != null - && !context.getEffectiveSessionTrackingModes().isEmpty()) { - Set sessionTrackingModes = new HashSet<>(); - for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { - sessionTrackingModes.add(mode.name()); + if (context.getEffectiveSessionTrackingModes() != null + && !context.getEffectiveSessionTrackingModes().isEmpty()) { + Set sessionTrackingModes = new HashSet<>(); + for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { + sessionTrackingModes.add(mode.name()); + } + applicationModule.checkSessionTrackingModes(sessionTrackingModes); } - sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes); } } } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy index b2431a5dd55..cbc19f024cd 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy @@ -1,7 +1,6 @@ import datadog.trace.agent.test.asserts.TraceAssert import datadog.trace.agent.test.base.HttpServer import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions -import datadog.trace.api.iast.sink.SessionRewritingModule import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule import datadog.trace.bootstrap.instrumentation.api.AgentSpan @@ -533,7 +532,6 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync { void 'test no calls if no modules registered'() { given: final appModule = Mock(ApplicationModule) - final sessionRewritingModule = Mock(SessionRewritingModule) def request = request(SUCCESS, "GET", null).build() when: @@ -541,16 +539,14 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync { then: 0 * appModule.onRealPath(_) - 0 * sessionRewritingModule.checkSessionTrackingModes(_) + 0 * appModule.checkSessionTrackingModes(_) 0 * _ } void 'test that iast modules are called'() { given: final appModule = Mock(ApplicationModule) - final sessionRewritingModule = Mock(SessionRewritingModule) InstrumentationBridge.registerIastModule(appModule) - InstrumentationBridge.registerIastModule(sessionRewritingModule) def request = request(SUCCESS, "GET", null).build() when: @@ -558,7 +554,7 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync { then: 1 * appModule.onRealPath(_) - 1 * sessionRewritingModule.checkSessionTrackingModes(_) + 1 * appModule.checkSessionTrackingModes(_) 0 * _ when: @@ -566,7 +562,7 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync { then: //Only call once per application context 0 * appModule.onRealPath(_) - 0 * sessionRewritingModule.checkSessionTrackingModes(_) + 0 * appModule.checkSessionTrackingModes(_) 0 * _ } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy index dc102a8b7e9..c8b265df0d1 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy @@ -5,7 +5,7 @@ import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions import datadog.trace.api.CorrelationIdentifier import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule -import datadog.trace.api.iast.sink.SessionRewritingModule + import datadog.trace.bootstrap.instrumentation.api.Tags import datadog.trace.instrumentation.servlet3.AsyncDispatcherDecorator import datadog.trace.instrumentation.servlet3.TestServlet3 @@ -539,7 +539,6 @@ class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync { void 'test no calls if no modules registered'() { given: final appModule = Mock(ApplicationModule) - final sessionRewritingModule = Mock(SessionRewritingModule) def request = request(SUCCESS, "GET", null).build() when: @@ -547,16 +546,14 @@ class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync { then: 0 * appModule.onRealPath(_) - 0 * sessionRewritingModule.checkSessionTrackingModes(_) + 0 * appModule.checkSessionTrackingModes(_) 0 * _ } void 'test that iast modules are called'() { given: final appModule = Mock(ApplicationModule) - final sessionRewritingModule = Mock(SessionRewritingModule) InstrumentationBridge.registerIastModule(appModule) - InstrumentationBridge.registerIastModule(sessionRewritingModule) def request = request(SUCCESS, "GET", null).build() when: @@ -564,7 +561,7 @@ class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync { then: 1 * appModule.onRealPath(_) - 1 * sessionRewritingModule.checkSessionTrackingModes(_) + 1 * appModule.checkSessionTrackingModes(_) 0 * _ when: @@ -572,7 +569,7 @@ class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync { then: //Only call once per application context 0 * appModule.onRealPath(_) - 0 * sessionRewritingModule.checkSessionTrackingModes(_) + 0 * appModule.checkSessionTrackingModes(_) 0 * _ } diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java index ae664b13cb2..24e0f2131d9 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java @@ -12,7 +12,6 @@ import datadog.trace.agent.tooling.InstrumenterModule; import datadog.trace.api.iast.InstrumentationBridge; import datadog.trace.api.iast.sink.ApplicationModule; -import datadog.trace.api.iast.sink.SessionRewritingModule; import datadog.trace.bootstrap.InstrumentationContext; import jakarta.servlet.ServletContext; import jakarta.servlet.SessionTrackingMode; @@ -64,8 +63,7 @@ public static class IastAdvice { @Advice.OnMethodExit(suppress = Throwable.class) public static void after(@Advice.This final HttpServlet servlet) { final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION; - final SessionRewritingModule sessionRewritingModule = InstrumentationBridge.SESSION_REWRITING; - if (applicationModule == null && sessionRewritingModule == null) { + if (applicationModule == null) { return; } final ServletContext context = servlet.getServletContext(); @@ -75,15 +73,14 @@ public static void after(@Advice.This final HttpServlet servlet) { InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true); if (applicationModule != null) { applicationModule.onRealPath(context.getRealPath("/")); - } - if (sessionRewritingModule != null - && context.getEffectiveSessionTrackingModes() != null - && !context.getEffectiveSessionTrackingModes().isEmpty()) { - Set sessionTrackingModes = new HashSet<>(); - for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { - sessionTrackingModes.add(mode.name()); + if (context.getEffectiveSessionTrackingModes() != null + && !context.getEffectiveSessionTrackingModes().isEmpty()) { + Set sessionTrackingModes = new HashSet<>(); + for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) { + sessionTrackingModes.add(mode.name()); + } + applicationModule.checkSessionTrackingModes(sessionTrackingModes); } - sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes); } } } diff --git a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy index 831fc769809..4b7ab747631 100644 --- a/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy +++ b/dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy @@ -1,7 +1,6 @@ import datadog.trace.agent.test.AgentTestRunner import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule -import datadog.trace.api.iast.sink.SessionRewritingModule import foo.bar.smoketest.DummyHttpServlet import foo.bar.smoketest.DummyRequest import foo.bar.smoketest.DummyResponse @@ -20,7 +19,6 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{ void 'test no modules'() { final appModule = Mock(ApplicationModule) - final sessionModule = Mock(SessionRewritingModule) final Servlet servlet = new DummyHttpServlet() final ServletResponse response = new DummyResponse() final ServletRequest request = new DummyRequest() @@ -30,7 +28,7 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{ then: 0 * appModule.onRealPath(_) - 0 * sessionModule.checkSessionTrackingModes(_) + 0 * appModule.checkSessionTrackingModes(_) 0 * _ } @@ -47,41 +45,7 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{ then: 1 * module.onRealPath(_) - 0 * _ - } - - void 'test SessionRewriting'() { - given: - final module = Mock(SessionRewritingModule) - InstrumentationBridge.registerIastModule(module) - final Servlet servlet = new DummyHttpServlet() - final ServletResponse response = new DummyResponse() - final ServletRequest request = new DummyRequest() - - when: - servlet.callPublicServiceMethod(request, response) - - then: 1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) 0 * _ } - - void 'test all modules'() { - given: - final appModule = Mock(ApplicationModule) - InstrumentationBridge.registerIastModule(appModule) - final sessionModule = Mock(SessionRewritingModule) - InstrumentationBridge.registerIastModule(sessionModule) - final Servlet servlet = new DummyHttpServlet() - final ServletResponse response = new DummyResponse() - final ServletRequest request = new DummyRequest() - - when: - servlet.callPublicServiceMethod(request, response) - - then: - 1 * appModule.onRealPath(_) - 1 * sessionModule.checkSessionTrackingModes(['COOKIE', 'URL'] as Set) - 0 * _ - } } 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 3501495cfe7..cb89379ce91 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 @@ -16,7 +16,6 @@ import datadog.trace.api.iast.sink.NoSameSiteCookieModule; import datadog.trace.api.iast.sink.PathTraversalModule; import datadog.trace.api.iast.sink.ReflectionInjectionModule; -import datadog.trace.api.iast.sink.SessionRewritingModule; import datadog.trace.api.iast.sink.SqlInjectionModule; import datadog.trace.api.iast.sink.SsrfModule; import datadog.trace.api.iast.sink.StacktraceLeakModule; @@ -66,7 +65,6 @@ public abstract class InstrumentationBridge { public static HardcodedSecretModule HARDCODED_SECRET; public static InsecureAuthProtocolModule INSECURE_AUTH_PROTOCOL; public static ReflectionInjectionModule REFLECTION_INJECTION; - public static SessionRewritingModule SESSION_REWRITING; private static final Map, Field> MODULE_MAP = buildModuleMap(); diff --git a/internal-api/src/main/java/datadog/trace/api/iast/sink/ApplicationModule.java b/internal-api/src/main/java/datadog/trace/api/iast/sink/ApplicationModule.java index 795f2cac277..2282324cdc8 100644 --- a/internal-api/src/main/java/datadog/trace/api/iast/sink/ApplicationModule.java +++ b/internal-api/src/main/java/datadog/trace/api/iast/sink/ApplicationModule.java @@ -2,10 +2,14 @@ import datadog.trace.api.iast.IastModule; import datadog.trace.api.iast.IastModule.OptOut; +import java.util.Set; +import javax.annotation.Nonnull; import javax.annotation.Nullable; @OptOut public interface ApplicationModule extends IastModule { void onRealPath(@Nullable String realPath); + + void checkSessionTrackingModes(@Nonnull Set sessionTrackingModes); } diff --git a/internal-api/src/main/java/datadog/trace/api/iast/sink/SessionRewritingModule.java b/internal-api/src/main/java/datadog/trace/api/iast/sink/SessionRewritingModule.java deleted file mode 100644 index a98fe53b31e..00000000000 --- a/internal-api/src/main/java/datadog/trace/api/iast/sink/SessionRewritingModule.java +++ /dev/null @@ -1,9 +0,0 @@ -package datadog.trace.api.iast.sink; - -import datadog.trace.api.iast.IastModule; -import java.util.Set; -import javax.annotation.Nonnull; - -public interface SessionRewritingModule extends IastModule { - void checkSessionTrackingModes(@Nonnull Set sessionTrackingModes); -} From 41cbeecab40a3d5efd1d8e346f2efa8a2df154aa Mon Sep 17 00:00:00 2001 From: "alejandro.gonzalez" Date: Mon, 1 Apr 2024 12:19:32 +0200 Subject: [PATCH 18/18] self review --- .../java/com/datadog/iast/sink/ApplicationModuleImpl.java | 3 +-- .../groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy | 4 +--- .../request-3/src/test/groovy/JettyServlet3Test.groovy | 3 +-- .../request-3/src/test/groovy/TomcatServlet3Test.groovy | 4 +--- 4 files changed, 4 insertions(+), 10 deletions(-) diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java index 9c6b55f6559..e653a758b9f 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java @@ -32,7 +32,6 @@ import java.util.stream.Stream; import javax.annotation.Nonnull; import javax.annotation.Nullable; -import org.jetbrains.annotations.NotNull; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -107,7 +106,7 @@ public void onRealPath(final @Nullable String realPath) { } @Override - public void checkSessionTrackingModes(@NotNull Set sessionTrackingModes) { + public void checkSessionTrackingModes(@Nonnull Set sessionTrackingModes) { if (!sessionTrackingModes.contains("URL")) { return; } diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy index 3cdb4efaea1..32b0947ce20 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy @@ -94,9 +94,7 @@ class ApplicationModuleTest extends IastModuleImplTestBase { } private static void assertSessionRewriting(final Vulnerability vuln, final String expected) { - assert vuln != null - assert vuln.getType() == VulnerabilityType.SESSION_REWRITING - assert vuln.getLocation() != null + assertVulnerability(vuln, VulnerabilityType.SESSION_REWRITING) final evidence = vuln.getEvidence() assert evidence.value == expected } diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy index cbc19f024cd..91976249092 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy @@ -12,7 +12,6 @@ import org.eclipse.jetty.server.Request import org.eclipse.jetty.server.Server import org.eclipse.jetty.server.handler.ErrorHandler import org.eclipse.jetty.servlet.ServletContextHandler - import javax.servlet.AsyncEvent import javax.servlet.AsyncListener import javax.servlet.Servlet @@ -543,7 +542,7 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync { 0 * _ } - void 'test that iast modules are called'() { + void 'test that iast module is called'() { given: final appModule = Mock(ApplicationModule) InstrumentationBridge.registerIastModule(appModule) diff --git a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy index c8b265df0d1..f4f2b55fd25 100644 --- a/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy +++ b/dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy @@ -5,7 +5,6 @@ import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions import datadog.trace.api.CorrelationIdentifier import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule - import datadog.trace.bootstrap.instrumentation.api.Tags import datadog.trace.instrumentation.servlet3.AsyncDispatcherDecorator import datadog.trace.instrumentation.servlet3.TestServlet3 @@ -20,7 +19,6 @@ import org.apache.catalina.valves.ValveBase import org.apache.tomcat.JarScanFilter import org.apache.tomcat.JarScanType import spock.lang.Shared - import javax.servlet.Servlet import javax.servlet.ServletException @@ -550,7 +548,7 @@ class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync { 0 * _ } - void 'test that iast modules are called'() { + void 'test that iast module is called'() { given: final appModule = Mock(ApplicationModule) InstrumentationBridge.registerIastModule(appModule)