Skip to content

Commit 5a9ed32

Browse files
JLLeitschuhJLLeitschuh
committed
Refactor TempDirHijacking to show complete path
1 parent 3e2beb0 commit 5a9ed32

File tree

2 files changed

+82
-52
lines changed

2 files changed

+82
-52
lines changed

‎java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql

Lines changed: 48 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,9 @@ import java
1111
import semmle.code.java.controlflow.Guards
1212
import semmle.code.java.dataflow.FlowSources
1313
import semmle.code.java.security.TempFileLib
14+
import semmle.code.java.dataflow.DataFlow3
15+
import semmle.code.java.dataflow.TaintTracking
16+
import semmle.code.java.dataflow.TaintTracking2
1417
import DataFlow::PathGraph
1518

1619
/**
@@ -31,33 +34,41 @@ predicate isDeleteFileExpr(Expr expr) {
3134
expr = any(MethodFileDelete m).getAReference().getQualifier()
3235
}
3336

34-
private class TempDirHijackingToDeleteConfig extends TaintTracking::Configuration {
35-
TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" }
36-
37-
override predicate isSource(DataFlow::Node source) {
38-
source.asExpr() =
37+
private class DirHijackingTaintSource extends DataFlow::Node {
38+
DirHijackingTaintSource() {
39+
this.asExpr() =
3940
any(MethodAccess ma |
4041
ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2
4142
)
4243
or
4344
// TODO: Replace with getSystemProperty("java.io.tmpdir")
44-
source.asExpr() =
45+
this.asExpr() =
4546
any(MethodAccessSystemGetProperty maSgp |
4647
maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir")
4748
)
4849
}
50+
}
51+
52+
private predicate isAdditionalTaintStepCommon(DataFlow::Node node1, DataFlow::Node node2) {
53+
node2.asExpr() =
54+
any(MethodAccess ma |
55+
ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr()
56+
)
57+
}
58+
59+
private class TempDirHijackingToDeleteConfig extends TaintTracking2::Configuration {
60+
TempDirHijackingToDeleteConfig() { this = "TempDirHijackingToDeleteConfig" }
61+
62+
override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource }
4963

5064
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
51-
node2.asExpr() =
52-
any(MethodAccess ma |
53-
ma.getMethod() instanceof MethodFileCreateTempFile and ma.getArgument(2) = node1.asExpr()
54-
)
65+
isAdditionalTaintStepCommon(node1, node2)
5566
}
5667

5768
override predicate isSink(DataFlow::Node sink) { isDeleteFileExpr(sink.asExpr()) }
5869
}
5970

60-
private class TempDirHijackingFromDeleteConfig extends DataFlow2::Configuration {
71+
private class TempDirHijackingFromDeleteConfig extends DataFlow3::Configuration {
6172
TempDirHijackingFromDeleteConfig() { this = "TempDirHijackingFromDeleteConfig" }
6273

6374
override predicate isSource(DataFlow::Node source) { isDeleteFileExpr(source.asExpr()) }
@@ -127,15 +138,31 @@ private predicate safeUse(Expr e) {
127138
)
128139
}
129140

141+
private class TempDirHijackingFullPath extends TaintTracking::Configuration {
142+
TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" }
143+
144+
override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource }
145+
146+
override predicate isSink(DataFlow::Node sink) {
147+
isNonThrowingDirectoryCreationExpression(sink.asExpr(), _)
148+
}
149+
150+
override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
151+
isAdditionalTaintStepCommon(node1, node2)
152+
or
153+
node1.asExpr() = node2.asExpr() and isDeleteFileExpr(node1.asExpr())
154+
}
155+
}
156+
130157
from
131-
DataFlow::PathNode source, DataFlow::PathNode deleteCheckpoint, DataFlow2::Node deleteCheckpoint2,
132-
DataFlow2::Node sink, MethodAccess creationCall, Expr unsafe
158+
DataFlow::PathNode pathSource, DataFlow::PathNode pathSink, DataFlow::Node deleteCheckpoint,
159+
MethodAccess creationCall, Expr unsafe
133160
where
134-
any(TempDirHijackingToDeleteConfig c).hasFlowPath(source, deleteCheckpoint) and
135-
any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint2, sink) and
136-
deleteCheckpoint.getNode().asExpr() = deleteCheckpoint2.asExpr() and
137-
isUnsafeUseUnconstrainedByIfCheck(sink, unsafe) and
138-
isNonThrowingDirectoryCreationExpression(sink.asExpr(), creationCall)
139-
select deleteCheckpoint.getNode(), source, deleteCheckpoint,
140-
"Local temporary directory hijacking race condition $@, file $@ may have been hijacked",
141-
creationCall, "here", unsafe, "here"
161+
any(TempDirHijackingFullPath c).hasFlowPath(pathSource, pathSink) and
162+
any(TempDirHijackingToDeleteConfig c).hasFlow(pathSource.getNode(), deleteCheckpoint) and
163+
any(TempDirHijackingFromDeleteConfig c).hasFlow(deleteCheckpoint, pathSink.getNode()) and
164+
isUnsafeUseUnconstrainedByIfCheck(pathSink.getNode(), unsafe) and
165+
isNonThrowingDirectoryCreationExpression(pathSink.getNode().asExpr(), creationCall)
166+
select pathSink, pathSource, pathSink,
167+
"Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user.",
168+
deleteCheckpoint.asExpr(), "delete here", unsafe, "here"

‎java/ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected

Lines changed: 34 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,54 +1,57 @@
11
edges
2-
| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp |
3-
| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp |
4-
| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:36:9:36:12 | temp |
5-
| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:46:9:46:12 | temp |
6-
| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:57:9:57:12 | temp |
7-
| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:67:20:67:23 | temp |
8-
| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:78:20:78:23 | temp |
9-
| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:13:89:16 | temp |
10-
| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:14:98:17 | temp |
11-
| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:30:110:33 | temp |
12-
| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:119:14:119:17 | temp |
13-
| Test.java:129:62:129:107 | new File(...) : File | Test.java:130:9:130:12 | temp |
2+
| Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp |
3+
| Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp |
4+
| Test.java:29:21:29:60 | createTempFile(...) : File | Test.java:30:9:30:12 | temp |
5+
| Test.java:35:21:35:60 | createTempFile(...) : File | Test.java:37:13:37:16 | temp |
6+
| Test.java:45:21:45:60 | createTempFile(...) : File | Test.java:47:29:47:32 | temp |
7+
| Test.java:56:21:56:60 | createTempFile(...) : File | Test.java:58:15:58:18 | temp |
8+
| Test.java:66:21:66:60 | createTempFile(...) : File | Test.java:68:20:68:23 | temp |
9+
| Test.java:77:21:77:60 | createTempFile(...) : File | Test.java:79:20:79:23 | temp |
10+
| Test.java:88:21:88:60 | createTempFile(...) : File | Test.java:89:30:89:33 | temp |
11+
| Test.java:97:21:97:60 | createTempFile(...) : File | Test.java:98:32:98:35 | temp |
12+
| Test.java:109:21:109:60 | createTempFile(...) : File | Test.java:110:48:110:51 | temp |
13+
| Test.java:118:21:118:60 | createTempFile(...) : File | Test.java:122:14:122:17 | temp |
14+
| Test.java:129:62:129:107 | new File(...) : File | Test.java:131:9:131:12 | temp |
1415
| Test.java:129:71:129:106 | getProperty(...) : String | Test.java:129:62:129:107 | new File(...) : File |
1516
| Test.java:146:13:146:58 | new File(...) : File | Test.java:146:13:146:76 | getAbsoluteFile(...) : File |
16-
| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:148:9:148:12 | temp |
17+
| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:149:9:149:12 | temp |
1718
| Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File |
1819
nodes
1920
| Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
20-
| Test.java:12:13:12:16 | temp | semmle.label | temp |
21+
| Test.java:13:13:13:16 | temp | semmle.label | temp |
2122
| Test.java:22:21:22:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
22-
| Test.java:23:9:23:12 | temp | semmle.label | temp |
23+
| Test.java:24:9:24:12 | temp | semmle.label | temp |
24+
| Test.java:29:21:29:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
25+
| Test.java:30:9:30:12 | temp | semmle.label | temp |
2326
| Test.java:35:21:35:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
24-
| Test.java:36:9:36:12 | temp | semmle.label | temp |
27+
| Test.java:37:13:37:16 | temp | semmle.label | temp |
2528
| Test.java:45:21:45:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
26-
| Test.java:46:9:46:12 | temp | semmle.label | temp |
29+
| Test.java:47:29:47:32 | temp | semmle.label | temp |
2730
| Test.java:56:21:56:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
28-
| Test.java:57:9:57:12 | temp | semmle.label | temp |
31+
| Test.java:58:15:58:18 | temp | semmle.label | temp |
2932
| Test.java:66:21:66:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
30-
| Test.java:67:20:67:23 | temp | semmle.label | temp |
33+
| Test.java:68:20:68:23 | temp | semmle.label | temp |
3134
| Test.java:77:21:77:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
32-
| Test.java:78:20:78:23 | temp | semmle.label | temp |
35+
| Test.java:79:20:79:23 | temp | semmle.label | temp |
3336
| Test.java:88:21:88:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
34-
| Test.java:89:13:89:16 | temp | semmle.label | temp |
37+
| Test.java:89:30:89:33 | temp | semmle.label | temp |
3538
| Test.java:97:21:97:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
36-
| Test.java:98:14:98:17 | temp | semmle.label | temp |
39+
| Test.java:98:32:98:35 | temp | semmle.label | temp |
3740
| Test.java:109:21:109:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
38-
| Test.java:110:30:110:33 | temp | semmle.label | temp |
41+
| Test.java:110:48:110:51 | temp | semmle.label | temp |
3942
| Test.java:118:21:118:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
40-
| Test.java:119:14:119:17 | temp | semmle.label | temp |
43+
| Test.java:122:14:122:17 | temp | semmle.label | temp |
4144
| Test.java:129:62:129:107 | new File(...) : File | semmle.label | new File(...) : File |
4245
| Test.java:129:71:129:106 | getProperty(...) : String | semmle.label | getProperty(...) : String |
43-
| Test.java:130:9:130:12 | temp | semmle.label | temp |
46+
| Test.java:131:9:131:12 | temp | semmle.label | temp |
4447
| Test.java:146:13:146:58 | new File(...) : File | semmle.label | new File(...) : File |
4548
| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File |
4649
| Test.java:146:22:146:57 | getProperty(...) : String | semmle.label | getProperty(...) : String |
47-
| Test.java:148:9:148:12 | temp | semmle.label | temp |
50+
| Test.java:149:9:149:12 | temp | semmle.label | temp |
4851
subpaths
4952
#select
50-
| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:9:18:33 | ...=... | here |
51-
| Test.java:12:13:12:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:12:13:12:16 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:13:13:13:24 | mkdir(...) | here | Test.java:18:30:18:33 | temp | here |
52-
| Test.java:23:9:23:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:23:9:23:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:24:9:24:20 | mkdir(...) | here | Test.java:25:16:25:19 | temp | here |
53-
| Test.java:130:9:130:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:130:9:130:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:131:9:131:20 | mkdir(...) | here | Test.java:132:16:132:19 | temp | here |
54-
| Test.java:148:9:148:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:148:9:148:12 | temp | Local temporary directory hijacking race condition $@, file $@ may have been hijacked | Test.java:149:9:149:20 | mkdir(...) | here | Test.java:150:16:150:19 | temp | here |
53+
| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here |
54+
| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:30:18:33 | temp | here |
55+
| Test.java:24:9:24:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:23:9:23:12 | temp | delete here | Test.java:25:16:25:19 | temp | here |
56+
| Test.java:131:9:131:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:131:9:131:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:130:9:130:12 | temp | delete here | Test.java:132:16:132:19 | temp | here |
57+
| Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here |

0 commit comments

Comments
 (0)