From 311c9e47191a06b905c09cd448fef4de565a92a0 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 17 May 2022 15:31:34 +0000 Subject: [PATCH 1/6] Query to detect unsafe resource loading in Java Spring applications --- .../CWE/CWE-552/UnsafeLoadSpringResource.java | 21 ++ .../CWE/CWE-552/UnsafeUrlForward.qhelp | 9 + .../Security/CWE/CWE-552/UnsafeUrlForward.ql | 6 +- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 31 ++ .../code/java/frameworks/SpringResource.qll | 62 ++++ .../CWE-552/UnsafeLoadSpringResource.java | 270 ++++++++++++++++++ .../CWE-552/UnsafeUrlForward.expected | 20 ++ .../query-tests/security/CWE-552/options | 2 +- .../core/io/ClassPathResource.java | 53 ++++ .../core/io/ResourceLoader.java | 6 +- 10 files changed, 475 insertions(+), 5 deletions(-) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-552/UnsafeLoadSpringResource.java create mode 100644 java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll create mode 100644 java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java create mode 100644 java/ql/test/stubs/springframework-5.3.8/org/springframework/core/io/ClassPathResource.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeLoadSpringResource.java b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeLoadSpringResource.java new file mode 100644 index 000000000000..ce462fe490ef --- /dev/null +++ b/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); + } +} 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 b1bcf6350c3c..2e425952edc3 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qhelp +++ b/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 +

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

