Skip to content

Java: Fix Local Temp File/Dir Incorrect Guard Logic #8681

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 7, 2022
Merged

Java: Fix Local Temp File/Dir Incorrect Guard Logic #8681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions java/ql/lib/semmle/code/java/os/OSCheck.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,23 +112,26 @@ private predicate isOsFromApacheCommons(FieldAccess fa, string fieldNamePattern)
}

private class IsWindowsFromApacheCommons extends IsWindowsGuard instanceof FieldAccess {
IsWindowsFromApacheCommons() { isOsFromApacheCommons(this, "IS_OS_WINDOWS") }
IsWindowsFromApacheCommons() { isOsFromApacheCommons(this, "IS\\_OS\\_WINDOWS") }
}

private class IsSpecificWindowsVariantFromApacheCommons extends IsSpecificWindowsVariant instanceof FieldAccess {
IsSpecificWindowsVariantFromApacheCommons() { isOsFromApacheCommons(this, "IS_OS_WINDOWS_%") }
IsSpecificWindowsVariantFromApacheCommons() {
isOsFromApacheCommons(this, "IS\\_OS\\_WINDOWS\\_%")
}
}

private class IsUnixFromApacheCommons extends IsUnixGuard instanceof FieldAccess {
IsUnixFromApacheCommons() { isOsFromApacheCommons(this, "IS_OS_UNIX") }
IsUnixFromApacheCommons() { isOsFromApacheCommons(this, "IS\\_OS\\_UNIX") }
}

private class IsSpecificUnixVariantFromApacheCommons extends IsSpecificUnixVariant instanceof FieldAccess {
IsSpecificUnixVariantFromApacheCommons() {
isOsFromApacheCommons(this,
[
"IS_OS_AIX", "IS_OS_HP_UX", "IS_OS_IRIX", "IS_OS_LINUX", "IS_OS_MAC%", "IS_OS_FREE_BSD",
"IS_OS_OPEN_BSD", "IS_OS_NET_BSD", "IS_OS_SOLARIS", "IS_OS_SUN_OS", "IS_OS_ZOS"
"IS\\_OS\\_AIX", "IS\\_OS\\_HP\\_UX", "IS\\_OS\\_IRIX", "IS\\_OS\\_LINUX", "IS\\_OS\\_MAC%",
"IS\\_OS\\_FREE\\_BSD", "IS\\_OS\\_OPEN\\_BSD", "IS\\_OS\\_NET\\_BSD", "IS\\_OS\\_SOLARIS",
"IS\\_OS\\_SUN\\_OS", "IS\\_OS\\_ZOS"
])
}
}
Expand Down
26 changes: 11 additions & 15 deletions java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,23 +105,21 @@ private class FileCreateTempFileSink extends FileCreationSink {
}

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

