Skip to content

Commit 598bb20

Browse files
jandro996jandro996
authored
Fix session rewriting false positives (#7323)
What Does This Do Remove Session Tracking Modes check calls from servlet instrumentation Add IAST instrumentation to HttpSession#getSession with calls to Session Tracking Modes check Solved for javax and jakarta implementations Motivation The current implementation for detecting Session Rewriting is causing false positives because it only considers the servlet configuration. This may be insecure, but if the session is not being used, an Session Rewriting should not be reported.
1 parent e0b3787 commit 598bb20

10 files changed

Lines changed: 258 additions & 26 deletions

File tree

‎dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastOptOutHttpServletRequest3Instrumentation.java‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,110 @@
1+
package datadog.trace.instrumentation.servlet3;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.ClassLoaderMatchers.hasClassNamed;
4+
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.extendsClass;
5+
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface;
6+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
7+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedNoneOf;
8+
import static net.bytebuddy.matcher.ElementMatchers.*;
9+
10+
import com.google.auto.service.AutoService;
11+
import datadog.trace.agent.tooling.Instrumenter;
12+
import datadog.trace.agent.tooling.InstrumenterModule;
13+
import datadog.trace.api.iast.InstrumentationBridge;
14+
import datadog.trace.api.iast.Sink;
15+
import datadog.trace.api.iast.VulnerabilityTypes;
16+
import datadog.trace.api.iast.sink.ApplicationModule;
17+
import datadog.trace.bootstrap.InstrumentationContext;
18+
import java.util.Collections;
19+
import java.util.HashSet;
20+
import java.util.Map;
21+
import java.util.Set;
22+
import javax.servlet.ServletContext;
23+
import javax.servlet.SessionTrackingMode;
24+
import javax.servlet.http.HttpServletRequest;
25+
import javax.servlet.http.HttpSession;
26+
import net.bytebuddy.asm.Advice;
27+
import net.bytebuddy.description.type.TypeDescription;
28+
import net.bytebuddy.matcher.ElementMatcher;
29+
30+
@AutoService(InstrumenterModule.class)
31+
public class IastOptOutHttpServletRequest3Instrumentation extends InstrumenterModule.Iast
32+
implements Instrumenter.ForTypeHierarchy {
33+
34+
public IastOptOutHttpServletRequest3Instrumentation() {
35+
super("servlet", "servlet-3");
36+
}
37+
38+
@Override
39+
public String muzzleDirective() {
40+
return "servlet-3.x";
41+
}
42+
43+
@Override
44+
public ElementMatcher.Junction<ClassLoader> classLoaderMatcher() {
45+
// Avoid matching request before servlet-3.x which don't have session tracking modes
46+
return hasClassNamed("javax.servlet.SessionTrackingMode");
47+
}
48+
49+
@Override
50+
public String hierarchyMarkerType() {
51+
return "javax.servlet.http.HttpServletRequest";
52+
}
53+
54+
@Override
55+
public ElementMatcher<TypeDescription> hierarchyMatcher() {
56+
return implementsInterface(named(hierarchyMarkerType()))
57+
// ignore wrappers that ship with servlet-api
58+
.and(namedNoneOf("javax.servlet.http.HttpServletRequestWrapper"))
59+
.and(not(extendsClass(named("javax.servlet.http.HttpServletRequestWrapper"))));
60+
}
61+
62+
@Override
63+
public void methodAdvice(MethodTransformer transformer) {
64+
transformer.applyAdvice(
65+
named("getSession").and(returns(named("javax.servlet.http.HttpSession"))).and(isPublic()),
66+
getClass().getName() + "$GetHttpSessionAdvice");
67+
}
68+
69+
@Override
70+
public Map<String, String> contextStore() {
71+
return Collections.singletonMap(
72+
"javax.servlet.ServletContext", "javax.servlet.SessionTrackingMode");
73+
}
74+
75+
@Override
76+
protected boolean isOptOutEnabled() {
77+
return true;
78+
}
79+
80+
public static class GetHttpSessionAdvice {
81+
@Advice.OnMethodExit(suppress = Throwable.class)
82+
@Sink(VulnerabilityTypes.SESSION_REWRITING)
83+
public static void onExit(
84+
@Advice.This final HttpServletRequest request, @Advice.Return final HttpSession session) {
85+
if (session == null) {
86+
return;
87+
}
88+
final ApplicationModule module = InstrumentationBridge.APPLICATION;
89+
if (module == null) {
90+
return;
91+
}
92+
final ServletContext context = request.getServletContext();
93+
if (InstrumentationContext.get(ServletContext.class, SessionTrackingMode.class).get(context)
94+
!= null) {
95+
return;
96+
}
97+
// We only want to report it once per application
98+
InstrumentationContext.get(ServletContext.class, SessionTrackingMode.class)
99+
.put(context, SessionTrackingMode.URL);
100+
if (context.getEffectiveSessionTrackingModes() != null
101+
&& !context.getEffectiveSessionTrackingModes().isEmpty()) {
102+
Set<String> sessionTrackingModes = new HashSet<>();
103+
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
104+
sessionTrackingModes.add(mode.name());
105+
}
106+
module.checkSessionTrackingModes(sessionTrackingModes);
107+
}
108+
}
109+
}
110+
}

