Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add session rewriting detection #6692

Merged
merged 22 commits into from
Apr 8, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -147,7 +148,8 @@ private static Stream<IastModule> iastModules(
ApplicationModuleImpl.class,
HardcodedSecretModuleImpl.class,
InsecureAuthProtocolModuleImpl.class,
ReflectionInjectionModuleImpl.class);
ReflectionInjectionModuleImpl.class,
SessionRewritingModuleImpl.class);
if (iast != FULLY_ENABLED) {
modules = modules.filter(IastSystem::isOptOut);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ public interface VulnerabilityType {
new VulnerabilityTypeImpl(
VulnerabilityTypes.REFLECTION_INJECTION, VulnerabilityMarks.REFLECTION_INJECTION_MARK);

VulnerabilityType SESSION_REWRITING =

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using class and line for the hash makes no sense for this vulnerability, perhaps vulnerability type and service name is closer to what we need.

new ServiceVulnerabilityType(VulnerabilityTypes.SESSION_REWRITING);

String name();

/** A bit flag to ignore tainted ranges for this vulnerability. Set to 0 if none. */
Expand Down Expand Up @@ -192,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();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
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<String> sessionTrackingModes) {
if (!sessionTrackingModes.contains("URL")) {
return;
}

final IastContext ctx = IastContext.Provider.get();
if (ctx == null) {
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you checking this?, you don't need access to the tainted map right?

}
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)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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<String>)

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
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +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 javax.servlet.http.HttpServletRequest;
import net.bytebuddy.asm.Advice;

Expand All @@ -16,7 +20,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)) {
Expand All @@ -27,6 +32,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<String> sessionTrackingModes = new HashSet<>();
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
sessionTrackingModes.add(mode.name());
}
sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
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
Expand Down Expand Up @@ -52,7 +53,7 @@ abstract class JettyServlet3Test extends AbstractServlet3Test<Server, ServletCon
it.setHost('localhost')
}

ServletContextHandler servletContext = new ServletContextHandler(null, "/$context")
ServletContextHandler servletContext = new ServletContextHandler(null, "/$context", ServletContextHandler.SESSIONS)
servletContext.errorHandler = new ErrorHandler() {
@Override
void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException {
Expand Down Expand Up @@ -532,35 +533,40 @@ 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:
client.newCall(request).execute()

then:
0 * appModule.onRealPath(_)
0 * sessionRewritingModule.checkSessionTrackingModes(_)
0 * _

}

void 'test that iast module is called'() {
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:
client.newCall(request).execute()

then:
1 * appModule.onRealPath(_)
1 * sessionRewritingModule.checkSessionTrackingModes(_)
0 * _

when:
client.newCall(request).execute()

then: //Only call once per application context
0 * appModule.onRealPath(_)
0 * sessionRewritingModule.checkSessionTrackingModes(_)
0 * _
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +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
Expand Down Expand Up @@ -538,34 +539,40 @@ 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:
client.newCall(request).execute()

then:
0 * appModule.onRealPath(_)
0 * sessionRewritingModule.checkSessionTrackingModes(_)
0 * _
}

void 'test that iast module is called'() {
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:
client.newCall(request).execute()

then:
1 * appModule.onRealPath(_)
1 * sessionRewritingModule.checkSessionTrackingModes(_)
0 * _

when:
client.newCall(request).execute()

then: //Only call once per application context
0 * appModule.onRealPath(_)
0 * sessionRewritingModule.checkSessionTrackingModes(_)
0 * _
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +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 net.bytebuddy.asm.Advice;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;
Expand Down Expand Up @@ -60,15 +64,27 @@ 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) {
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<String> sessionTrackingModes = new HashSet<>();
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
sessionTrackingModes.add(mode.name());
}
sessionRewritingModule.checkSessionTrackingModes(sessionTrackingModes);
}
}
}
}
Loading
Loading