private class IsNotUnixBarrierGuard extends WindowsOsBarrierGuard instanceof IsUnixGuard {
override predicate checks(Expr e, boolean branch) {
this.controls(e.getBasicBlock(), branch.booleanNot())
}
private class IsNotUnixSanitizer extends WindowsOsSanitizer {
IsNotUnixSanitizer() { any(IsUnixGuard guard).controls(this.asExpr().getBasicBlock(), false) }
}

private class IsWindowsBarrierGuard extends WindowsOsBarrierGuard instanceof IsWindowsGuard {
override predicate checks(Expr e, boolean branch) { this.controls(e.getBasicBlock(), branch) }
private class IsWindowsSanitizer extends WindowsOsSanitizer {
IsWindowsSanitizer() { any(IsWindowsGuard guard).controls(this.asExpr().getBasicBlock(), true) }
}

private class IsSpecificWindowsBarrierGuard extends WindowsOsBarrierGuard instanceof IsSpecificWindowsVariant {
override predicate checks(Expr e, boolean branch) {
branch = true and this.controls(e.getBasicBlock(), branch)
private class IsSpecificWindowsSanitizer extends WindowsOsSanitizer {
IsSpecificWindowsSanitizer() {
any(IsSpecificWindowsVariant guard).controls(this.asExpr().getBasicBlock(), true)
}
}

Expand Down Expand Up @@ -155,10 +153,8 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
exists(FilesSanitizingCreationMethodAccess sanitisingMethodAccess |
sanitizer.asExpr() = sanitisingMethodAccess.getArgument(0)
)
}

override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
guard instanceof WindowsOsBarrierGuard
or
sanitizer instanceof WindowsOsSanitizer
}
}

Expand Down
1 change: 0 additions & 1 deletion java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ private predicate isTaintPropagatingFileTransformation(Expr expSource, Expr expr
* For example, `taintedFile.getCanonicalFile()` is itself tainted.
*/
predicate isAdditionalFileTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
isFileConstructorArgument(node1.asExpr(), node2.asExpr(), _) or
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to comment on this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this, I was having two paths get generated. When rendered, one of the paths wasn't showing the taint step where new File([tainted], [other]) was being called, but the other was. I'm not sure if it's because isAdditionalFileTaintStep is written wrong, or not. But regardless, this line handles the case I was trying to cover anyways:

codeql/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll

Line 331 in 879b8a1

"java.io;File;false;File;;;Argument[0];Argument[-1];taint",

isTaintPropagatingFileTransformation(node1.asExpr(), node2.asExpr())
}

Expand Down
4 changes: 4 additions & 0 deletions java/ql/src/change-notes/2022-04-06-local-temp-file-or-directory-bugfix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* 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.
34 changes: 16 additions & 18 deletions ...test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,26 @@
edges
| Files.java:10:24:10:69 | new File(...) : File | Files.java:14:37:14:43 | baseDir : File |
| Files.java:10:24:10:69 | new File(...) : File | Files.java:15:17:15:23 | tempDir |
| Files.java:10:33:10:68 | getProperty(...) : String | Files.java:10:24:10:69 | new File(...) : File |
| Files.java:10:33:10:68 | getProperty(...) : String | Files.java:14:37:14:43 | baseDir : File |
| Files.java:10:33:10:68 | getProperty(...) : String | Files.java:15:17:15:23 | tempDir |
| Files.java:14:28:14:64 | new File(...) : File | Files.java:15:17:15:23 | tempDir |
| Files.java:14:37:14:43 | baseDir : File | Files.java:14:28:14:64 | new File(...) : File |
| Test.java:36:24:36:69 | new File(...) : File | Test.java:39:63:39:69 | tempDir |
| Test.java:36:33:36:68 | getProperty(...) : String | Test.java:36:24:36:69 | new File(...) : File |
| Test.java:36:33:36:68 | getProperty(...) : String | Test.java:39:63:39:69 | tempDir |
| Test.java:50:29:50:94 | new File(...) : File | Test.java:53:63:53:74 | tempDirChild |
| Test.java:50:38:50:83 | new File(...) : File | Test.java:50:29:50:94 | new File(...) : File |
| Test.java:50:38:50:83 | new File(...) : File | Test.java:53:63:53:74 | tempDirChild |
| Test.java:50:47:50:82 | getProperty(...) : String | Test.java:50:38:50:83 | new File(...) : File |
| Test.java:50:47:50:82 | getProperty(...) : String | Test.java:53:63:53:74 | tempDirChild |
| Test.java:61:24:61:69 | new File(...) : File | Test.java:64:63:64:69 | tempDir |
| Test.java:61:33:61:68 | getProperty(...) : String | Test.java:61:24:61:69 | new File(...) : File |
| Test.java:61:33:61:68 | getProperty(...) : String | Test.java:64:63:64:69 | tempDir |
| Test.java:75:24:75:69 | new File(...) : File | Test.java:78:63:78:69 | tempDir |
| Test.java:75:33:75:68 | getProperty(...) : String | Test.java:75:24:75:69 | new File(...) : File |
| Test.java:75:33:75:68 | getProperty(...) : String | Test.java:78:63:78:69 | tempDir |
| Test.java:110:29:110:84 | new File(...) : File | Test.java:113:9:113:20 | tempDirChild |
| Test.java:110:38:110:73 | getProperty(...) : String | Test.java:110:29:110:84 | new File(...) : File |
| Test.java:110:38:110:73 | getProperty(...) : String | Test.java:113:9:113:20 | tempDirChild |
| Test.java:134:29:134:84 | new File(...) : File | Test.java:137:9:137:20 | tempDirChild |
| Test.java:134:38:134:73 | getProperty(...) : String | Test.java:134:29:134:84 | new File(...) : File |
| Test.java:134:38:134:73 | getProperty(...) : String | Test.java:137:9:137:20 | tempDirChild |
| Test.java:158:29:158:88 | new File(...) : File | Test.java:159:21:159:32 | tempDirChild : File |
| Test.java:158:38:158:73 | getProperty(...) : String | Test.java:158:29:158:88 | new File(...) : File |
| Test.java:158:38:158:73 | getProperty(...) : String | Test.java:159:21:159:32 | tempDirChild : File |
| Test.java:159:21:159:32 | tempDirChild : File | Test.java:159:21:159:41 | toPath(...) |
| Test.java:187:29:187:88 | new File(...) : File | Test.java:188:21:188:32 | tempDirChild : File |
| Test.java:187:38:187:73 | getProperty(...) : String | Test.java:187:29:187:88 | new File(...) : File |
| Test.java:187:38:187:73 | getProperty(...) : String | Test.java:188:21:188:32 | tempDirChild : File |
| Test.java:188:21:188:32 | tempDirChild : File | Test.java:188:21:188:41 | toPath(...) |
| Test.java:204:29:204:104 | new File(...) : File | Test.java:204:29:204:113 | toPath(...) : Path |
| Test.java:204:29:204:113 | toPath(...) : Path | Test.java:207:33:207:44 | tempDirChild |
Expand All @@ -42,28 +30,28 @@ edges
| Test.java:216:38:216:73 | getProperty(...) : String | Test.java:216:29:216:102 | new File(...) : File |
| Test.java:228:29:228:100 | new File(...) : File | Test.java:231:26:231:37 | tempDirChild : File |
| Test.java:228:38:228:73 | getProperty(...) : String | Test.java:228:29:228:100 | new File(...) : File |
| Test.java:228:38:228:73 | getProperty(...) : String | Test.java:231:26:231:37 | tempDirChild : File |
| Test.java:231:26:231:37 | tempDirChild : File | Test.java:231:26:231:46 | toPath(...) |
| Test.java:249:29:249:101 | new File(...) : File | Test.java:252:31:252:42 | tempDirChild : File |
| Test.java:249:38:249:73 | getProperty(...) : String | Test.java:249:29:249:101 | new File(...) : File |
| Test.java:249:38:249:73 | getProperty(...) : String | Test.java:252:31:252:42 | tempDirChild : File |
| Test.java:252:31:252:42 | tempDirChild : File | Test.java:252:31:252:51 | toPath(...) |
| Test.java:260:29:260:109 | new File(...) : File | Test.java:263:33:263:44 | tempDirChild : File |
| Test.java:260:38:260:73 | getProperty(...) : String | Test.java:260:29:260:109 | new File(...) : File |
| Test.java:260:38:260:73 | getProperty(...) : String | Test.java:263:33:263:44 | tempDirChild : File |
| Test.java:263:33:263:44 | tempDirChild : File | Test.java:263:33:263:53 | toPath(...) |
| Test.java:294:29:294:101 | new File(...) : File | Test.java:298:35:298:46 | tempDirChild : File |
| Test.java:294:38:294:73 | getProperty(...) : String | Test.java:294:29:294:101 | new File(...) : File |
| Test.java:294:38:294:73 | getProperty(...) : String | Test.java:298:35:298:46 | tempDirChild : File |
| Test.java:298:35:298:46 | tempDirChild : File | Test.java:298:35:298:55 | toPath(...) |
| Test.java:313:29:313:101 | new File(...) : File | Test.java:316:35:316:46 | tempDirChild : File |
| Test.java:313:38:313:73 | getProperty(...) : String | Test.java:313:29:313:101 | new File(...) : File |
| Test.java:313:38:313:73 | getProperty(...) : String | Test.java:316:35:316:46 | tempDirChild : File |
| Test.java:316:35:316:46 | tempDirChild : File | Test.java:316:35:316:55 | toPath(...) |
| Test.java:322:29:322:101 | new File(...) : File | Test.java:326:35:326:46 | tempDirChild : File |
| Test.java:322:38:322:73 | getProperty(...) : String | Test.java:322:29:322:101 | new File(...) : File |
| Test.java:322:38:322:73 | getProperty(...) : String | Test.java:326:35:326:46 | tempDirChild : File |
| Test.java:326:35:326:46 | tempDirChild : File | Test.java:326:35:326:55 | toPath(...) |
| Test.java:350:29:350:101 | new File(...) : File | Test.java:355:35:355:46 | tempDirChild : File |
| Test.java:350:38:350:73 | getProperty(...) : String | Test.java:350:29:350:101 | new File(...) : File |
| Test.java:355:35:355:46 | tempDirChild : File | Test.java:355:35:355:55 | toPath(...) |
| Test.java:361:29:361:101 | new File(...) : File | Test.java:366:35:366:46 | tempDirChild : File |
| Test.java:361:38:361:73 | getProperty(...) : String | Test.java:361:29:361:101 | new File(...) : File |
| Test.java:366:35:366:46 | tempDirChild : File | Test.java:366:35:366:55 | toPath(...) |
nodes
| Files.java:10:24:10:69 | new File(...) : File | semmle.label | new File(...) : File |
| Files.java:10:33:10:68 | getProperty(...) : String | semmle.label | getProperty(...) : String |
Expand Down Expand Up @@ -133,6 +121,14 @@ nodes
| Test.java:322:38:322:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
| Test.java:326:35:326:46 | tempDirChild : File | semmle.label | tempDirChild : File |
| Test.java:326:35:326:55 | toPath(...) | semmle.label | toPath(...) |
| Test.java:350:29:350:101 | new File(...) : File | semmle.label | new File(...) : File |
| Test.java:350:38:350:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
| Test.java:355:35:355:46 | tempDirChild : File | semmle.label | tempDirChild : File |
| Test.java:355:35:355:55 | toPath(...) | semmle.label | toPath(...) |
| Test.java:361:29:361:101 | new File(...) : File | semmle.label | new File(...) : File |
| Test.java:361:38:361:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
| Test.java:366:35:366:46 | tempDirChild : File | semmle.label | tempDirChild : File |
| Test.java:366:35:366:55 | toPath(...) | semmle.label | toPath(...) |
subpaths
#select
| 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 |
Expand All @@ -155,3 +151,5 @@ subpaths
| 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 |
| 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 |
| 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 |
| 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 |
| 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 |
22 changes: 22 additions & 0 deletions java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,4 +344,26 @@ void safeBecauseInvertedFileSeperatorCheck() throws IOException {
Files.createDirectory(tempDirChild.toPath());
}
}

void vulnerableBecauseFileSeparatorCheckElseCase() throws IOException {
// GIVEN:
File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory");

if (File.separatorChar == '\\') {
Files.createDirectory(tempDirChild.toPath()); // Safe
} else {
Files.createDirectory(tempDirChild.toPath()); // Vulnerable
}
}

void vulnerableBecauseInvertedFileSeperatorCheckElseCase() throws IOException {
// GIVEN:
File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory");

if (File.separatorChar != '/') {
Files.createDirectory(tempDirChild.toPath()); // Safe
} else {
Files.createDirectory(tempDirChild.toPath()); // Vulnerable
}
}
}