Java: copy files from experimental

Author: Jami Cogswell

java/ql/lib/ext/unsafeUrlForwardExperimentalMove.model.yml

@@ -0,0 +1,61 @@
+extensions:
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sourceModel
+    data:
+      - ["jakarta.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual"]
+      - ["javax.servlet.http", "HttpServletRequest", True, "getServletPath", "", "", "ReturnValue", "remote", "manual"]
+
+      # # ! below added by me when debugging CVEs:
+      # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "retrieve", "(String,String,String,ServletWebRequest,boolean)", "", "Parameter[3]", "remote", "manual"]
+      # - ["org.springframework.web.context.request", "ServletWebRequest", True, "getContextPath", "()", "", "ReturnValue", "remote", "manual"]
+
+  - addsTo:
+      pack: codeql/java-all
+      extensible: sinkModel
+    data:
+      - ["java.util.concurrent", "TimeUnit", True, "sleep", "", "", "Argument[0]", "thread-pause", "manual"] # ! this seems like a typo; doesn't look like it's used in the query at all
+
+      - ["org.springframework.core.io", "ClassPathResource", True, "getFilename", "", "", "Argument[this]", "get-resource", "manual"] # ! Note: `ClassPathResource` implements `Resource`, so it might make more sense to model some of these as `Resource` with subtype True.
+      - ["org.springframework.core.io", "ClassPathResource", True, "getPath", "", "", "Argument[this]", "get-resource", "manual"]
+      - ["org.springframework.core.io", "ClassPathResource", True, "getURL", "", "", "Argument[this]", "get-resource", "manual"]
+      - ["org.springframework.core.io", "ClassPathResource", True, "resolveURL", "", "", "Argument[this]", "get-resource", "manual"]
+      # # ! below added by me when debugging CVEs:
+      # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "retrieve", "", "", "Argument[0]", "get-resource", "manual"] # don't need
+      # # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "getFilePath", "", "", "Argument[0..3]", "get-resource", "manual"] # don't need
+      # # - ["org.springframework.cloud.config.server.resource", "ResourceRepository", True, "findOne", "", "", "Argument[0..3]", "get-resource", "manual"] # convert to summary
+      # # - ["org.springframework.core.io", "InputStreamSource", True, "getInputStream", "", "", "Argument[this]", "get-resource", "manual"] # convert to summary
+      # - ["org.springframework.util", "StreamUtils", True, "copyToString", "", "", "Argument[0]", "get-resource", "manual"] # * public class with good docs
+      # # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator", True, "getLocations", "(String,String,String)", "", "Argument[0..2]", "get-resource", "manual"] # convert to summary
+      # # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator$Locations", True, "getLocations", "()", "", "Argument[this]", "get-resource", "manual"] # convert to summary
+      # - ["org.springframework.core.io", "ResourceLoader", True, "getResource", "", "", "Argument[0]", "get-resource", "manual"] # * public interface with good docs, might be problematic for FPs based on fact that the ext contributor changed this to a taint step to avoid "exists" FPS (maybe there's another way to exclude those FPs though).
+      # - ["javax.servlet", "ServletContext", True, "getResource", "", "", "Argument[0]", "get-resource", "manual"]
+      # - ["javax.servlet", "ServletContext", True, "getResourceAsStream", "", "", "Argument[0]", "get-resource", "manual"]
+      # - ["javax.servlet", "ServletContext", True, "getResourcePaths", "", "", "Argument[0]", "get-resource", "manual"]
+      # - ["javax.servlet", "ServletContext", True, "getResource", "", "", "Argument[this]", "get-resource", "manual"]
+      # - ["javax.servlet", "ServletContext", True, "getResourceAsStream", "", "", "Argument[this]", "get-resource", "manual"]
+      # - ["javax.servlet", "ServletContext", True, "getResourcePaths", "", "", "Argument[this]", "get-resource", "manual"]
+      # # - ["org.apache.tomcat.util.http", "RequestUtil", True, "normalize", "", "", "Argument[0]", "get-resource", "manual"]
+
+  - addsTo:
+      pack: codeql/java-all
+      extensible: summaryModel
+    data:
+      - ["io.undertow.server.handlers.resource", "Resource", True, "getFile", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["io.undertow.server.handlers.resource", "Resource", True, "getFilePath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["io.undertow.server.handlers.resource", "Resource", True, "getPath", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! this as a taint step seems to contradict the fact that they did `ClassPathResource.getPath` as a sink for Spring...
+      - ["java.nio.file", "Path", True, "normalize", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! shouldn't this be a sanitizer instead??? Or no because WEB-INF ones don't care about normalization?
+      - ["java.nio.file", "Path", True, "resolve", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! check if this and the below are already in the default models
+      - ["java.nio.file", "Path", True, "resolve", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      - ["java.nio.file", "Path", True, "toString", "", "", "Argument[this]", "ReturnValue", "taint", "manual"]
+      - ["java.nio.file", "Paths", True, "get", "", "", "Argument[0..1]", "ReturnValue", "taint", "manual"]
+
+      - ["org.springframework.core.io", "ClassPathResource", False, "ClassPathResource", "", "", "Argument[0]", "Argument[this]", "taint", "manual"]
+      - ["org.springframework.core.io", "Resource", True, "createRelative", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      # # ! below added/modified by me when debugging CVEs:
+      - ["org.springframework.core.io", "ResourceLoader", True, "getResource", "", "", "Argument[0]", "ReturnValue", "taint", "manual"]
+      # - ["org.springframework.cloud.config.server.resource", "ResourceRepository", True, "findOne", "", "", "Argument[0..3]", "ReturnValue", "taint", "manual"] # * public interface, but might be too specific, no easily findable docs...
+      # - ["org.springframework.core.io", "InputStreamSource", True, "getInputStream", "", "", "Argument[this]", "ReturnValue", "taint", "manual"] # * public interface with good docs, Note: other `getInputStream`s are remote source and/or taint step, so this as taint step versus sink probably is more consistent
+      # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator", True, "getLocations", "(String,String,String)", "", "Argument[0..2]", "ReturnValue", "taint", "manual"] # * public interface with docs: https://www.javadoc.io/static/org.springframework.cloud/spring-cloud-config-server/2.1.0.RELEASE/org/springframework/cloud/config/server/environment/SearchPathLocator.html
+      # - ["org.springframework.cloud.config.server.environment", "SearchPathLocator$Locations", True, "getLocations", "()", "", "Argument[this]", "ReturnValue", "taint", "manual"] # ! is the `Locations` class package-private? Or does it inherit public from it's enclosing interface?
+      # - ["org.springframework.cloud.config.server.resource", "ResourceController", True, "getFilePath", "", "", "Argument[0..3]", "ReturnValue", "taint", "manual"] # don't need

java/ql/src/Security/CWE/CWE-552/UnsafeLoadSpringResource.java

@@ -0,0 +1,21 @@
+//BAD: no path validation in Spring resource loading
+@GetMapping("/file")
+public String getFileContent(@RequestParam(name="fileName") String fileName) {
+	ClassPathResource clr = new ClassPathResource(fileName);
+
+	File file = ResourceUtils.getFile(fileName);
+
+	Resource resource = resourceLoader.getResource(fileName);
+}
+
+//GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix in Spring resource loading:
+@GetMapping("/file")
+public String getFileContent(@RequestParam(name="fileName") String fileName) {
+	if (!fileName.contains("..") && fileName.hasPrefix("/public-content")) {
+		ClassPathResource clr = new ClassPathResource(fileName);
+
+		File file = ResourceUtils.getFile(fileName);
+
+		Resource resource = resourceLoader.getResource(fileName);
+	}
+}

java/ql/src/Security/CWE/CWE-552/UnsafeResourceGet.java

@@ -0,0 +1,18 @@
+// BAD: no URI validation
+URL url = request.getServletContext().getResource(requestUrl);
+url = getClass().getResource(requestUrl);
+InputStream in = url.openStream();
+
+InputStream in = request.getServletContext().getResourceAsStream(requestPath);
+in = getClass().getClassLoader().getResourceAsStream(requestPath);
+
+// 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 (!requestPath.contains("..") && requestPath.startsWith("/trusted")) {
+	InputStream in = request.getServletContext().getResourceAsStream(requestPath);
+}
+
+Path path = Paths.get(requestUrl).normalize().toRealPath();
+if (path.startsWith("/trusted")) {
+	URL url = request.getServletContext().getResource(path.toString());
+}

java/ql/src/Security/CWE/CWE-552/UnsafeServletRequestDispatch.java

@@ -0,0 +1,11 @@
+// BAD: no URI validation
+String returnURL = request.getParameter("returnURL");
+RequestDispatcher rd = sc.getRequestDispatcher(returnURL);
+rd.forward(request, response);
+
+// GOOD: check for a trusted prefix, ensuring path traversal is not used to erase that prefix:
+// (alternatively use `Path.normalize` instead of checking for `..`)
+if (!returnURL.contains("..") && returnURL.hasPrefix("/pages")) { ... }
+// Also GOOD: check for a forbidden prefix, ensuring URL-encoding is not used to evade the check:
+// (alternatively use `URLDecoder.decode` before `hasPrefix`)
+if (returnURL.hasPrefix("/internal") && !returnURL.contains("%")) { ... }

java/ql/src/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();
+		}
+	}
+}

java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.qhelp

@@ -0,0 +1,70 @@
+<!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>Unsanitized user provided data must not be used to construct the path for URL forwarding. In order to prevent
+untrusted URL forwarding, it is recommended to avoid concatenating user input directly into the forwarding URL.
+Instead, user input should be checked against allowed (e.g., must come within <code>user_content/</code>) or disallowed
+(e.g. must not come within <code>/internal</code>) paths, ensuring that neither path traversal using <code>../</code>
+or URL encoding are used to evade these checks.
+</p>
+
+</recommendation>
+<example>
+
+<p>The following examples show the bad case and the good case respectively.
+The <code>bad</code> methods show an HTTP request parameter being used directly in a URL forward
+without validating the input, which may cause file leakage. In the <code>good1</code> method,
+ordinary forwarding requests are shown, which will not cause file leakage.
+</p>
+
+<sample src="UnsafeUrlForward.java" />
+
+<p>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.
+</p>
+
+<sample src="UnsafeServletRequestDispatch.java" />
+
+<p>The following examples show an HTTP request parameter or request path being used directly to
+retrieve a resource of a Java EE application without validating the input, which allows sensitive
+file exposure attacks. It also shows how to remedy the problem by validating the user input.
+</p>
+
+<sample src="UnsafeResourceGet.java" />
+
+<p>The following examples show an HTTP request parameter being used directly to retrieve a resource
+  of a Java Spring application without validating the input, which allows sensitive file exposure
+  attacks. It also shows how to remedy the problem by validating the user input.
+  </p>
+
+  <sample src="UnsafeLoadSpringResource.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>
+<li>Jakarta Javadoc:
+  <a href="https://jakarta.ee/specifications/webprofile/9/apidocs/jakarta/servlet/servletrequest#getRequestDispatcher-java.lang.String-">Security vulnerability with unsafe usage of RequestDispatcher</a>.
+</li>
+<li>Micro Focus:
+  <a href="https://vulncat.fortify.com/en/detail?id=desc.dataflow.java.file_disclosure_j2ee">File Disclosure: J2EE</a>
+</li>
+<li>CVE-2015-5174:
+  <a href="https://vuldb.com/?id.81084">Apache Tomcat 6.0/7.0/8.0/9.0 Servletcontext getResource/getResourceAsStream/getResourcePaths Path Traversal</a>
+</li>
+<li>CVE-2019-3799:
+  <a href="https://github.com/mpgn/CVE-2019-3799">CVE-2019-3799 - Spring-Cloud-Config-Server Directory Traversal &lt; 2.1.2, 2.0.4, 1.4.6</a>
+</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-552/UnsafeUrlForward.ql

@@ -0,0 +1,63 @@
+/**
+ * @name Unsafe URL forward or include from a remote source
+ * @description URL forward or include based on unvalidated user-input
+ *              may cause file information disclosure.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/unsafe-url-forward-include
+ * @tags security
+ *       external/cwe-552
+ */
+
+import java
+import UnsafeUrlForward
+import semmle.code.java.dataflow.FlowSources
+import semmle.code.java.dataflow.TaintTracking
+import experimental.semmle.code.java.frameworks.Jsf
+import semmle.code.java.security.PathSanitizer
+import UnsafeUrlForwardFlow::PathGraph
+
+module UnsafeUrlForwardFlowConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source instanceof ThreatModelFlowSource and
+    not exists(MethodCall ma, Method m | ma.getMethod() = m |
+      (
+        m instanceof HttpServletRequestGetRequestUriMethod or
+        m instanceof HttpServletRequestGetRequestUrlMethod or
+        m instanceof HttpServletRequestGetPathMethod
+      ) and
+      ma = source.asExpr()
+    )
+  }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof UnsafeUrlForwardSink }
+
+  predicate isBarrier(DataFlow::Node node) {
+    node instanceof UnsafeUrlForwardSanitizer or
+    node instanceof PathInjectionSanitizer
+  }
+
+  DataFlow::FlowFeature getAFeature() { result instanceof DataFlow::FeatureHasSourceCallContext }
+
+  predicate isAdditionalFlowStep(DataFlow::Node prev, DataFlow::Node succ) {
+    exists(MethodCall ma |
+      (
+        ma.getMethod() instanceof GetServletResourceMethod or
+        ma.getMethod() instanceof GetFacesResourceMethod or
+        ma.getMethod() instanceof GetClassResourceMethod or
+        ma.getMethod() instanceof GetClassLoaderResourceMethod or
+        ma.getMethod() instanceof GetWildflyResourceMethod
+      ) and
+      ma.getArgument(0) = prev.asExpr() and
+      ma = succ.asExpr()
+    )
+  }
+}
+
+module UnsafeUrlForwardFlow = TaintTracking::Global<UnsafeUrlForwardFlowConfig>;
+
+from UnsafeUrlForwardFlow::PathNode source, UnsafeUrlForwardFlow::PathNode sink
+where UnsafeUrlForwardFlow::flowPath(source, sink)
+select sink.getNode(), source, sink, "Potentially untrusted URL forward due to $@.",
+  source.getNode(), "user-provided value"