+ +
  • File Disclosure: @@ -57,5 +63,8 @@ file exposure attacks. It also shows how to remedy the problem by validating the
  • CVE-2015-5174: Apache Tomcat 6.0/7.0/8.0/9.0 Servletcontext getResource/getResourceAsStream/getResourcePaths Path Traversal
    • +
  • CVE-2019-3799: + CVE-2019-3799 - Spring-Cloud-Config-Server Directory Traversal < 2.1.2, 2.0.4, 1.4.6 +
    •  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 f7e7545d068f..138f07455f1f 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 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 */ 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 8f96700b46b0..20481ca7b5f6 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/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 { } @@ -99,6 +100,22 @@ private class GetResourceSink extends UnsafeUrlForwardSink { } } +/** Sink of Spring resource loading. */ +private class SpringResourceSink extends UnsafeUrlForwardSink { + SpringResourceSink() { + exists(MethodAccess ma | + ( + ma.getMethod() instanceof GetClassPathResourceInputStreamMethod or + ma.getMethod() instanceof GetResourceMethod + ) and + ma.getQualifier() = this.asExpr() + or + 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 +192,17 @@ private class FilePathFlowStep extends SummaryModelCsv { ] } } + +/** Taint model 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", + "org.springframework.core.io;ClassPathResource;true;" + + ["getFilename", "getPath", "getURL", "resolveURL"] + ";;;Argument[-1];ReturnValue;value", + "org.springframework.core.io;ResourceLoader;true;getResource;;;Argument[0];ReturnValue;taint", + "org.springframework.core.io;Resource;true;createRelative;;;Argument[0];ReturnValue;taint" + ] + } +} diff --git a/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll b/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll new file mode 100644 index 000000000000..ddcca22aa0fe --- /dev/null +++ b/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll @@ -0,0 +1,62 @@ +/** + * 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 class for class path resources in the Spring framework. */ +class ClassPathResource extends Class { + ClassPathResource() { this.hasQualifiedName("org.springframework.core.io", "ClassPathResource") } +} + +/** An interface for objects that are sources for an InputStream in the Spring framework. */ +class InputStreamResource extends RefType { + InputStreamResource() { + this.hasQualifiedName("org.springframework.core.io", "InputStreamSource") + } +} + +/** An interface that abstracts from the underlying resource, such as a file or class path resource in the Spring framework. */ +class Resource extends RefType { + Resource() { this.hasQualifiedName("org.springframework.core.io", "Resource") } +} + +/** 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") } +} + +/** + * The method `getInputStream()` declared in Spring `ClassPathResource`. + */ +class GetClassPathResourceInputStreamMethod extends Method { + GetClassPathResourceInputStreamMethod() { + this.getDeclaringType().getASupertype*() instanceof ClassPathResource and + this.hasName("getInputStream") + } +} + +/** + * Resource loading method declared in Spring `Resource` with `getInputStream` inherited from the parent interface. + */ +class GetResourceMethod extends Method { + GetResourceMethod() { + ( + this.getDeclaringType() instanceof InputStreamResource or + this.getDeclaringType() instanceof Resource + ) and + this.hasName(["getFile", "getFilename", "getInputStream", "getURI", "getURL"]) + } +} + +/** + * Resource loading method declared in Spring `ResourceUtils`. + */ +class GetResourceUtilsMethod extends Method { + GetResourceUtilsMethod() { + this.getDeclaringType().getASupertype*() instanceof ResourceUtils and + this.hasName(["extractArchiveURL", "extractJarFileURL", "getFile", "getURL", "toURI"]) + } +} diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java new file mode 100644 index 000000000000..37ce866fd4db --- /dev/null +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java @@ -0,0 +1,270 @@ +package com.example; + +import java.io.File; +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 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(); + } + 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 (!isInvalidPath(fileName) && !isInvalidEncodedPath(fileName)) { + 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 (!isInvalidPath(fileName) && !isInvalidEncodedPath(fileName)) { + 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 + 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 (!isInvalidPath(fileName) && !isInvalidEncodedPath(fileName)) { + 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; + } + + /** + * Check whether the given path contains invalid escape sequences. + * @param path the path to validate + * @return {@code true} if the path is invalid, {@code false} otherwise + * @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 + */ + private boolean isInvalidEncodedPath(String path) { + if (path.contains("%")) { + try { + // Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars + String decodedPath = URLDecoder.decode(path, "UTF-8"); + if (isInvalidPath(decodedPath)) { + return true; + } + decodedPath = processPath(decodedPath); + if (isInvalidPath(decodedPath)) { + return true; + } + } catch (IllegalArgumentException | UnsupportedEncodingException ex) { + // Should never happen... + } + } + return false; + } + + /** + * Process the given resource path. + *

    The default implementation replaces: + *

      + *
    • Backslash with forward slash. + *
    • Duplicate occurrences of slash with a single slash. + *
    • Any combination of leading slash and control characters (00-1F and 7F) + * with a single "/" or "". For example {@code " / // foo/bar"} + * becomes {@code "/foo/bar"}. + *
    + * @since 3.2.12 + * @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 + */ + protected String processPath(String path) { + path = StringUtils.replace(path, "\\", "/"); + path = cleanDuplicateSlashes(path); + return cleanLeadingSlash(path); + } + + /** @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 **/ + private String cleanDuplicateSlashes(String path) { + StringBuilder sb = null; + char prev = 0; + for (int i = 0; i < path.length(); i++) { + char curr = path.charAt(i); + try { + if ((curr == '/') && (prev == '/')) { + if (sb == null) { + sb = new StringBuilder(path.substring(0, i)); + } + continue; + } + if (sb != null) { + sb.append(path.charAt(i)); + } + } finally { + prev = curr; + } + } + return sb != null ? sb.toString() : path; + } + + /** @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 **/ + private String cleanLeadingSlash(String path) { + boolean slash = false; + for (int i = 0; i < path.length(); i++) { + if (path.charAt(i) == '/') { + slash = true; + } + else if (path.charAt(i) > ' ' && path.charAt(i) != 127) { + if (i == 0 || (i == 1 && slash)) { + return path; + } + return (slash ? "/" + path.substring(i) : path.substring(i)); + } + } + return (slash ? "/" : ""); + } + + /** + * Identifies invalid resource paths. By default rejects: + *
      + *
    • Paths that contain "WEB-INF" or "META-INF" + *
    • Paths that contain "../" after a call to + * {@link org.springframework.util.StringUtils#cleanPath}. + *
    • Paths that represent a {@link org.springframework.util.ResourceUtils#isUrl + * valid URL} or would represent one after the leading slash is removed. + *
    + *

    Note: this method assumes that leading, duplicate '/' + * or control characters (e.g. white space) have been trimmed so that the + * path starts predictably with a single '/' or does not have one. + * @param path the path to validate + * @return {@code true} if the path is invalid, {@code false} otherwise + * @since 3.0.6 + * @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 + */ + protected boolean isInvalidPath(String path) { + if (path.contains("WEB-INF") || path.contains("META-INF")) { + return true; + } + if (path.contains(":/")) { + String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path); + if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) { + return true; + } + } + if (path.contains("..") && StringUtils.cleanPath(path).contains("../")) { + return true; + } + return false; + } +} 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 179d2b372655..ca909d137414 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,11 @@ edges +| UnsafeLoadSpringResource.java:26:32:26:77 | fileName : String | UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String | +| UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource | UnsafeLoadSpringResource.java:34:38:34:40 | clr | +| UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String | UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource | +| UnsafeLoadSpringResource.java:67:32:67:77 | fileName : String | UnsafeLoadSpringResource.java:75:38:75:45 | fileName | +| UnsafeLoadSpringResource.java:106:32:106:77 | fileName : String | UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String | +| UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource | UnsafeLoadSpringResource.java:119:38:119:45 | resource | +| UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String | UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource | | 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 +37,16 @@ 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:26:32:26:77 | fileName : String | semmle.label | fileName : String | +| UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource | semmle.label | new ClassPathResource(...) : ClassPathResource | +| UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String | semmle.label | fileName : String | +| UnsafeLoadSpringResource.java:34:38:34:40 | clr | semmle.label | clr | +| UnsafeLoadSpringResource.java:67:32:67:77 | fileName : String | semmle.label | fileName : String | +| UnsafeLoadSpringResource.java:75:38:75:45 | fileName | semmle.label | fileName | +| UnsafeLoadSpringResource.java:106:32:106:77 | fileName : String | semmle.label | fileName : String | +| UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource | semmle.label | getResource(...) : Resource | +| UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String | semmle.label | fileName : String | +| UnsafeLoadSpringResource.java:119:38:119:45 | resource | semmle.label | resource | | 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 +99,9 @@ nodes | UnsafeUrlForward.java:60:33:60:62 | ... + ... | semmle.label | ... + ... | subpaths #select +| UnsafeLoadSpringResource.java:34:38:34:40 | clr | UnsafeLoadSpringResource.java:26:32:26:77 | fileName : String | UnsafeLoadSpringResource.java:34:38:34:40 | clr | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:26:32:26:77 | fileName | user-provided value | +| UnsafeLoadSpringResource.java:75:38:75:45 | fileName | UnsafeLoadSpringResource.java:67:32:67:77 | fileName : String | UnsafeLoadSpringResource.java:75:38:75:45 | fileName | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:67:32:67:77 | fileName | user-provided value | +| UnsafeLoadSpringResource.java:119:38:119:45 | resource | UnsafeLoadSpringResource.java:106:32:106:77 | fileName : String | UnsafeLoadSpringResource.java:119:38:119:45 | resource | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:106:32:106: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 | diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/options b/java/ql/test/experimental/query-tests/security/CWE-552/options index 095b86c3e9a4..8fbf23e17dff 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/options +++ b/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/ diff --git a/java/ql/test/stubs/springframework-5.3.8/org/springframework/core/io/ClassPathResource.java b/java/ql/test/stubs/springframework-5.3.8/org/springframework/core/io/ClassPathResource.java new file mode 100644 index 000000000000..02fc6fbc2f29 --- /dev/null +++ b/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; + } +} diff --git a/java/ql/test/stubs/springframework-5.3.8/org/springframework/core/io/ResourceLoader.java b/java/ql/test/stubs/springframework-5.3.8/org/springframework/core/io/ResourceLoader.java index 0422a77c54c9..93273f56006d 100644 --- a/java/ql/test/stubs/springframework-5.3.8/org/springframework/core/io/ResourceLoader.java +++ b/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(); +} From b3572747f01ad9b9b06f144f293daf316da4ce67 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Tue, 21 Jun 2022 20:39:41 +0000 Subject: [PATCH 2/6] Simplify test case and minor update to the query --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 2 +- .../CWE-552/UnsafeLoadSpringResource.java | 125 +----------------- 2 files changed, 5 insertions(+), 122 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 20481ca7b5f6..90df69c30989 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -200,7 +200,7 @@ private class LoadSpringResourceFlowStep extends SummaryModelCsv { [ "org.springframework.core.io;ClassPathResource;false;ClassPathResource;;;Argument[0];Argument[-1];taint", "org.springframework.core.io;ClassPathResource;true;" + - ["getFilename", "getPath", "getURL", "resolveURL"] + ";;;Argument[-1];ReturnValue;value", + ["getFilename", "getPath", "getURL", "resolveURL"] + ";;;Argument[-1];ReturnValue;taint", "org.springframework.core.io;ResourceLoader;true;getResource;;;Argument[0];ReturnValue;taint", "org.springframework.core.io;Resource;true;createRelative;;;Argument[0];ReturnValue;taint" ] diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java index 37ce866fd4db..129a2df733f8 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java @@ -45,7 +45,7 @@ public String getFileContent1(@RequestParam(name="fileName") String fileName) { //GOOD: Get resource from ClassPathResource with input path validation public String getFileContent1a(@RequestParam(name="fileName") String fileName) { String result = null; - if (!isInvalidPath(fileName) && !isInvalidEncodedPath(fileName)) { + if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { ClassPathResource clr = new ClassPathResource(fileName); char[] buffer = new char[4096]; StringBuilder out = new StringBuilder(); @@ -86,9 +86,9 @@ public String getFileContent2(@RequestParam(name="fileName") String fileName) { public String getFileContent2a(@RequestParam(name="fileName") String fileName) { String content = null; - if (!isInvalidPath(fileName) && !isInvalidEncodedPath(fileName)) { + if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { try { - File file = ResourceUtils.getFile(fileName); + File file = ResourceUtils.getFile(fileName); //Read File Content content = new String(Files.readAllBytes(file.toPath())); } catch (IOException ie) { @@ -132,7 +132,7 @@ public String getFileContent3(@RequestParam(name="fileName") String fileName) { public String getFileContent3a(@RequestParam(name="fileName") String fileName) { String content = null; - if (!isInvalidPath(fileName) && !isInvalidEncodedPath(fileName)) { + if (fileName.startsWith("/safe_dir") && !fileName.contains("..")) { try { Resource resource = resourceLoader.getResource(fileName); @@ -150,121 +150,4 @@ public String getFileContent3a(@RequestParam(name="fileName") String fileName) { } return content; } - - /** - * Check whether the given path contains invalid escape sequences. - * @param path the path to validate - * @return {@code true} if the path is invalid, {@code false} otherwise - * @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 - */ - private boolean isInvalidEncodedPath(String path) { - if (path.contains("%")) { - try { - // Use URLDecoder (vs UriUtils) to preserve potentially decoded UTF-8 chars - String decodedPath = URLDecoder.decode(path, "UTF-8"); - if (isInvalidPath(decodedPath)) { - return true; - } - decodedPath = processPath(decodedPath); - if (isInvalidPath(decodedPath)) { - return true; - } - } catch (IllegalArgumentException | UnsupportedEncodingException ex) { - // Should never happen... - } - } - return false; - } - - /** - * Process the given resource path. - *

    The default implementation replaces: - *

      - *
    • Backslash with forward slash. - *
    • Duplicate occurrences of slash with a single slash. - *
    • Any combination of leading slash and control characters (00-1F and 7F) - * with a single "/" or "". For example {@code " / // foo/bar"} - * becomes {@code "/foo/bar"}. - *
    - * @since 3.2.12 - * @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 - */ - protected String processPath(String path) { - path = StringUtils.replace(path, "\\", "/"); - path = cleanDuplicateSlashes(path); - return cleanLeadingSlash(path); - } - - /** @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 **/ - private String cleanDuplicateSlashes(String path) { - StringBuilder sb = null; - char prev = 0; - for (int i = 0; i < path.length(); i++) { - char curr = path.charAt(i); - try { - if ((curr == '/') && (prev == '/')) { - if (sb == null) { - sb = new StringBuilder(path.substring(0, i)); - } - continue; - } - if (sb != null) { - sb.append(path.charAt(i)); - } - } finally { - prev = curr; - } - } - return sb != null ? sb.toString() : path; - } - - /** @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 **/ - private String cleanLeadingSlash(String path) { - boolean slash = false; - for (int i = 0; i < path.length(); i++) { - if (path.charAt(i) == '/') { - slash = true; - } - else if (path.charAt(i) > ' ' && path.charAt(i) != 127) { - if (i == 0 || (i == 1 && slash)) { - return path; - } - return (slash ? "/" + path.substring(i) : path.substring(i)); - } - } - return (slash ? "/" : ""); - } - - /** - * Identifies invalid resource paths. By default rejects: - *
      - *
    • Paths that contain "WEB-INF" or "META-INF" - *
    • Paths that contain "../" after a call to - * {@link org.springframework.util.StringUtils#cleanPath}. - *
    • Paths that represent a {@link org.springframework.util.ResourceUtils#isUrl - * valid URL} or would represent one after the leading slash is removed. - *
    - *

    Note: this method assumes that leading, duplicate '/' - * or control characters (e.g. white space) have been trimmed so that the - * path starts predictably with a single '/' or does not have one. - * @param path the path to validate - * @return {@code true} if the path is invalid, {@code false} otherwise - * @since 3.0.6 - * @see Referenced code for path validation fix in https://github.com/mpgn/CVE-2019-3799 - */ - protected boolean isInvalidPath(String path) { - if (path.contains("WEB-INF") || path.contains("META-INF")) { - return true; - } - if (path.contains(":/")) { - String relativePath = (path.charAt(0) == '/' ? path.substring(1) : path); - if (ResourceUtils.isUrl(relativePath) || relativePath.startsWith("url:")) { - return true; - } - } - if (path.contains("..") && StringUtils.cleanPath(path).contains("../")) { - return true; - } - return false; - } } From 251f67dcf375237b3b0847a8c39957628af5d768 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Wed, 22 Jun 2022 22:08:27 +0000 Subject: [PATCH 3/6] Use the new CSV model --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 9 +++++---- 1 file changed, 5 insertions(+), 4 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 90df69c30989..37991d2f17b5 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -198,11 +198,12 @@ private class LoadSpringResourceFlowStep extends SummaryModelCsv { override predicate row(string row) { row = [ - "org.springframework.core.io;ClassPathResource;false;ClassPathResource;;;Argument[0];Argument[-1];taint", + "org.springframework.core.io;ClassPathResource;false;ClassPathResource;;;Argument[0];Argument[-1];taint;manual", "org.springframework.core.io;ClassPathResource;true;" + - ["getFilename", "getPath", "getURL", "resolveURL"] + ";;;Argument[-1];ReturnValue;taint", - "org.springframework.core.io;ResourceLoader;true;getResource;;;Argument[0];ReturnValue;taint", - "org.springframework.core.io;Resource;true;createRelative;;;Argument[0];ReturnValue;taint" + ["getFilename", "getPath", "getURL", "resolveURL"] + + ";;;Argument[-1];ReturnValue;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" ] } } From e33d78674565b64f9ef4ab2001b49bfd5b29b3c0 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 1 Jul 2022 23:03:29 +0000 Subject: [PATCH 4/6] Add test cases and reduce FPs --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 29 +++++++++---- .../code/java/frameworks/SpringResource.qll | 42 +------------------ .../CWE-552/UnsafeLoadSpringResource.java | 4 +- .../CWE-552/UnsafeUrlForward.expected | 32 ++++++-------- 4 files changed, 36 insertions(+), 71 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 37991d2f17b5..41154bf15844 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -87,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 @@ -104,12 +106,6 @@ private class GetResourceSink extends UnsafeUrlForwardSink { private class SpringResourceSink extends UnsafeUrlForwardSink { SpringResourceSink() { exists(MethodAccess ma | - ( - ma.getMethod() instanceof GetClassPathResourceInputStreamMethod or - ma.getMethod() instanceof GetResourceMethod - ) and - ma.getQualifier() = this.asExpr() - or ma.getMethod() instanceof GetResourceUtilsMethod and ma.getArgument(0) = this.asExpr() ) @@ -199,11 +195,26 @@ private class LoadSpringResourceFlowStep extends SummaryModelCsv { row = [ "org.springframework.core.io;ClassPathResource;false;ClassPathResource;;;Argument[0];Argument[-1];taint;manual", - "org.springframework.core.io;ClassPathResource;true;" + - ["getFilename", "getPath", "getURL", "resolveURL"] + - ";;;Argument[-1];ReturnValue;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 related to spring resource. */ +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", + // "org.springframework.core.io;Resource;true;" + + // ["getFile", "getFilename", "getURI", "getURL"] + + // ";;;Argument[-1];get-resource;manual", + // "org.springframework.core.io;InputStreamSource;true;" + + // ["getInputStream"] + + // ";;;Argument[-1];get-resource;manual" + ] + } +} diff --git a/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll b/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll index ddcca22aa0fe..ab7bc722cad4 100644 --- a/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll +++ b/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll @@ -6,57 +6,17 @@ import java private import semmle.code.java.dataflow.ExternalFlow private import semmle.code.java.dataflow.FlowSources -/** A class for class path resources in the Spring framework. */ -class ClassPathResource extends Class { - ClassPathResource() { this.hasQualifiedName("org.springframework.core.io", "ClassPathResource") } -} - -/** An interface for objects that are sources for an InputStream in the Spring framework. */ -class InputStreamResource extends RefType { - InputStreamResource() { - this.hasQualifiedName("org.springframework.core.io", "InputStreamSource") - } -} - -/** An interface that abstracts from the underlying resource, such as a file or class path resource in the Spring framework. */ -class Resource extends RefType { - Resource() { this.hasQualifiedName("org.springframework.core.io", "Resource") } -} - /** 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") } } -/** - * The method `getInputStream()` declared in Spring `ClassPathResource`. - */ -class GetClassPathResourceInputStreamMethod extends Method { - GetClassPathResourceInputStreamMethod() { - this.getDeclaringType().getASupertype*() instanceof ClassPathResource and - this.hasName("getInputStream") - } -} - -/** - * Resource loading method declared in Spring `Resource` with `getInputStream` inherited from the parent interface. - */ -class GetResourceMethod extends Method { - GetResourceMethod() { - ( - this.getDeclaringType() instanceof InputStreamResource or - this.getDeclaringType() instanceof Resource - ) and - this.hasName(["getFile", "getFilename", "getInputStream", "getURI", "getURL"]) - } -} - /** * Resource loading method declared in Spring `ResourceUtils`. */ class GetResourceUtilsMethod extends Method { GetResourceUtilsMethod() { this.getDeclaringType().getASupertype*() instanceof ResourceUtils and - this.hasName(["extractArchiveURL", "extractJarFileURL", "getFile", "getURL", "toURI"]) + this.hasName(["extractArchiveURL", "extractJarFileURL", "getFile", "getURL"]) } } diff --git a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java index 129a2df733f8..c7e114aede35 100644 --- a/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java +++ b/java/ql/test/experimental/query-tests/security/CWE-552/UnsafeLoadSpringResource.java @@ -1,6 +1,7 @@ package com.example; import java.io.File; +import java.io.FileReader; import java.io.InputStreamReader; import java.io.IOException; import java.io.Reader; @@ -31,7 +32,7 @@ public String getFileContent1(@RequestParam(name="fileName") String fileName) { char[] buffer = new char[4096]; StringBuilder out = new StringBuilder(); try { - Reader in = new InputStreamReader(clr.getInputStream(), "UTF-8"); + Reader in = new FileReader(clr.getFilename()); for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) { out.append(buffer, 0, numRead); } @@ -103,6 +104,7 @@ public String getFileContent2a(@RequestParam(name="fileName") String fileName) { @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; 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 ca909d137414..e227c58d7c7b 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,11 +1,8 @@ edges -| UnsafeLoadSpringResource.java:26:32:26:77 | fileName : String | UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String | -| UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource | UnsafeLoadSpringResource.java:34:38:34:40 | clr | -| UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String | UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource | -| UnsafeLoadSpringResource.java:67:32:67:77 | fileName : String | UnsafeLoadSpringResource.java:75:38:75:45 | fileName | -| UnsafeLoadSpringResource.java:106:32:106:77 | fileName : String | UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String | -| UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource | UnsafeLoadSpringResource.java:119:38:119:45 | resource | -| UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String | UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource | +| 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 | @@ -37,16 +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:26:32:26:77 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource | semmle.label | new ClassPathResource(...) : ClassPathResource | -| UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:34:38:34:40 | clr | semmle.label | clr | -| UnsafeLoadSpringResource.java:67:32:67:77 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:75:38:75:45 | fileName | semmle.label | fileName | -| UnsafeLoadSpringResource.java:106:32:106:77 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource | semmle.label | getResource(...) : Resource | -| UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String | semmle.label | fileName : String | -| UnsafeLoadSpringResource.java:119:38:119:45 | resource | semmle.label | resource | +| 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 | @@ -99,9 +92,8 @@ nodes | UnsafeUrlForward.java:60:33:60:62 | ... + ... | semmle.label | ... + ... | subpaths #select -| UnsafeLoadSpringResource.java:34:38:34:40 | clr | UnsafeLoadSpringResource.java:26:32:26:77 | fileName : String | UnsafeLoadSpringResource.java:34:38:34:40 | clr | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:26:32:26:77 | fileName | user-provided value | -| UnsafeLoadSpringResource.java:75:38:75:45 | fileName | UnsafeLoadSpringResource.java:67:32:67:77 | fileName : String | UnsafeLoadSpringResource.java:75:38:75:45 | fileName | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:67:32:67:77 | fileName | user-provided value | -| UnsafeLoadSpringResource.java:119:38:119:45 | resource | UnsafeLoadSpringResource.java:106:32:106:77 | fileName : String | UnsafeLoadSpringResource.java:119:38:119:45 | resource | Potentially untrusted URL forward due to $@. | UnsafeLoadSpringResource.java:106:32:106:77 | fileName | user-provided value | +| 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 | From 8effbff817e87db04e73976e3634ed3173a563ca Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 23 Sep 2022 12:43:39 +0000 Subject: [PATCH 5/6] Remove unused code and update qldoc --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 12 +++--------- .../semmle/code/java/frameworks/SpringResource.qll | 2 +- 2 files changed, 4 insertions(+), 10 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 41154bf15844..8d419f171fac 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -102,7 +102,7 @@ private class GetResourceSink extends UnsafeUrlForwardSink { } } -/** Sink of Spring resource loading. */ +/** A sink for methods that load Spring resources. */ private class SpringResourceSink extends UnsafeUrlForwardSink { SpringResourceSink() { exists(MethodAccess ma | @@ -189,7 +189,7 @@ private class FilePathFlowStep extends SummaryModelCsv { } } -/** Taint model related to resource loading in Spring. */ +/** Taint models related to resource loading in Spring. */ private class LoadSpringResourceFlowStep extends SummaryModelCsv { override predicate row(string row) { row = @@ -201,7 +201,7 @@ private class LoadSpringResourceFlowStep extends SummaryModelCsv { } } -/** Sink related to spring resource. */ +/** Sink models for methods that load Spring resources. */ private class SpringResourceCsvSink extends SinkModelCsv { override predicate row(string row) { row = @@ -209,12 +209,6 @@ private class SpringResourceCsvSink extends SinkModelCsv { // Get spring resource "org.springframework.core.io;ClassPathResource;true;" + ["getFilename", "getPath", "getURL", "resolveURL"] + ";;;Argument[-1];get-resource;manual", - // "org.springframework.core.io;Resource;true;" + - // ["getFile", "getFilename", "getURI", "getURL"] + - // ";;;Argument[-1];get-resource;manual", - // "org.springframework.core.io;InputStreamSource;true;" + - // ["getInputStream"] + - // ";;;Argument[-1];get-resource;manual" ] } } diff --git a/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll b/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll index ab7bc722cad4..e8c86a265127 100644 --- a/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll +++ b/java/ql/src/experimental/semmle/code/java/frameworks/SpringResource.qll @@ -12,7 +12,7 @@ class ResourceUtils extends Class { } /** - * Resource loading method declared in Spring `ResourceUtils`. + * A method declared in `org.springframework.util.ResourceUtils` that loads Spring resources. */ class GetResourceUtilsMethod extends Method { GetResourceUtilsMethod() { From 7ff82bbed35282d8040b8bf5db9d0225f866b059 Mon Sep 17 00:00:00 2001 From: Tony Torralba Date: Tue, 27 Sep 2022 13:26:21 +0200 Subject: [PATCH 6/6] Update java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll --- .../Security/CWE/CWE-552/UnsafeUrlForward.qll | 8 +++----- 1 file changed, 3 insertions(+), 5 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 8d419f171fac..aa2999a99557 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll +++ b/java/ql/src/experimental/Security/CWE/CWE-552/UnsafeUrlForward.qll @@ -205,10 +205,8 @@ private class LoadSpringResourceFlowStep extends SummaryModelCsv { 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", - ] + // Get spring resource + "org.springframework.core.io;ClassPathResource;true;" + + ["getFilename", "getPath", "getURL", "resolveURL"] + ";;;Argument[-1];get-resource;manual" } }