Skip to content

Commit 2753521

Browse files
JLLeitschuhJLLeitschuh
committed
Java: Fix Local Temp File/Dir Incorrect Guard Logic
Resolves github#8032 (comment)
1 parent 879b8a1 commit 2753521

File tree

6 files changed

+61
-39
lines changed

6 files changed

+61
-39
lines changed

‎java/ql/lib/semmle/code/java/os/OSCheck.qll

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -112,23 +112,26 @@ private predicate isOsFromApacheCommons(FieldAccess fa, string fieldNamePattern)
112112
}
113113

114114
private class IsWindowsFromApacheCommons extends IsWindowsGuard instanceof FieldAccess {
115-
IsWindowsFromApacheCommons() { isOsFromApacheCommons(this, "IS_OS_WINDOWS") }
115+
IsWindowsFromApacheCommons() { isOsFromApacheCommons(this, "IS\\_OS\\_WINDOWS") }
116116
}
117117

118118
private class IsSpecificWindowsVariantFromApacheCommons extends IsSpecificWindowsVariant instanceof FieldAccess {
119-
IsSpecificWindowsVariantFromApacheCommons() { isOsFromApacheCommons(this, "IS_OS_WINDOWS_%") }
119+
IsSpecificWindowsVariantFromApacheCommons() {
120+
isOsFromApacheCommons(this, "IS\\_OS\\_WINDOWS\\_%")
121+
}
120122
}
121123

122124
private class IsUnixFromApacheCommons extends IsUnixGuard instanceof FieldAccess {
123-
IsUnixFromApacheCommons() { isOsFromApacheCommons(this, "IS_OS_UNIX") }
125+
IsUnixFromApacheCommons() { isOsFromApacheCommons(this, "IS\\_OS\\_UNIX") }
124126
}
125127

126128
private class IsSpecificUnixVariantFromApacheCommons extends IsSpecificUnixVariant instanceof FieldAccess {
127129
IsSpecificUnixVariantFromApacheCommons() {
128130
isOsFromApacheCommons(this,
129131
[
130-
"IS_OS_AIX", "IS_OS_HP_UX", "IS_OS_IRIX", "IS_OS_LINUX", "IS_OS_MAC%", "IS_OS_FREE_BSD",
131-
"IS_OS_OPEN_BSD", "IS_OS_NET_BSD", "IS_OS_SOLARIS", "IS_OS_SUN_OS", "IS_OS_ZOS"
132+
"IS\\_OS\\_AIX", "IS\\_OS\\_HP\\_UX", "IS\\_OS\\_IRIX", "IS\\_OS\\_LINUX", "IS\\_OS\\_MAC%",
133+
"IS\\_OS\\_FREE\\_BSD", "IS\\_OS\\_OPEN\\_BSD", "IS\\_OS\\_NET\\_BSD", "IS\\_OS\\_SOLARIS",
134+
"IS\\_OS\\_SUN\\_OS", "IS\\_OS\\_ZOS"
132135
])
133136
}
134137
}

‎java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -105,23 +105,21 @@ private class FileCreateTempFileSink extends FileCreationSink {
105105
}
106106

107107
/**
108-
* A guard that holds when the program is definitely running under some version of Windows.
108+
* A sanitizer that holds when the program is definitely running under some version of Windows.
109109
*/
110-
abstract private class WindowsOsBarrierGuard extends DataFlow::BarrierGuard { }
110+
abstract private class WindowsOsSanitizer extends DataFlow::Node { }
111111

