Skip to content

Commit

Permalink
Improve coverage of application vulnerabilities for servlet (#6803)
Browse files Browse the repository at this point in the history
What Does This Do
Get the realPath from the context when HttpServlet#service or javax.servlet.FilterChain#doFilter (This implementation is based on the current tracer instrumentation for servlet)

Remove javax.servlet.ServletContext#getRealPath instrumentation

Add new Iast Instrumentations for:

servlet2 -> javax.servlet.Servlet
servlet3 -> javax.servlet.Servlet
servlet5 -> jakarta.servlet.Servlet

Motivation
We noticed that javax.servlet.ServletContext#getRealPath is not always called, so we need to find a more reliable point to check the vulnerabilities

Add coverage to servlet 5
  • Loading branch information
jandro996 authored Mar 14, 2024
1 parent ee9c0f8 commit f19053f
Show file tree
Hide file tree
Showing 15 changed files with 1,173 additions and 112 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,7 @@ private static class InsecureJspFolderVisitor implements FileVisitor<Path> {
private final Set<Path> folders = new HashSet<>();

@Override
public FileVisitResult preVisitDirectory(final Path dir, final BasicFileAttributes attrs)
throws IOException {
public FileVisitResult preVisitDirectory(final Path dir, final BasicFileAttributes attrs) {
final String folder = dir.getFileName().toString();
if (endsWithIgnoreCase(folder, WEB_INF)) {
return FileVisitResult.SKIP_SUBTREE;
Expand All @@ -305,8 +304,7 @@ public FileVisitResult preVisitDirectory(final Path dir, final BasicFileAttribut
}

@Override
public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs)
throws IOException {
public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) {
final String fileName = file.getFileName().toString();
if (endsWithIgnoreCase(fileName, ".jsp") || endsWithIgnoreCase(fileName, ".jspx")) {
folders.add(file.getParent());
Expand All @@ -316,14 +314,12 @@ public FileVisitResult visitFile(final Path file, final BasicFileAttributes attr
}

@Override
public FileVisitResult visitFileFailed(final Path file, final IOException exc)
throws IOException {
public FileVisitResult visitFileFailed(final Path file, final IOException exc) {
return FileVisitResult.CONTINUE;
}

@Override
public FileVisitResult postVisitDirectory(final Path dir, final IOException exc)
throws IOException {
public FileVisitResult postVisitDirectory(final Path dir, final IOException exc) {
return FileVisitResult.CONTINUE;
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package datadog.trace.instrumentation.servlet2;

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.bootstrap.InstrumentationContext;
import javax.servlet.ServletContext;
import javax.servlet.http.HttpServlet;
import net.bytebuddy.asm.Advice;

public class IastServlet2Advice {

@Sink(VulnerabilityTypes.APPLICATION)
@Advice.OnMethodExit(suppress = Throwable.class)
public static void onExit(@Advice.This Object servlet) {
final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION;
if (applicationModule == null) {
return;
}
if (!(servlet instanceof HttpServlet)) {
return;
}
final ServletContext context = ((HttpServlet) servlet).getServletContext();
if (InstrumentationContext.get(ServletContext.class, Boolean.class).get(context) != null) {
return;
}
InstrumentationContext.get(ServletContext.class, Boolean.class).put(context, true);
if (applicationModule != null) {
applicationModule.onRealPath(context.getRealPath("/"));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package datadog.trace.instrumentation.servlet2;

import static datadog.trace.agent.tooling.bytebuddy.matcher.ClassLoaderMatchers.hasClassNamed;
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.extendsClass;
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.namedOneOf;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.not;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;

import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.InstrumenterModule;
import java.util.Collections;
import java.util.Map;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

@AutoService(InstrumenterModule.class)
public final class IastServlet2Instrumentation extends InstrumenterModule.Iast
implements Instrumenter.ForTypeHierarchy {

public IastServlet2Instrumentation() {
super("servlet", "servlet-2");
}

@Override
public String muzzleDirective() {
return "servlet-2.x";
}

// Avoid matching servlet 3 which has its own instrumentation
static final ElementMatcher<ClassLoader> NOT_SERVLET_3 =
not(hasClassNamed("javax.servlet.AsyncEvent"));

@Override
public ElementMatcher<ClassLoader> classLoaderMatcher() {
return NOT_SERVLET_3;
}

@Override
public String hierarchyMarkerType() {
return "javax.servlet.http.HttpServlet";
}

@Override
public ElementMatcher<TypeDescription> hierarchyMatcher() {
return extendsClass(named("javax.servlet.http.HttpServlet"))
.or(implementsInterface(named("javax.servlet.FilterChain")));
}

@Override
public Map<String, String> contextStore() {
return Collections.singletonMap("javax.servlet.ServletContext", Boolean.class.getName());
}

@Override
public void methodAdvice(MethodTransformer transformer) {
transformer.applyAdvice(
namedOneOf("doFilter", "service")
.and(takesArgument(0, named("javax.servlet.ServletRequest")))
.and(takesArgument(1, named("javax.servlet.ServletResponse")))
.and(isPublic()),
packageName + ".IastServlet2Advice");
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import datadog.trace.agent.test.asserts.TraceAssert
import datadog.trace.agent.test.base.HttpServer
import datadog.trace.agent.test.base.HttpServerTest
import datadog.trace.agent.test.naming.TestingGenericHttpNamingConventions
import datadog.trace.api.iast.InstrumentationBridge
import datadog.trace.api.iast.sink.ApplicationModule
import org.eclipse.jetty.server.Server
import org.eclipse.jetty.server.handler.ErrorHandler
import org.eclipse.jetty.servlet.ServletContextHandler
Expand All @@ -14,6 +16,7 @@ import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPT
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.MATRIX_PARAM
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.REDIRECT
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS
import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.UNKNOWN

abstract class JettyServlet2Test extends HttpServerTest<Server> {
Expand Down Expand Up @@ -201,3 +204,47 @@ class JettyServlet2V0ForkedTest extends JettyServlet2Test implements TestingGene
class JettyServlet2V1ForkedTest extends JettyServlet2Test implements TestingGenericHttpNamingConventions.ServerV1 {

}

class IastJettyServlet2ForkedTest extends JettyServlet2Test implements TestingGenericHttpNamingConventions.ServerV0 {

@Override
void configurePreAgent() {
super.configurePreAgent()
injectSysConfig('dd.iast.enabled', 'true')
}

void 'test no calls if no modules registered'() {
given:
final appModule = Mock(ApplicationModule)
def request = request(SUCCESS, "GET", null).build()

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

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

void 'test that iast modules are called'() {
given:
final appModule = Mock(ApplicationModule)
InstrumentationBridge.registerIastModule(appModule)
def request = request(SUCCESS, "GET", null).build()

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

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

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

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

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package datadog.trace.instrumentation.servlet3;

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.bootstrap.InstrumentationContext;
import javax.servlet.ServletContext;
import javax.servlet.ServletRequest;
import javax.servlet.http.HttpServletRequest;
import net.bytebuddy.asm.Advice;

public class IastServlet3Advice {

@Sink(VulnerabilityTypes.APPLICATION)
@Advice.OnMethodExit(suppress = Throwable.class)
public static void onExit(@Advice.Argument(0) ServletRequest request) {
final ApplicationModule applicationModule = InstrumentationBridge.APPLICATION;
if (applicationModule == null) {
return;
}
if (!(request instanceof HttpServletRequest)) {
return;
}
final ServletContext context = request.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("/"));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package datadog.trace.instrumentation.servlet3;

import static datadog.trace.agent.tooling.bytebuddy.matcher.ClassLoaderMatchers.hasClassNamed;
import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.extendsClass;
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.namedOneOf;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;

import com.google.auto.service.AutoService;
import datadog.trace.agent.tooling.Instrumenter;
import datadog.trace.agent.tooling.InstrumenterModule;
import java.util.Collections;
import java.util.Map;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;

@AutoService(InstrumenterModule.class)
public final class IastServlet3Instrumentation extends InstrumenterModule.Iast
implements Instrumenter.ForTypeHierarchy {
public IastServlet3Instrumentation() {
super("servlet", "servlet-3");
}

@Override
public String muzzleDirective() {
return "servlet-3.x";
}

@Override
public ElementMatcher<ClassLoader> classLoaderMatcher() {
// Avoid matching servlet 2 which has its own instrumentation
return hasClassNamed("javax.servlet.AsyncEvent");
}

@Override
public String hierarchyMarkerType() {
return "javax.servlet.http.HttpServlet";
}

@Override
public ElementMatcher<TypeDescription> hierarchyMatcher() {
return extendsClass(named("javax.servlet.http.HttpServlet"))
.or(implementsInterface(named("javax.servlet.FilterChain")));
}

@Override
public Map<String, String> contextStore() {
return Collections.singletonMap("javax.servlet.ServletContext", Boolean.class.getName());
}

@Override
public void methodAdvice(MethodTransformer transformer) {
transformer.applyAdvice(
namedOneOf("doFilter", "service")
.and(takesArgument(0, named("javax.servlet.ServletRequest")))
.and(takesArgument(1, named("javax.servlet.ServletResponse")))
.and(isPublic()),
packageName + ".IastServlet3Advice");
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
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.bootstrap.instrumentation.api.AgentSpan
import datadog.trace.bootstrap.instrumentation.api.Tags
import datadog.trace.instrumentation.servlet3.AsyncDispatcherDecorator
Expand Down Expand Up @@ -518,3 +520,48 @@ class JettyServlet3ServeFromAsyncTimeout extends JettyServlet3Test {
false
}
}

class IastJettyServlet3ForkedTest extends JettyServlet3TestSync {

@Override
void configurePreAgent() {
super.configurePreAgent()
injectSysConfig('dd.iast.enabled', 'true')
}

void 'test no calls if no modules registered'() {
given:
final appModule = Mock(ApplicationModule)
def request = request(SUCCESS, "GET", null).build()

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

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

}

void 'test that iast module is called'() {
given:
final appModule = Mock(ApplicationModule)
InstrumentationBridge.registerIastModule(appModule)
def request = request(SUCCESS, "GET", null).build()

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

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

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

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

}
Loading

0 comments on commit f19053f

Please sign in to comment.