From 27535216502aba81889c3a76ee45861ea1adcbec Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Wed, 6 Apr 2022 10:07:44 -0400 Subject: [PATCH] Java: Fix Local Temp File/Dir Incorrect Guard Logic Resolves https://github.com/github/codeql/pull/8032#discussion_r841723906 --- java/ql/lib/semmle/code/java/os/OSCheck.qll | 13 ++++--- .../TempDirLocalInformationDisclosure.ql | 26 ++++++-------- .../src/Security/CWE/CWE-200/TempDirUtils.qll | 1 - ...-06-local-temp-file-or-directory-bugfix.md | 4 +++ ...TempDirLocalInformationDisclosure.expected | 34 +++++++++---------- .../security/CWE-200/semmle/tests/Test.java | 22 ++++++++++++ 6 files changed, 61 insertions(+), 39 deletions(-) create mode 100644 java/ql/src/change-notes/2022-04-06-local-temp-file-or-directory-bugfix.md diff --git a/java/ql/lib/semmle/code/java/os/OSCheck.qll b/java/ql/lib/semmle/code/java/os/OSCheck.qll index f78086476dea..d43e2015705b 100644 --- a/java/ql/lib/semmle/code/java/os/OSCheck.qll +++ b/java/ql/lib/semmle/code/java/os/OSCheck.qll @@ -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" ]) } } diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index e4a4786a09a9..01d60f1062ed 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql +++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql @@ -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) } } @@ -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 } } diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll index a2ee4fc13d17..1984f16a58c0 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll +++ b/java/ql/src/Security/CWE/CWE-200/TempDirUtils.qll @@ -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 isTaintPropagatingFileTransformation(node1.asExpr(), node2.asExpr()) } diff --git a/java/ql/src/change-notes/2022-04-06-local-temp-file-or-directory-bugfix.md b/java/ql/src/change-notes/2022-04-06-local-temp-file-or-directory-bugfix.md new file mode 100644 index 000000000000..52cb8c5fc63b --- /dev/null +++ b/java/ql/src/change-notes/2022-04-06-local-temp-file-or-directory-bugfix.md @@ -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. diff --git a/java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected b/java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected index 5d471dcd671e..0791e49c71b0 100644 --- a/java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected +++ b/java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected @@ -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 | @@ -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 | @@ -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 | @@ -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 | diff --git a/java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java b/java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java index 3ea5fe3e112f..e1ec05ac51c7 100644 --- a/java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java +++ b/java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java @@ -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 + } + } }