From d0eec1e3813962313f2d417a84196d37ad5da6ed Mon Sep 17 00:00:00 2001
From: haby0 <ha_yub@163.com>
Date: Thu, 8 Jul 2021 19:40:42 +0800
Subject: [PATCH] Add CWE-552-UnsafeUrlForward

---
 .../semmle/code/java/frameworks/Servlets.qll  | 12 +++
 .../CWE/CWE-552/UnsafeUrlForward.java         | 67 ++++++++++++++++
 .../CWE/CWE-552/UnsafeUrlForward.qhelp        | 32 ++++++++
 .../Security/CWE/CWE-552/UnsafeUrlForward.ql  | 44 +++++++++++
 .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 76 +++++++++++++++++++
 .../CWE-552/UnsafeUrlForward.expected         | 30 ++++++++
 .../security/CWE-552/UnsafeUrlForward.java    | 67 ++++++++++++++++
 .../security/CWE-552/UnsafeUrlForward.qlref   |  1 +
 .../query-tests/security/CWE-552/options      |  1 +
 9 files changed, 330 insertions(+)
 create mode 100644 java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.java
 create mode 100644 java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp
 create mode 100644 java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
 create mode 100644 java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
 create mode 100644 java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected
 create mode 100644 java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.java
 create mode 100644 java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.qlref
 create mode 100644 java/ql/test/experimental/query-tests/security/CWE-552/options

diff --git a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll
index 1dd4373fb54..f68e606124d 100644
--- a/java/ql/lib/semmle/code/java/frameworks/Servlets.qll
+++ b/java/ql/lib/semmle/code/java/frameworks/Servlets.qll
@@ -191,6 +191,18 @@ 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") and
+    getNumberOfParameters() = 1 and
+    getParameter(0).getType() instanceof TypeString
+  }
+}
+
 /**
  * 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 00000000000..1e7ce3a97c5
--- /dev/null
+++ b/java/ql/src/experimental/Security/CWE/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/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp
new file mode 100644
index 00000000000..b220636885d
--- /dev/null
+++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp
@@ -0,0 +1,32 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p>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.</p>
+
+</overview>
+<recommendation>
+
+<p>In order to prevent untrusted URL forwarding, it is recommended to avoid entering user input directly into the forwarding URL.</p>
+
+</recommendation>
+<example>
+
+<p>The following examples show the bad case and the good case respectively.
+In <code>bad1</code> method and <code>bad2</code> method and <code>bad3</code> method and 
+<code>bad4</code> method and <code>bad5</code> method and <code>bad6</code> method, shows an HTTP request parameter being used directly in a URL forward
+without validating the input, which may cause file leakage. In <code>good1</code> method, 
+ordinary forwarding requests are shown, which will not cause file leakage.
+</p>
+
+<sample src="UnsafeUrlForward.java" />
+
+</example>
+<references>
+<li>File Disclosure: <a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_spring">Unsafe Url Forward</a>.</li>
+</references>
+</qhelp>
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 00000000000..704d0f92a06
--- /dev/null
+++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql
@@ -0,0 +1,44 @@
+/**
+ * @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 DataFlow::PathGraph
+
+class UnsafeUrlForwardFlowConfig extends TaintTracking::Configuration {
+  UnsafeUrlForwardFlowConfig() { this = "UnsafeUrlForwardFlowConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink }
+
+  override predicate isSanitizer(DataFlow::Node node) {
+    node.getType() instanceof BoxedType
+    or
+    node.getType() instanceof PrimitiveType
+    or
+    exists(AddExpr ae |
+      ae.getRightOperand() = node.asExpr() and
+      (
+        not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue().matches("/WEB-INF/%")
+        and
+        not ae.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:"
+      )
+    )
+  }
+}
+
+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 00000000000..fb1136369c0
--- /dev/null
+++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll
@@ -0,0 +1,76 @@
+import java
+import DataFlow
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.frameworks.Servlets
+
+/**
+ * A concatenate expression using the string `forward:` on the left.
+ *
+ * E.g: `"forward:" + url`
+ */
+class ForwardBuilderExpr extends AddExpr {
+  ForwardBuilderExpr() {
+    this.getLeftOperand().(CompileTimeConstantExpr).getStringValue() = "forward:"
+  }
+}
+
+/**
+ * A call to `StringBuilder.append` or `StringBuffer.append` method, and the parameter value is `"forward:"`.
+ *
+ * E.g: `StringBuilder.append("forward:")`
+ */
+class ForwardAppendCall extends MethodAccess {
+  ForwardAppendCall() {
+    this.getMethod().hasName("append") and
+    this.getMethod().getDeclaringType() instanceof StringBuildingType and
+    this.getArgument(0).(CompileTimeConstantExpr).getStringValue() = "forward:"
+  }
+}
+
+abstract class UnsafeUrlForwardSink extends DataFlow::Node { }
+
+/** A Unsafe url forward sink from getRequestDispatcher method. */
+private class RequestDispatcherSink extends UnsafeUrlForwardSink {
+  RequestDispatcherSink() {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof ServletRequestGetRequestDispatcherMethod and
+      ma.getArgument(0) = this.asExpr()
+    )
+  }
+}
+
+/** A Unsafe url forward sink from spring controller method. */
+private class SpringUrlForwardSink extends UnsafeUrlForwardSink {
+  SpringUrlForwardSink() {
+    exists(ForwardBuilderExpr rbe |
+      rbe.getRightOperand() = this.asExpr() and
+      any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
+    )
+    or
+    exists(MethodAccess ma, ForwardAppendCall rac |
+      DataFlow2::localExprFlow(rac.getQualifier(), ma.getQualifier()) and
+      ma.getMethod().hasName("append") and
+      ma.getArgument(0) = this.asExpr() and
+      any(SpringRequestMappingMethod sqmm).polyCalls*(this.getEnclosingCallable())
+    )
+    or
+    exists(ClassInstanceExpr cie |
+      cie.getConstructedType().hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and
+      (
+        exists(ForwardBuilderExpr rbe |
+          rbe = cie.getArgument(0) and rbe.getRightOperand() = this.asExpr()
+        )
+        or
+        cie.getArgument(0) = this.asExpr()
+      )
+    )
+    or
+    exists(MethodAccess ma |
+      ma.getMethod().hasName("setViewName") and
+      ma.getMethod()
+          .getDeclaringType()
+          .hasQualifiedName("org.springframework.web.servlet", "ModelAndView") and
+      ma.getArgument(0) = this.asExpr()
+    )
+  }
+}
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 00000000000..5b9c80ff598
--- /dev/null
+++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected
@@ -0,0 +1,30 @@
+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 | ... + ... |
+#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 00000000000..1e7ce3a97c5
--- /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 00000000000..2e4cb5e726a
--- /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 00000000000..a9289108747
--- /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.2.3/
\ No newline at end of file