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 all 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 @@ -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
Expand Up @@ -70,6 +70,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(
Expand Down Expand Up @@ -103,6 +105,21 @@ public void onRealPath(final @Nullable String realPath) {
checkWebXmlVulnerabilities(root, span);
}

@Override
public void checkSessionTrackingModes(@Nonnull Set<String> 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) {
Expand Down
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
Expand Up @@ -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

Expand All @@ -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 {

Expand Down Expand Up @@ -71,6 +74,31 @@ 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<String>)

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) {
assertVulnerability(vuln, VulnerabilityType.SESSION_REWRITING)
final evidence = vuln.getEvidence()
assert evidence.value == expected
}

private static void assertVulnerability(final vuln, final expectedVulnType) {
assert vuln != null
assert vuln.getType() == expectedVulnType
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@
import datadog.trace.api.iast.VulnerabilityTypes;
import datadog.trace.api.iast.sink.ApplicationModule;
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 @@ -27,6 +30,16 @@ 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 (context.getEffectiveSessionTrackingModes() != null
&& !context.getEffectiveSessionTrackingModes().isEmpty()) {
Set<String> sessionTrackingModes = new HashSet<>();
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
sessionTrackingModes.add(mode.name());
}
applicationModule.checkSessionTrackingModes(sessionTrackingModes);
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -52,7 +51,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 @@ -539,8 +538,8 @@ class IastJettyServlet3ForkedTest extends JettyServlet3TestSync {

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

}

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

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

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

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,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

Expand Down Expand Up @@ -545,6 +544,7 @@ class IastTomcatServlet3ForkedTest extends TomcatServlet3TestSync {

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

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

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

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

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,12 @@
import datadog.trace.api.iast.sink.ApplicationModule;
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 @@ -68,7 +71,17 @@ public static void after(@Advice.This final HttpServlet servlet) {
return;
}
InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true);
applicationModule.onRealPath(context.getRealPath("/"));
if (applicationModule != null) {
applicationModule.onRealPath(context.getRealPath("/"));
if (context.getEffectiveSessionTrackingModes() != null
&& !context.getEffectiveSessionTrackingModes().isEmpty()) {
Set<String> sessionTrackingModes = new HashSet<>();
for (SessionTrackingMode mode : context.getEffectiveSessionTrackingModes()) {
sessionTrackingModes.add(mode.name());
}
applicationModule.checkSessionTrackingModes(sessionTrackingModes);
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ class IastJakartaServletInstrumentationTest extends AgentTestRunner{

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

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

then:
1 * module.onRealPath(_)
1 * module.checkSessionTrackingModes(['COOKIE', 'URL'] as Set<String>)
0 * _
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -968,6 +969,20 @@ abstract class AbstractIastSpringBootTest extends AbstractIastServerSmokeTest {
}
}

void 'find session rewriting'() {
given:
String url = "http://localhost:${httpPort}/greeting"

when:
Response response = client.newCall(new Request.Builder().url(url).get().build()).execute()

then:
response.successful
hasVulnerabilityInLogs { vul ->
vul.type == 'SESSION_REWRITING'
}
}



}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -73,7 +74,8 @@ private VulnerabilityTypes() {}
DEFAULT_HTML_ESCAPE_INVALID,
SESSION_TIMEOUT,
DIRECTORY_LISTING_LEAK,
INSECURE_JSP_LAYOUT
INSECURE_JSP_LAYOUT,
SESSION_REWRITING
};

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