Merge pull request #9199 from luchua-bc/java/unsafe-url-forward-dispatch-load

Java: CWE-552 Query to detect unsafe resource loading in Java Spring applications

java/ql/src/experimental/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/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp

@@ -43,6 +43,12 @@ file exposure attacks. It also shows how to remedy the problem by validating the
 
 <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:
@@ -57,5 +63,8 @@ file exposure attacks. It also shows how to remedy the problem by validating the
 <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/experimental/Security/CWE/CWE-552/UnsafeUrlForward.ql

@@ -1,11 +1,11 @@
 /**
- * @name Unsafe URL forward or dispatch from remote source
- * @description URL forward or dispatch based on unvalidated user-input
+ * @name Unsafe URL forward, dispatch, or load from remote source
+ * @description URL forward, dispatch, or load based on unvalidated user-input
  *              may cause file information disclosure.
  * @kind path-problem
  * @problem.severity error
  * @precision high
- * @id java/unsafe-url-forward-dispatch
+ * @id java/unsafe-url-forward-dispatch-load
  * @tags security
  *       external/cwe-552
  */

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

@@ -4,6 +4,7 @@ private import semmle.code.java.dataflow.ExternalFlow
 private import semmle.code.java.dataflow.FlowSources
 private import semmle.code.java.dataflow.StringPrefixes
 private import semmle.code.java.frameworks.javaee.ejb.EJBRestrictions
+private import experimental.semmle.code.java.frameworks.SpringResource
 
 /** A sink for unsafe URL forward vulnerabilities. */
 abstract class UnsafeUrlForwardSink extends DataFlow::Node { }