‎dd-java-agent/instrumentation/servlet/request-3/src/main/java/datadog/trace/instrumentation/servlet3/IastServlet3Advice.java‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,8 @@
55
import datadog.trace.api.iast.VulnerabilityTypes;
66
import datadog.trace.api.iast.sink.ApplicationModule;
77
import datadog.trace.bootstrap.InstrumentationContext;
8-
import java.util.HashSet;
9-
import java.util.Set;
108
import javax.servlet.ServletContext;
119
import javax.servlet.ServletRequest;
12-
import javax.servlet.SessionTrackingMode;
1310
import javax.servlet.http.HttpServletRequest;
1411
import net.bytebuddy.asm.Advice;
1512

@@ -32,14 +29,6 @@ public static void onExit(@Advice.Argument(0) ServletRequest request) {
3229
InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true);
3330
if (applicationModule != null) {
3431
applicationModule.onRealPath(context.getRealPath("/"));
35-
if (context.getEffectiveSessionTrackingModes() != null
36-
&& !context.getEffectiveSessionTrackingModes().isEmpty()) {
37-
Set<String> sessionTrackingModes = new HashSet<>();
38-
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
39-
sessionTrackingModes.add(mode.name());
40-
}
41-
applicationModule.checkSessionTrackingModes(sessionTrackingModes);
42-
}
4332
}
4433
}
4534
}

‎dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/JettyServlet3Test.groovy‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import groovy.servlet.AbstractHttpServlet
1111
import org.eclipse.jetty.server.Request
1212
import org.eclipse.jetty.server.Server
1313
import org.eclipse.jetty.server.handler.ErrorHandler
14+
import org.eclipse.jetty.server.session.SessionHandler
1415
import org.eclipse.jetty.servlet.ServletContextHandler
1516
import javax.servlet.AsyncEvent
1617
import javax.servlet.AsyncListener
@@ -52,6 +53,7 @@ abstract class JettyServlet3Test extends AbstractServlet3Test<Server, ServletCon
5253
}
5354

