From e33d78674565b64f9ef4ab2001b49bfd5b29b3c0 Mon Sep 17 00:00:00 2001 From: luchua-bc Date: Fri, 1 Jul 2022 23:03:29 +0000 Subject: [PATCH] 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 37991d2f17b..41154bf1584 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 ddcca22aa0f..ab7bc722cad 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 129a2df733f..c7e114aede3 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 class UnsafeLoadSpringResource { 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 class UnsafeLoadSpringResource { @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 ca909d13741..e227c58d7c7 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 |