@@ -86,6 +87,8 @@ private class GetResourceSink extends UnsafeUrlForwardSink {
   GetResourceSink() {
     sinkNode(this, "open-url")
     or
+    sinkNode(this, "get-resource")
+    or
     exists(MethodAccess ma |
       (
         ma.getMethod() instanceof GetServletResourceAsStreamMethod or
@@ -99,6 +102,16 @@ private class GetResourceSink extends UnsafeUrlForwardSink {
   }
 }
 
+/** A sink for methods that load Spring resources. */
+private class SpringResourceSink extends UnsafeUrlForwardSink {
+  SpringResourceSink() {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof GetResourceUtilsMethod and
+      ma.getArgument(0) = this.asExpr()
+    )
+  }
+}
+
 /** An argument to `new ModelAndView` or `ModelAndView.setViewName`. */
 private class SpringModelAndViewSink extends UnsafeUrlForwardSink {
   SpringModelAndViewSink() {
@@ -175,3 +188,25 @@ private class FilePathFlowStep extends SummaryModelCsv {
       ]
   }
 }
+
+/** Taint models related to resource loading in Spring. */
+private class LoadSpringResourceFlowStep extends SummaryModelCsv {
+  override predicate row(string row) {
+    row =
+      [
+        "org.springframework.core.io;ClassPathResource;false;ClassPathResource;;;Argument[0];Argument[-1];taint;manual",
+        "org.springframework.core.io;ResourceLoader;true;getResource;;;Argument[0];ReturnValue;taint;manual",
+        "org.springframework.core.io;Resource;true;createRelative;;;Argument[0];ReturnValue;taint;manual"
+      ]
+  }
+}
+
+/** Sink models for methods that load Spring resources. */
+private class SpringResourceCsvSink extends SinkModelCsv {
+  override predicate row(string row) {
+    row =
+      // Get spring resource
+      "org.springframework.core.io;ClassPathResource;true;" +
+        ["getFilename", "getPath", "getURL", "resolveURL"] + ";;;Argument[-1];get-resource;manual"
+  }
+}

java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll

@@ -0,0 +1,22 @@
+/**
+ * Provides classes for working with resource loading in Spring.
+ */
+
+import java
+private import semmle.code.java.dataflow.ExternalFlow
+private import semmle.code.java.dataflow.FlowSources
+
+/** A utility class for resolving resource locations to files in the file system in the Spring framework. */
+class ResourceUtils extends Class {
+  ResourceUtils() { this.hasQualifiedName("org.springframework.util", "ResourceUtils") }
+}
+
+/**
+ * A method declared in `org.springframework.util.ResourceUtils` that loads Spring resources.
+ */
+class GetResourceUtilsMethod extends Method {
+  GetResourceUtilsMethod() {
+    this.getDeclaringType().getASupertype*() instanceof ResourceUtils and
+    this.hasName(["extractArchiveURL", "extractJarFileURL", "getFile", "getURL"])
+  }
+}

java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java

@@ -0,0 +1,155 @@
+package com.example;
+
+import java.io.File;
+import java.io.FileReader;
+import java.io.InputStreamReader;
+import java.io.IOException;
+import java.io.Reader;
+import java.io.UnsupportedEncodingException;
+import java.net.URLDecoder;
+import java.nio.file.Files;
+
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.core.io.ClassPathResource;
+import org.springframework.core.io.Resource;
+import org.springframework.core.io.ResourceLoader;
+import org.springframework.util.ResourceUtils;
+import org.springframework.util.StringUtils;
+import org.springframework.web.bind.annotation.GetMapping;
+import org.springframework.web.bind.annotation.RequestParam;
+import org.springframework.web.bind.annotation.RestController;
+
+/** Sample class of Spring RestController */
+@RestController
+public class UnsafeLoadSpringResource {
+	@GetMapping("/file1")
+	//BAD: Get resource from ClassPathResource without input validation
+	public String getFileContent1(@RequestParam(name="fileName") String fileName) {
+		// A request such as the following can disclose source code and application configuration
+		// fileName=/../../WEB-INF/views/page.jsp
+		// fileName=/com/example/package/SampleController.class
+		ClassPathResource clr = new ClassPathResource(fileName);
+		char[] buffer = new char[4096];
+		StringBuilder out = new StringBuilder();
+		try {
+			Reader in = new FileReader(clr.getFilename());
+			for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) {
+				out.append(buffer, 0, numRead);
+			}
+		} catch (IOException ie) {
+			ie.printStackTrace();
+		}
+		return out.toString();
+	}
+
+	@GetMapping("/file1a")
+	//GOOD: Get resource from ClassPathResource with input path validation
+	public String getFileContent1a(@RequestParam(name="fileName") String fileName) {
+		String result = null;
+		if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) {
+			ClassPathResource clr = new ClassPathResource(fileName);
+			char[] buffer = new char[4096];
+			StringBuilder out = new StringBuilder();
+			try {
+				Reader in = new InputStreamReader(clr.getInputStream(), "UTF-8");
+				for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) {
+					out.append(buffer, 0, numRead);
+				}
+			} catch (IOException ie) {
+				ie.printStackTrace();
+			}
+			result = out.toString();
+		}
+		return result;
+	}
+
+	@GetMapping("/file2")
+	//BAD: Get resource from ResourceUtils without input validation
+	public String getFileContent2(@RequestParam(name="fileName") String fileName) {
+		String content = null;
+		
+		try {
+			// A request such as the following can disclose source code and system configuration
+			// fileName=/etc/hosts
+			// fileName=file:/etc/hosts
+			// fileName=/opt/appdir/WEB-INF/views/page.jsp
+			File file = ResourceUtils.getFile(fileName);
+			//Read File Content
+			content = new String(Files.readAllBytes(file.toPath()));
+		} catch (IOException ie) {
+			ie.printStackTrace();
+		}
+		return content;
+	}
+
+	@GetMapping("/file2a")
+	//GOOD: Get resource from ResourceUtils with input path validation
+	public String getFileContent2a(@RequestParam(name="fileName") String fileName) {
+		String content = null;
+		
+		if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) {
+			try {
+					File file = ResourceUtils.getFile(fileName);
+				//Read File Content
+				content = new String(Files.readAllBytes(file.toPath()));
+			} catch (IOException ie) {
+				ie.printStackTrace();
+			}
+		}
+		return content;
+	}
+
+	@Autowired
+	ResourceLoader resourceLoader;
+
+	@GetMapping("/file3")
+	//BAD: Get resource from ResourceLoader (same as application context) without input validation
+	// Note it is not detected without the generic `resource.getInputStream()` check
+	public String getFileContent3(@RequestParam(name="fileName") String fileName) {
+		String content = null;
+
+		try {
+			// A request such as the following can disclose source code and system configuration
+			// fileName=/WEB-INF/views/page.jsp
+			// fileName=/WEB-INF/classes/com/example/package/SampleController.class
+			// fileName=file:/etc/hosts
+			Resource resource = resourceLoader.getResource(fileName);
+
+			char[] buffer = new char[4096];
+			StringBuilder out = new StringBuilder();
+
+			Reader in = new InputStreamReader(resource.getInputStream(), "UTF-8");
+			for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) {
+				out.append(buffer, 0, numRead);
+			}
+			content = out.toString();
+		} catch (IOException ie) {
+			ie.printStackTrace();
+		}
+		return content;
+	}
+
+	@GetMapping("/file3a")
+	//GOOD: Get resource from ResourceLoader (same as application context) with input path validation
+	public String getFileContent3a(@RequestParam(name="fileName") String fileName) {
+		String content = null;
+
+		if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) {
+			try {
+				Resource resource = resourceLoader.getResource(fileName);
+
+				char[] buffer = new char[4096];
+				StringBuilder out = new StringBuilder();
+
+				Reader in = new InputStreamReader(resource.getInputStream(), "UTF-8");
+				for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) {
+					out.append(buffer, 0, numRead);
+				}
+				content = out.toString();
+			} catch (IOException ie) {
+				ie.printStackTrace();
+			}
+		}
+		return content;
+	}
+}

java/ql/test/experimental/query-tests/security/CWE-552/UnsafeUrlForward.expected

