Skip to content

Commit e140f04

Browse files
atorralbaatorralba
authored
Merge pull request #10393 from zbazztian/uri-constructor-flow
Java: Model taint flow for java.net.URI constructors in tainted path queries
2 parents e6d4e87 + 8c35803 commit e140f04

File tree

6 files changed

+83
-1
lines changed

6 files changed

+83
-1
lines changed

‎java/ql/src/Security/CWE/CWE-022/TaintedPath.ql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ class TaintedPathConfig extends TaintTracking::Configuration {
4848
or
4949
node = DataFlow::BarrierGuard<containsDotDotSanitizer/3>::getABarrierNode()
5050
}
51+
52+
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
53+
any(TaintedPathAdditionalTaintStep s).step(n1, n2)
54+
}
5155
}
5256

5357
/**

‎java/ql/src/Security/CWE/CWE-022/TaintedPathCommon.qll

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,49 @@
55
import java
66
import semmle.code.java.controlflow.Guards
77
import semmle.code.java.security.PathCreation
8+
import semmle.code.java.frameworks.Networking
9+
import semmle.code.java.dataflow.DataFlow
10+
11+
/**
12+
* A unit class for adding additional taint steps.
13+
*
14+
* Extend this class to add additional taint steps that should apply to tainted path flow configurations.
15+
*/
16+
class TaintedPathAdditionalTaintStep extends Unit {
17+
abstract predicate step(DataFlow::Node n1, DataFlow::Node n2);
18+
}
19+
20+
private class DefaultTaintedPathAdditionalTaintStep extends TaintedPathAdditionalTaintStep {
21+
override predicate step(DataFlow::Node n1, DataFlow::Node n2) {
22+
exists(Argument a |
23+
a = n1.asExpr() and
24+
a.getCall() = n2.asExpr() and
25+
a = any(TaintPreservingUriCtorParam tpp).getAnArgument()
26+
)
27+
}
28+
}
29+
30+
private class TaintPreservingUriCtorParam extends Parameter {
31+
TaintPreservingUriCtorParam() {
32+
exists(Constructor ctor, int idx, int nParams |
33+
ctor.getDeclaringType() instanceof TypeUri and
34+
this = ctor.getParameter(idx) and
35+
nParams = ctor.getNumberOfParameters()
36+
|
37+
// URI(String scheme, String ssp, String fragment)
38+
idx = 1 and nParams = 3
39+
or
40+
// URI(String scheme, String host, String path, String fragment)
41+
idx = [1, 2] and nParams = 4
42+
or
43+
// URI(String scheme, String authority, String path, String query, String fragment)
44+
idx = 2 and nParams = 5
45+
or
46+
// URI(String scheme, String userInfo, String host, int port, String path, String query, String fragment)
47+
idx = 4 and nParams = 7
48+
)
49+
}
50+
}
851

