From 39828fd5961aa89f1ae81f5ba739f7de5ed933d3 Mon Sep 17 00:00:00 2001 From: Jonathan Leitschuh Date: Mon, 14 Feb 2022 16:52:23 -0500 Subject: [PATCH] Apply OS guard checks to TempDirLocalInformationDisclosure --- .../TempDirLocalInformationDisclosure.ql | 20 +++++ .../CWE/CWE-200/TempDirUsageSafe.java | 6 +- .../src/change-notes/2022-02-14-os-guards.md | 6 ++ ...TempDirLocalInformationDisclosure.expected | 9 ++ .../security/CWE-200/semmle/tests/Test.java | 17 ++++ .../CWE-200/semmle/tests/TestSafe.java | 88 +++++++++++++++++++ 6 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 java/ql/src/change-notes/2022-02-14-os-guards.md create mode 100644 java/ql/test/query-tests/security/CWE-200/semmle/tests/TestSafe.java diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql index a3faddf6ab7..6582a446f4f 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql +++ b/java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql @@ -11,6 +11,7 @@ */ import java +import semmle.code.java.os.OSCheck import TempDirUtils import DataFlow::PathGraph import semmle.code.java.dataflow.TaintTracking2 @@ -102,6 +103,21 @@ private class FileCreateTempFileSink extends FileCreationSink { } } +/** + * A guard that checks what OS the program is running on. + */ +abstract private class OsBarrierGuard extends DataFlow::BarrierGuard { } + +private class IsUnixBarrierGuard extends OsBarrierGuard instanceof IsUnixGuard { + override predicate checks(Expr e, boolean branch) { + this.controls(e.getBasicBlock(), branch.booleanNot()) + } +} + +private class IsWindowsBarrierGuard extends OsBarrierGuard instanceof IsWindowsGuard { + override predicate checks(Expr e, boolean branch) { this.controls(e.getBasicBlock(), branch) } +} + private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Configuration { TempDirSystemGetPropertyToCreateConfig() { this = "TempDirSystemGetPropertyToCreateConfig" } @@ -129,6 +145,10 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf sanitizer.asExpr() = sanitisingMethodAccess.getArgument(0) ) } + + override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) { + guard instanceof OsBarrierGuard + } } /** diff --git a/java/ql/src/Security/CWE/CWE-200/TempDirUsageSafe.java b/java/ql/src/Security/CWE/CWE-200/TempDirUsageSafe.java index f44ead7accb..75efa6af6ec 100644 --- a/java/ql/src/Security/CWE/CWE-200/TempDirUsageSafe.java +++ b/java/ql/src/Security/CWE/CWE-200/TempDirUsageSafe.java @@ -1,10 +1,14 @@ import java.io.File; import java.io.IOException; +import java.io.UncheckedIOException; import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.attribute.PosixFilePermission; import java.nio.file.attribute.PosixFilePermissions; + import java.util.EnumSet; + public class TempDirUsageSafe { void exampleSafe() throws IOException { Path temp1 = Files.createTempFile("random", ".txt"); // GOOD: File has permissions `-rw-------` @@ -30,7 +34,7 @@ public class TempDirUsageSafe { createTempFile(tempChildFile.toPath()); // GOOD: Good has permissions `-rw-------` } - static void createTempFile(Path tempDir) { + static void createTempFile(Path tempDirChild) { try { if (tempDirChild.getFileSystem().supportedFileAttributeViews().contains("posix")) { // Explicit permissions setting is only required on unix-like systems because diff --git a/java/ql/src/change-notes/2022-02-14-os-guards.md b/java/ql/src/change-notes/2022-02-14-os-guards.md new file mode 100644 index 00000000000..5dea181ca1a --- /dev/null +++ b/java/ql/src/change-notes/2022-02-14-os-guards.md @@ -0,0 +1,6 @@ +--- +category: minorAnalysis +--- + * Add new guards `IsWindowsGuard` and `IsUnixGuard` to detect OS specific guards. + * Update "Local information disclosure in a temporary directory" (`java/local-temp-file-or-directory-information-disclosure`) to remove false-positives when OS is properly used as logical guard. + \ No newline at end of file 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 7c21c3667a3..24b63c304a5 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 @@ -52,6 +52,10 @@ edges | Test.java:258:38:258:73 | getProperty(...) : String | Test.java:258:29:258:109 | new File(...) : File | | Test.java:258:38:258:73 | getProperty(...) : String | Test.java:261:33:261:44 | tempDirChild : File | | Test.java:261:33:261:44 | tempDirChild : File | Test.java:261:33:261:53 | toPath(...) | +| Test.java:292:29:292:101 | new File(...) : File | Test.java:296:35:296:46 | tempDirChild : File | +| Test.java:292:38:292:73 | getProperty(...) : String | Test.java:292:29:292:101 | new File(...) : File | +| Test.java:292:38:292:73 | getProperty(...) : String | Test.java:296:35:296:46 | tempDirChild : File | +| Test.java:296:35:296:46 | tempDirChild : File | Test.java:296:35:296: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 | @@ -109,6 +113,10 @@ nodes | Test.java:261:33:261:44 | tempDirChild : File | semmle.label | tempDirChild : File | | Test.java:261:33:261:53 | toPath(...) | semmle.label | toPath(...) | | Test.java:268:25:268:63 | createTempFile(...) | semmle.label | createTempFile(...) | +| Test.java:292:29:292:101 | new File(...) : File | semmle.label | new File(...) : File | +| Test.java:292:38:292:73 | getProperty(...) : String | semmle.label | getProperty(...) : String | +| Test.java:296:35:296:46 | tempDirChild : File | semmle.label | tempDirChild : File | +| Test.java:296:35:296: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 | @@ -128,3 +136,4 @@ subpaths | Test.java:226:38:226:73 | getProperty(...) | Test.java:226:38:226:73 | getProperty(...) : String | Test.java:229:26:229:46 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:226:38:226:73 | getProperty(...) | system temp directory | | Test.java:247:38:247:73 | getProperty(...) | Test.java:247:38:247:73 | getProperty(...) : String | Test.java:250:31:250:51 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:247:38:247:73 | getProperty(...) | system temp directory | | Test.java:258:38:258:73 | getProperty(...) | Test.java:258:38:258:73 | getProperty(...) : String | Test.java:261:33:261:53 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:258:38:258:73 | getProperty(...) | system temp directory | +| Test.java:292:38:292:73 | getProperty(...) | Test.java:292:38:292:73 | getProperty(...) : String | Test.java:296:35:296:55 | toPath(...) | Local information disclosure vulnerability from $@ due to use of file or directory readable by other local users. | Test.java:292:38:292: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 b5b708692f1..c458dafd7e6 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 @@ -279,4 +279,21 @@ public class Test { File tempDir = new File(System.getProperty("java.io.tmpdir")); tempDir.mkdirs(); } + + void safeBecauseWindows() { + File tempDir = new File(System.getProperty("java.io.tmpdir"), "child"); + if (System.getProperty("os.name").toLowerCase().contains("windows")) { + tempDir.mkdir(); // Safe on windows + } + } + + void vulnerableBecauseInvertedPosixCheck() throws IOException { + // GIVEN: + File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory"); + + // Oops, this check should be inverted + if (tempDirChild.toPath().getFileSystem().supportedFileAttributeViews().contains("posix")) { + Files.createDirectory(tempDirChild.toPath()); // Creates with permissions 'drwxr-xr-x' + } + } } diff --git a/java/ql/test/query-tests/security/CWE-200/semmle/tests/TestSafe.java b/java/ql/test/query-tests/security/CWE-200/semmle/tests/TestSafe.java new file mode 100644 index 00000000000..8ac7e8ac0e8 --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-200/semmle/tests/TestSafe.java @@ -0,0 +1,88 @@ +import java.io.File; +import java.io.IOException; +import java.io.UncheckedIOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.attribute.PosixFilePermission; +import java.nio.file.attribute.PosixFilePermissions; + +import java.util.EnumSet; + +public class TestSafe { + /* + * An example of a safe use of createFile or createDirectory if your code must support windows and unix-like systems. + */ + void exampleSafeWithWindowsSupportFile() { + // Creating a temporary file with a non-randomly generated name + File tempChildFile = new File(System.getProperty("java.io.tmpdir"), "/child-create-file.txt"); + createTempFile(tempChildFile.toPath()); // GOOD: Good has permissions `-rw-------` + } + + static void createTempFile(Path tempDirChild) { + try { + if (tempDirChild.getFileSystem().supportedFileAttributeViews().contains("posix")) { + // Explicit permissions setting is only required on unix-like systems because + // the temporary directory is shared between all users. + // This is not necessary on Windows, each user has their own temp directory + final EnumSet posixFilePermissions = + EnumSet.of( + PosixFilePermission.OWNER_READ, + PosixFilePermission.OWNER_WRITE + ); + if (!Files.exists(tempDirChild)) { + Files.createFile( + tempDirChild, + PosixFilePermissions.asFileAttribute(posixFilePermissions) + ); // GOOD: Directory has permissions `-rw-------` + } else { + Files.setPosixFilePermissions( + tempDirChild, + posixFilePermissions + ); // GOOD: Good has permissions `-rw-------`, or will throw an exception if this fails + } + } else if (!Files.exists(tempDirChild)) { + // On Windows, we still need to create the directory, when it doesn't already exist. + Files.createDirectory(tempDirChild); // GOOD: Windows doesn't share the temp directory between users + } + } catch (IOException exception) { + throw new UncheckedIOException("Failed to create temp file", exception); + } + } + + void exampleSafeWithWindowsSupportDirectory() { + File tempDirChildDir = new File(System.getProperty("java.io.tmpdir"), "/child-dir"); + createTempDirectories(tempDirChildDir.toPath()); // GOOD: Directory has permissions `drwx------` + } + + static void createTempDirectories(Path tempDirChild) { + try { + if (tempDirChild.getFileSystem().supportedFileAttributeViews().contains("posix")) { + // Explicit permissions setting is only required on unix-like systems because + // the temporary directory is shared between all users. + // This is not necessary on Windows, each user has their own temp directory + final EnumSet posixFilePermissions = + EnumSet.of( + PosixFilePermission.OWNER_READ, + PosixFilePermission.OWNER_WRITE, + PosixFilePermission.OWNER_EXECUTE + ); + if (!Files.exists(tempDirChild)) { + Files.createDirectories( + tempDirChild, + PosixFilePermissions.asFileAttribute(posixFilePermissions) + ); // GOOD: Directory has permissions `drwx------` + } else { + Files.setPosixFilePermissions( + tempDirChild, + posixFilePermissions + ); // GOOD: Good has permissions `drwx------`, or will throw an exception if this fails + } + } else if (!Files.exists(tempDirChild)) { + // On Windows, we still need to create the directory, when it doesn't already exist. + Files.createDirectories(tempDirChild); // GOOD: Windows doesn't share the temp directory between users + } + } catch (IOException exception) { + throw new UncheckedIOException("Failed to create temp dir", exception); + } + } +}