diff --git a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll index 1dd4373fb548..76c82b377866 100644 --- a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll +++ b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll @@ -74,7 +74,7 @@ library class HttpServletRequestGetQueryStringMethod extends Method { /** * The method `getPathInfo()` declared in `javax.servlet.http.HttpServletRequest`. */ -library class HttpServletRequestGetPathMethod extends Method { +class HttpServletRequestGetPathMethod extends Method { HttpServletRequestGetPathMethod() { getDeclaringType() instanceof HttpServletRequest and hasName("getPathInfo") and @@ -120,7 +120,7 @@ library class HttpServletRequestGetHeaderNamesMethod extends Method { /** * The method `getRequestURL()` declared in `javax.servlet.http.HttpServletRequest`. */ -library class HttpServletRequestGetRequestURLMethod extends Method { +class HttpServletRequestGetRequestURLMethod extends Method { HttpServletRequestGetRequestURLMethod() { getDeclaringType() instanceof HttpServletRequest and hasName("getRequestURL") and @@ -131,7 +131,7 @@ library class HttpServletRequestGetRequestURLMethod extends Method { /** * The method `getRequestURI()` declared in `javax.servlet.http.HttpServletRequest`. */ -library class HttpServletRequestGetRequestURIMethod extends Method { +class HttpServletRequestGetRequestURIMethod extends Method { HttpServletRequestGetRequestURIMethod() { getDeclaringType() instanceof HttpServletRequest and hasName("getRequestURI") and @@ -191,6 +191,16 @@ class HttpServletResponseSendErrorMethod extends Method { } } +/** + * The method `getRequestDispatcher(String)` declared in `javax.servlet.http.HttpServletRequest` or `javax.servlet.ServletRequest`. + */ +class ServletRequestGetRequestDispatcherMethod extends Method { + ServletRequestGetRequestDispatcherMethod() { + getDeclaringType() instanceof ServletRequest and + hasName("getRequestDispatcher") + } +} + /** * The method `sendRedirect(String)` declared in `javax.servlet.http.HttpServletResponse`. */ diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java new file mode 100644 index 000000000000..d159c4057362 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java @@ -0,0 +1,38 @@ +import java.io.IOException; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.servlet.ModelAndView; + +@Controller +public class UnsafeUrlForward { + + @GetMapping("/bad1") + public ModelAndView bad1(String url) { + return new ModelAndView(url); + } + + @GetMapping("/bad2") + public void bad2(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response); + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + @GetMapping("/good1") + public void good1(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response); + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } +} diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp new file mode 100644 index 000000000000..572b14bb02fe --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp @@ -0,0 +1,31 @@ + + + + + +

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.

+ +
+ + +

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, +ordinary forwarding requests are shown, which will not cause file leakage. +

+ + + +
+ +
  • File Disclosure: Unsafe Url Forward.
  • +
    +
    diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql new file mode 100644 index 000000000000..f9f13f728866 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql @@ -0,0 +1,58 @@ +/** + * @name Unsafe url forward from remote source + * @description URL forward based on unvalidated user-input + * may cause file information disclosure. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/unsafe-url-forward + * @tags security + * external/cwe-552 + */ + +import java +import UnsafeUrlForward +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.frameworks.Servlets +import DataFlow::PathGraph + +private class StartsWithSanitizer extends DataFlow::BarrierGuard { + StartsWithSanitizer() { + this.(MethodAccess).getMethod().hasName("startsWith") and + this.(MethodAccess).getMethod().getDeclaringType() instanceof TypeString and + this.(MethodAccess).getMethod().getNumberOfParameters() = 1 + } + + override predicate checks(Expr e, boolean branch) { + e = this.(MethodAccess).getQualifier() and branch = true + } +} + +class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration { + UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" } + + override predicate isSource(DataFlow::Node source) { + source instanceof RemoteFlowSource and + not exists(MethodAccess ma, Method m | ma.getMethod() = m | + ( + m instanceof HttpServletRequestGetRequestURIMethod or + m instanceof HttpServletRequestGetRequestURLMethod or + m instanceof HttpServletRequestGetPathMethod + ) and + ma = source.asExpr() + ) + } + + 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 } +} + +from DataFlow::PathNode source, DataFlow::PathNode sink, UnsafeUrlForwardFlowConfig conf +where conf.hasFlowPath(source, sink) +select sink.getNode(), source, sink, "Potentially untrusted URL forward due to $@.", + source.getNode(), "user-provided value" diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll new file mode 100644 index 000000000000..f26563162035 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -0,0 +1,72 @@ +import java +import DataFlow +import semmle.code.java.dataflow.FlowSources +import semmle.code.java.frameworks.Servlets +import semmle.code.java.frameworks.spring.SpringWeb +import semmle.code.java.security.RequestForgery +private import semmle.code.java.dataflow.StringPrefixes + +/** A sanitizer for unsafe url forward vulnerabilities. */ +abstract class UnsafeUrlForwardSanitizer extends DataFlow::Node { } + +private class PrimitiveSanitizer extends UnsafeUrlForwardSanitizer { + PrimitiveSanitizer() { + this.getType() instanceof PrimitiveType or + this.getType() instanceof BoxedType or + this.getType() instanceof NumberType + } +} + +private class SanitizingPrefix extends InterestingPrefix { + SanitizingPrefix() { + not this.getStringValue().matches("/WEB-INF/%") and + not this.getStringValue() = "forward:" + } + + override int getOffset() { result = 0 } +} + +private class FollowsSanitizingPrefix extends UnsafeUrlForwardSanitizer { + FollowsSanitizingPrefix() { this.asExpr() = any(SanitizingPrefix fp).getAnAppendedExpression() } +} + +abstract class UnsafeUrlForwardSink extends DataFlow::Node { } + +/** An argument to `ServletRequest.getRequestDispatcher`. */ +private class RequestDispatcherSink extends UnsafeUrlForwardSink { + RequestDispatcherSink() { + exists(MethodAccess ma | + ma.getMethod() instanceof ServletRequestGetRequestDispatcherMethod 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 ForwardPrefix extends InterestingPrefix { + ForwardPrefix() { this.getStringValue() = "forward:" } + + override int getOffset() { result = 0 } +} + +/** + * An expression appended (perhaps indirectly) to `"forward:"`, and which + * is reachable from a Spring entry point. + */ +private class SpringUrlForwardSink extends UnsafeUrlForwardSink { + SpringUrlForwardSink() { + any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable()) and + this.asExpr() = any(ForwardPrefix fp).getAnAppendedExpression() + } +} 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 new file mode 100644 index 000000000000..b70a131f93c1 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected @@ -0,0 +1,31 @@ +edges +| 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 | +| UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:48:31:63 | ... + ... | +| 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 | ... + ... | +nodes +| 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 | +| UnsafeUrlForward.java:20:28:20:30 | url | semmle.label | url | +| UnsafeUrlForward.java:25:21:25:30 | url : String | semmle.label | url : String | +| UnsafeUrlForward.java:26:23:26:25 | url | semmle.label | url | +| UnsafeUrlForward.java:30:27:30:36 | url : String | semmle.label | url : String | +| UnsafeUrlForward.java:31:48:31:63 | ... + ... | semmle.label | ... + ... | +| UnsafeUrlForward.java:31:61:31:63 | url | semmle.label | url | +| UnsafeUrlForward.java:36:19:36:28 | url : String | semmle.label | url : String | +| 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 | ... + ... | +subpaths +#select +| 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 | +| UnsafeUrlForward.java:31:48:31:63 | ... + ... | UnsafeUrlForward.java:30:27:30:36 | url : String | UnsafeUrlForward.java:31:48:31:63 | ... + ... | Potentially untrusted URL forward due to $@. | UnsafeUrlForward.java:30:27:30:36 | url | user-provided value | +| 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 | 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 new file mode 100644 index 000000000000..1e7ce3a97c52 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java @@ -0,0 +1,67 @@ +import java.io.IOException; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.GetMapping; +import org.springframework.web.servlet.ModelAndView; + +@Controller +public class UnsafeUrlForward { + + @GetMapping("/bad1") + public ModelAndView bad1(String url) { + return new ModelAndView(url); + } + + @GetMapping("/bad2") + public ModelAndView bad2(String url) { + ModelAndView modelAndView = new ModelAndView(); + modelAndView.setViewName(url); + return modelAndView; + } + + @GetMapping("/bad3") + public String bad3(String url) { + return "forward:" + url + "/swagger-ui/index.html"; + } + + @GetMapping("/bad4") + public ModelAndView bad4(String url) { + ModelAndView modelAndView = new ModelAndView("forward:" + url); + return modelAndView; + } + + @GetMapping("/bad5") + public void bad5(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher(url).include(request, response); + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + @GetMapping("/bad6") + public void bad6(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher("/WEB-INF/jsp/" + url + ".jsp").include(request, response); + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } + + @GetMapping("/good1") + public void good1(String url, HttpServletRequest request, HttpServletResponse response) { + try { + request.getRequestDispatcher("/index.jsp?token=" + url).forward(request, response); + } catch (ServletException e) { + e.printStackTrace(); + } catch (IOException e) { + e.printStackTrace(); + } + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref new file mode 100644 index 000000000000..2e4cb5e726a9 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql \ No newline at end of file diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/options b/java/ql/test/experimental/query-tests/security/CWE-552/options new file mode 100644 index 000000000000..ba166b547a02 --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-552/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/ \ No newline at end of file