diff --git a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll index 76c82b377866..d9988e31f67f 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll @@ -347,3 +347,27 @@ predicate isRequestGetParamMethod(MethodAccess ma) { ma.getMethod() instanceof ServletRequestGetParameterMapMethod or ma.getMethod() instanceof HttpServletRequestGetQueryStringMethod } + +/** The Java EE RequestDispatcher. */ +class RequestDispatcher extends RefType { + RequestDispatcher() { + this.hasQualifiedName(["javax.servlet", "jakarta.servlet"], "RequestDispatcher") or + this.hasQualifiedName("javax.portlet", "PortletRequestDispatcher") + } +} + +/** The `getRequestDispatcher` method. */ +class GetRequestDispatcherMethod extends Method { + GetRequestDispatcherMethod() { + this.getReturnType() instanceof RequestDispatcher and + this.getName() = "getRequestDispatcher" + } +} + +/** The request dispatch method. */ +class RequestDispatchMethod extends Method { + RequestDispatchMethod() { + this.getDeclaringType() instanceof RequestDispatcher and + this.hasName(["forward", "include"]) + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java new file mode 100644 index 000000000000..88a794ab9c64 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java @@ -0,0 +1,11 @@ +// BAD: no URI validation +String returnURL = request.getParameter("returnURL"); +RequestDispatcher rd = sc.getRequestDispatcher(returnURL); +rd.forward(request, response); + +// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix: +// (alternatively use `Path.normalize` instead of checking for `..`) +if (!returnURL.contains("..") && returnURL.hasPrefix("/pages")) { ... } +// Also GOOD: check for a forbidden prefix, ensuring URL-encoding is not used to evade the check: +// (alternatively use `URLDecoder.decode` before `hasPrefix`) +if (returnURL.hasPrefix("/internal") && !returnURL.contains("%")) { ... } \ No newline at end of file diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp index 572b14bb02fe..345eca1e5d43 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp @@ -5,27 +5,47 @@ -

Constructing a server-side redirect path with user input could allow an attacker to download application binaries +

Constructing a server-side redirect path with user input could allow an attacker to download application binaries (including application classes or jar files) or view arbitrary files within protected directories.

 -

In order to prevent untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL.

+

Unsanitized user provided data must not be used to construct the path for URL forwarding. In order to prevent +untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL. +Instead, user input should be checked against allowed (e.g., must come within user_content/) or disallowed +(e.g. must not come within /internal) paths, ensuring that neither path traversal using ../ +or URL encoding are used to evade these checks. +

The following examples show the bad case and the good case respectively. The bad methods show an HTTP request parameter being used directly in a URL forward -without validating the input, which may cause file leakage. In good1 method, +without validating the input, which may cause file leakage. In the good1 method, ordinary forwarding requests are shown, which will not cause file leakage.

+

The following examples show an HTTP request parameter or request path being used directly in a +request dispatcher of Java EE without validating the input, which allows sensitive file exposure +attacks. It also shows how to remedy the problem by validating the user input. +

+ + + -
  • File Disclosure: Unsafe Url Forward.
    • +
  • File Disclosure: + Unsafe Url Forward. +
    • +
  • Jakarta Javadoc: + Security vulnerability with unsafe usage of RequestDispatcher. +
    • +
  • Micro Focus: + File Disclosure: J2EE +
    •  diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql index f9f13f728866..a39076f89c1a 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql @@ -1,11 +1,11 @@ /** - * @name Unsafe url forward from remote source - * @description URL forward based on unvalidated user-input + * @name Unsafe URL forward or dispatch from remote source + * @description URL forward or dispatch based on unvalidated user-input * may cause file information disclosure. * @kind path-problem * @problem.severity error * @precision high - * @id java/unsafe-url-forward + * @id java/unsafe-url-forward-dispatch * @tags security * external/cwe-552 */ @@ -13,21 +13,8 @@ import java import UnsafeUrlForward import semmle.code.java.dataflow.FlowSources -import semmle.code.java.frameworks.Servlets import DataFlow::PathGraph -private class StartsWithSanitizer extends DataFlow::BarrierGuard { - StartsWithSanitizer() { - this.(MethodAccess).getMethod().hasName("startsWith") and - this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and - this.(MethodAccess).getMethod().getNumberOfParameters() = 1 - } - - override predicate checks(Expr e, boolean branch) { - e = this.(MethodAccess).getQualifier() and branch = true - } -} - class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" } @@ -45,11 +32,15 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink } + override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer } + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { - guard instanceof StartsWithSanitizer + guard instanceof UnsafeUrlForwardBarrierGuard } - override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer } + override DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureHasSourceCallContext + } } from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll index f26563162035..5c07b67f43d9 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -1,14 +1,43 @@ import java import DataFlow +import semmle.code.java.controlflow.Guards import semmle.code.java.dataflow.FlowSources import semmle.code.java.frameworks.Servlets import semmle.code.java.frameworks.spring.SpringWeb import semmle.code.java.security.RequestForgery private import semmle.code.java.dataflow.StringPrefixes -/** A sanitizer for unsafe url forward vulnerabilities. */ +/** A sink for unsafe URL forward vulnerabilities. */ +abstract class UnsafeUrlForwardSink extends DataFlow::Node { } + +/** A sanitizer for unsafe URL forward vulnerabilities. */ abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { } +/** A barrier guard that protects against URL forward vulnerabilities. */ +abstract class UnsafeUrlForwardBarrierGuard extends DataFlow::BarrierGuard { } + +/** An argument to `getRequestDispatcher`. */ +private class RequestDispatcherSink extends UnsafeUrlForwardSink { + RequestDispatcherSink() { + exists(MethodAccess ma | + ma.getMethod() instanceof GetRequestDispatcherMethod and + ma.getArgument(0) = this.asExpr() + ) + } +} + +/** An argument to `new ModelAndView` or `ModelAndView.setViewName`. */ +private class SpringModelAndViewSink extends UnsafeUrlForwardSink { + SpringModelAndViewSink() { + exists(ClassInstanceExpr cie | + cie.getConstructedType() instanceof ModelAndView and + cie.getArgument(0) = this.asExpr() + ) + or + exists(SpringModelAndViewSetViewNameCall smavsvnc | smavsvnc.getArgument(0) = this.asExpr()) + } +} + private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer { PrimitiveSanitizer() { this.getType() instanceof PrimitiveType or @@ -30,27 +59,172 @@ private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() } } -abstract class UnsafeUrlForwardSink extends DataFlow::Node { } +/** + * A guard that considers safe a string being exactly compared to a trusted value. + */ +private class ExactStringPathMatchGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { + ExactStringPathMatchGuard() { + super.getMethod().getDeclaringType() instanceof TypeString and + super.getMethod().getName() = ["equals", "equalsIgnoreCase"] + } -/** An argument to `ServletRequest.getRequestDispatcher`. */ -private class RequestDispatcherSink extends UnsafeUrlForwardSink { - RequestDispatcherSink() { - exists(MethodAccess ma | - ma.getMethod() instanceof ServletRequestGetRequestDispatcherMethod and - ma.getArgument(0) = this.asExpr() + override predicate checks(Expr e, boolean branch) { + e = super.getQualifier() and + branch = true + } +} + +private class AllowListGuard extends Guard instanceof MethodAccess { + AllowListGuard() { + (isStringPartialMatch(this) or isPathPartialMatch(this)) and + not isDisallowedWord(super.getAnArgument()) + } + + Expr getCheckedExpr() { result = super.getQualifier() } +} + +/** + * A guard that considers a path safe because it is checked against an allowlist of partial trusted values. + * This requires additional protection against path traversal, either another guard (`PathTraversalGuard`) + * or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path. + */ +private class AllowListBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof AllowListGuard { + override predicate checks(Expr e, boolean branch) { + e = super.getCheckedExpr() and + branch = true and + ( + // Either a path normalization sanitizer comes before the guard, + exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e)) + or + // or a check like `!path.contains("..")` comes before the guard + exists(PathTraversalGuard previousGuard | + DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and + previousGuard.controls(this.getBasicBlock().(ConditionBlock), false) + ) ) } } -/** An argument to `new ModelAndView` or `ModelAndView.setViewName`. */ -private class SpringModelAndViewSink extends UnsafeUrlForwardSink { - SpringModelAndViewSink() { - exists(ClassInstanceExpr cie | - cie.getConstructedType() instanceof ModelAndView and - cie.getArgument(0) = this.asExpr() +/** + * A guard that considers a path safe because it is checked for `..` components, having previously + * been checked for a trusted prefix. + */ +private class DotDotCheckBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof PathTraversalGuard { + override predicate checks(Expr e, boolean branch) { + e = super.getCheckedExpr() and + branch = false and + // The same value has previously been checked against a list of allowed prefixes: + exists(AllowListGuard previousGuard | + DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and + previousGuard.controls(this.getBasicBlock().(ConditionBlock), true) ) - or - exists(SpringModelAndViewSetViewNameCall smavsvnc | smavsvnc.getArgument(0) = this.asExpr()) + } +} + +private class BlockListGuard extends Guard instanceof MethodAccess { + BlockListGuard() { + (isStringPartialMatch(this) or isPathPartialMatch(this)) and + isDisallowedWord(super.getAnArgument()) + } + + Expr getCheckedExpr() { result = super.getQualifier() } +} + +/** + * A guard that considers a string safe because it is checked against a blocklist of known dangerous values. + * This requires a prior check for URL encoding concealing a forbidden value, either a guard (`UrlEncodingGuard`) + * or a sanitizer (`UrlDecodeSanitizer`). + */ +private class BlockListBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof BlockListGuard { + override predicate checks(Expr e, boolean branch) { + e = super.getCheckedExpr() and + branch = false and + ( + // Either `e` has been URL decoded: + exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e)) + or + // or `e` has previously been checked for URL encoding sequences: + exists(UrlEncodingGuard previousGuard | + DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and + previousGuard.controls(this.getBasicBlock(), false) + ) + ) + } +} + +/** + * A guard that considers a string safe because it is checked for URL encoding sequences, + * having previously been checked against a block-list of forbidden values. + */ +private class URLEncodingBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof UrlEncodingGuard { + override predicate checks(Expr e, boolean branch) { + e = super.getCheckedExpr() and + branch = false and + exists(BlockListGuard previousGuard | + DataFlow::localExprFlow(previousGuard.getCheckedExpr(), e) and + previousGuard.controls(this.getBasicBlock(), false) + ) + } +} + +/** + * Holds if `ma` is a call to a method that checks a partial string match. + */ +private predicate isStringPartialMatch(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getName() = + ["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"] +} + +/** + * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a partial path match. + */ +private predicate isPathPartialMatch(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypePath and + ma.getMethod().getName() = "startsWith" +} + +private predicate isDisallowedWord(CompileTimeConstantExpr word) { + word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"]) +} + +/** A complementary guard that protects against path traversal, by looking for the literal `..`. */ +private class PathTraversalGuard extends Guard instanceof MethodAccess { + Expr checked; + + PathTraversalGuard() { + super.getMethod().getDeclaringType() instanceof TypeString and + super.getMethod().hasName(["contains", "indexOf"]) and + super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." + } + + Expr getCheckedExpr() { result = super.getQualifier() } +} + +/** A complementary sanitizer that protects against path traversal using path normalization. */ +private class PathNormalizeSanitizer extends MethodAccess { + PathNormalizeSanitizer() { + this.getMethod().getDeclaringType().hasQualifiedName("java.nio.file", "Path") and + this.getMethod().hasName("normalize") + } +} + +/** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */ +private class UrlEncodingGuard extends Guard instanceof MethodAccess { + UrlEncodingGuard() { + super.getMethod().getDeclaringType() instanceof TypeString and + super.getMethod().hasName(["contains", "indexOf"]) and + super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" + } + + Expr getCheckedExpr() { result = super.getQualifier() } +} + +/** A complementary sanitizer that protects against double URL encoding using URL decoding. */ +private class UrlDecodeSanitizer extends MethodAccess { + UrlDecodeSanitizer() { + this.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and + this.getMethod().hasName("decode") } } @@ -70,3 +244,27 @@ private class SpringUrlForwardSink extends UnsafeUrlForwardSink { this.asExpr() = any(ForwardPrefix fp).getAnAppendedExpression() } } + +/** Source model of remote flow source from `getServletPath`. */ +private class ServletGetPathSource extends SourceModelCsv { + override predicate row(string row) { + row = + [ + "javax.servlet.http;HttpServletRequest;true;getServletPath;;;ReturnValue;remote", + "jakarta.servlet.http;HttpServletRequest;true;getServletPath;;;ReturnValue;remote" + ] + } +} + +/** Taint model related to `java.nio.file.Path`. */ +private class FilePathFlowStep extends SummaryModelCsv { + override predicate row(string row) { + row = + [ + "java.nio.file;Paths;true;get;;;Argument[0..1];ReturnValue;taint", + "java.nio.file;Path;true;resolve;;;Argument[-1..0];ReturnValue;taint", + "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint", + "java.nio.file;Path;true;toString;;;Argument[-1];ReturnValue;taint" + ] + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeRequestPath.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeRequestPath.java new file mode 100644 index 000000000000..2de0cae0d3c5 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeRequestPath.java @@ -0,0 +1,52 @@ +import java.io.IOException; + +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +// @WebFilter("/*") +public class UnsafeRequestPath implements Filter { + private static final String BASE_PATH = "/pages"; + + @Override + // BAD: Request dispatcher from servlet path without check + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + String path = ((HttpServletRequest) request).getServletPath(); + // A sample payload "/%57EB-INF/web.xml" can bypass this `startsWith` check + if (path != null && !path.startsWith("/WEB-INF")) { + request.getRequestDispatcher(path).forward(request, response); + } else { + chain.doFilter(request, response); + } + } + + // GOOD: Request dispatcher from servlet path with check + public void doFilter2(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + String path = ((HttpServletRequest) request).getServletPath(); + + if (path.startsWith(BASE_PATH) && !path.contains("..")) { + request.getRequestDispatcher(path).forward(request, response); + } else { + chain.doFilter(request, response); + } + } + + // GOOD: Request dispatcher from servlet path with whitelisted string comparison + public void doFilter3(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + String path = ((HttpServletRequest) request).getServletPath(); + + if (path.equals("/comaction")) { + request.getRequestDispatcher(path).forward(request, response); + } else { + chain.doFilter(request, response); + } + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java new file mode 100644 index 000000000000..279764fba8b8 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java @@ -0,0 +1,128 @@ +import java.io.IOException; +import java.net.URLDecoder; +import java.io.File; +import java.nio.file.Path; +import java.nio.file.Paths; + +import javax.servlet.http.HttpServlet; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import javax.servlet.RequestDispatcher; +import javax.servlet.ServletException; +import javax.servlet.ServletConfig; +import javax.servlet.ServletContext; + +public class UnsafeServletRequestDispatch extends HttpServlet { + private static final String BASE_PATH = "/pages"; + + @Override + // BAD: Request dispatcher constructed from `ServletContext` without input validation + protected void doGet(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String action = request.getParameter("action"); + String returnURL = request.getParameter("returnURL"); + + ServletConfig cfg = getServletConfig(); + if (action.equals("Login")) { + ServletContext sc = cfg.getServletContext(); + RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp"); + rd.forward(request, response); + } else { + ServletContext sc = cfg.getServletContext(); + RequestDispatcher rd = sc.getRequestDispatcher(returnURL); + rd.forward(request, response); + } + } + + @Override + // BAD: Request dispatcher constructed from `HttpServletRequest` without input validation + protected void doPost(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String action = request.getParameter("action"); + String returnURL = request.getParameter("returnURL"); + + if (action.equals("Login")) { + RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp"); + rd.forward(request, response); + } else { + RequestDispatcher rd = request.getRequestDispatcher(returnURL); + rd.forward(request, response); + } + } + + @Override + // GOOD: Request dispatcher with a whitelisted URI + protected void doPut(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String action = request.getParameter("action"); + + if (action.equals("Login")) { + RequestDispatcher rd = request.getRequestDispatcher("/Login.jsp"); + rd.forward(request, response); + } else if (action.equals("Register")) { + RequestDispatcher rd = request.getRequestDispatcher("/Register.jsp"); + rd.forward(request, response); + } + } + + // BAD: Request dispatcher without path traversal check + protected void doHead2(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + + // A sample payload "/pages/welcome.jsp/../WEB-INF/web.xml" can bypass the `startsWith` check + // The payload "/pages/welcome.jsp/../../%57EB-INF/web.xml" can bypass the check as well since RequestDispatcher will decode `%57` as `W` + if (path.startsWith(BASE_PATH)) { + request.getServletContext().getRequestDispatcher(path).include(request, response); + } + } + + // GOOD: Request dispatcher with path traversal check + protected void doHead3(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + + if (path.startsWith(BASE_PATH) && !path.contains("..")) { + request.getServletContext().getRequestDispatcher(path).include(request, response); + } + } + + // GOOD: Request dispatcher with path normalization and comparison + protected void doHead4(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize(); + + // /pages/welcome.jsp/../../WEB-INF/web.xml becomes /WEB-INF/web.xml + // /pages/welcome.jsp/../../%57EB-INF/web.xml becomes /%57EB-INF/web.xml + if (requestedPath.startsWith(BASE_PATH)) { + request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); + } + } + + // BAD: Request dispatcher with negation check and path normalization, but without URL decoding + protected void doHead5(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize(); + + if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) { + request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); + } + } + + // GOOD: Request dispatcher with path traversal check and URL decoding + protected void doHead6(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + String path = request.getParameter("path"); + boolean hasEncoding = path.contains("%"); + while (hasEncoding) { + path = URLDecoder.decode(path, "UTF-8"); + hasEncoding = path.contains("%"); + } + + if (!path.startsWith("/WEB-INF/") && !path.contains("..")) { + request.getServletContext().getRequestDispatcher(path).include(request, response); + } + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected index b70a131f93c1..185508dfc575 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected @@ -1,4 +1,13 @@ edges +| UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | UnsafeRequestPath.java:23:33:23:36 | path | +| UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | +| UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | +| UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | +| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | +| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | +| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | +| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | +| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | | UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | @@ -6,7 +15,22 @@ edges | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:61:31:63 | url | | UnsafeUrlForward.java:36:19:36:28 | url : String | UnsafeUrlForward.java:38:33:38:35 | url | | UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... | +| UnsafeUrlForward.java:58:19:58:28 | url : String | UnsafeUrlForward.java:60:33:60:62 | ... + ... | nodes +| UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | semmle.label | getServletPath(...) : String | +| UnsafeRequestPath.java:23:33:23:36 | path | semmle.label | path | +| UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | semmle.label | returnURL | +| UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | semmle.label | returnURL | +| UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| UnsafeServletRequestDispatch.java:76:53:76:56 | path | semmle.label | path | +| UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | semmle.label | getParameter(...) : String | +| UnsafeServletRequestDispatch.java:107:24:107:57 | resolve(...) : Path | semmle.label | resolve(...) : Path | +| UnsafeServletRequestDispatch.java:107:24:107:69 | normalize(...) : Path | semmle.label | normalize(...) : Path | +| UnsafeServletRequestDispatch.java:107:53:107:56 | path : String | semmle.label | path : String | +| UnsafeServletRequestDispatch.java:110:53:110:65 | requestedPath : Path | semmle.label | requestedPath : Path | +| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | semmle.label | toString(...) | | UnsafeUrlForward.java:13:27:13:36 | url : String | semmle.label | url : String | | UnsafeUrlForward.java:14:27:14:29 | url | semmle.label | url | | UnsafeUrlForward.java:18:27:18:36 | url : String | semmle.label | url : String | @@ -20,8 +44,15 @@ nodes | UnsafeUrlForward.java:38:33:38:35 | url | semmle.label | url | | UnsafeUrlForward.java:47:19:47:28 | url : String | semmle.label | url : String | | UnsafeUrlForward.java:49:33:49:62 | ... + ... | semmle.label | ... + ... | +| UnsafeUrlForward.java:58:19:58:28 | url : String | semmle.label | url : String | +| UnsafeUrlForward.java:60:33:60:62 | ... + ... | semmle.label | ... + ... | subpaths #select +| UnsafeRequestPath.java:23:33:23:36 | path | UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | UnsafeRequestPath.java:23:33:23:36 | path | Potentially untrusted URL forward due to $@. | UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) | user-provided value | +| UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:32:51:32:59 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:23:22:23:54 | getParameter(...) | user-provided value | +| UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) : String | UnsafeServletRequestDispatch.java:48:56:48:64 | returnURL | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:42:22:42:54 | getParameter(...) | user-provided value | +| UnsafeServletRequestDispatch.java:76:53:76:56 | path | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:76:53:76:56 | path | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:71:17:71:44 | getParameter(...) | user-provided value | +| UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) : String | UnsafeServletRequestDispatch.java:110:53:110:76 | toString(...) | Potentially untrusted URL forward due to $@. | UnsafeServletRequestDispatch.java:106:17:106:44 | getParameter(...) | user-provided value | | UnsafeUrlForward.java:14:27:14:29 | url | UnsafeUrlForward.java:13:27:13:36 | url : String | UnsafeUrlForward.java:14:27:14:29 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:13:27:13:36 | url | user-provided value | | UnsafeUrlForward.java:20:28:20:30 | url | UnsafeUrlForward.java:18:27:18:36 | url : String | UnsafeUrlForward.java:20:28:20:30 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:18:27:18:36 | url | user-provided value | | UnsafeUrlForward.java:26:23:26:25 | url | UnsafeUrlForward.java:25:21:25:30 | url : String | UnsafeUrlForward.java:26:23:26:25 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:25:21:25:30 | url | user-provided value | @@ -29,3 +60,4 @@ subpaths | UnsafeUrlForward.java:31:61:31:63 | url | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:61:31:63 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:30:27:30:36 | url | user-provided value | | UnsafeUrlForward.java:38:33:38:35 | url | UnsafeUrlForward.java:36:19:36:28 | url : String | UnsafeUrlForward.java:38:33:38:35 | url | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:36:19:36:28 | url | user-provided value | | UnsafeUrlForward.java:49:33:49:62 | ... + ... | UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:47:19:47:28 | url | user-provided value | +| UnsafeUrlForward.java:60:33:60:62 | ... + ... | UnsafeUrlForward.java:58:19:58:28 | url : String | UnsafeUrlForward.java:60:33:60:62 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:58:19:58:28 | url | user-provided value | diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java index 1e7ce3a97c52..4018ed289481 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java @@ -54,6 +54,17 @@ public void bad6(String url, HttpServletRequest request, HttpServletResponse res } } + @GetMapping("/bad7") + public void bad7(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").forward(request, response); + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + @GetMapping("/good1") public void good1(String url, HttpServletRequest request, HttpServletResponse response) { try { diff --git a/java/ql/test/stubs/servlet-api-2.4/javax/servlet/http/HttpServletRequest.java b/java/ql/test/stubs/servlet-api-2.4/javax/servlet/http/HttpServletRequest.java index bc15d102eca2..5767e07ca5b4 100644 --- a/java/ql/test/stubs/servlet-api-2.4/javax/servlet/http/HttpServletRequest.java +++ b/java/ql/test/stubs/servlet-api-2.4/javax/servlet/http/HttpServletRequest.java @@ -25,6 +25,7 @@ import java.util.Enumeration; import javax.servlet.ServletRequest; +import javax.servlet.ServletContext; public interface HttpServletRequest extends ServletRequest { public String getAuthType(); @@ -52,4 +53,5 @@ public interface HttpServletRequest extends ServletRequest { public boolean isRequestedSessionIdFromCookie(); public boolean isRequestedSessionIdFromURL(); public boolean isRequestedSessionIdFromUrl(); + public ServletContext getServletContext(); }