diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java index 10bbef8d362..e655b1796b1 100644 --- a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/sink/ApplicationModuleImpl.java @@ -1,5 +1,9 @@ package com.datadog.iast.sink; +import static com.datadog.iast.util.StringUtils.endsWithIgnoreCase; +import static com.datadog.iast.util.StringUtils.substringTrim; +import static datadog.trace.api.telemetry.LogCollector.SEND_TELEMETRY; + import com.datadog.iast.Dependencies; import com.datadog.iast.model.Evidence; import com.datadog.iast.model.Location; @@ -11,28 +15,37 @@ import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.nio.file.FileVisitOption; +import java.nio.file.FileVisitResult; +import java.nio.file.FileVisitor; import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.util.ArrayList; -import java.util.List; +import java.nio.file.attribute.BasicFileAttributes; +import java.util.Collection; +import java.util.EnumSet; +import java.util.HashSet; +import java.util.Set; import java.util.regex.Matcher; import java.util.regex.Pattern; import java.util.stream.Collectors; +import java.util.stream.Stream; +import javax.annotation.Nonnull; import javax.annotation.Nullable; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class ApplicationModuleImpl extends SinkModuleBase implements ApplicationModule { - private static final String CONTEXT_LOADER_LISTENER_PATTERN = - "org\\.springframework\\.web\\.context\\.ContextLoaderListener"; + private static final Logger LOGGER = LoggerFactory.getLogger(ApplicationModule.class); - private static final String CONTEXT_LOADER_LISTENER_VALUE = - "org.springframework.web.context.ContextLoaderListener"; + /** Bounds for the file visitor depth when trying to locate insecure JSP folders */ + private static final int JSP_MAX_WALK_DEPTH = 32; - private static final String DISPATCHER_SERVLET_PATTERN = - "org\\.springframework\\.web\\.servlet\\.DispatcherServlet"; + private static final String CONTEXT_LOADER_LISTENER = + "org.springframework.web.context.ContextLoaderListener"; - private static final String DISPATCHER_SERVLET_VALUE = + private static final String DISPATCHER_SERVLET = "org.springframework.web.servlet.DispatcherServlet"; private static final String DEFAULT_HTML_ESCAPE = "defaultHtmlEscape"; @@ -57,18 +70,18 @@ public class ApplicationModuleImpl extends SinkModuleBase implements Application public static final String WEB_XML = "web.xml"; - private static final String REGEX = - String.join( - "|", - CONTEXT_LOADER_LISTENER_PATTERN, - DISPATCHER_SERVLET_PATTERN, - DEFAULT_HTML_ESCAPE, - TOMCAT_MANAGER_APPLICATION, - LISTINGS_PATTERN, - SESSION_TIMEOUT_START_TAG, - SECURITY_CONSTRAINT_START_TAG); - - private static final Pattern PATTERN = Pattern.compile(REGEX); + private static final Pattern PATTERN = + Pattern.compile( + Stream.of( + CONTEXT_LOADER_LISTENER, + DISPATCHER_SERVLET, + DEFAULT_HTML_ESCAPE, + TOMCAT_MANAGER_APPLICATION, + LISTINGS_PATTERN, + SESSION_TIMEOUT_START_TAG, + SECURITY_CONSTRAINT_START_TAG) + .map(Pattern::quote) + .collect(Collectors.joining("|"))); private static final int NO_LINE = -1; @@ -78,16 +91,20 @@ public ApplicationModuleImpl(final Dependencies dependencies) { @Override public void onRealPath(final @Nullable String realPath) { - if (realPath == null) { return; } - + final Path root = Paths.get(realPath); + if (!Files.exists(root)) { + return; + } final AgentSpan span = AgentTracer.activeSpan(); + checkInsecureJSPLayout(root, span); + checkWebXmlVulnerabilities(root, span); + } - checkInsecureJSPLayout(realPath, span); - - String webXmlContent = webXmlContent(realPath); + private void checkWebXmlVulnerabilities(@Nonnull Path path, AgentSpan span) { + String webXmlContent = webXmlContent(path); if (webXmlContent == null) { return; } @@ -99,8 +116,8 @@ public void onRealPath(final @Nullable String realPath) { while (matcher.find()) { String match = matcher.group(); switch (match) { - case DISPATCHER_SERVLET_VALUE: - case CONTEXT_LOADER_LISTENER_VALUE: + case DISPATCHER_SERVLET: + case CONTEXT_LOADER_LISTENER: isSpring = true; break; case DEFAULT_HTML_ESCAPE: @@ -124,19 +141,19 @@ public void onRealPath(final @Nullable String realPath) { } if (isSpring) { - checkDefaultHtmlEscapeInvalid(webXmlContent, span, defaultHtmlEscapeIndex); + checkDefaultHtmlEscapeInvalid(webXmlContent, defaultHtmlEscapeIndex, span); } } private void checkDefaultHtmlEscapeInvalid( - String webXmlContent, AgentSpan span, int defaultHtmlEscapeIndex) { + @Nonnull String webXmlContent, int defaultHtmlEscapeIndex, AgentSpan span) { if (defaultHtmlEscapeIndex != -1) { int start = webXmlContent.indexOf(PARAM_VALUE_START_TAG, defaultHtmlEscapeIndex) + PARAM_VALUE_START_TAG.length(); String value = - webXmlContent.substring(start, webXmlContent.indexOf(PARAM_VALUE_END_TAG, start)); - if (!value.trim().toLowerCase().equals("true")) { + substringTrim(webXmlContent, start, webXmlContent.indexOf(PARAM_VALUE_END_TAG, start)); + if (!value.equalsIgnoreCase("true")) { report( span, VulnerabilityType.DEFAULT_HTML_ESCAPE_INVALID, @@ -161,8 +178,8 @@ private void checkDirectoryListingLeak( int valueIndex = webXmlContent.indexOf(PARAM_VALUE_START_TAG, index) + PARAM_VALUE_START_TAG.length(); int valueLast = webXmlContent.indexOf(PARAM_VALUE_END_TAG, valueIndex); - String data = webXmlContent.substring(valueIndex, valueLast); - if (data.trim().toLowerCase().equals("true")) { + String data = substringTrim(webXmlContent, valueIndex, valueLast); + if (data.equalsIgnoreCase("true")) { report( span, VulnerabilityType.DIRECTORY_LISTING_LEAK, @@ -174,11 +191,10 @@ private void checkDirectoryListingLeak( private void checkSessionTimeOut(final String webXmlContent, int index, final AgentSpan span) { try { String innerText = - webXmlContent - .substring( - index + SESSION_TIMEOUT_START_TAG.length(), - webXmlContent.indexOf(SESSION_TIMEOUT_END_TAG, index)) - .trim(); + substringTrim( + webXmlContent, + index + SESSION_TIMEOUT_START_TAG.length(), + webXmlContent.indexOf(SESSION_TIMEOUT_END_TAG, index)); int timeoutValue = Integer.parseInt(innerText); if (timeoutValue > 30 || timeoutValue == -1) { report( @@ -194,11 +210,10 @@ private void checkSessionTimeOut(final String webXmlContent, int index, final Ag private void checkVerbTampering(final String webXmlContent, int index, final AgentSpan span) { String innerText = - webXmlContent - .substring( - index + SECURITY_CONSTRAINT_START_TAG.length(), - webXmlContent.indexOf(SECURITY_CONSTRAINT_END_TAG, index)) - .trim(); + substringTrim( + webXmlContent, + index + SECURITY_CONSTRAINT_START_TAG.length(), + webXmlContent.indexOf(SECURITY_CONSTRAINT_END_TAG, index)); if (!innerText.contains("")) { report( span, @@ -208,17 +223,6 @@ private void checkVerbTampering(final String webXmlContent, int index, final Age } } - private int getLine(String webXmlContent, int index) { - int line = 1; - int limit = Math.min(index, webXmlContent.length()); - for (int i = limit; i > 0; i--) { - if (webXmlContent.charAt(i) == '\n') { - line++; - } - } - return line; - } - private void report(AgentSpan span, VulnerabilityType type, String value, int line) { reporter.report( span, @@ -226,14 +230,14 @@ private void report(AgentSpan span, VulnerabilityType type, String value, int li type, Location.forSpanAndFileAndLine(span, WEB_XML, line), new Evidence(value))); } - private void checkInsecureJSPLayout(String realPath, AgentSpan span) { - List jspPaths = findRecursively(new File(realPath)); + private void checkInsecureJSPLayout(@Nonnull Path path, AgentSpan span) { + final Collection jspPaths = findInsecureJspPaths(path); if (jspPaths.isEmpty()) { return; } String result = jspPaths.stream() - .map(s -> File.separatorChar + s) + .map(jspFolder -> relativize(path, jspFolder)) .collect(Collectors.joining(System.lineSeparator())); reporter.report( span, @@ -241,49 +245,86 @@ private void checkInsecureJSPLayout(String realPath, AgentSpan span) { VulnerabilityType.INSECURE_JSP_LAYOUT, Location.forSpan(span), new Evidence(result))); } + private static int getLine(String webXmlContent, int index) { + int line = 1; + int limit = Math.min(index, webXmlContent.length()); + for (int i = limit; i > 0; i--) { + if (webXmlContent.charAt(i) == '\n') { + line++; + } + } + return line; + } + @Nullable - private String webXmlContent(final String realPath) { - if (realPath != null) { - Path path = Paths.get(realPath + File.separatorChar + WEB_INF + File.separatorChar + WEB_XML); - if (path.toFile().exists()) { - try { - return new String(Files.readAllBytes(path), StandardCharsets.UTF_8); - } catch (IOException e) { - // Nothing to do - } + private static String webXmlContent(final Path realPath) { + Path path = realPath.resolve(WEB_INF).resolve(WEB_XML); + if (Files.exists(path)) { + try { + return new String(Files.readAllBytes(path), StandardCharsets.UTF_8); + } catch (IOException e) { + LOGGER.debug(SEND_TELEMETRY, "Failed to read {}, encoding issue?", path, e); } } return null; } - private List findRecursively(final File root) { - List jspPaths = new ArrayList<>(); - if (root.listFiles() != null) { - for (File file : root.listFiles()) { - if (file.isFile() && isJsp(file.getName().toLowerCase())) { - jspPaths.add(file.getName()); - } else if (file.isDirectory() && !WEB_INF.equals(file.getName())) { - addDirectoryJSPs("", file, jspPaths); - } - } + private static Collection findInsecureJspPaths(final Path root) { + try { + final InsecureJspFolderVisitor visitor = new InsecureJspFolderVisitor(); + // TODO is it OK to ignore symlinks here? + Files.walkFileTree(root, EnumSet.noneOf(FileVisitOption.class), JSP_MAX_WALK_DEPTH, visitor); + return visitor.folders; + } catch (Exception e) { + throw new RuntimeException(e); } - return jspPaths; } - private void addDirectoryJSPs(final String path, final File dir, final List jspPaths) { - String currentPath = path + dir.getName() + File.separatorChar; - if (dir.listFiles() != null) { - for (File file : dir.listFiles()) { - if (file.isFile() && isJsp(file.getName().toLowerCase())) { - jspPaths.add(currentPath + file.getName()); - } else if (file.isDirectory()) { - addDirectoryJSPs(currentPath, file, jspPaths); - } - } + private static String relativize(final Path root, final Path path) { + final String relative = root.relativize(path).toString(); + if (relative.isEmpty()) { + return File.separator; } + if (relative.charAt(0) == File.separatorChar) { + return relative; + } + return File.separatorChar + relative; } - private boolean isJsp(final String name) { - return name.endsWith(".jsp") || name.endsWith(".jspx"); + private static class InsecureJspFolderVisitor implements FileVisitor { + private final Set folders = new HashSet<>(); + + @Override + public FileVisitResult preVisitDirectory(final Path dir, final BasicFileAttributes attrs) + throws IOException { + final String folder = dir.getFileName().toString(); + if (endsWithIgnoreCase(folder, WEB_INF)) { + return FileVisitResult.SKIP_SUBTREE; + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFile(final Path file, final BasicFileAttributes attrs) + throws IOException { + final String fileName = file.getFileName().toString(); + if (endsWithIgnoreCase(fileName, ".jsp") || endsWithIgnoreCase(fileName, ".jspx")) { + folders.add(file.getParent()); + return FileVisitResult.SKIP_SIBLINGS; + } + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult visitFileFailed(final Path file, final IOException exc) + throws IOException { + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult postVisitDirectory(final Path dir, final IOException exc) + throws IOException { + return FileVisitResult.CONTINUE; + } } } diff --git a/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/util/StringUtils.java b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/util/StringUtils.java new file mode 100644 index 00000000000..4e45a44cde7 --- /dev/null +++ b/dd-java-agent/agent-iast/src/main/java/com/datadog/iast/util/StringUtils.java @@ -0,0 +1,41 @@ +package com.datadog.iast.util; + +import javax.annotation.Nonnull; + +public abstract class StringUtils { + + private StringUtils() {} + + /** + * Checks if the string ends with the selected suffix ignoring case. Note that this method does + * not take locale into account. + */ + public static boolean endsWithIgnoreCase( + @Nonnull final String value, @Nonnull final String suffix) { + if (value.length() < suffix.length()) { + return false; + } + if (suffix.isEmpty()) { + return true; + } + final int offset = value.length() - suffix.length(); + return value.regionMatches(true, offset, suffix, 0, suffix.length()); + } + + /** + * Performs a substring of the selected string taking care of leading and trailing whitespaces. + */ + @Nonnull + public static String substringTrim(@Nonnull final String value, int start, int end) { + if (start >= end) { + return ""; + } + while (start < end && Character.isWhitespace(value.charAt(start))) { + start++; + } + while (end > start && Character.isWhitespace(value.charAt(end - 1))) { + end--; + } + return start >= end ? "" : value.substring(start, end); + } +} diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy index 3b7a09cad8b..ab249758beb 100644 --- a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/sink/ApplicationModuleTest.groovy @@ -2,10 +2,16 @@ package com.datadog.iast.sink import com.datadog.iast.IastModuleImplTestBase import com.datadog.iast.Reporter -import com.datadog.iast.model.VulnerabilityType import datadog.trace.api.iast.InstrumentationBridge import datadog.trace.api.iast.sink.ApplicationModule +import static com.datadog.iast.model.VulnerabilityType.ADMIN_CONSOLE_ACTIVE +import static com.datadog.iast.model.VulnerabilityType.DEFAULT_HTML_ESCAPE_INVALID +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 + class ApplicationModuleTest extends IastModuleImplTestBase { private static final int NO_LINE = -1 @@ -47,27 +53,22 @@ class ApplicationModuleTest extends IastModuleImplTestBase { } where: - path | expectedVulnType | expectedEvidence | line - 'application/insecurejsplayout/secure' | null | null | _ - 'application/insecurejsplayout/insecure' | VulnerabilityType.INSECURE_JSP_LAYOUT | [ - '/nestedinsecure/insecure2.jsp', - '/nestedinsecure/nestedinsecure/insecure3.jsp' , - '/insecure.jsp', - '/insecure.jspx' - ] as String [] | NO_LINE - 'application/verbtampering/secure' | null | null | _ - 'application/verbtampering/insecure' | VulnerabilityType.VERB_TAMPERING | 'http-method not defined in web.xml' | 6 - 'application/sessiontimeout/secure' | null | null | _ - 'application/sessiontimeout/insecure' | VulnerabilityType.SESSION_TIMEOUT | 'Found vulnerable timeout value: 80' | 7 - 'application/directorylistingleak/secure' | null | null | _ - 'application/directorylistingleak/insecure' | VulnerabilityType.DIRECTORY_LISTING_LEAK | 'Directory listings configured' | 14 - 'application/adminconsoleactive/secure' | null | null | _ - 'application/adminconsoleactive/insecure' | VulnerabilityType.ADMIN_CONSOLE_ACTIVE | 'Tomcat Manager Application' | NO_LINE - 'application/defaulthtmlescapeinvalid/secure' | null | null | _ - 'application/defaulthtmlescapeinvalid/secure_tag' | null | null | _ - 'application/defaulthtmlescapeinvalid/false_tag' | VulnerabilityType.DEFAULT_HTML_ESCAPE_INVALID | 'defaultHtmlEscape tag should be true' | 8 - 'application/defaulthtmlescapeinvalid/no_tag_1' | VulnerabilityType.DEFAULT_HTML_ESCAPE_INVALID | 'defaultHtmlEscape tag should be set' | NO_LINE - 'application/defaulthtmlescapeinvalid/no_tag_2' | VulnerabilityType.DEFAULT_HTML_ESCAPE_INVALID | 'defaultHtmlEscape tag should be set' | NO_LINE + path | expectedVulnType | expectedEvidence | line + 'application/insecurejsplayout/secure' | null | null | _ + 'application/insecurejsplayout/insecure' | INSECURE_JSP_LAYOUT | ['/nestedinsecure', '/nestedinsecure/nestedinsecure', '/'] | NO_LINE + 'application/verbtampering/secure' | null | null | _ + 'application/verbtampering/insecure' | VERB_TAMPERING | 'http-method not defined in web.xml' | 6 + 'application/sessiontimeout/secure' | null | null | _ + 'application/sessiontimeout/insecure' | SESSION_TIMEOUT | 'Found vulnerable timeout value: 80' | 7 + 'application/directorylistingleak/secure' | null | null | _ + 'application/directorylistingleak/insecure' | DIRECTORY_LISTING_LEAK | 'Directory listings configured' | 14 + 'application/adminconsoleactive/secure' | null | null | _ + 'application/adminconsoleactive/insecure' | ADMIN_CONSOLE_ACTIVE | 'Tomcat Manager Application' | NO_LINE + 'application/defaulthtmlescapeinvalid/secure' | null | null | _ + 'application/defaulthtmlescapeinvalid/secure_tag' | null | null | _ + 'application/defaulthtmlescapeinvalid/false_tag' | DEFAULT_HTML_ESCAPE_INVALID | 'defaultHtmlEscape tag should be true' | 8 + 'application/defaulthtmlescapeinvalid/no_tag_1' | DEFAULT_HTML_ESCAPE_INVALID | 'defaultHtmlEscape tag should be set' | NO_LINE + 'application/defaulthtmlescapeinvalid/no_tag_2' | DEFAULT_HTML_ESCAPE_INVALID | 'defaultHtmlEscape tag should be set' | NO_LINE } private static void assertVulnerability(final vuln, final expectedVulnType) { @@ -80,9 +81,15 @@ class ApplicationModuleTest extends IastModuleImplTestBase { assertVulnerability(vuln, expectedVulnType) final evidence = vuln.evidence assert evidence != null - if (expectedEvidence instanceof String[]) { - for (String s : expectedEvidence) { - assert evidence.value.contains(s) + if (expectedEvidence instanceof Collection) { + if (expectedVulnType == INSECURE_JSP_LAYOUT) { + // some of the nested paths can be dropped by the file visitor + final parts = (evidence.value as String).split('\n')*.trim() + assert expectedEvidence.any { parts.contains(it) } + } else { + expectedEvidence.each { + assert evidence.value.contains(it) + } } } else { assert evidence.value == expectedEvidence diff --git a/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/util/StringUtilsTest.groovy b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/util/StringUtilsTest.groovy new file mode 100644 index 00000000000..6109e366910 --- /dev/null +++ b/dd-java-agent/agent-iast/src/test/groovy/com/datadog/iast/util/StringUtilsTest.groovy @@ -0,0 +1,46 @@ +package com.datadog.iast.util + +import spock.lang.Specification + +class StringUtilsTest extends Specification { + + void 'test ends with ignore case'() { + when: + final endsWith = StringUtils.endsWithIgnoreCase(value, pattern) + + then: + endsWith == expected + + where: + value | pattern | expected + '' | '' | true + 'abc' | '' | true + 'abc' | 'def' | false + 'abc' | '0abc' | false + '0abc' | 'abc' | true + 'abc' | 'aBc' | true + '0abc' | 'c' | true + 'abc' | 'C' | true + } + + void 'test substring with trim'() { + when: + final trimmed = StringUtils.substringTrim(value, start, end) + + then: + trimmed == expected + + where: + value | start | end | expected + '' | 0 | 0 | '' + ' ' | 0 | 1 | '' + 'abc' | 1 | 3 | 'bc' + ' abc' | 1 | 3 | 'a' + 'abc ' | 1 | 3 | 'bc' + ' abc ' | 1 | 3 | 'a' + ' ab c ' | 2 | 6 | 'ab' + ' ab c ' | 0 | 6 | 'ab' + ' ab' | 0 | 3 | '' + ' ab ' | 4 | 7 | '' + } +} diff --git a/dd-java-agent/agent-iast/src/test/resources/application/insecurejsplayout/secure/nested/README.md b/dd-java-agent/agent-iast/src/test/resources/application/insecurejsplayout/secure/nested/README.md new file mode 100644 index 00000000000..ca60906e742 --- /dev/null +++ b/dd-java-agent/agent-iast/src/test/resources/application/insecurejsplayout/secure/nested/README.md @@ -0,0 +1,3 @@ +# Secure JSP layout + +Sample file included in a secure layout