5455
ServletContextHandler servletContext = new ServletContextHandler(null, "/$context", ServletContextHandler.SESSIONS)
56+
servletContext.sessionHandler = new SessionHandler()
5557
servletContext.errorHandler = new ErrorHandler() {
5658
@Override
5759
void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException {
@@ -522,6 +524,11 @@ class JettyServlet3ServeFromAsyncTimeout extends JettyServlet3Test {
522524

523525
class IastJettyServlet3ForkedTest extends JettyServlet3TestSync {
524526

527+
@Override
528+
Class<Servlet> servlet() {
529+
return TestServlet3.GetSession
530+
}
531+
525532
@Override
526533
void configurePreAgent() {
527534
super.configurePreAgent()

‎dd-java-agent/instrumentation/servlet/request-3/src/test/groovy/TomcatServlet3Test.groovy‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,11 @@ class TomcatServlet3TestDispatchAsync extends TomcatServlet3Test {
528528

529529
class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync {
530530

531+
@Override
532+
Class<Servlet> servlet() {
533+
return TestServlet3.GetSession
534+
}
535+
531536
@Override
532537
void configurePreAgent() {
533538
super.configurePreAgent()

‎dd-java-agent/instrumentation/servlet/request-3/src/testFixtures/groovy/datadog/trace/instrumentation/servlet3/TestServlet3.groovy‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,4 +305,13 @@ class TestServlet3 {
305305
}
306306
}
307307
}
308+
309+
@WebServlet
310+
static class GetSession extends Sync {
311+
@Override
312+
void service(HttpServletRequest req, HttpServletResponse resp) {
313+
req.getSession(true)
314+
super.service(req,resp)
315+
}
316+
}
308317
}

‎dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastJakartaServletInstrumentation.java‎

Lines changed: 0 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,9 @@
1414
import datadog.trace.api.iast.sink.ApplicationModule;
1515
import datadog.trace.bootstrap.InstrumentationContext;
1616
import jakarta.servlet.ServletContext;
17-
import jakarta.servlet.SessionTrackingMode;
1817
import jakarta.servlet.http.HttpServlet;
1918
import java.util.Collections;
20-
import java.util.HashSet;
2119
import java.util.Map;
22-
import java.util.Set;
2320
import net.bytebuddy.asm.Advice;
2421
import net.bytebuddy.description.type.TypeDescription;
2522
import net.bytebuddy.matcher.ElementMatcher;
@@ -78,14 +75,6 @@ public static void after(@Advice.This final HttpServlet servlet) {
7875
InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true);
7976
if (applicationModule != null) {
8077
applicationModule.onRealPath(context.getRealPath("/"));
81-
if (context.getEffectiveSessionTrackingModes() != null
82-
&& !context.getEffectiveSessionTrackingModes().isEmpty()) {
83-
Set<String> sessionTrackingModes = new HashSet<>();
84-
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
85-
sessionTrackingModes.add(mode.name());
86-
}
87-
applicationModule.checkSessionTrackingModes(sessionTrackingModes);
88-
}
8978
}
9079
}
9180
}

‎dd-java-agent/instrumentation/servlet/request-5/src/main/java/datadog/trace/instrumentation/servlet5/IastOptOutJakartaHttpServletRequestInstrumentation.java‎

Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
package datadog.trace.instrumentation.servlet5;
2+
3+
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.extendsClass;
4+
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface;
5+
import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
6+
import static net.bytebuddy.matcher.ElementMatchers.*;
7+
8+
import com.google.auto.service.AutoService;
9+
import datadog.trace.agent.tooling.Instrumenter;
10+
import datadog.trace.agent.tooling.InstrumenterModule;
11+
import datadog.trace.api.iast.*;
12+
import datadog.trace.api.iast.sink.ApplicationModule;
13+
import datadog.trace.bootstrap.InstrumentationContext;
14+
import jakarta.servlet.ServletContext;
15+
import jakarta.servlet.SessionTrackingMode;
16+
import jakarta.servlet.http.HttpServletRequest;
17+
import jakarta.servlet.http.HttpSession;
18+
import java.util.*;
19+
import net.bytebuddy.asm.Advice;
20+
import net.bytebuddy.description.type.TypeDescription;
21+
import net.bytebuddy.matcher.ElementMatcher;
22+
23+
@SuppressWarnings("unused")
24+
@AutoService(InstrumenterModule.class)
25+
public class IastOptOutJakartaHttpServletRequestInstrumentation extends InstrumenterModule.Iast
26+
implements Instrumenter.ForTypeHierarchy {
27+
28+
private static final String CLASS_NAME =
29+
IastOptOutJakartaHttpServletRequestInstrumentation.class.getName();
30+
private static final ElementMatcher.Junction<? super TypeDescription> WRAPPER_CLASS =
31+
named("jakarta.servlet.http.HttpServletRequestWrapper");
32+
33+
public IastOptOutJakartaHttpServletRequestInstrumentation() {
34+
super("servlet", "servlet-5", "servlet-request");
35+
}
36+
37+
@Override
38+
public String hierarchyMarkerType() {
39+
return "jakarta.servlet.http.HttpServletRequest";
40+
}
41+
42+
@Override
43+
public ElementMatcher<TypeDescription> hierarchyMatcher() {
44+
return implementsInterface(named(hierarchyMarkerType()))
45+
.and(not(WRAPPER_CLASS))
46+
.and(not(extendsClass(WRAPPER_CLASS)));
47+
}
48+
49+
@Override
50+
protected boolean isOptOutEnabled() {
51+
return true;
52+
}
53+
54+
@Override
55+
public void methodAdvice(MethodTransformer transformer) {
56+
transformer.applyAdvice(
57+
named("getSession").and(returns(named("jakarta.servlet.http.HttpSession"))).and(isPublic()),
58+
CLASS_NAME + "$GetHttpSessionAdvice");
59+
}
60+
61+
@Override
62+
public Map<String, String> contextStore() {
63+
return Collections.singletonMap(
64+
"jakarta.servlet.ServletContext", "jakarta.servlet.SessionTrackingMode");
65+
}
66+
67+
public static class GetHttpSessionAdvice {
68+
@Advice.OnMethodExit(suppress = Throwable.class)
69+
@Sink(VulnerabilityTypes.SESSION_REWRITING)
70+
public static void onExit(
71+
@Advice.This final HttpServletRequest request, @Advice.Return final HttpSession session) {
72+
if (session == null) {
73+
return;
74+
}
75+
final ApplicationModule module = InstrumentationBridge.APPLICATION;
76+
if (module == null) {
77+
return;
78+
}
79+
final ServletContext context = request.getServletContext();
80+
81+
if (InstrumentationContext.get(ServletContext.class, SessionTrackingMode.class).get(context)
82+
!= null) {
83+
return;
84+
}
85+
// We only want to report it once per application
86+
InstrumentationContext.get(ServletContext.class, SessionTrackingMode.class)
87+
.put(context, SessionTrackingMode.URL);
88+
if (context.getEffectiveSessionTrackingModes() != null
89+
&& !context.getEffectiveSessionTrackingModes().isEmpty()) {
90+
Set<String> sessionTrackingModes = new HashSet<>();
91+
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
92+
sessionTrackingModes.add(mode.name());
93+
}
94+
module.checkSessionTrackingModes(sessionTrackingModes);
95+
}
96+
}
97+
}
98+
}

‎dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/IastJakartaServletInstrumentationTest.groovy‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ import jakarta.servlet.Servlet
88
import jakarta.servlet.ServletRequest
99
import jakarta.servlet.ServletResponse
1010

11-
12-
1311
class IastJakartaServletInstrumentationTest extends AgentTestRunner{
1412

1513
@Override
@@ -45,7 +43,6 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{
4543

4644
then:
4745
1 * module.onRealPath(_)
48-
1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set<String>)
4946
0 * _
5047
}
5148
}

