Skip to content

Commit b773b0f

Browse files
jandro996jandro996
authored
Add session rewriting detection (#6692)
##What Does This Do Add session rewriting detection in servlet3 and servlet. The main condition to report the vulnerability is that ServletContext#getEffectiveSessionTrackingModes contains SessionTrackingMode.URL We will take advantage of the current IastServletInstrumentation
1 parent 734e3c5 commit b773b0f

File tree

13 files changed

+131
-8
lines changed

13 files changed

+131
-8
lines changed

‎dd-java-agent/agent-iast/src/main/java/com/datadog/iast/model/VulnerabilityType.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,9 @@ public interface VulnerabilityType {
8686
new VulnerabilityTypeImpl(
8787
VulnerabilityTypes.REFLECTION_INJECTION, VulnerabilityMarks.REFLECTION_INJECTION_MARK);
8888

89+
VulnerabilityType SESSION_REWRITING =
90+
new ServiceVulnerabilityType(VulnerabilityTypes.SESSION_REWRITING);
91+
8992
String name();
9093

9194
/** A bit flag to ignore tainted ranges for this vulnerability. Set to 0 if none. */
@@ -192,4 +195,21 @@ public long calculateHash(@Nonnull final Vulnerability vulnerability) {
192195
return crc.getValue();
193196
}
194197
}
198+
199+
class ServiceVulnerabilityType extends VulnerabilityTypeImpl {
200+
public ServiceVulnerabilityType(byte type, int... marks) {
201+
super(type, marks);
202+
}
203+
204+
@Override
205+
public long calculateHash(@Nonnull final Vulnerability vulnerability) {
206+
CRC32 crc = new CRC32();
207+
update(crc, name());
208+
String serviceName = vulnerability.getLocation().getServiceName();
209+
if (serviceName != null) {
210+
update(crc, serviceName);
211+
}
212+
return crc.getValue();
213+
}
214+
}
195215
}

‎dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,8 @@ public class ApplicationModuleImpl extends SinkModuleBase implements Application
7070

7171
public static final String WEB_XML = "web.xml";
7272