@@ -1,4 +1,8 @@
 edges
+| UnsafeLoadSpringResource.java:27:32:27:77 | fileName : String | UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String |
+| UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource | UnsafeLoadSpringResource.java:35:31:35:33 | clr |
+| UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String | UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource |
+| UnsafeLoadSpringResource.java:68:32:68:77 | fileName : String | UnsafeLoadSpringResource.java:76:38:76:45 | fileName |
 | UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | UnsafeRequestPath.java:23:33:23:36 | path |
 | UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:17:20:17:25 | params : Map |
 | UnsafeResourceGet2.java:17:20:17:25 | params : Map | UnsafeResourceGet2.java:17:20:17:40 | get(...) : Object |
@@ -30,6 +34,12 @@ edges
 | 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
+| UnsafeLoadSpringResource.java:27:32:27:77 | fileName : String | semmle.label | fileName : String |
+| UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource | semmle.label | new ClassPathResource(...) : ClassPathResource |
+| UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String | semmle.label | fileName : String |
+| UnsafeLoadSpringResource.java:35:31:35:33 | clr | semmle.label | clr |
+| UnsafeLoadSpringResource.java:68:32:68:77 | fileName : String | semmle.label | fileName : String |
+| UnsafeLoadSpringResource.java:76:38:76:45 | fileName | semmle.label | fileName |
 | UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | semmle.label | getServletPath(...) : String |
 | UnsafeRequestPath.java:23:33:23:36 | path | semmle.label | path |
 | UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | semmle.label | getRequestParameterMap(...) : Map |
@@ -82,6 +92,8 @@ nodes
 | UnsafeUrlForward.java:60:33:60:62 | ... + ... | semmle.label | ... + ... |
 subpaths
 #select
+| UnsafeLoadSpringResource.java:35:31:35:33 | clr | UnsafeLoadSpringResource.java:27:32:27:77 | fileName : String | UnsafeLoadSpringResource.java:35:31:35:33 | clr | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:27:32:27:77 | fileName | user-provided value |
+| UnsafeLoadSpringResource.java:76:38:76:45 | fileName | UnsafeLoadSpringResource.java:68:32:68:77 | fileName : String | UnsafeLoadSpringResource.java:76:38:76:45 | fileName | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:68:32:68:77 | fileName | user-provided value |
 | 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 |
 | UnsafeResourceGet2.java:19:93:19:99 | loadUrl | UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:19:93:19:99 | loadUrl | Potentially untrusted URL forward due to $@. | UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) | user-provided value |
 | UnsafeResourceGet2.java:37:20:37:22 | url | UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:37:20:37:22 | url | Potentially untrusted URL forward due to $@. | UnsafeResourceGet2.java:32:32:32:79 | getRequestParameterMap(...) | user-provided value |

java/ql/test/experimental/query-tests/security/CWE-552/options

@@ -1 +1 @@
-//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/:${testdir}/../../../../stubs/javax-faces-2.3/:${testdir}/../../../../stubs/undertow-io-2.2/:${testdir}/../../../../stubs/jboss-vfs-3.2/
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../../stubs/servlet-api-2.4:${testdir}/../../../../stubs/springframework-5.3.8/:${testdir}/../../../../stubs/javax-faces-2.3/:${testdir}/../../../../stubs/undertow-io-2.2/:${testdir}/../../../../stubs/jboss-vfs-3.2/:${testdir}/../../../../stubs/springframework-5.3.8/

java/ql/test/stubs/springframework-5.3.8/org/springframework/core/io/ClassPathResource.java

@@ -0,0 +1,53 @@
+// Stub class from org.springframework.core.io.ClassPathResource for testing purposes
+package org.springframework.core.io;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URL;
+
+public class ClassPathResource {
+    public ClassPathResource(String path) {
+    }
+
+    public ClassPathResource(String path, ClassLoader classLoader) {
+    }
+
+    public ClassPathResource(String path, Class<?> clazz) {
+    }
+
+    public final String getPath() {
+        return null;
+    }
+
+    public final ClassLoader getClassLoader() {
+        return null;
+    }
+
+    public boolean exists() {
+        return false;
+    }
+
+    public boolean isReadable() {
+        return false;
+    }
+
+    protected URL resolveURL() {
+        return null;
+    }
+
+    public InputStream getInputStream() throws IOException {
+        return null;
+    }
+
+    public URL getURL() throws IOException {
+        return null;
+    }
+
+    public Resource createRelative(String relativePath) {
+        return null;
+    }
+
+    public String getFilename() {
+        return null;
+    }
+}

java/ql/test/stubs/springframework-5.3.8/org/springframework/core/io/ResourceLoader.java

@@ -1,3 +1,7 @@
 package org.springframework.core.io;
 
-public interface ResourceLoader {}
+public interface ResourceLoader {
+    Resource getResource(String location);
+
+    ClassLoader getClassLoader();
+}