‎dd-java-agent/instrumentation/servlet/request-5/src/test/groovy/JakartaHttpServletRequestInstrumentationTest.groovy‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,19 +3,23 @@ import datadog.trace.api.iast.IastContext
33
import datadog.trace.api.iast.InstrumentationBridge
44
import datadog.trace.api.iast.SourceTypes
55
import datadog.trace.api.iast.propagation.PropagationModule
6+
import datadog.trace.api.iast.sink.ApplicationModule
67
import datadog.trace.api.iast.sink.UnvalidatedRedirectModule
78
import datadog.trace.bootstrap.instrumentation.api.AgentTracer
89
import datadog.trace.bootstrap.instrumentation.api.TagContext
910
import foo.bar.smoketest.JakartaHttpServletRequestTestSuite
1011
import foo.bar.smoketest.JakartaHttpServletRequestWrapperTestSuite
1112
import foo.bar.smoketest.ServletRequestTestSuite
1213
import jakarta.servlet.RequestDispatcher
14+
import jakarta.servlet.ServletContext
1315
import jakarta.servlet.ServletInputStream
16+
import jakarta.servlet.SessionTrackingMode
1417
import jakarta.servlet.http.Cookie
1518
import jakarta.servlet.http.HttpServletRequest
1619
import jakarta.servlet.http.HttpServletRequestWrapper
1720

1821
import datadog.trace.agent.tooling.iast.TaintableEnumeration
22+
import jakarta.servlet.http.HttpSession
1923

2024
class JakartaHttpServletRequestInstrumentationTest extends AgentTestRunner {
2125

@@ -431,6 +435,31 @@ class JakartaHttpServletRequestInstrumentationTest extends AgentTestRunner {
431435
suite << testSuiteCallSites()
432436
}
433437

438+
void 'test getSession'() {
439+
setup:
440+
final iastModule = Mock(ApplicationModule)
441+
InstrumentationBridge.registerIastModule(iastModule)
442+
final session = Mock(HttpSession)
443+
final servletContext = Mock(ServletContext) {
444+
getEffectiveSessionTrackingModes() >> new HashSet<SessionTrackingMode>(Arrays.asList(SessionTrackingMode.URL))
445+
}
446+
final mock = Mock(HttpServletRequest)
447+
final request = suite.call(mock)
448+
449+
when:
450+
final result = runUnderIastTrace { request.getSession() }
451+
452+
then:
453+
result == session
454+
1 * mock.getSession() >> session
455+
1 * mock.getServletContext() >> servletContext
456+
1 * iastModule.checkSessionTrackingModes(_)
457+
0 * iastModule._
458+
459+
where:
460+
suite << testSuite()
461+
}
462+
434463
protected <E> E runUnderIastTrace(Closure<E> cl) {
435464
final ddctx = new TagContext().withRequestContextDataIast(iastCtx)
436465
final span = TEST_TRACER.startSpan("test", "test-iast-span", ddctx)

‎internal-api/src/main/java/datadog/trace/api/iast/VulnerabilityTypes.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,6 @@ private VulnerabilityTypes() {}
7777
SESSION_TIMEOUT,
7878
DIRECTORY_LISTING_LEAK,
7979
INSECURE_JSP_LAYOUT,
80-
SESSION_REWRITING,
8180
DEFAULT_APP_DEPLOYED,
8281
};
8382

0 commit comments

Comments
 (0)