Skip to content

Commit 201b519

Browse files
JLLeitschuhJLLeitschuh
committed
Expand isDeleteFileExpr to include delete method wrappers
1 parent 01d4464 commit 201b519

3 files changed

Lines changed: 61 additions & 36 deletions

File tree

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

Lines changed: 25 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,14 @@ private predicate isNonThrowingDirectoryCreationExpression(
101101
) {
102102
creationCall.getMethod() instanceof MethodFileCreatesDirs and
103103
creationCall.getQualifier() = fileInstanceExpr and
104-
// Check for 'throwing creations' and ensure that this call is not used in a throwing case.
105-
if creationCall.(Guard).directlyControls(_, _)
106-
then
107-
// The creation call is used in a guard
104+
(
105+
// Check for 'throwing creations' and ensure that this call is not used in a throwing case.
106+
// If the creation call is used in a guard:
107+
creationCall.(Guard).directlyControls(_, _)
108+
implies
109+
// Then it must not be a 'safe' creation guard. Thus `creationCall` is not a 'throwing creation'.
108110
not creationCall = safeDirectoryCreationGuard(_) // Ensure that the guard is insufficient.
109-
else any() // The creation call is not used in a guard. Thus is not a 'throwing creation'.
111+
)
110112
or
111113
// Recursively look for methods that encapsulate the above.
112114
// Thus, the use of 'helper directory creation methods' are still considered
@@ -142,7 +144,23 @@ private class MethodFileExists extends Method {
142144
}
143145

144146
predicate isDeleteFileExpr(Expr expr) {
145-
expr = any(MethodFileDelete m).getAReference().getQualifier()
147+
exists(Expr deleteMethodQualifier |
148+
deleteMethodQualifier = any(MethodFileDelete m).getAReference().getQualifier()
149+
|
150+
// The expression is the qualifier of the `delete` method call.
151+
expr = deleteMethodQualifier
152+
or
153+
// Any wrapper method that calls the `delete` method on a file instance argument.
154+
expr =
155+
any(Method m, int argIndex, Expr arg |
156+
arg.(VarAccess).getVariable() = m.getParameter(argIndex) and
157+
// We intentionally don't call this method recursively as it will increase the false positive rate.
158+
// A delete call at a depth of one call is enough to cover most cases.
159+
DataFlow::localExprFlow(arg, deleteMethodQualifier)
160+
|
161+
m.getAReference().getArgument(argIndex)
162+
)
163+
)
146164
}
147165

148166
abstract private class DirHijackingTaintSource extends DataFlow::Node {
@@ -402,7 +420,7 @@ class TempDirHijackingFullPathInlcudingUnsafeUse extends TaintTracking2::Configu
402420
or
403421
// `File::mkdir(s)` is not an end-state when looking for an unsafe use
404422
isNonThrowingDirectoryCreationExpression(node1.asExpr(), _) and
405-
node1.asExpr() = node2.asExpr() and
423+
node1 = node2 and
406424
state1 instanceof FromDeleteFileFlowState and
407425
state2 instanceof FromMkdirsFileFlowState
408426
}

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

Lines changed: 15 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -34,15 +34,12 @@ edges
3434
| Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File |
3535
| Test.java:148:9:148:12 | temp : File | Test.java:148:9:148:12 | temp : File |
3636
| Test.java:148:9:148:12 | temp : File | Test.java:149:9:149:12 | temp |
37-
| Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:21:158:24 | temp : File |
38-
| Test.java:158:21:158:24 | temp : File | Test.java:158:21:158:24 | temp : File |
39-
| Test.java:158:21:158:24 | temp : File | Test.java:158:38:158:41 | temp |
40-
| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | Test.java:171:21:171:30 | this <.field> [safe12temp] : File |
41-
| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File |
42-
| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:21:171:30 | safe12temp : File |
43-
| Test.java:171:21:171:30 | safe12temp : File | Test.java:171:21:171:30 | safe12temp : File |
44-
| Test.java:171:21:171:30 | safe12temp : File | Test.java:171:44:171:53 | safe12temp |
45-
| Test.java:171:21:171:30 | this <.field> [safe12temp] : File | Test.java:171:21:171:30 | safe12temp : File |
37+
| Test.java:156:20:156:59 | createTempFile(...) : File | Test.java:157:17:157:20 | temp : File |
38+
| Test.java:157:17:157:20 | temp : File | Test.java:157:17:157:20 | temp : File |
39+
| Test.java:157:17:157:20 | temp : File | Test.java:157:34:157:37 | temp |
40+
| Test.java:169:24:169:63 | createTempFile(...) : File | Test.java:170:21:170:24 | safe : File |
41+
| Test.java:170:21:170:24 | safe : File | Test.java:170:21:170:24 | safe : File |
42+
| Test.java:170:21:170:24 | safe : File | Test.java:170:38:170:41 | safe |
4643
| Test.java:211:21:211:66 | new File(...) : File | Test.java:213:65:213:68 | temp : File |
4744
| Test.java:211:30:211:65 | getProperty(...) : String | Test.java:211:21:211:66 | new File(...) : File |
4845
| Test.java:213:24:213:69 | createTempFile(...) : File | Test.java:214:14:214:20 | workDir : File |
@@ -137,16 +134,14 @@ nodes
137134
| Test.java:148:9:148:12 | temp : File | semmle.label | temp : File |
138135
| Test.java:148:9:148:12 | temp : File | semmle.label | temp : File |
139136
| Test.java:149:9:149:12 | temp | semmle.label | temp |
140-
| Test.java:157:24:157:63 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
141-
| Test.java:158:21:158:24 | temp : File | semmle.label | temp : File |
142-
| Test.java:158:21:158:24 | temp : File | semmle.label | temp : File |
143-
| Test.java:158:38:158:41 | temp | semmle.label | temp |
144-
| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | semmle.label | this <.field> [post update] [safe12temp] : File |
145-
| Test.java:170:30:170:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
146-
| Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File |
147-
| Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File |
148-
| Test.java:171:21:171:30 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File |
149-
| Test.java:171:44:171:53 | safe12temp | semmle.label | safe12temp |
137+
| Test.java:156:20:156:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
138+
| Test.java:157:17:157:20 | temp : File | semmle.label | temp : File |
139+
| Test.java:157:17:157:20 | temp : File | semmle.label | temp : File |
140+
| Test.java:157:34:157:37 | temp | semmle.label | temp |
141+
| Test.java:169:24:169:63 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
142+
| Test.java:170:21:170:24 | safe : File | semmle.label | safe : File |
143+
| Test.java:170:21:170:24 | safe : File | semmle.label | safe : File |
144+
| Test.java:170:38:170:41 | safe | semmle.label | safe |
150145
| Test.java:211:21:211:66 | new File(...) : File | semmle.label | new File(...) : File |
151146
| Test.java:211:30:211:65 | getProperty(...) : String | semmle.label | getProperty(...) : String |
152147
| Test.java:213:24:213:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
@@ -208,8 +203,7 @@ subpaths
208203
| 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 |
209204
| 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 |
210205
| 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 |
211-
| Test.java:158:38:158:41 | temp | Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | 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:158:21:158:24 | temp | delete here | Test.java:163:16:163:19 | temp | here |
212-
| Test.java:171:44:171:53 | safe12temp | Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp | 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:171:21:171:30 | safe12temp | delete here | Test.java:176:16:176:25 | safe12temp | here |
206+
| Test.java:170:38:170:41 | safe | Test.java:169:24:169:63 | createTempFile(...) : File | Test.java:170:38:170:41 | safe | 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:170:21:170:24 | safe | delete here | Test.java:176:16:176:25 | safe12temp | here |
213207
| Test.java:222:14:222:16 | dir | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:222:14:222:16 | dir | 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:214:14:214:20 | workDir | delete here | Test.java:218:16:218:22 | workDir | here |
214208
| Test.java:222:14:222:16 | dir | Test.java:211:30:211:65 | getProperty(...) : String | Test.java:222:14:222:16 | dir | 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:214:14:214:20 | workDir | delete here | Test.java:222:31:222:33 | dir | here |
215209
| Test.java:230:9:230:12 | temp | Test.java:228:21:228:66 | createTempFile(...) : File | Test.java:230:9:230: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:229:9:229:12 | temp | delete here | Test.java:231:16:231:19 | temp | here |

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

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,10 @@ static File vulnerable3() throws IOException {
152152

153153
static File safe11() throws IOException {
154154
File temp = null;
155-
if (temp == null) {
156-
while (true) {
157-
temp = File.createTempFile("test", "directory");
158-
if (temp.delete() && temp.mkdir()) {
159-
break;
160-
}
155+
while (true) {
156+
temp = File.createTempFile("test", "directory");
157+
if (temp.delete() && temp.mkdir()) {
158+
break;
161159
}
162160
}
163161
return temp;
@@ -166,12 +164,14 @@ static File safe11() throws IOException {
166164
File safe12temp;
167165
File safe12() throws IOException {
168166
if (safe12temp == null) {
167+
File safe = null;
169168
while (true) {
170-
safe12temp = File.createTempFile("test", "directory");
171-
if (safe12temp.delete() && safe12temp.mkdir()) {
169+
safe = File.createTempFile("test", "directory");
170+
if (safe.delete() && safe.mkdir()) {
172171
break;
173172
}
174173
}
174+
safe12temp = safe;
175175
}
176176
return safe12temp;
177177
}
@@ -307,4 +307,17 @@ static void mkdirsWrapper3(File file) throws IOException {
307307
throw new IOException("Mkdirs failed to create " + file.toString());
308308
}
309309
}
310+
311+
File vulnerable10() throws IOException {
312+
File temp = new File(System.getProperty("java.io.tmpdir"));
313+
temp.mkdirs();
314+
File workDir = File.createTempFile("test", "directory", temp);
315+
deleteWrapper(workDir);
316+
workDir.mkdirs();
317+
return workDir;
318+
}
319+
320+
static void deleteWrapper(File file) {
321+
file.delete();
322+
}
310323
}

0 commit comments

Comments
 (0)