hohn/codeql
1
0
Fork 0
mirror of https://github.com/github/codeql.git synced 2025-12-24 04:36:35 +01:00
Code Issues Packages Projects Releases Wiki Activity

Apply OS guard checks to TempDirLocalInformationDisclosure

Browse Source
This commit is contained in:
Jonathan Leitschuh
2022-02-14 16:52:23 -05:00
parent cd073a2173
commit 39828fd596
6 changed files with 145 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
View File

@@ -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
}
}
/**

6
java/ql/src/Security/CWE/CWE-200/TempDirUsageSafe.java
View File

@@ -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

6
java/ql/src/change-notes/2022-02-14-os-guards.md Normal file
View File

@@ -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.

9
java/ql/test/query-tests/security/CWE-200/semmle/tests/TempDirLocalInformationDisclosure.expected
View File

@@ -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 |

17
java/ql/test/query-tests/security/CWE-200/semmle/tests/Test.java
View File

@@ -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'
}
}
}

88
java/ql/test/query-tests/security/CWE-200/semmle/tests/TestSafe.java Normal file
View File

@@ -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<PosixFilePermission> 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<PosixFilePermission> 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);
}
}
}