73+
static final String SESSION_REWRITING_EVIDENCE_VALUE = "Servlet URL Session Tracking Mode";
74+
7375
private static final Pattern PATTERN =
7476
Pattern.compile(
7577
Stream.of(
@@ -103,6 +105,21 @@ public void onRealPath(final @Nullable String realPath) {
103105
checkWebXmlVulnerabilities(root, span);
104106
}
105107

108+
@Override
109+
public void checkSessionTrackingModes(@Nonnull Set<String> sessionTrackingModes) {
110+
if (!sessionTrackingModes.contains("URL")) {
111+
return;
112+
}
113+
final AgentSpan span = AgentTracer.activeSpan();
114+
// overhead is not checked here as it's called once per application context
115+
reporter.report(
116+
span,
117+
new Vulnerability(
118+
VulnerabilityType.SESSION_REWRITING,
119+
Location.forSpan(span),
120+
new Evidence(SESSION_REWRITING_EVIDENCE_VALUE)));
121+
}
122+
106123
private void checkWebXmlVulnerabilities(@Nonnull Path path, AgentSpan span) {
107124
String webXmlContent = webXmlContent(path);
108125
if (webXmlContent == null) {

‎dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/model/VulnerabilityTypeTest.groovy‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import datadog.trace.test.util.DDSpecification
66
import static com.datadog.iast.model.VulnerabilityType.INSECURE_COOKIE
77
import static com.datadog.iast.model.VulnerabilityType.NO_HTTPONLY_COOKIE
88
import static com.datadog.iast.model.VulnerabilityType.NO_SAMESITE_COOKIE
9+
import static com.datadog.iast.model.VulnerabilityType.SESSION_REWRITING
910
import static com.datadog.iast.model.VulnerabilityType.WEAK_CIPHER
1011
import static com.datadog.iast.model.VulnerabilityType.XCONTENTTYPE_HEADER_MISSING
1112
import static com.datadog.iast.model.VulnerabilityType.HSTS_HEADER_MISSING
@@ -42,6 +43,9 @@ class VulnerabilityTypeTest extends DDSpecification {
4243
HSTS_HEADER_MISSING | getSpanLocation(123, null) | null | 121310697
4344
HSTS_HEADER_MISSING | getSpanLocation(123, 'serviceName1') | null | 3533496951
4445
HSTS_HEADER_MISSING | getSpanLocation(123, 'serviceName2') | null | 1268102093
46+
SESSION_REWRITING | getSpanLocation(123, null) | null | 2255304761
47+
SESSION_REWRITING | getSpanLocation(123, 'serviceName1') | null | 305779398
48+
SESSION_REWRITING | getSpanLocation(123, 'serviceName2') | null | 2335212412
4549
}
4650

4751
private Location getSpanAndStackLocation(final long spanId) {

‎dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ package com.datadog.iast.sink
22

33
import com.datadog.iast.IastModuleImplTestBase
44
import com.datadog.iast.Reporter
5+
import com.datadog.iast.model.Vulnerability
6+
import com.datadog.iast.model.VulnerabilityType
57
import datadog.trace.api.iast.InstrumentationBridge
68
import datadog.trace.api.iast.sink.ApplicationModule
79

@@ -11,6 +13,7 @@ import static com.datadog.iast.model.VulnerabilityType.DIRECTORY_LISTING_LEAK
1113
import static com.datadog.iast.model.VulnerabilityType.INSECURE_JSP_LAYOUT
1214
import static com.datadog.iast.model.VulnerabilityType.SESSION_TIMEOUT
1315
import static com.datadog.iast.model.VulnerabilityType.VERB_TAMPERING
16+
import static com.datadog.iast.sink.ApplicationModuleImpl.SESSION_REWRITING_EVIDENCE_VALUE
1417

1518
class ApplicationModuleTest extends IastModuleImplTestBase {
1619

@@ -71,6 +74,31 @@ class ApplicationModuleTest extends IastModuleImplTestBase {
7174
'application/defaulthtmlescapeinvalid/no_tag_2' | DEFAULT_HTML_ESCAPE_INVALID | 'defaultHtmlEscape tag should be set' | NO_LINE
7275
}
7376

77+
void 'iast module detects session rewriting on sessionTrackingModes'() {
78+
when:
79+
module.checkSessionTrackingModes(sessionTrackingModes as Set<String>)
80+
81+
then:
82+
if (expected != null) {
83+
1 * reporter.report(_, _) >> { args -> assertSessionRewriting(args[1] as Vulnerability, expected) }
84+
} else {
85+
0 * reporter.report(_, _)
86+
}
87+
88+
where:
89+
sessionTrackingModes | expected
90+
[] | null
91+
['COOKIE'] | null
92+
['URL'] | SESSION_REWRITING_EVIDENCE_VALUE
93+
['COOKIE', 'URL'] | SESSION_REWRITING_EVIDENCE_VALUE
94+
}
95+
96+
private static void assertSessionRewriting(final Vulnerability vuln, final String expected) {
97+
assertVulnerability(vuln, VulnerabilityType.SESSION_REWRITING)
98+
final evidence = vuln.getEvidence()
99+
assert evidence.value == expected
100+
}
101+
74102
private static void assertVulnerability(final vuln, final expectedVulnType) {
75103
assert vuln != null
76104
assert vuln.getType() == expectedVulnType

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,11 @@
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;
810
import javax.servlet.ServletContext;
911
import javax.servlet.ServletRequest;
12+
import javax.servlet.SessionTrackingMode;
1013
import javax.servlet.http.HttpServletRequest;
1114
import net.bytebuddy.asm.Advice;
1215

@@ -27,6 +30,16 @@ public static void onExit(@Advice.Argument(0) ServletRequest request) {
2730
return;
2831
}
2932
InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true);
30-
applicationModule.onRealPath(context.getRealPath("/"));
33+
if (applicationModule != null) {
34+
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+
}
43+
}
3144
}
3245
}

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import org.eclipse.jetty.server.Request
1212
import org.eclipse.jetty.server.Server
1313
import org.eclipse.jetty.server.handler.ErrorHandler
1414
import org.eclipse.jetty.servlet.ServletContextHandler
15-
1615
import javax.servlet.AsyncEvent
1716
import javax.servlet.AsyncListener
1817
import javax.servlet.Servlet
@@ -52,7 +51,7 @@ abstract class JettyServlet3Test extends AbstractServlet3Test<Server, ServletCon
5251
it.setHost('localhost')
5352
}
5453

55-
ServletContextHandler servletContext = new ServletContextHandler(null, "/$context")
54+
ServletContextHandler servletContext = new ServletContextHandler(null, "/$context", ServletContextHandler.SESSIONS)
5655
servletContext.errorHandler = new ErrorHandler() {
5756
@Override
5857
void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException {
@@ -539,8 +538,8 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync {
539538

540539
then:
541540
0 * appModule.onRealPath(_)
541+
0 * appModule.checkSessionTrackingModes(_)
542542
0 * _
543-
544543
}
545544

546545
void 'test that iast module is called'() {
@@ -554,13 +553,15 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync {
554553

555554
then:
556555
1 * appModule.onRealPath(_)
556+
1 * appModule.checkSessionTrackingModes(_)
557557
0 * _
558558

559559
when:
560560
client.newCall(request).execute()
561561

562562
then: //Only call once per application context
563563
0 * appModule.onRealPath(_)
564+
0 * appModule.checkSessionTrackingModes(_)
564565
0 * _
565566
}
566567

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import org.apache.catalina.valves.ValveBase
1919
import org.apache.tomcat.JarScanFilter
2020
import org.apache.tomcat.JarScanType
2121
import spock.lang.Shared
22-
2322
import javax.servlet.Servlet
2423
import javax.servlet.ServletException
2524

@@ -545,6 +544,7 @@ class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync {
545544

546545
then:
547546
0 * appModule.onRealPath(_)
547+
0 * appModule.checkSessionTrackingModes(_)
548548
0 * _
549549
}
550550

@@ -559,13 +559,15 @@ class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync {
559559

560560
then:
561561
1 * appModule.onRealPath(_)
562+
1 * appModule.checkSessionTrackingModes(_)
562563
0 * _
563564

564565
when:
565566
client.newCall(request).execute()
566567

567568
then: //Only call once per application context
568569
0 * appModule.onRealPath(_)
570+
0 * appModule.checkSessionTrackingModes(_)
569571
0 * _
570572
}
571573

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

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,12 @@
1414
import datadog.trace.api.iast.sink.ApplicationModule;
1515
import datadog.trace.bootstrap.InstrumentationContext;
1616
import jakarta.servlet.ServletContext;
17+
import jakarta.servlet.SessionTrackingMode;
1718
import jakarta.servlet.http.HttpServlet;
1819
import java.util.Collections;
20+
import java.util.HashSet;
1921
import java.util.Map;
22+
import java.util.Set;
2023
import net.bytebuddy.asm.Advice;
2124
import net.bytebuddy.description.type.TypeDescription;
2225
import net.bytebuddy.matcher.ElementMatcher;
@@ -68,7 +71,17 @@ public static void after(@Advice.This final HttpServlet servlet) {
6871
return;
6972
}
7073
InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true);
71-
applicationModule.onRealPath(context.getRealPath("/"));
74+
if (applicationModule != null) {
75+
applicationModule.onRealPath(context.getRealPath("/"));
76+
if (context.getEffectiveSessionTrackingModes() != null
77+
&& !context.getEffectiveSessionTrackingModes().isEmpty()) {
78+
Set<String> sessionTrackingModes = new HashSet<>();
79+
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
80+
sessionTrackingModes.add(mode.name());
81+
}
82+
applicationModule.checkSessionTrackingModes(sessionTrackingModes);
83+
}
84+
}
7285
}
7386
}
7487
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{
2828

2929
then:
3030
0 * appModule.onRealPath(_)
31+
0 * appModule.checkSessionTrackingModes(_)
3132
0 * _
3233
}
3334

@@ -44,6 +45,7 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{
4445

4546
then:
4647
1 * module.onRealPath(_)
48+
1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set<String>)
4749
0 * _
4850
}
4951
}

‎dd-smoke-tests/iast-util/src/testFixtures/groovy/datadog/smoketest/AbstractIastSpringBootTest.groovy‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import okhttp3.MediaType
55
import okhttp3.MultipartBody
66
import okhttp3.Request
77
import okhttp3.RequestBody
8+
import okhttp3.Response
89

910
import static datadog.trace.api.config.IastConfig.IAST_DEBUG_ENABLED
1011
import static datadog.trace.api.config.IastConfig.IAST_DETECTION_MODE
@@ -968,6 +969,20 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest {
968969
}
969970
}
970971

972+
void 'find session rewriting'() {
973+
given:
974+
String url = "http://localhost:${httpPort}/greeting"
975+
976+
when:
977+
Response response = client.newCall(new Request.Builder().url(url).get().build()).execute()
978+
979+
then:
980+
response.successful
981+
hasVulnerabilityInLogs { vul ->
982+
vul.type == 'SESSION_REWRITING'
983+
}
984+
}
985+
971986

972987

973988
}

0 commit comments

Comments
 (0)