112-
private class IsNotUnixBarrierGuard extends WindowsOsBarrierGuard instanceof IsUnixGuard {
113-
override predicate checks(Expr e, boolean branch) {
114-
this.controls(e.getBasicBlock(), branch.booleanNot())
115-
}
112+
private class IsNotUnixSanitizer extends WindowsOsSanitizer {
113+
IsNotUnixSanitizer() { any(IsUnixGuard guard).controls(this.asExpr().getBasicBlock(), false) }
116114
}
117115

118-
private class IsWindowsBarrierGuard extends WindowsOsBarrierGuard instanceof IsWindowsGuard {
119-
override predicate checks(Expr e, boolean branch) { this.controls(e.getBasicBlock(), branch) }
116+
private class IsWindowsSanitizer extends WindowsOsSanitizer {
117+
IsWindowsSanitizer() { any(IsWindowsGuard guard).controls(this.asExpr().getBasicBlock(), true) }
120118
}
121119

122-
private class IsSpecificWindowsBarrierGuard extends WindowsOsBarrierGuard instanceof IsSpecificWindowsVariant {
123-
override predicate checks(Expr e, boolean branch) {
124-
branch = true and this.controls(e.getBasicBlock(), branch)
120+
private class IsSpecificWindowsSanitizer extends WindowsOsSanitizer {
121+
IsSpecificWindowsSanitizer() {
122+
any(IsSpecificWindowsVariant guard).controls(this.asExpr().getBasicBlock(), true)
125123
}
126124
}
127125

@@ -155,10 +153,8 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
155153
exists(FilesSanitizingCreationMethodAccess sanitisingMethodAccess |
156154
sanitizer.asExpr() = sanitisingMethodAccess.getArgument(0)
157155
)
158-
}
159-
160-
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
161-
guard instanceof WindowsOsBarrierGuard
156+
or
157+
sanitizer instanceof WindowsOsSanitizer
162158
}
163159
}
164160

‎java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,6 @@ private predicate isTaintPropagatingFileTransformation(Expr expSource, Expr expr
5858
* For example, `taintedFile.getCanonicalFile()` is itself tainted.
5959
*/
6060
predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
61-
isFileConstructorArgument(node1.asExpr(), node2.asExpr(), _) or
6261
isTaintPropagatingFileTransformation(node1.asExpr(), node2.asExpr())
6362
}
6463

‎java/ql/src/change-notes/2022-04-06-local-temp-file-or-directory-bugfix.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+
* Fixed "Local information disclosure in a temporary directory" (`java/local-temp-file-or-directory-information-disclosure`) to resolve false-negatives when OS isn't properly used as logical guard.

‎java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,38 +1,26 @@
11
edges
22
| Files.java:10:24:10:69 | new File(...) : File | Files.java:14:37:14:43 | baseDir : File |
3-
| Files.java:10:24:10:69 | new File(...) : File | Files.java:15:17:15:23 | tempDir |
43
| Files.java:10:33:10:68 | getProperty(...) : String | Files.java:10:24:10:69 | new File(...) : File |
5-
| Files.java:10:33:10:68 | getProperty(...) : String | Files.java:14:37:14:43 | baseDir : File |
6-
| Files.java:10:33:10:68 | getProperty(...) : String | Files.java:15:17:15:23 | tempDir |
74
| Files.java:14:28:14:64 | new File(...) : File | Files.java:15:17:15:23 | tempDir |
85
| Files.java:14:37:14:43 | baseDir : File | Files.java:14:28:14:64 | new File(...) : File |
96
| Test.java:36:24:36:69 | new File(...) : File | Test.java:39:63:39:69 | tempDir |
107
| Test.java:36:33:36:68 | getProperty(...) : String | Test.java:36:24:36:69 | new File(...) : File |
11-
| Test.java:36:33:36:68 | getProperty(...) : String | Test.java:39:63:39:69 | tempDir |
128
| Test.java:50:29:50:94 | new File(...) : File | Test.java:53:63:53:74 | tempDirChild |
139
| Test.java:50:38:50:83 | new File(...) : File | Test.java:50:29:50:94 | new File(...) : File |
14-
| Test.java:50:38:50:83 | new File(...) : File | Test.java:53:63:53:74 | tempDirChild |
1510
| Test.java:50:47:50:82 | getProperty(...) : String | Test.java:50:38:50:83 | new File(...) : File |
16-
| Test.java:50:47:50:82 | getProperty(...) : String | Test.java:53:63:53:74 | tempDirChild |
1711
| Test.java:61:24:61:69 | new File(...) : File | Test.java:64:63:64:69 | tempDir |
1812
| Test.java:61:33:61:68 | getProperty(...) : String | Test.java:61:24:61:69 | new File(...) : File |
19-
| Test.java:61:33:61:68 | getProperty(...) : String | Test.java:64:63:64:69 | tempDir |
2013
| Test.java:75:24:75:69 | new File(...) : File | Test.java:78:63:78:69 | tempDir |
2114
| Test.java:75:33:75:68 | getProperty(...) : String | Test.java:75:24:75:69 | new File(...) : File |
22-
| Test.java:75:33:75:68 | getProperty(...) : String | Test.java:78:63:78:69 | tempDir |
2315
| Test.java:110:29:110:84 | new File(...) : File | Test.java:113:9:113:20 | tempDirChild |
2416
| Test.java:110:38:110:73 | getProperty(...) : String | Test.java:110:29:110:84 | new File(...) : File |
25-
| Test.java:110:38:110:73 | getProperty(...) : String | Test.java:113:9:113:20 | tempDirChild |
2617
| Test.java:134:29:134:84 | new File(...) : File | Test.java:137:9:137:20 | tempDirChild |
2718
| Test.java:134:38:134:73 | getProperty(...) : String | Test.java:134:29:134:84 | new File(...) : File |
28-
| Test.java:134:38:134:73 | getProperty(...) : String | Test.java:137:9:137:20 | tempDirChild |
2919
| Test.java:158:29:158:88 | new File(...) : File | Test.java:159:21:159:32 | tempDirChild : File |
3020
| Test.java:158:38:158:73 | getProperty(...) : String | Test.java:158:29:158:88 | new File(...) : File |
31-
| Test.java:158:38:158:73 | getProperty(...) : String | Test.java:159:21:159:32 | tempDirChild : File |
3221
| Test.java:159:21:159:32 | tempDirChild : File | Test.java:159:21:159:41 | toPath(...) |
3322
| Test.java:187:29:187:88 | new File(...) : File | Test.java:188:21:188:32 | tempDirChild : File |
3423
| Test.java:187:38:187:73 | getProperty(...) : String | Test.java:187:29:187:88 | new File(...) : File |
35-
| Test.java:187:38:187:73 | getProperty(...) : String | Test.java:188:21:188:32 | tempDirChild : File |
3624
| Test.java:188:21:188:32 | tempDirChild : File | Test.java:188:21:188:41 | toPath(...) |
3725
| Test.java:204:29:204:104 | new File(...) : File | Test.java:204:29:204:113 | toPath(...) : Path |
3826
| Test.java:204:29:204:113 | toPath(...) : Path | Test.java:207:33:207:44 | tempDirChild |
@@ -42,28 +30,28 @@ edges
4230
| Test.java:216:38:216:73 | getProperty(...) : String | Test.java:216:29:216:102 | new File(...) : File |
4331
| Test.java:228:29:228:100 | new File(...) : File | Test.java:231:26:231:37 | tempDirChild : File |
4432
| Test.java:228:38:228:73 | getProperty(...) : String | Test.java:228:29:228:100 | new File(...) : File |
45-
| Test.java:228:38:228:73 | getProperty(...) : String | Test.java:231:26:231:37 | tempDirChild : File |
4633
| Test.java:231:26:231:37 | tempDirChild : File | Test.java:231:26:231:46 | toPath(...) |
4734
| Test.java:249:29:249:101 | new File(...) : File | Test.java:252:31:252:42 | tempDirChild : File |
4835
| Test.java:249:38:249:73 | getProperty(...) : String | Test.java:249:29:249:101 | new File(...) : File |
49-
| Test.java:249:38:249:73 | getProperty(...) : String | Test.java:252:31:252:42 | tempDirChild : File |
5036
| Test.java:252:31:252:42 | tempDirChild : File | Test.java:252:31:252:51 | toPath(...) |
5137
| Test.java:260:29:260:109 | new File(...) : File | Test.java:263:33:263:44 | tempDirChild : File |
5238
| Test.java:260:38:260:73 | getProperty(...) : String | Test.java:260:29:260:109 | new File(...) : File |
53-
| Test.java:260:38:260:73 | getProperty(...) : String | Test.java:263:33:263:44 | tempDirChild : File |
5439
| Test.java:263:33:263:44 | tempDirChild : File | Test.java:263:33:263:53 | toPath(...) |
5540
| Test.java:294:29:294:101 | new File(...) : File | Test.java:298:35:298:46 | tempDirChild : File |
5641
| Test.java:294:38:294:73 | getProperty(...) : String | Test.java:294:29:294:101 | new File(...) : File |
57-
| Test.java:294:38:294:73 | getProperty(...) : String | Test.java:298:35:298:46 | tempDirChild : File |
5842
| Test.java:298:35:298:46 | tempDirChild : File | Test.java:298:35:298:55 | toPath(...) |
5943
| Test.java:313:29:313:101 | new File(...) : File | Test.java:316:35:316:46 | tempDirChild : File |
6044
| Test.java:313:38:313:73 | getProperty(...) : String | Test.java:313:29:313:101 | new File(...) : File |
61-
| Test.java:313:38:313:73 | getProperty(...) : String | Test.java:316:35:316:46 | tempDirChild : File |
6245
| Test.java:316:35:316:46 | tempDirChild : File | Test.java:316:35:316:55 | toPath(...) |
6346
| Test.java:322:29:322:101 | new File(...) : File | Test.java:326:35:326:46 | tempDirChild : File |
6447
| Test.java:322:38:322:73 | getProperty(...) : String | Test.java:322:29:322:101 | new File(...) : File |
65-
| Test.java:322:38:322:73 | getProperty(...) : String | Test.java:326:35:326:46 | tempDirChild : File |
6648
| Test.java:326:35:326:46 | tempDirChild : File | Test.java:326:35:326:55 | toPath(...) |
49+
| Test.java:350:29:350:101 | new File(...) : File | Test.java:355:35:355:46 | tempDirChild : File |
50+
| Test.java:350:38:350:73 | getProperty(...) : String | Test.java:350:29:350:101 | new File(...) : File |
51+
| Test.java:355:35:355:46 | tempDirChild : File | Test.java:355:35:355:55 | toPath(...) |
52+
| Test.java:361:29:361:101 | new File(...) : File | Test.java:366:35:366:46 | tempDirChild : File |
53+
| Test.java:361:38:361:73 | getProperty(...) : String | Test.java:361:29:361:101 | new File(...) : File |
54+
| Test.java:366:35:366:46 | tempDirChild : File | Test.java:366:35:366:55 | toPath(...) |
6755
nodes
6856
| Files.java:10:24:10:69 | new File(...) : File | semmle.label | new File(...) : File |
6957
| Files.java:10:33:10:68 | getProperty(...) : String | semmle.label | getProperty(...) : String |
@@ -133,6 +121,14 @@ nodes
133121
| Test.java:322:38:322:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
134122
| Test.java:326:35:326:46 | tempDirChild : File | semmle.label | tempDirChild : File |
135123
| Test.java:326:35:326:55 | toPath(...) | semmle.label | toPath(...) |
124+
| Test.java:350:29:350:101 | new File(...) : File | semmle.label | new File(...) : File |
125+
| Test.java:350:38:350:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
126+
| Test.java:355:35:355:46 | tempDirChild : File | semmle.label | tempDirChild : File |
127+
| Test.java:355:35:355:55 | toPath(...) | semmle.label | toPath(...) |
128+
| Test.java:361:29:361:101 | new File(...) : File | semmle.label | new File(...) : File |
129+
| Test.java:361:38:361:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
130+
| Test.java:366:35:366:46 | tempDirChild : File | semmle.label | tempDirChild : File |
131+
| Test.java:366:35:366:55 | toPath(...) | semmle.label | toPath(...) |
136132
subpaths
137133
#select
138134
| Files.java:10:33:10:68 | getProperty(...) | Files.java:10:33:10:68 | getProperty(...) : String | Files.java:15:17:15:23 | tempDir | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Files.java:10:33:10:68 | getProperty(...) | system temp directory |
@@ -155,3 +151,5 @@ subpaths
155151
| Test.java:294:38:294:73 | getProperty(...) | Test.java:294:38:294:73 | getProperty(...) : String | Test.java:298:35:298:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:294:38:294:73 | getProperty(...) | system temp directory |
156152
| Test.java:313:38:313:73 | getProperty(...) | Test.java:313:38:313:73 | getProperty(...) : String | Test.java:316:35:316:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:313:38:313:73 | getProperty(...) | system temp directory |
157153
| Test.java:322:38:322:73 | getProperty(...) | Test.java:322:38:322:73 | getProperty(...) : String | Test.java:326:35:326:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:322:38:322:73 | getProperty(...) | system temp directory |
154+
| Test.java:350:38:350:73 | getProperty(...) | Test.java:350:38:350:73 | getProperty(...) : String | Test.java:355:35:355:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:350:38:350:73 | getProperty(...) | system temp directory |
155+
| Test.java:361:38:361:73 | getProperty(...) | Test.java:361:38:361:73 | getProperty(...) : String | Test.java:366:35:366:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:361:38:361:73 | getProperty(...) | system temp directory |

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,4 +344,26 @@ void safeBecauseInvertedFileSeperatorCheck() throws IOException {
344344
Files.createDirectory(tempDirChild.toPath());
345345
}
346346
}
347+
348+
void vulnerableBecauseFileSeparatorCheckElseCase() throws IOException {
349+
// GIVEN:
350+
File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory");
351+
352+
if (File.separatorChar == '\\') {
353+
Files.createDirectory(tempDirChild.toPath()); // Safe
354+
} else {
355+
Files.createDirectory(tempDirChild.toPath()); // Vulnerable
356+
}
357+
}
358+
359+
void vulnerableBecauseInvertedFileSeperatorCheckElseCase() throws IOException {
360+
// GIVEN:
361+
File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory");
362+
363+
if (File.separatorChar != '/') {
364+
Files.createDirectory(tempDirChild.toPath()); // Safe
365+
} else {
366+
Files.createDirectory(tempDirChild.toPath()); // Vulnerable
367+
}
368+
}
347369
}

0 commit comments

Comments
 (0)