Skip to content

Commit e33d786

Browse files
luchua-bcluchua-bc
committed
Add test cases and reduce FPs
1 parent 251f67d commit e33d786

File tree

4 files changed

+36
-71
lines changed

4 files changed

+36
-71
lines changed

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

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ private class GetResourceSink extends UnsafeUrlForwardSink {
8787
GetResourceSink() {
8888
sinkNode(this, "open-url")
8989
or
90+
sinkNode(this, "get-resource")
91+
or
9092
exists(MethodAccess ma |
9193
(
9294
ma.getMethod() instanceof GetServletResourceAsStreamMethod or
@@ -104,12 +106,6 @@ private class GetResourceSink extends UnsafeUrlForwardSink {
104106
private class SpringResourceSink extends UnsafeUrlForwardSink {
105107
SpringResourceSink() {
106108
exists(MethodAccess ma |
107-
(
108-
ma.getMethod() instanceof GetClassPathResourceInputStreamMethod or
109-
ma.getMethod() instanceof GetResourceMethod
110-
) and
111-
ma.getQualifier() = this.asExpr()
112-
or
113109
ma.getMethod() instanceof GetResourceUtilsMethod and
114110
ma.getArgument(0) = this.asExpr()
115111
)
@@ -199,11 +195,26 @@ private class LoadSpringResourceFlowStep extends SummaryModelCsv {
199195
row =
200196
[
201197
"org.springframework.core.io;ClassPathResource;false;ClassPathResource;;;Argument[0];Argument[-1];taint;manual",
202-
"org.springframework.core.io;ClassPathResource;true;" +
203-
["getFilename", "getPath", "getURL", "resolveURL"] +
204-
";;;Argument[-1];ReturnValue;taint;manual",
205198
"org.springframework.core.io;ResourceLoader;true;getResource;;;Argument[0];ReturnValue;taint;manual",
206199
"org.springframework.core.io;Resource;true;createRelative;;;Argument[0];ReturnValue;taint;manual"
207200
]
208201
}
209202
}
203+
204+
/** Sink related to spring resource. */
205+
private class SpringResourceCsvSink extends SinkModelCsv {
206+
override predicate row(string row) {
207+
row =
208+
[
209+
// Get spring resource
210+
"org.springframework.core.io;ClassPathResource;true;" +
211+
["getFilename", "getPath", "getURL", "resolveURL"] + ";;;Argument[-1];get-resource;manual",
212+
// "org.springframework.core.io;Resource;true;" +
213+
// ["getFile", "getFilename", "getURI", "getURL"] +
214+
// ";;;Argument[-1];get-resource;manual",
215+
// "org.springframework.core.io;InputStreamSource;true;" +
216+
// ["getInputStream"] +
217+
// ";;;Argument[-1];get-resource;manual"
218+
]
219+
}
220+
}

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

Lines changed: 1 addition & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -6,57 +6,17 @@ import java
66
private import semmle.code.java.dataflow.ExternalFlow
77
private import semmle.code.java.dataflow.FlowSources
88

9-
/** A class for class path resources in the Spring framework. */
10-
class ClassPathResource extends Class {
11-
ClassPathResource() { this.hasQualifiedName("org.springframework.core.io", "ClassPathResource") }
12-
}
13-
14-
/** An interface for objects that are sources for an InputStream in the Spring framework. */
15-
class InputStreamResource extends RefType {
16-
InputStreamResource() {
17-
this.hasQualifiedName("org.springframework.core.io", "InputStreamSource")
18-
}
19-
}
20-
21-
/** An interface that abstracts from the underlying resource, such as a file or class path resource in the Spring framework. */
22-
class Resource extends RefType {
23-
Resource() { this.hasQualifiedName("org.springframework.core.io", "Resource") }
24-
}
25-
269
/** A utility class for resolving resource locations to files in the file system in the Spring framework. */
2710
class ResourceUtils extends Class {
2811
ResourceUtils() { this.hasQualifiedName("org.springframework.util", "ResourceUtils") }
2912
}
3013

31-
/**
32-
* The method `getInputStream()` declared in Spring `ClassPathResource`.
33-
*/
34-
class GetClassPathResourceInputStreamMethod extends Method {
35-
GetClassPathResourceInputStreamMethod() {
36-
this.getDeclaringType().getASupertype*() instanceof ClassPathResource and
37-
this.hasName("getInputStream")
38-
}
39-
}
40-
41-
/**
42-
* Resource loading method declared in Spring `Resource` with `getInputStream` inherited from the parent interface.
43-
*/
44-
class GetResourceMethod extends Method {
45-
GetResourceMethod() {
46-
(
47-
this.getDeclaringType() instanceof InputStreamResource or
48-
this.getDeclaringType() instanceof Resource
49-
) and
50-
this.hasName(["getFile", "getFilename", "getInputStream", "getURI", "getURL"])
51-
}
52-
}
53-
5414
/**
5515
* Resource loading method declared in Spring `ResourceUtils`.
5616
*/
5717
class GetResourceUtilsMethod extends Method {
5818
GetResourceUtilsMethod() {
5919
this.getDeclaringType().getASupertype*() instanceof ResourceUtils and
60-
this.hasName(["extractArchiveURL", "extractJarFileURL", "getFile", "getURL", "toURI"])
20+
this.hasName(["extractArchiveURL", "extractJarFileURL", "getFile", "getURL"])
6121
}
6222
}

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package com.example;
22

33
import java.io.File;
4+
import java.io.FileReader;
45
import java.io.InputStreamReader;
56
import java.io.IOException;
67
import java.io.Reader;
@@ -31,7 +32,7 @@ public String getFileContent1(@RequestParam(name="fileName") String fileName) {
3132
char[] buffer = new char[4096];
3233
StringBuilder out = new StringBuilder();
3334
try {
34-
Reader in = new InputStreamReader(clr.getInputStream(), "UTF-8");
35+
Reader in = new FileReader(clr.getFilename());
3536
for (int numRead; (numRead = in.read(buffer, 0, buffer.length)) > 0; ) {
3637
out.append(buffer, 0, numRead);
3738
}
@@ -103,6 +104,7 @@ public String getFileContent2a(@RequestParam(name="fileName") String fileName) {
103104

104105
@GetMapping("/file3")
105106
//BAD: Get resource from ResourceLoader (same as application context) without input validation
107+
// Note it is not detected without the generic `resource.getInputStream()` check
106108
public String getFileContent3(@RequestParam(name="fileName") String fileName) {
107109
String content = null;
108110

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

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,8 @@
11
edges
2-
| UnsafeLoadSpringResource.java:26:32:26:77 | fileName : String | UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String |
3-
| UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource | UnsafeLoadSpringResource.java:34:38:34:40 | clr |
4-
| UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String | UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource |
5-
| UnsafeLoadSpringResource.java:67:32:67:77 | fileName : String | UnsafeLoadSpringResource.java:75:38:75:45 | fileName |
6-
| UnsafeLoadSpringResource.java:106:32:106:77 | fileName : String | UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String |
7-
| UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource | UnsafeLoadSpringResource.java:119:38:119:45 | resource |
8-
| UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String | UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource |
2+
| UnsafeLoadSpringResource.java:27:32:27:77 | fileName : String | UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String |
3+
| UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource | UnsafeLoadSpringResource.java:35:31:35:33 | clr |
4+
| UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String | UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource |
5+
| UnsafeLoadSpringResource.java:68:32:68:77 | fileName : String | UnsafeLoadSpringResource.java:76:38:76:45 | fileName |
96
| UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | UnsafeRequestPath.java:23:33:23:36 | path |
107
| UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | UnsafeResourceGet2.java:17:20:17:25 | params : Map |
118
| UnsafeResourceGet2.java:17:20:17:25 | params : Map | UnsafeResourceGet2.java:17:20:17:40 | get(...) : Object |
@@ -37,16 +34,12 @@ edges
3734
| UnsafeUrlForward.java:47:19:47:28 | url : String | UnsafeUrlForward.java:49:33:49:62 | ... + ... |
3835
| UnsafeUrlForward.java:58:19:58:28 | url : String | UnsafeUrlForward.java:60:33:60:62 | ... + ... |
3936
nodes
40-
| UnsafeLoadSpringResource.java:26:32:26:77 | fileName : String | semmle.label | fileName : String |
41-
| UnsafeLoadSpringResource.java:30:27:30:57 | new ClassPathResource(...) : ClassPathResource | semmle.label | new ClassPathResource(...) : ClassPathResource |
42-
| UnsafeLoadSpringResource.java:30:49:30:56 | fileName : String | semmle.label | fileName : String |
43-
| UnsafeLoadSpringResource.java:34:38:34:40 | clr | semmle.label | clr |
44-
| UnsafeLoadSpringResource.java:67:32:67:77 | fileName : String | semmle.label | fileName : String |
45-
| UnsafeLoadSpringResource.java:75:38:75:45 | fileName | semmle.label | fileName |
46-
| UnsafeLoadSpringResource.java:106:32:106:77 | fileName : String | semmle.label | fileName : String |
47-
| UnsafeLoadSpringResource.java:114:24:114:59 | getResource(...) : Resource | semmle.label | getResource(...) : Resource |
48-
| UnsafeLoadSpringResource.java:114:51:114:58 | fileName : String | semmle.label | fileName : String |
49-
| UnsafeLoadSpringResource.java:119:38:119:45 | resource | semmle.label | resource |
37+
| UnsafeLoadSpringResource.java:27:32:27:77 | fileName : String | semmle.label | fileName : String |
38+
| UnsafeLoadSpringResource.java:31:27:31:57 | new ClassPathResource(...) : ClassPathResource | semmle.label | new ClassPathResource(...) : ClassPathResource |
39+
| UnsafeLoadSpringResource.java:31:49:31:56 | fileName : String | semmle.label | fileName : String |
40+
| UnsafeLoadSpringResource.java:35:31:35:33 | clr | semmle.label | clr |
41+
| UnsafeLoadSpringResource.java:68:32:68:77 | fileName : String | semmle.label | fileName : String |
42+
| UnsafeLoadSpringResource.java:76:38:76:45 | fileName | semmle.label | fileName |
5043
| UnsafeRequestPath.java:20:17:20:63 | getServletPath(...) : String | semmle.label | getServletPath(...) : String |
5144
| UnsafeRequestPath.java:23:33:23:36 | path | semmle.label | path |
5245
| UnsafeResourceGet2.java:16:32:16:79 | getRequestParameterMap(...) : Map | semmle.label | getRequestParameterMap(...) : Map |
@@ -99,9 +92,8 @@ nodes
9992
| UnsafeUrlForward.java:60:33:60:62 | ... + ... | semmle.label | ... + ... |
10093
subpaths
10194
#select
102-
| 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 |
103-
| 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 |
104-
| 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 |
95+
| 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 |
96+
| 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 |
10597
| 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 |
10698
| 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 |
10799
| 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 |

0 commit comments

Comments
 (0)