952
private predicate inWeakCheck(Expr e) {
1053
// None of these are sufficient to guarantee that a string is safe.

‎java/ql/src/Security/CWE/CWE-022/TaintedPathLocal.ql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ class TaintedPathLocalConfig extends TaintTracking::Configuration {
2727
override predicate isSink(DataFlow::Node sink) {
2828
sink.asExpr() = any(PathCreation p).getAnInput()
2929
}
30+
31+
override predicate isAdditionalTaintStep(DataFlow::Node n1, DataFlow::Node n2) {
32+
any(TaintedPathAdditionalTaintStep s).step(n1, n2)
33+
}
3034
}
3135

3236
from

‎java/ql/src/change-notes/2022-09-13-flow-model-for-uri-constructors.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Added taint model for arguments of `java.net.URI` constructors to the queries `java/path-injection` and `java/path-injection-local`.

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/TaintedPath.expected

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ edges
99
| Test.java:80:31:80:32 | br : BufferedReader | Test.java:80:31:80:43 | readLine(...) : String |
1010
| Test.java:80:31:80:43 | readLine(...) : String | Test.java:82:67:82:81 | ... + ... |
1111
| Test.java:88:17:88:37 | getHostName(...) : String | Test.java:90:26:90:29 | temp |
12+
| Test.java:95:14:95:34 | getHostName(...) : String | Test.java:97:12:97:33 | new URI(...) |
13+
| Test.java:95:14:95:34 | getHostName(...) : String | Test.java:98:12:98:33 | new URI(...) |
14+
| Test.java:95:14:95:34 | getHostName(...) : String | Test.java:99:12:99:33 | new URI(...) |
15+
| Test.java:95:14:95:34 | getHostName(...) : String | Test.java:100:12:100:45 | new URI(...) |
16+
| Test.java:95:14:95:34 | getHostName(...) : String | Test.java:101:12:101:54 | new URI(...) |
1217
nodes
1318
| Test.java:19:18:19:38 | getHostName(...) : String | semmle.label | getHostName(...) : String |
1419
| Test.java:24:20:24:23 | temp | semmle.label | temp |
@@ -23,6 +28,12 @@ nodes
2328
| Test.java:82:67:82:81 | ... + ... | semmle.label | ... + ... |
2429
| Test.java:88:17:88:37 | getHostName(...) : String | semmle.label | getHostName(...) : String |
2530
| Test.java:90:26:90:29 | temp | semmle.label | temp |
31+
| Test.java:95:14:95:34 | getHostName(...) : String | semmle.label | getHostName(...) : String |
32+
| Test.java:97:12:97:33 | new URI(...) | semmle.label | new URI(...) |
33+
| Test.java:98:12:98:33 | new URI(...) | semmle.label | new URI(...) |
34+
| Test.java:99:12:99:33 | new URI(...) | semmle.label | new URI(...) |
35+
| Test.java:100:12:100:45 | new URI(...) | semmle.label | new URI(...) |
36+
| Test.java:101:12:101:54 | new URI(...) | semmle.label | new URI(...) |
2637
subpaths
2738
#select
2839
| Test.java:24:11:24:24 | new File(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:24:20:24:23 | temp | $@ flows to here and is used in a path. | Test.java:19:18:19:38 | getHostName(...) | User-provided value |
@@ -31,3 +42,8 @@ subpaths
3142
| Test.java:34:12:34:25 | new File(...) | Test.java:19:18:19:38 | getHostName(...) : String | Test.java:34:21:34:24 | temp | $@ flows to here and is used in a path. | Test.java:19:18:19:38 | getHostName(...) | User-provided value |
3243
| Test.java:82:52:82:88 | new FileWriter(...) | Test.java:79:74:79:97 | getInputStream(...) : ServletInputStream | Test.java:82:67:82:81 | ... + ... | $@ flows to here and is used in a path. | Test.java:79:74:79:97 | getInputStream(...) | User-provided value |
3344
| Test.java:90:26:90:29 | temp | Test.java:88:17:88:37 | getHostName(...) : String | Test.java:90:26:90:29 | temp | $@ flows to here and is used in a path. | Test.java:88:17:88:37 | getHostName(...) | User-provided value |
45+
| Test.java:97:3:97:34 | new File(...) | Test.java:95:14:95:34 | getHostName(...) : String | Test.java:97:12:97:33 | new URI(...) | $@ flows to here and is used in a path. | Test.java:95:14:95:34 | getHostName(...) | User-provided value |
46+
| Test.java:98:3:98:34 | new File(...) | Test.java:95:14:95:34 | getHostName(...) : String | Test.java:98:12:98:33 | new URI(...) | $@ flows to here and is used in a path. | Test.java:95:14:95:34 | getHostName(...) | User-provided value |
47+
| Test.java:99:3:99:34 | new File(...) | Test.java:95:14:95:34 | getHostName(...) : String | Test.java:99:12:99:33 | new URI(...) | $@ flows to here and is used in a path. | Test.java:95:14:95:34 | getHostName(...) | User-provided value |
48+
| Test.java:100:3:100:46 | new File(...) | Test.java:95:14:95:34 | getHostName(...) : String | Test.java:100:12:100:45 | new URI(...) | $@ flows to here and is used in a path. | Test.java:95:14:95:34 | getHostName(...) | User-provided value |
49+
| Test.java:101:3:101:55 | new File(...) | Test.java:95:14:95:34 | getHostName(...) : String | Test.java:101:12:101:54 | new URI(...) | $@ flows to here and is used in a path. | Test.java:95:14:95:34 | getHostName(...) | User-provided value |

‎java/ql/test/query-tests/security/CWE-022/semmle/tests/Test.java

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
import javax.servlet.ServletException;
77

88
import java.io.*;
9-
import java.net.InetAddress;
9+
import java.net.*;
1010
import java.nio.file.Path;
1111
import java.nio.file.Paths;
1212
import java.nio.file.FileSystems;
@@ -89,4 +89,15 @@ void doGet4(InetAddress address)
8989
// BAD: open a file based on user input, using a MaD-documented API
9090
new LockableFileWriter(temp);
9191
}
92+
93+
void doGet5(InetAddress address)
94+
throws URISyntaxException {
95+
String t = address.getHostName();
96+
// BAD: construct a file path with user input
97+
new File(new URI(null, t, null));
98+
new File(new URI(t, t, null, t));
99+
new File(new URI(t, null, t, t));
100+
new File(new URI(null, null, t, null, null));
101+
new File(new URI(null, null, null, 0, t, null, null));
102+
}
92103
}

0 commit comments

Comments
 (0)