From 8bcffc2886f3c17d6083287d4177f053683e36db Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Thu, 2 Dec 2021 04:00:29 +0000 Subject: [PATCH 01/17] Query to detect unsafe request dispatcher usage --- .../semmle/code/java/frameworks/Servlets.qll | 24 +++ .../CWE/CWE-552/UnsafeRequestPath.java | 30 ++++ .../CWE-552/UnsafeServletRequestDispatch.java | 82 ++++++++++ .../CWE/CWE-552/UnsafeUrlForward.qhelp | 26 ++- .../Security/CWE/CWE-552/UnsafeUrlForward.ql | 148 ++++++++++++++++-- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 29 +++- .../security/CWE-552/UnsafeRequestPath.java | 52 ++++++ .../CWE-552/UnsafeServletRequestDispatch.java | 128 +++++++++++++++ .../CWE-552/UnsafeUrlForward.expected | 32 ++++ .../security/CWE-552/UnsafeUrlForward.java | 11 ++ .../servlet/http/HttpServletRequest.java | 2 + 11 files changed, 543 insertions(+), 21 deletions(-) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-552/UnsafeRequestPath.java create mode 100644 java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java diff --git a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll index 76c82b377866..2ab2c62c4d46 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. */ +library class RequestDispatcher extends RefType { + RequestDispatcher() { + this.hasQualifiedName(["javax.servlet", "jakarta.servlet"], "RequestDispatcher") or + this.hasQualifiedName("javax.portlet", "PortletRequestDispatcher") + } +} + +/** The `getRequestDispatcher` method. */ +library class GetRequestDispatcherMethod extends Method { + GetRequestDispatcherMethod() { + this.getReturnType() instanceof RequestDispatcher and + this.getName() = "getRequestDispatcher" + } +} + +/** The request dispatch method. */ +library 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/UnsafeRequestPath.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java new file mode 100644 index 000000000000..ffa0ddf63bd5 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java @@ -0,0 +1,30 @@ +public class UnsafeRequestPath implements Filter { + private static final String BASE_PATH = "/pages"; + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + + { + // BAD: Request dispatcher from servlet path without check + 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 path traversal check + String path = ((HttpServletRequest) request).getServletPath(); + + if (path.startsWith(BASE_PATH) && !path.contains("..")) { + request.getRequestDispatcher(path).forward(request, response); + } else { + chain.doFilter(request, response); + } + } + } +} 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..042a188c4c71 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java @@ -0,0 +1,82 @@ +public class UnsafeServletRequestDispatch extends HttpServlet { + private static final String BASE_PATH = "/pages"; + + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) + throws ServletException, IOException { + { + // GOOD: whitelisted URI + if (action.equals("Login")) { + ServletContext sc = cfg.getServletContext(); + RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp"); + rd.forward(request, response); + } + } + + { + // BAD: Request dispatcher constructed from `ServletContext` without input validation + String returnURL = request.getParameter("returnURL"); + ServletConfig cfg = getServletConfig(); + + ServletContext sc = cfg.getServletContext(); + RequestDispatcher rd = sc.getRequestDispatcher(returnURL); + rd.forward(request, response); + } + + { + // BAD: Request dispatcher without path traversal check + 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 + 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 + 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 improper negation check and without url decoding + 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 + 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/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp index 572b14bb02fe..8daef391db86 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,45 @@ -

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. +

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 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..dd3b54de4c89 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 */ @@ -14,20 +14,137 @@ import java import UnsafeUrlForward import semmle.code.java.dataflow.FlowSources import semmle.code.java.frameworks.Servlets +import semmle.code.java.controlflow.Guards 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 - } +/** + * Holds if `ma` is a method call of matching with a path string, probably a whitelisted one. + */ +predicate isStringPathMatch(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getName() = ["startsWith", "matches", "regionMatches"] +} + +/** + * Holds if `ma` is a method call of `java.nio.file.Path` which matches with another + * path, probably a whitelisted one. + */ +predicate isFilePathMatch(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypePath and + ma.getMethod().getName() = "startsWith" +} + +/** + * Holds if `ma` is a method call that checks an input doesn't match using the `!` + * logical negation expression. + */ +predicate checkNoPathMatch(MethodAccess ma) { + exists(LogNotExpr lne | + (isStringPathMatch(ma) or isFilePathMatch(ma)) and + lne.getExpr() = ma + ) +} + +/** + * Holds if `ma` is a method call to check special characters `..` used in path traversal. + */ +predicate isPathTraversalCheck(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().hasName(["contains", "indexOf"]) and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." +} + +/** + * Holds if `ma` is a method call to decode a url string or check url encoding. + */ +predicate isPathDecoding(MethodAccess ma) { + // Search the special character `%` used in url encoding + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().hasName(["contains", "indexOf"]) and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" + or + // Call to `URLDecoder` assuming the implementation handles double encoding correctly + ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and + ma.getMethod().hasName("decode") +} - override predicate checks(Expr e, boolean branch) { - e = this.(MethodAccess).getQualifier() and branch = true +private class PathMatchSanitizer extends DataFlow::Node { + PathMatchSanitizer() { + exists(MethodAccess ma | + ( + isStringPathMatch(ma) and + exists(MethodAccess ma2 | + isPathTraversalCheck(ma2) and + ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier() + ) + or + isFilePathMatch(ma) + ) and + ( + not checkNoPathMatch(ma) + or + // non-match check needs decoding e.g. !path.startsWith("/WEB-INF/") won't detect /%57EB-INF/web.xml, which will be decoded and served by RequestDispatcher + checkNoPathMatch(ma) and + exists(MethodAccess ma2 | + isPathDecoding(ma2) and + ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier() + ) + ) and + this.asExpr() = ma.getQualifier() + ) } } +/** + * Holds if `ma` is a method call to check string content, which means an input string is not + * blindly trusted and helps to reduce FPs. + */ +predicate checkStringContent(MethodAccess ma, Expr expr) { + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod() + .hasName([ + "charAt", "contains", "equals", "equalsIgnoreCase", "getBytes", "getChars", "indexOf", + "lastIndexOf", "length", "matches", "regionMatches", "replace", "replaceAll", + "replaceFirst", "substring" + ]) and + expr = ma.getQualifier() + or + ( + ma.getMethod().getDeclaringType() instanceof TypeStringBuffer or + ma.getMethod().getDeclaringType() instanceof TypeStringBuilder + ) and + expr = ma.getAnArgument() +} + +private class StringOperationSanitizer extends DataFlow::Node { + StringOperationSanitizer() { exists(MethodAccess ma | checkStringContent(ma, this.asExpr())) } +} + +/** + * Holds if `expr` is an expression returned from null or empty string check. + */ +predicate isNullOrEmptyCheck(Expr expr) { + exists(ConditionBlock cb, ReturnStmt rt | + cb.controls(rt.getBasicBlock(), true) and + ( + cb.getCondition().(EQExpr).getAnOperand() instanceof NullLiteral // if (path == null) + or + // if (path.equals("")) + exists(MethodAccess ma | + cb.getCondition() = ma and + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().hasName("equals") and + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "" + ) + ) and + expr.getParent+() = rt + ) +} + +private class NullOrEmptyCheckSanitizer extends DataFlow::Node { + NullOrEmptyCheckSanitizer() { isNullOrEmptyCheck(this.asExpr()) } +} + class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" } @@ -45,11 +162,12 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink } - override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { - guard instanceof StartsWithSanitizer + override predicate isSanitizer(DataFlow::Node node) { + node instanceof UnsafeUrlForwardSanitizer or + node instanceof PathMatchSanitizer or + node instanceof StringOperationSanitizer or + node instanceof NullOrEmptyCheckSanitizer } - - override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer } } 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..ab15549b9e1b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -32,11 +32,11 @@ private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { abstract class UnsafeUrlForwardSink extends DataFlow::Node { } -/** An argument to `ServletRequest.getRequestDispatcher`. */ +/** An argument to `getRequestDispatcher`. */ private class RequestDispatcherSink extends UnsafeUrlForwardSink { RequestDispatcherSink() { exists(MethodAccess ma | - ma.getMethod() instanceof ServletRequestGetRequestDispatcherMethod and + ma.getMethod() instanceof GetRequestDispatcherMethod and ma.getArgument(0) = this.asExpr() ) } @@ -70,3 +70,28 @@ 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[-1];ReturnValue;taint", + "java.nio.file;Path;true;resolve;;;Argument[0];ReturnValue;taint", + "java.nio.file;Path;true;normalize;;;Argument[-1];ReturnValue;taint", + "java.nio.file;Path;true;startsWith;;;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..f0b0a71ea0fa --- /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 improper negation check and 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(); } From 29ce0e9ef19a46bf756472b8a3956a19cda443be Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 15 Dec 2021 16:19:50 +0000 Subject: [PATCH 02/17] Add sanitizer for virtual method calls --- .../Security/CWE/CWE-552/UnsafeUrlForward.ql | 20 ++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) 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 dd3b54de4c89..f77acf99e848 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql @@ -145,6 +145,23 @@ private class NullOrEmptyCheckSanitizer extends DataFlow::Node { NullOrEmptyCheckSanitizer() { isNullOrEmptyCheck(this.asExpr()) } } +/** Holds if `ma` is a virtual method call of Map::get or Object::toString. */ +predicate isVirtualMethod(MethodAccess ma, Expr expr) { + ma.getMethod().getDeclaringType() instanceof TypeObject and + ma.getMethod().hasName("toString") and + (expr = ma or expr = ma.getQualifier()) + or + ( + ma.getMethod().getDeclaringType().getASupertype*().hasQualifiedName("java.util", "Map") and + ma.getMethod().hasName(["get", "getOrDefault"]) + ) and + (expr = ma or expr = ma.getAnArgument()) +} + +private class VirtualMethodSanitizer extends DataFlow::Node { + VirtualMethodSanitizer() { exists(MethodAccess ma | isVirtualMethod(ma, this.asExpr())) } +} + class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" } @@ -166,7 +183,8 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { node instanceof UnsafeUrlForwardSanitizer or node instanceof PathMatchSanitizer or node instanceof StringOperationSanitizer or - node instanceof NullOrEmptyCheckSanitizer + node instanceof NullOrEmptyCheckSanitizer or + node instanceof VirtualMethodSanitizer } } From 263dbd33f6b908940e1b6d205652ccde050a35d6 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 12 Jan 2022 02:33:17 +0000 Subject: [PATCH 03/17] Optimize the query --- .../CWE/CWE-552/UnsafeUrlForward.qhelp | 2 +- .../Security/CWE/CWE-552/UnsafeUrlForward.ql | 125 ++++++++++-------- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 5 +- .../CWE-552/UnsafeServletRequestDispatch.java | 2 +- .../CWE-552/UnsafeUrlForward.expected | 12 -- 5 files changed, 76 insertions(+), 70 deletions(-) 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 8daef391db86..b8a3ddda77f0 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp @@ -20,7 +20,7 @@ untrusted URL forwarding, it is recommended to avoid concatenating user input di

    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.

    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 f77acf99e848..0bb939f66dd4 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql @@ -18,16 +18,25 @@ import semmle.code.java.controlflow.Guards import DataFlow::PathGraph /** - * Holds if `ma` is a method call of matching with a path string, probably a whitelisted one. + * Holds if `ma` is a call to a method that checks exact match of string, probably a whitelisted one. + */ +predicate isExactStringPathMatch(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] +} + +/** + * Holds if `ma` is a call to a method that checks a path string, probably a whitelisted one. */ predicate isStringPathMatch(MethodAccess ma) { ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().getName() = ["startsWith", "matches", "regionMatches"] + ma.getMethod().getName() = + ["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"] } /** - * Holds if `ma` is a method call of `java.nio.file.Path` which matches with another - * path, probably a whitelisted one. + * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a path, probably + * a whitelisted one. */ predicate isFilePathMatch(MethodAccess ma) { ma.getMethod().getDeclaringType() instanceof TypePath and @@ -35,7 +44,7 @@ predicate isFilePathMatch(MethodAccess ma) { } /** - * Holds if `ma` is a method call that checks an input doesn't match using the `!` + * Holds if `ma` is a call to a method that checks an input doesn't match using the `!` * logical negation expression. */ predicate checkNoPathMatch(MethodAccess ma) { @@ -46,7 +55,7 @@ predicate checkNoPathMatch(MethodAccess ma) { } /** - * Holds if `ma` is a method call to check special characters `..` used in path traversal. + * Holds if `ma` is a call to a method that checks special characters `..` used in path traversal. */ predicate isPathTraversalCheck(MethodAccess ma) { ma.getMethod().getDeclaringType() instanceof TypeString and @@ -55,7 +64,7 @@ predicate isPathTraversalCheck(MethodAccess ma) { } /** - * Holds if `ma` is a method call to decode a url string or check url encoding. + * Holds if `ma` is a call to a method that decodes a URL string or check URL encoding. */ predicate isPathDecoding(MethodAccess ma) { // Search the special character `%` used in url encoding @@ -68,44 +77,65 @@ predicate isPathDecoding(MethodAccess ma) { ma.getMethod().hasName("decode") } -private class PathMatchSanitizer extends DataFlow::Node { +/** The Java method `normalize` of `java.nio.file.Path`. */ +class PathNormalizeMethod extends Method { + PathNormalizeMethod() { + this.getDeclaringType().hasQualifiedName("java.nio.file", "Path") and + this.hasName("normalize") + } +} + +/** + * Sanitizer to check the following scenarios in a web application: + * 1. Exact string match + * 2. String startsWith or match check with path traversal validation + * 3. String not startsWith or not match check with decoding processing + * 4. java.nio.file.Path startsWith check having path normalization + */ +private class PathMatchSanitizer extends DataFlow::BarrierGuard { PathMatchSanitizer() { - exists(MethodAccess ma | - ( - isStringPathMatch(ma) and - exists(MethodAccess ma2 | - isPathTraversalCheck(ma2) and - ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier() - ) - or - isFilePathMatch(ma) - ) and - ( - not checkNoPathMatch(ma) - or - // non-match check needs decoding e.g. !path.startsWith("/WEB-INF/") won't detect /%57EB-INF/web.xml, which will be decoded and served by RequestDispatcher - checkNoPathMatch(ma) and - exists(MethodAccess ma2 | - isPathDecoding(ma2) and - ma.getQualifier().(VarAccess).getVariable().getAnAccess() = ma2.getQualifier() - ) - ) and - this.asExpr() = ma.getQualifier() + isExactStringPathMatch(this) + or + isStringPathMatch(this) and + not checkNoPathMatch(this) and + exists(MethodAccess tma | + isPathTraversalCheck(tma) and + DataFlow::localExprFlow(this.(MethodAccess).getQualifier(), tma.getQualifier()) + ) + or + checkNoPathMatch(this) and + exists(MethodAccess dma | + isPathDecoding(dma) and + DataFlow::localExprFlow(dma, this.(MethodAccess).getQualifier()) + ) + or + isFilePathMatch(this) and + exists(MethodAccess pma | + pma.getMethod() instanceof PathNormalizeMethod and + DataFlow::localExprFlow(pma, this.(MethodAccess).getQualifier()) + ) + } + + override predicate checks(Expr e, boolean branch) { + e = this.(MethodAccess).getQualifier() and + ( + branch = true and not checkNoPathMatch(this) + or + branch = false and checkNoPathMatch(this) ) } } /** - * Holds if `ma` is a method call to check string content, which means an input string is not + * Holds if `ma` is a call to a method that checks string content, which means an input string is not * blindly trusted and helps to reduce FPs. */ predicate checkStringContent(MethodAccess ma, Expr expr) { ma.getMethod().getDeclaringType() instanceof TypeString and ma.getMethod() .hasName([ - "charAt", "contains", "equals", "equalsIgnoreCase", "getBytes", "getChars", "indexOf", - "lastIndexOf", "length", "matches", "regionMatches", "replace", "replaceAll", - "replaceFirst", "substring" + "charAt", "getBytes", "getChars", "length", "replace", "replaceAll", "replaceFirst", + "substring" ]) and expr = ma.getQualifier() or @@ -145,23 +175,6 @@ private class NullOrEmptyCheckSanitizer extends DataFlow::Node { NullOrEmptyCheckSanitizer() { isNullOrEmptyCheck(this.asExpr()) } } -/** Holds if `ma` is a virtual method call of Map::get or Object::toString. */ -predicate isVirtualMethod(MethodAccess ma, Expr expr) { - ma.getMethod().getDeclaringType() instanceof TypeObject and - ma.getMethod().hasName("toString") and - (expr = ma or expr = ma.getQualifier()) - or - ( - ma.getMethod().getDeclaringType().getASupertype*().hasQualifiedName("java.util", "Map") and - ma.getMethod().hasName(["get", "getOrDefault"]) - ) and - (expr = ma or expr = ma.getAnArgument()) -} - -private class VirtualMethodSanitizer extends DataFlow::Node { - VirtualMethodSanitizer() { exists(MethodAccess ma | isVirtualMethod(ma, this.asExpr())) } -} - class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" } @@ -181,10 +194,16 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer or - node instanceof PathMatchSanitizer or node instanceof StringOperationSanitizer or - node instanceof NullOrEmptyCheckSanitizer or - node instanceof VirtualMethodSanitizer + node instanceof NullOrEmptyCheckSanitizer + } + + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + guard instanceof PathMatchSanitizer + } + + override DataFlow::FlowFeature getAFeature() { + result instanceof DataFlow::FeatureHasSourceCallContext } } 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 ab15549b9e1b..cb70dffbafc2 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -87,10 +87,9 @@ private class FilePathFlowStep extends SummaryModelCsv { override predicate row(string row) { row = [ - "java.nio.file;Paths;true;get;;;Argument[-1];ReturnValue;taint", - "java.nio.file;Path;true;resolve;;;Argument[0];ReturnValue;taint", + "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;startsWith;;;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/UnsafeServletRequestDispatch.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java index f0b0a71ea0fa..b79db0fc6e6c 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java @@ -100,7 +100,7 @@ protected void doHead4(HttpServletRequest request, HttpServletResponse response) } } - // BAD: Request dispatcher with improper negation check and without url decoding + // GOOD: Request dispatcher with negation check and path normalization protected void doHead5(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { String path = request.getParameter("path"); 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 185508dfc575..d3f8fc247d2a 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 @@ -3,11 +3,6 @@ edges | 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 | @@ -25,12 +20,6 @@ nodes | 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 | @@ -52,7 +41,6 @@ subpaths | 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 | From cd9a485c47a9ae8c4145fffaaba8e4bc435bdfdf Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 13 Jan 2022 14:44:08 +0100 Subject: [PATCH 04/17] Refactor NullOrEmptyCheckGuard --- .../Security/CWE/CWE-552/UnsafeUrlForward.ql | 49 +++++++++---------- 1 file changed, 23 insertions(+), 26 deletions(-) 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 0bb939f66dd4..c43b48e76214 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql @@ -15,6 +15,7 @@ import UnsafeUrlForward import semmle.code.java.dataflow.FlowSources import semmle.code.java.frameworks.Servlets import semmle.code.java.controlflow.Guards +import semmle.code.java.dataflow.NullGuards import DataFlow::PathGraph /** @@ -92,8 +93,8 @@ class PathNormalizeMethod extends Method { * 3. String not startsWith or not match check with decoding processing * 4. java.nio.file.Path startsWith check having path normalization */ -private class PathMatchSanitizer extends DataFlow::BarrierGuard { - PathMatchSanitizer() { +private class PathMatchGuard extends DataFlow::BarrierGuard { + PathMatchGuard() { isExactStringPathMatch(this) or isStringPathMatch(this) and @@ -150,29 +151,25 @@ private class StringOperationSanitizer extends DataFlow::Node { StringOperationSanitizer() { exists(MethodAccess ma | checkStringContent(ma, this.asExpr())) } } -/** - * Holds if `expr` is an expression returned from null or empty string check. - */ -predicate isNullOrEmptyCheck(Expr expr) { - exists(ConditionBlock cb, ReturnStmt rt | - cb.controls(rt.getBasicBlock(), true) and - ( - cb.getCondition().(EQExpr).getAnOperand() instanceof NullLiteral // if (path == null) - or - // if (path.equals("")) - exists(MethodAccess ma | +private class NullOrEmptyCheckGuard extends DataFlow::BarrierGuard { + NullOrEmptyCheckGuard() { + this = nullGuard(_, _, _) + or + exists(MethodAccess ma | cb.getCondition() = ma and - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().hasName("equals") and - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "" - ) - ) and - expr.getParent+() = rt - ) -} + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().hasName("equals") and + ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "" and + this = ma + ) + } -private class NullOrEmptyCheckSanitizer extends DataFlow::Node { - NullOrEmptyCheckSanitizer() { isNullOrEmptyCheck(this.asExpr()) } + override predicate checks(Expr e, boolean branch) { + exists(SsaVariable ssa | this = nullGuard(ssa, branch, true) and e = ssa.getAFirstUse()) + or + e = this.(MethodAccess).getQualifier() and + branch = true + } } class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { @@ -194,12 +191,12 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer or - node instanceof StringOperationSanitizer or - node instanceof NullOrEmptyCheckSanitizer + node instanceof StringOperationSanitizer } override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { - guard instanceof PathMatchSanitizer + guard instanceof PathMatchGuard or + guard instanceof NullOrEmptyCheckGuard } override DataFlow::FlowFeature getAFeature() { From 81feaaec0270c7b9635d539eaa7f3877d34e9340 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 13 Jan 2022 15:03:31 +0100 Subject: [PATCH 05/17] Refactor PathMatchGuard --- .../Security/CWE/CWE-552/UnsafeUrlForward.ql | 156 +++++++----------- .../CWE-552/UnsafeServletRequestDispatch.java | 6 +- .../CWE-552/UnsafeUrlForward.expected | 12 ++ 3 files changed, 72 insertions(+), 102 deletions(-) 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 c43b48e76214..89baadd53888 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql @@ -19,7 +19,7 @@ import semmle.code.java.dataflow.NullGuards import DataFlow::PathGraph /** - * Holds if `ma` is a call to a method that checks exact match of string, probably a whitelisted one. + * Holds if `ma` is a call to a method that checks exact match of string. */ predicate isExactStringPathMatch(MethodAccess ma) { ma.getMethod().getDeclaringType() instanceof TypeString and @@ -27,7 +27,7 @@ predicate isExactStringPathMatch(MethodAccess ma) { } /** - * Holds if `ma` is a call to a method that checks a path string, probably a whitelisted one. + * Holds if `ma` is a call to a method that checks a path string. */ predicate isStringPathMatch(MethodAccess ma) { ma.getMethod().getDeclaringType() instanceof TypeString and @@ -36,8 +36,7 @@ predicate isStringPathMatch(MethodAccess ma) { } /** - * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a path, probably - * a whitelisted one. + * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a path. */ predicate isFilePathMatch(MethodAccess ma) { ma.getMethod().getDeclaringType() instanceof TypePath and @@ -45,37 +44,36 @@ predicate isFilePathMatch(MethodAccess ma) { } /** - * Holds if `ma` is a call to a method that checks an input doesn't match using the `!` - * logical negation expression. + * Holds if `ma` protects against path traversal, by either: + * * looking for the literal `..` + * * performing path normalization */ -predicate checkNoPathMatch(MethodAccess ma) { - exists(LogNotExpr lne | - (isStringPathMatch(ma) or isFilePathMatch(ma)) and - lne.getExpr() = ma - ) -} - -/** - * Holds if `ma` is a call to a method that checks special characters `..` used in path traversal. - */ -predicate isPathTraversalCheck(MethodAccess ma) { +predicate isPathTraversalCheck(MethodAccess ma, Expr checked) { ma.getMethod().getDeclaringType() instanceof TypeString and ma.getMethod().hasName(["contains", "indexOf"]) and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and + ma.(Guard).controls(checked.getBasicBlock(), false) + or + ma.getMethod() instanceof PathNormalizeMethod and + checked = ma } /** - * Holds if `ma` is a call to a method that decodes a URL string or check URL encoding. + * Holds if `ma` protects against double URL encoding, by either: + * * looking for the literal `%` + * * performing URL decoding */ -predicate isPathDecoding(MethodAccess ma) { +predicate isURLEncodingCheck(MethodAccess ma, Expr checked) { // Search the special character `%` used in url encoding ma.getMethod().getDeclaringType() instanceof TypeString and ma.getMethod().hasName(["contains", "indexOf"]) and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and + ma.(Guard).controls(checked.getBasicBlock(), false) or // Call to `URLDecoder` assuming the implementation handles double encoding correctly ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and - ma.getMethod().hasName("decode") + ma.getMethod().hasName("decode") and + checked = ma } /** The Java method `normalize` of `java.nio.file.Path`. */ @@ -86,90 +84,54 @@ class PathNormalizeMethod extends Method { } } +private predicate isDisallowedWord(CompileTimeConstantExpr word) { + word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"]) +} + +private predicate isAllowListCheck(MethodAccess ma) { + (isStringPathMatch(ma) or isFilePathMatch(ma)) and + not isDisallowedWord(ma.getAnArgument()) +} + +private predicate isDisallowListCheck(MethodAccess ma) { + (isStringPathMatch(ma) or isFilePathMatch(ma)) and + isDisallowedWord(ma.getAnArgument()) +} + /** - * Sanitizer to check the following scenarios in a web application: + * A guard that checks a path with the following methods: * 1. Exact string match - * 2. String startsWith or match check with path traversal validation - * 3. String not startsWith or not match check with decoding processing - * 4. java.nio.file.Path startsWith check having path normalization + * 2. Path matches allowed values (needs to protect against path traversal) + * 3. Path matches disallowed values (needs to protect against URL encoding) */ private class PathMatchGuard extends DataFlow::BarrierGuard { PathMatchGuard() { - isExactStringPathMatch(this) - or - isStringPathMatch(this) and - not checkNoPathMatch(this) and - exists(MethodAccess tma | - isPathTraversalCheck(tma) and - DataFlow::localExprFlow(this.(MethodAccess).getQualifier(), tma.getQualifier()) - ) - or - checkNoPathMatch(this) and - exists(MethodAccess dma | - isPathDecoding(dma) and - DataFlow::localExprFlow(dma, this.(MethodAccess).getQualifier()) - ) - or - isFilePathMatch(this) and - exists(MethodAccess pma | - pma.getMethod() instanceof PathNormalizeMethod and - DataFlow::localExprFlow(pma, this.(MethodAccess).getQualifier()) - ) + isExactStringPathMatch(this) or isStringPathMatch(this) or isFilePathMatch(this) } override predicate checks(Expr e, boolean branch) { e = this.(MethodAccess).getQualifier() and ( - branch = true and not checkNoPathMatch(this) + isExactStringPathMatch(this) and + branch = true or - branch = false and checkNoPathMatch(this) - ) - } -} - -/** - * Holds if `ma` is a call to a method that checks string content, which means an input string is not - * blindly trusted and helps to reduce FPs. - */ -predicate checkStringContent(MethodAccess ma, Expr expr) { - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod() - .hasName([ - "charAt", "getBytes", "getChars", "length", "replace", "replaceAll", "replaceFirst", - "substring" - ]) and - expr = ma.getQualifier() - or - ( - ma.getMethod().getDeclaringType() instanceof TypeStringBuffer or - ma.getMethod().getDeclaringType() instanceof TypeStringBuilder - ) and - expr = ma.getAnArgument() -} - -private class StringOperationSanitizer extends DataFlow::Node { - StringOperationSanitizer() { exists(MethodAccess ma | checkStringContent(ma, this.asExpr())) } -} - -private class NullOrEmptyCheckGuard extends DataFlow::BarrierGuard { - NullOrEmptyCheckGuard() { - this = nullGuard(_, _, _) - or - exists(MethodAccess ma | - cb.getCondition() = ma and - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().hasName("equals") and - ma.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "" and - this = ma + isAllowListCheck(this) and + exists(MethodAccess ma, Expr checked | isPathTraversalCheck(ma, checked) | + DataFlow::localExprFlow(checked, e) + or + ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*() + ) and + branch = true + or + isDisallowListCheck(this) and + exists(MethodAccess ma, Expr checked | isURLEncodingCheck(ma, checked) | + DataFlow::localExprFlow(checked, e) + or + ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*() + ) and + branch = false ) } - - override predicate checks(Expr e, boolean branch) { - exists(SsaVariable ssa | this = nullGuard(ssa, branch, true) and e = ssa.getAFirstUse()) - or - e = this.(MethodAccess).getQualifier() and - branch = true - } } class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { @@ -189,14 +151,10 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink } - override predicate isSanitizer(DataFlow::Node node) { - node instanceof UnsafeUrlForwardSanitizer or - node instanceof StringOperationSanitizer - } + override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer } override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { - guard instanceof PathMatchGuard or - guard instanceof NullOrEmptyCheckGuard + guard instanceof PathMatchGuard } override DataFlow::FlowFeature getAFeature() { 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 index b79db0fc6e6c..279764fba8b8 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeServletRequestDispatch.java @@ -80,7 +80,7 @@ protected void doHead2(HttpServletRequest request, HttpServletResponse response) // GOOD: Request dispatcher with path traversal check protected void doHead3(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { - String path = request.getParameter("path"); + String path = request.getParameter("path"); if (path.startsWith(BASE_PATH) && !path.contains("..")) { request.getServletContext().getRequestDispatcher(path).include(request, response); @@ -100,7 +100,7 @@ protected void doHead4(HttpServletRequest request, HttpServletResponse response) } } - // GOOD: Request dispatcher with negation check and path normalization + // 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"); @@ -111,7 +111,7 @@ protected void doHead5(HttpServletRequest request, HttpServletResponse response) } } - // GOOD: Request dispatcher with path traversal check and url decoding + // 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"); 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 d3f8fc247d2a..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 @@ -3,6 +3,11 @@ edges | 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 | @@ -20,6 +25,12 @@ nodes | 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 | @@ -41,6 +52,7 @@ subpaths | 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 | From b6886b8e4385f5857f62d27395d85d1ee2fe4227 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Thu, 13 Jan 2022 15:28:57 +0100 Subject: [PATCH 06/17] Move code to qll file --- .../Security/CWE/CWE-552/UnsafeUrlForward.ql | 123 +----------------- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 120 +++++++++++++++++ 2 files changed, 122 insertions(+), 121 deletions(-) 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 89baadd53888..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,5 +1,5 @@ /** - * @name Unsafe url forward or dispatch from remote source + * @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 @@ -13,127 +13,8 @@ import java import UnsafeUrlForward import semmle.code.java.dataflow.FlowSources -import semmle.code.java.frameworks.Servlets -import semmle.code.java.controlflow.Guards -import semmle.code.java.dataflow.NullGuards import DataFlow::PathGraph -/** - * Holds if `ma` is a call to a method that checks exact match of string. - */ -predicate isExactStringPathMatch(MethodAccess ma) { - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] -} - -/** - * Holds if `ma` is a call to a method that checks a path string. - */ -predicate isStringPathMatch(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 path. - */ -predicate isFilePathMatch(MethodAccess ma) { - ma.getMethod().getDeclaringType() instanceof TypePath and - ma.getMethod().getName() = "startsWith" -} - -/** - * Holds if `ma` protects against path traversal, by either: - * * looking for the literal `..` - * * performing path normalization - */ -predicate isPathTraversalCheck(MethodAccess ma, Expr checked) { - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().hasName(["contains", "indexOf"]) and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and - ma.(Guard).controls(checked.getBasicBlock(), false) - or - ma.getMethod() instanceof PathNormalizeMethod and - checked = ma -} - -/** - * Holds if `ma` protects against double URL encoding, by either: - * * looking for the literal `%` - * * performing URL decoding - */ -predicate isURLEncodingCheck(MethodAccess ma, Expr checked) { - // Search the special character `%` used in url encoding - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().hasName(["contains", "indexOf"]) and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and - ma.(Guard).controls(checked.getBasicBlock(), false) - or - // Call to `URLDecoder` assuming the implementation handles double encoding correctly - ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and - ma.getMethod().hasName("decode") and - checked = ma -} - -/** The Java method `normalize` of `java.nio.file.Path`. */ -class PathNormalizeMethod extends Method { - PathNormalizeMethod() { - this.getDeclaringType().hasQualifiedName("java.nio.file", "Path") and - this.hasName("normalize") - } -} - -private predicate isDisallowedWord(CompileTimeConstantExpr word) { - word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"]) -} - -private predicate isAllowListCheck(MethodAccess ma) { - (isStringPathMatch(ma) or isFilePathMatch(ma)) and - not isDisallowedWord(ma.getAnArgument()) -} - -private predicate isDisallowListCheck(MethodAccess ma) { - (isStringPathMatch(ma) or isFilePathMatch(ma)) and - isDisallowedWord(ma.getAnArgument()) -} - -/** - * A guard that checks a path with the following methods: - * 1. Exact string match - * 2. Path matches allowed values (needs to protect against path traversal) - * 3. Path matches disallowed values (needs to protect against URL encoding) - */ -private class PathMatchGuard extends DataFlow::BarrierGuard { - PathMatchGuard() { - isExactStringPathMatch(this) or isStringPathMatch(this) or isFilePathMatch(this) - } - - override predicate checks(Expr e, boolean branch) { - e = this.(MethodAccess).getQualifier() and - ( - isExactStringPathMatch(this) and - branch = true - or - isAllowListCheck(this) and - exists(MethodAccess ma, Expr checked | isPathTraversalCheck(ma, checked) | - DataFlow::localExprFlow(checked, e) - or - ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*() - ) and - branch = true - or - isDisallowListCheck(this) and - exists(MethodAccess ma, Expr checked | isURLEncodingCheck(ma, checked) | - DataFlow::localExprFlow(checked, e) - or - ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*() - ) and - branch = false - ) - } -} - class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" } @@ -154,7 +35,7 @@ class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { override predicate isSanitizer(DataFlow::Node node) { node instanceof UnsafeUrlForwardSanitizer } override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { - guard instanceof PathMatchGuard + guard instanceof UnsafeUrlForwardBarrierGuard } override DataFlow::FlowFeature getAFeature() { 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 cb70dffbafc2..3979e8dd96de 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -1,5 +1,6 @@ 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 @@ -30,6 +31,125 @@ private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() } } +/** A barrier guard that protects against URL forward vulnerabilities. */ +abstract class UnsafeUrlForwardBarrierGuard extends DataFlow::BarrierGuard { } + +/** + * Holds if `ma` is a call to a method that checks exact match of string. + */ +private predicate isExactStringPathMatch(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] +} + +/** + * Holds if `ma` is a call to a method that checks a path string. + */ +private predicate isStringPathMatch(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 path. + */ +private predicate isFilePathMatch(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypePath and + ma.getMethod().getName() = "startsWith" +} + +/** + * Holds if `ma` protects against path traversal, by either: + * * looking for the literal `..` + * * performing path normalization + */ +private predicate isPathTraversalCheck(MethodAccess ma, Expr checked) { + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().hasName(["contains", "indexOf"]) and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and + ma.(Guard).controls(checked.getBasicBlock(), false) + or + ma.getMethod() instanceof PathNormalizeMethod and + checked = ma +} + +/** + * Holds if `ma` protects against double URL encoding, by either: + * * looking for the literal `%` + * * performing URL decoding + */ +private predicate isURLEncodingCheck(MethodAccess ma, Expr checked) { + // Search the special character `%` used in url encoding + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().hasName(["contains", "indexOf"]) and + ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and + ma.(Guard).controls(checked.getBasicBlock(), false) + or + // Call to `URLDecoder` assuming the implementation handles double encoding correctly + ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and + ma.getMethod().hasName("decode") and + checked = ma +} + +/** The Java method `normalize` of `java.nio.file.Path`. */ +private class PathNormalizeMethod extends Method { + PathNormalizeMethod() { + this.getDeclaringType().hasQualifiedName("java.nio.file", "Path") and + this.hasName("normalize") + } +} + +private predicate isDisallowedWord(CompileTimeConstantExpr word) { + word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"]) +} + +private predicate isAllowListCheck(MethodAccess ma) { + (isStringPathMatch(ma) or isFilePathMatch(ma)) and + not isDisallowedWord(ma.getAnArgument()) +} + +private predicate isDisallowListCheck(MethodAccess ma) { + (isStringPathMatch(ma) or isFilePathMatch(ma)) and + isDisallowedWord(ma.getAnArgument()) +} + +/** + * A guard that checks a path with the following methods: + * 1. Exact string match + * 2. Path matches allowed values (needs to protect against path traversal) + * 3. Path matches disallowed values (needs to protect against URL encoding) + */ +private class PathMatchGuard extends UnsafeUrlForwardBarrierGuard { + PathMatchGuard() { + isExactStringPathMatch(this) or isStringPathMatch(this) or isFilePathMatch(this) + } + + override predicate checks(Expr e, boolean branch) { + e = this.(MethodAccess).getQualifier() and + ( + isExactStringPathMatch(this) and + branch = true + or + isAllowListCheck(this) and + exists(MethodAccess ma, Expr checked | isPathTraversalCheck(ma, checked) | + DataFlow::localExprFlow(checked, e) + or + ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*() + ) and + branch = true + or + isDisallowListCheck(this) and + exists(MethodAccess ma, Expr checked | isURLEncodingCheck(ma, checked) | + DataFlow::localExprFlow(checked, e) + or + ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*() + ) and + branch = false + ) + } +} + abstract class UnsafeUrlForwardSink extends DataFlow::Node { } /** An argument to `getRequestDispatcher`. */ From 877c52981ff3b333d13cdb802b7f3592e5d753bc Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 14 Jan 2022 12:13:41 +0000 Subject: [PATCH 07/17] Remove the deprecated library keyword --- java/ql/lib/semmle/code/java/frameworks/Servlets.qll | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll index 2ab2c62c4d46..d9988e31f67f 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll @@ -349,7 +349,7 @@ predicate isRequestGetParamMethod(MethodAccess ma) { } /** The Java EE RequestDispatcher. */ -library class RequestDispatcher extends RefType { +class RequestDispatcher extends RefType { RequestDispatcher() { this.hasQualifiedName(["javax.servlet", "jakarta.servlet"], "RequestDispatcher") or this.hasQualifiedName("javax.portlet", "PortletRequestDispatcher") @@ -357,7 +357,7 @@ library class RequestDispatcher extends RefType { } /** The `getRequestDispatcher` method. */ -library class GetRequestDispatcherMethod extends Method { +class GetRequestDispatcherMethod extends Method { GetRequestDispatcherMethod() { this.getReturnType() instanceof RequestDispatcher and this.getName() = "getRequestDispatcher" @@ -365,7 +365,7 @@ library class GetRequestDispatcherMethod extends Method { } /** The request dispatch method. */ -library class RequestDispatchMethod extends Method { +class RequestDispatchMethod extends Method { RequestDispatchMethod() { this.getDeclaringType() instanceof RequestDispatcher and this.hasName(["forward", "include"]) From 136fefbab55fe4ec9f2cec51feeddbf2c37ec9fa Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 14 Jan 2022 13:38:17 +0100 Subject: [PATCH 08/17] Apply suggestions from code review Co-authored-by: Chris Smowton --- .../src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3979e8dd96de..9d7272ea9feb 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -122,7 +122,7 @@ private predicate isDisallowListCheck(MethodAccess ma) { */ private class PathMatchGuard extends UnsafeUrlForwardBarrierGuard { PathMatchGuard() { - isExactStringPathMatch(this) or isStringPathMatch(this) or isFilePathMatch(this) + isExactStringPathMatch(this) or isAllowListCheck(this) or isDisallowListCheck(this) } override predicate checks(Expr e, boolean branch) { From fb1287d577d09cfd6710b51c466351e47fd624c4 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 14 Jan 2022 15:20:41 +0100 Subject: [PATCH 09/17] Use dominance instead of getParent Add clarification comments to PathMatchGuard --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) 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 9d7272ea9feb..119670cdd14f 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -131,19 +131,31 @@ private class PathMatchGuard extends UnsafeUrlForwardBarrierGuard { isExactStringPathMatch(this) and branch = true or + // When using an allowlist, that is, checking for known safe paths + // (for example, `path.startsWith(BASE_PATH)`) + // the application needs to protect against path traversal bypasses. isAllowListCheck(this) and exists(MethodAccess ma, Expr checked | isPathTraversalCheck(ma, checked) | - DataFlow::localExprFlow(checked, e) - or - ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*() + // Either the path traversal check comes before the guard + DataFlow::localExprFlow(checked, e) or + // or both checks are in the same condition + // (for example, `path.startsWith(BASE_PATH) && !path.contains("..")`) + ma.(Guard).controls(this.getBasicBlock(), _) or + this.controls(ma.getBasicBlock(), branch) ) and branch = true or + // When using a blocklist, that is, checking for known bad patterns in the path, + // (for example, `path.startsWith("/WEB-INF/")` or `path.contains("..")`) + // the application needs to protect against double URL encoding bypasses. isDisallowListCheck(this) and exists(MethodAccess ma, Expr checked | isURLEncodingCheck(ma, checked) | - DataFlow::localExprFlow(checked, e) - or - ma.getParent*().(BinaryExpr) = this.(MethodAccess).getParent*() + // Either the URL encoding check comes before the guard + DataFlow::localExprFlow(checked, e) or + // or both checks are in the same condition + // (for example, `!path.contains("..") && !path.contains("%")`) + ma.(Guard).controls(this.getBasicBlock(), _) or + this.controls(ma.getBasicBlock(), branch) ) and branch = false ) From eb1806c0a92d23dbd87fcc9b62fb11d1c5370711 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 14 Jan 2022 17:12:02 +0100 Subject: [PATCH 10/17] Split PathMatchGuard into three guards --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 174 ++++++++++-------- 1 file changed, 94 insertions(+), 80 deletions(-) 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 119670cdd14f..89b2ca393004 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -34,14 +34,6 @@ private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { /** A barrier guard that protects against URL forward vulnerabilities. */ abstract class UnsafeUrlForwardBarrierGuard extends DataFlow::BarrierGuard { } -/** - * Holds if `ma` is a call to a method that checks exact match of string. - */ -private predicate isExactStringPathMatch(MethodAccess ma) { - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().getName() = ["equals", "equalsIgnoreCase"] -} - /** * Holds if `ma` is a call to a method that checks a path string. */ @@ -59,44 +51,47 @@ private predicate isFilePathMatch(MethodAccess ma) { ma.getMethod().getName() = "startsWith" } -/** - * Holds if `ma` protects against path traversal, by either: - * * looking for the literal `..` - * * performing path normalization - */ -private predicate isPathTraversalCheck(MethodAccess ma, Expr checked) { - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().hasName(["contains", "indexOf"]) and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and - ma.(Guard).controls(checked.getBasicBlock(), false) - or - ma.getMethod() instanceof PathNormalizeMethod and - checked = ma +/** A complementary guard that protects against path traversal, by looking for the literal `..`. */ +private class PathTraversalGuard extends Guard instanceof MethodAccess { + Expr checked; + + PathTraversalGuard() { + this.getMethod().getDeclaringType() instanceof TypeString and + this.getMethod().hasName(["contains", "indexOf"]) and + this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and + this.controls(checked.getBasicBlock(), false) + } + + predicate checks(Expr e) { checked = e } } -/** - * Holds if `ma` protects against double URL encoding, by either: - * * looking for the literal `%` - * * performing URL decoding - */ -private predicate isURLEncodingCheck(MethodAccess ma, Expr checked) { - // Search the special character `%` used in url encoding - ma.getMethod().getDeclaringType() instanceof TypeString and - ma.getMethod().hasName(["contains", "indexOf"]) and - ma.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and - ma.(Guard).controls(checked.getBasicBlock(), false) - or - // Call to `URLDecoder` assuming the implementation handles double encoding correctly - ma.getMethod().getDeclaringType().hasQualifiedName("java.net", "URLDecoder") and - ma.getMethod().hasName("decode") and - checked = ma +/** 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 { + Expr checked; + + UrlEncodingGuard() { + this.getMethod().getDeclaringType() instanceof TypeString and + this.getMethod().hasName(["contains", "indexOf"]) and + this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and + this.controls(checked.getBasicBlock(), false) + } + + predicate checks(Expr e) { checked = e } } -/** The Java method `normalize` of `java.nio.file.Path`. */ -private class PathNormalizeMethod extends Method { - PathNormalizeMethod() { - this.getDeclaringType().hasQualifiedName("java.nio.file", "Path") and - this.hasName("normalize") +/** 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") } } @@ -104,60 +99,79 @@ private predicate isDisallowedWord(CompileTimeConstantExpr word) { word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"]) } -private predicate isAllowListCheck(MethodAccess ma) { - (isStringPathMatch(ma) or isFilePathMatch(ma)) and - not isDisallowedWord(ma.getAnArgument()) -} +/** + * A guard that considers safe a string being exactly compared to a trusted value. + */ +private class ExactStringPathMatchGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { + ExactStringPathMatchGuard() { + this.getMethod().getDeclaringType() instanceof TypeString and + this.getMethod().getName() = ["equals", "equalsIgnoreCase"] + } -private predicate isDisallowListCheck(MethodAccess ma) { - (isStringPathMatch(ma) or isFilePathMatch(ma)) and - isDisallowedWord(ma.getAnArgument()) + override predicate checks(Expr e, boolean branch) { + e = this.(MethodAccess).getQualifier() and + branch = true + } } /** - * A guard that checks a path with the following methods: - * 1. Exact string match - * 2. Path matches allowed values (needs to protect against path traversal) - * 3. Path matches disallowed values (needs to protect against URL encoding) + * A guard that considers safe a string being matched against an allowlist of partial trusted values. + * This requires additional protection against path traversal, either another guard (`PathTraversalGuard`) + * or a sanitizer (`PathNormalizeSanitizer`). */ -private class PathMatchGuard extends UnsafeUrlForwardBarrierGuard { - PathMatchGuard() { - isExactStringPathMatch(this) or isAllowListCheck(this) or isDisallowListCheck(this) +private class AllowListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { + AllowListCheckGuard() { + (isStringPathMatch(this) or isFilePathMatch(this)) and + not isDisallowedWord(this.getAnArgument()) } override predicate checks(Expr e, boolean branch) { e = this.(MethodAccess).getQualifier() and + branch = true and ( - isExactStringPathMatch(this) and - branch = true + // Either the path normalization sanitizer comes before the guard + exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e)) or - // When using an allowlist, that is, checking for known safe paths - // (for example, `path.startsWith(BASE_PATH)`) - // the application needs to protect against path traversal bypasses. - isAllowListCheck(this) and - exists(MethodAccess ma, Expr checked | isPathTraversalCheck(ma, checked) | - // Either the path traversal check comes before the guard - DataFlow::localExprFlow(checked, e) or + // or the path traversal check comes before the guard + exists(PathTraversalGuard guard | + guard.checks(any(Expr checked | DataFlow::localExprFlow(checked, e))) or // or both checks are in the same condition // (for example, `path.startsWith(BASE_PATH) && !path.contains("..")`) - ma.(Guard).controls(this.getBasicBlock(), _) or - this.controls(ma.getBasicBlock(), branch) - ) and - branch = true + guard.controls(this.getBasicBlock().(ConditionBlock), false) or + this.controls(guard.getBasicBlock().(ConditionBlock), branch) + ) + ) + } +} + +/** + * A guard that considers safe a string being matched against a blocklist of known dangerous values. + * This requires additional protection against path traversal, either another guard (`UrlEncodingGuard`) + * or a sanitizer (`UrlDecodeSanitizer`). + */ +private class BlockListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { + BlockListCheckGuard() { + (isStringPathMatch(this) or isFilePathMatch(this)) and + isDisallowedWord(this.getAnArgument()) + } + + override predicate checks(Expr e, boolean branch) { + e = this.(MethodAccess).getQualifier() and + branch = false and + ( + // Either the URL decode sanitization comes before the guard + exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e)) or - // When using a blocklist, that is, checking for known bad patterns in the path, - // (for example, `path.startsWith("/WEB-INF/")` or `path.contains("..")`) - // the application needs to protect against double URL encoding bypasses. - isDisallowListCheck(this) and - exists(MethodAccess ma, Expr checked | isURLEncodingCheck(ma, checked) | - // Either the URL encoding check comes before the guard - DataFlow::localExprFlow(checked, e) or + // or the URL encoding check comes before the guard + exists(UrlEncodingGuard guard | + guard.checks(any(Expr checked | DataFlow::localExprFlow(checked, e))) + or // or both checks are in the same condition // (for example, `!path.contains("..") && !path.contains("%")`) - ma.(Guard).controls(this.getBasicBlock(), _) or - this.controls(ma.getBasicBlock(), branch) - ) and - branch = false + guard.controls(this.getBasicBlock().(ConditionBlock), false) + or + this.controls(guard.getBasicBlock().(ConditionBlock), branch) + ) ) } } From a2c98baf29941f171128f21e189b18b7604b8b68 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Fri, 14 Jan 2022 17:14:28 +0100 Subject: [PATCH 11/17] Reordering --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 173 +++++++++--------- 1 file changed, 87 insertions(+), 86 deletions(-) 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 89b2ca393004..924757ddf138 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -7,9 +7,37 @@ 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 @@ -31,74 +59,6 @@ private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() } } -/** A barrier guard that protects against URL forward vulnerabilities. */ -abstract class UnsafeUrlForwardBarrierGuard extends DataFlow::BarrierGuard { } - -/** - * Holds if `ma` is a call to a method that checks a path string. - */ -private predicate isStringPathMatch(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 path. - */ -private predicate isFilePathMatch(MethodAccess ma) { - ma.getMethod().getDeclaringType() instanceof TypePath and - ma.getMethod().getName() = "startsWith" -} - -/** A complementary guard that protects against path traversal, by looking for the literal `..`. */ -private class PathTraversalGuard extends Guard instanceof MethodAccess { - Expr checked; - - PathTraversalGuard() { - this.getMethod().getDeclaringType() instanceof TypeString and - this.getMethod().hasName(["contains", "indexOf"]) and - this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and - this.controls(checked.getBasicBlock(), false) - } - - predicate checks(Expr e) { checked = e } -} - -/** 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 { - Expr checked; - - UrlEncodingGuard() { - this.getMethod().getDeclaringType() instanceof TypeString and - this.getMethod().hasName(["contains", "indexOf"]) and - this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and - this.controls(checked.getBasicBlock(), false) - } - - predicate checks(Expr e) { checked = e } -} - -/** 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") - } -} - -private predicate isDisallowedWord(CompileTimeConstantExpr word) { - word.getStringValue().matches(["%WEB-INF%", "%META-INF%", "%..%"]) -} - /** * A guard that considers safe a string being exactly compared to a trusted value. */ @@ -176,27 +136,68 @@ private class BlockListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceo } } -abstract class UnsafeUrlForwardSink extends DataFlow::Node { } +/** + * Holds if `ma` is a call to a method that checks a path string. + */ +private predicate isStringPathMatch(MethodAccess ma) { + ma.getMethod().getDeclaringType() instanceof TypeString and + ma.getMethod().getName() = + ["contains", "startsWith", "matches", "regionMatches", "indexOf", "lastIndexOf"] +} -/** An argument to `getRequestDispatcher`. */ -private class RequestDispatcherSink extends UnsafeUrlForwardSink { - RequestDispatcherSink() { - exists(MethodAccess ma | - ma.getMethod() instanceof GetRequestDispatcherMethod and - ma.getArgument(0) = this.asExpr() - ) +/** + * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a path. + */ +private predicate isFilePathMatch(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() { + this.getMethod().getDeclaringType() instanceof TypeString and + this.getMethod().hasName(["contains", "indexOf"]) and + this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and + this.controls(checked.getBasicBlock(), false) } + + predicate checks(Expr e) { checked = e } } -/** 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()) +/** 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 { + Expr checked; + + UrlEncodingGuard() { + this.getMethod().getDeclaringType() instanceof TypeString and + this.getMethod().hasName(["contains", "indexOf"]) and + this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and + this.controls(checked.getBasicBlock(), false) + } + + predicate checks(Expr e) { checked = e } +} + +/** 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") } } From 978ef1570a7fcbefdedb5a75a5a484e33f45b323 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Sun, 16 Jan 2022 01:11:25 +0000 Subject: [PATCH 12/17] Update method names --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 924757ddf138..c245b7ffb0a4 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -81,7 +81,7 @@ private class ExactStringPathMatchGuard extends UnsafeUrlForwardBarrierGuard ins */ private class AllowListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { AllowListCheckGuard() { - (isStringPathMatch(this) or isFilePathMatch(this)) and + (isStringPartialMatch(this) or isPathPartialMatch(this)) and not isDisallowedWord(this.getAnArgument()) } @@ -111,7 +111,7 @@ private class AllowListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceo */ private class BlockListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { BlockListCheckGuard() { - (isStringPathMatch(this) or isFilePathMatch(this)) and + (isStringPartialMatch(this) or isPathPartialMatch(this)) and isDisallowedWord(this.getAnArgument()) } @@ -137,18 +137,18 @@ private class BlockListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceo } /** - * Holds if `ma` is a call to a method that checks a path string. + * Holds if `ma` is a call to a method that checks a partial string match. */ -private predicate isStringPathMatch(MethodAccess ma) { +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 path. + * Holds if `ma` is a call to a method of `java.nio.file.Path` that checks a partial path match. */ -private predicate isFilePathMatch(MethodAccess ma) { +private predicate isPathPartialMatch(MethodAccess ma) { ma.getMethod().getDeclaringType() instanceof TypePath and ma.getMethod().getName() = "startsWith" } From 4797fce48a3d78adc95223a25a027f5e992cf078 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Sun, 16 Jan 2022 01:15:29 +0000 Subject: [PATCH 13/17] Update use cases and qldoc --- .../CWE-552/UnsafeServletRequestDispatch.java | 83 +++++++------------ .../CWE/CWE-552/UnsafeUrlForward.qhelp | 3 + 2 files changed, 31 insertions(+), 55 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java index 042a188c4c71..59458912f241 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java @@ -5,78 +5,51 @@ public class UnsafeServletRequestDispatch extends HttpServlet { protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { { - // GOOD: whitelisted URI + ServletConfig cfg = getServletConfig(); + ServletContext sc = cfg.getServletContext(); + + // GOOD: check for an explicitly permitted URI + String action = request.getParameter("action"); if (action.equals("Login")) { - ServletContext sc = cfg.getServletContext(); RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp"); rd.forward(request, response); - } - } + } - { - // BAD: Request dispatcher constructed from `ServletContext` without input validation + // BAD: no URI validation String returnURL = request.getParameter("returnURL"); - ServletConfig cfg = getServletConfig(); - - ServletContext sc = cfg.getServletContext(); RequestDispatcher rd = sc.getRequestDispatcher(returnURL); rd.forward(request, response); - } - { - // BAD: Request dispatcher without path traversal check + // 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` 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` + // BAD: no check for path traversal if (path.startsWith(BASE_PATH)) { request.getServletContext().getRequestDispatcher(path).include(request, response); } - } - } - - { - // GOOD: Request dispatcher with path traversal check - 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 - 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 improper negation check and without url decoding - String path = request.getParameter("path"); - Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize(); + // GOOD: To check there won't be unexpected path traversal, we can check for any `..` sequences and whether the URI starts with a given web root path. + if (path.startsWith(BASE_PATH) && !path.contains("..")) { + request.getServletContext().getRequestDispatcher(path).include(request, response); + } - if (!requestedPath.startsWith("/WEB-INF") && !requestedPath.startsWith("/META-INF")) { - request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); - } - } + // GOOD: Or alternatively we can use Path.normalize and check whether the URI starts with a given web root path. + Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize(); + if (requestedPath.startsWith(BASE_PATH)) { + request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); + } - { - // GOOD: Request dispatcher with path traversal check and url decoding - String path = request.getParameter("path"); - boolean hasEncoding = path.contains("%"); - while (hasEncoding) { - path = URLDecoder.decode(path, "UTF-8"); - hasEncoding = path.contains("%"); - } + // GOOD: Or alternatively ensure URL encoding is removed and then check for any `..` sequences. + 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); + if (!path.startsWith("/WEB-INF/") && !path.contains("..")) { + request.getServletContext().getRequestDispatcher(path).include(request, response); + } } } } 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 b8a3ddda77f0..5b1022d50ee6 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp @@ -13,6 +13,9 @@

    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 for path traversal using .. sequences or the user input should +be normalized using Path.normalize, any URL encoding should be removed, and user input should be +checked against a permitted URI.

    From a3d65a8ed0663b044f6c9d9828cc3f60db5395c9 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 18 Jan 2022 17:01:26 +0000 Subject: [PATCH 14/17] Update recommendation in qldoc and make examples more comprehendible --- .../CWE/CWE-552/UnsafeRequestPath.java | 30 --------- .../CWE-552/UnsafeServletRequestDispatch.java | 66 ++++--------------- .../CWE/CWE-552/UnsafeUrlForward.qhelp | 6 +- 3 files changed, 14 insertions(+), 88 deletions(-) delete mode 100644 java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java deleted file mode 100644 index ffa0ddf63bd5..000000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeRequestPath.java +++ /dev/null @@ -1,30 +0,0 @@ -public class UnsafeRequestPath implements Filter { - private static final String BASE_PATH = "/pages"; - - @Override - public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) - throws IOException, ServletException { - - { - // BAD: Request dispatcher from servlet path without check - 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 path traversal check - String path = ((HttpServletRequest) request).getServletPath(); - - if (path.startsWith(BASE_PATH) && !path.contains("..")) { - request.getRequestDispatcher(path).forward(request, response); - } else { - chain.doFilter(request, response); - } - } - } -} diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java index 59458912f241..88a794ab9c64 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java @@ -1,55 +1,11 @@ -public class UnsafeServletRequestDispatch extends HttpServlet { - private static final String BASE_PATH = "/pages"; - - @Override - protected void doGet(HttpServletRequest request, HttpServletResponse response) - throws ServletException, IOException { - { - ServletConfig cfg = getServletConfig(); - ServletContext sc = cfg.getServletContext(); - - // GOOD: check for an explicitly permitted URI - String action = request.getParameter("action"); - if (action.equals("Login")) { - RequestDispatcher rd = sc.getRequestDispatcher("/Login.jsp"); - rd.forward(request, response); - } - - // BAD: no URI validation - String returnURL = request.getParameter("returnURL"); - RequestDispatcher rd = sc.getRequestDispatcher(returnURL); - rd.forward(request, response); - - // 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` - String path = request.getParameter("path"); - - // BAD: no check for path traversal - if (path.startsWith(BASE_PATH)) { - request.getServletContext().getRequestDispatcher(path).include(request, response); - } - - // GOOD: To check there won't be unexpected path traversal, we can check for any `..` sequences and whether the URI starts with a given web root path. - if (path.startsWith(BASE_PATH) && !path.contains("..")) { - request.getServletContext().getRequestDispatcher(path).include(request, response); - } - - // GOOD: Or alternatively we can use Path.normalize and check whether the URI starts with a given web root path. - Path requestedPath = Paths.get(BASE_PATH).resolve(path).normalize(); - if (requestedPath.startsWith(BASE_PATH)) { - request.getServletContext().getRequestDispatcher(requestedPath.toString()).forward(request, response); - } - - // GOOD: Or alternatively ensure URL encoding is removed and then check for any `..` sequences. - 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); - } - } - } -} +// 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 5b1022d50ee6..1ccfaf5078c8 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp @@ -13,9 +13,9 @@

    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 for path traversal using .. sequences or the user input should -be normalized using Path.normalize, any URL encoding should be removed, and user input should be -checked against a permitted URI. +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.

    From 748008ad517be80dfff4de9a867b7d9e181a19ae Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 18 Jan 2022 17:08:38 +0000 Subject: [PATCH 15/17] Remove dangling reference to UnsafeRequestPath.java --- .../src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp | 1 - 1 file changed, 1 deletion(-) 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 1ccfaf5078c8..345eca1e5d43 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp @@ -35,7 +35,6 @@ attacks. It also shows how to remedy the problem by validating the user input.

    - From d744cf9053442a1067162ff75bef8028b686f270 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 18 Jan 2022 12:28:11 +0000 Subject: [PATCH 16/17] Clean up guard logic: * Always sanitize after the second guard, not the first * Only check basic-block dominance in one place * One BarrierGuard extension per final guard --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 109 +++++++++++------- 1 file changed, 68 insertions(+), 41 deletions(-) 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 c245b7ffb0a4..785813a0d014 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -74,68 +74,99 @@ private class ExactStringPathMatchGuard extends UnsafeUrlForwardBarrierGuard ins } } +private class AllowListGuard extends Guard instanceof MethodAccess { + AllowListGuard() { + (isStringPartialMatch(this.(MethodAccess)) or isPathPartialMatch(this.(MethodAccess))) and + not isDisallowedWord(this.(MethodAccess).getAnArgument()) + } + + Expr getCheckedExpr() { result = super.getQualifier() } +} + /** - * A guard that considers safe a string being matched against an allowlist of partial trusted values. + * 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`). + * or a sanitizer (`PathNormalizeSanitizer`), to ensure any internal `..` components are removed from the path. */ -private class AllowListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { - AllowListCheckGuard() { - (isStringPartialMatch(this) or isPathPartialMatch(this)) and - not isDisallowedWord(this.getAnArgument()) - } - +private class AllowListBarrierGuard extends UnsafeUrlForwardBarrierGuard instanceof AllowListGuard { override predicate checks(Expr e, boolean branch) { - e = this.(MethodAccess).getQualifier() and + e = super.getCheckedExpr() and branch = true and ( - // Either the path normalization sanitizer comes before the guard + // Either a path normalization sanitizer comes before the guard, exists(PathNormalizeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e)) or - // or the path traversal check comes before the guard - exists(PathTraversalGuard guard | - guard.checks(any(Expr checked | DataFlow::localExprFlow(checked, e))) or - // or both checks are in the same condition - // (for example, `path.startsWith(BASE_PATH) && !path.contains("..")`) - guard.controls(this.getBasicBlock().(ConditionBlock), false) or - this.controls(guard.getBasicBlock().(ConditionBlock), branch) + // 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) ) ) } } /** - * A guard that considers safe a string being matched against a blocklist of known dangerous values. - * This requires additional protection against path traversal, either another guard (`UrlEncodingGuard`) - * or a sanitizer (`UrlDecodeSanitizer`). + * A guard that considers a path safe because it is checked for `..` components, having previously + * been checked for a trusted prefix. */ -private class BlockListCheckGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { - BlockListCheckGuard() { +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) + ) + } +} + +private class BlockListGuard extends Guard instanceof MethodAccess { + BlockListGuard() { (isStringPartialMatch(this) or isPathPartialMatch(this)) and isDisallowedWord(this.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 = this.(MethodAccess).getQualifier() and + e = super.getCheckedExpr() and branch = false and ( - // Either the URL decode sanitization comes before the guard + // Either `e` has been URL decoded: exists(UrlDecodeSanitizer sanitizer | DataFlow::localExprFlow(sanitizer, e)) or - // or the URL encoding check comes before the guard - exists(UrlEncodingGuard guard | - guard.checks(any(Expr checked | DataFlow::localExprFlow(checked, e))) - or - // or both checks are in the same condition - // (for example, `!path.contains("..") && !path.contains("%")`) - guard.controls(this.getBasicBlock().(ConditionBlock), false) - or - this.controls(guard.getBasicBlock().(ConditionBlock), branch) + // 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. */ @@ -164,11 +195,10 @@ private class PathTraversalGuard extends Guard instanceof MethodAccess { PathTraversalGuard() { this.getMethod().getDeclaringType() instanceof TypeString and this.getMethod().hasName(["contains", "indexOf"]) and - this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." and - this.controls(checked.getBasicBlock(), false) + this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." } - predicate checks(Expr e) { checked = e } + Expr getCheckedExpr() { result = super.getQualifier() } } /** A complementary sanitizer that protects against path traversal using path normalization. */ @@ -181,16 +211,13 @@ private class PathNormalizeSanitizer extends MethodAccess { /** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */ private class UrlEncodingGuard extends Guard instanceof MethodAccess { - Expr checked; - UrlEncodingGuard() { this.getMethod().getDeclaringType() instanceof TypeString and this.getMethod().hasName(["contains", "indexOf"]) and - this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" and - this.controls(checked.getBasicBlock(), false) + this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" } - predicate checks(Expr e) { checked = e } + Expr getCheckedExpr() { result = super.getQualifier() } } /** A complementary sanitizer that protects against double URL encoding using URL decoding. */ From 1e32514600132eb40d7a5c9944efb0ff926992c6 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 18 Jan 2022 17:20:40 +0000 Subject: [PATCH 17/17] Avoid using `this` for a non-extending supertype, and remove needless casts --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) 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 785813a0d014..5c07b67f43d9 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -64,20 +64,20 @@ private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { */ private class ExactStringPathMatchGuard extends UnsafeUrlForwardBarrierGuard instanceof MethodAccess { ExactStringPathMatchGuard() { - this.getMethod().getDeclaringType() instanceof TypeString and - this.getMethod().getName() = ["equals", "equalsIgnoreCase"] + super.getMethod().getDeclaringType() instanceof TypeString and + super.getMethod().getName() = ["equals", "equalsIgnoreCase"] } override predicate checks(Expr e, boolean branch) { - e = this.(MethodAccess).getQualifier() and + e = super.getQualifier() and branch = true } } private class AllowListGuard extends Guard instanceof MethodAccess { AllowListGuard() { - (isStringPartialMatch(this.(MethodAccess)) or isPathPartialMatch(this.(MethodAccess))) and - not isDisallowedWord(this.(MethodAccess).getAnArgument()) + (isStringPartialMatch(this) or isPathPartialMatch(this)) and + not isDisallowedWord(super.getAnArgument()) } Expr getCheckedExpr() { result = super.getQualifier() } @@ -124,7 +124,7 @@ private class DotDotCheckBarrierGuard extends UnsafeUrlForwardBarrierGuard insta private class BlockListGuard extends Guard instanceof MethodAccess { BlockListGuard() { (isStringPartialMatch(this) or isPathPartialMatch(this)) and - isDisallowedWord(this.getAnArgument()) + isDisallowedWord(super.getAnArgument()) } Expr getCheckedExpr() { result = super.getQualifier() } @@ -193,9 +193,9 @@ private class PathTraversalGuard extends Guard instanceof MethodAccess { Expr checked; PathTraversalGuard() { - this.getMethod().getDeclaringType() instanceof TypeString and - this.getMethod().hasName(["contains", "indexOf"]) and - this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." + super.getMethod().getDeclaringType() instanceof TypeString and + super.getMethod().hasName(["contains", "indexOf"]) and + super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = ".." } Expr getCheckedExpr() { result = super.getQualifier() } @@ -212,9 +212,9 @@ private class PathNormalizeSanitizer extends MethodAccess { /** A complementary guard that protects against double URL encoding, by looking for the literal `%`. */ private class UrlEncodingGuard extends Guard instanceof MethodAccess { UrlEncodingGuard() { - this.getMethod().getDeclaringType() instanceof TypeString and - this.getMethod().hasName(["contains", "indexOf"]) and - this.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" + super.getMethod().getDeclaringType() instanceof TypeString and + super.getMethod().hasName(["contains", "indexOf"]) and + super.getAnArgument().(CompileTimeConstantExpr).getStringValue() = "%" } Expr getCheckedExpr() { result = super.getQualifier() }