Skip to content

Commit f55ef0e

Browse files
JLLeitschuhJLLeitschuh
committed
Apply OS guard checks to TempDirLocalInformationDisclosure
1 parent e5c7e88 commit f55ef0e

File tree

6 files changed

+145
-1
lines changed

6 files changed

+145
-1
lines changed

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
*/
1212

1313
import java
14+
import semmle.code.java.os.OSCheck
1415
import TempDirUtils
1516
import DataFlow::PathGraph
1617
import semmle.code.java.dataflow.TaintTracking2
@@ -102,6 +103,21 @@ private class FileCreateTempFileSink extends FileCreationSink {
102103
}
103104
}
104105

106+
/**
107+
* A guard that checks what OS the program is running on.
108+
*/
109+
abstract private class OsBarrierGuard extends DataFlow::BarrierGuard { }
110+
111+
private class IsUnixBarrierGuard extends OsBarrierGuard instanceof IsUnixGuard {
112+
override predicate checks(Expr e, boolean branch) {
113+
this.controls(e.getBasicBlock(), branch.booleanNot())
114+
}
115+
}
116+
117+
private class IsWindowsBarrierGuard extends OsBarrierGuard instanceof IsWindowsGuard {
118+
override predicate checks(Expr e, boolean branch) { this.controls(e.getBasicBlock(), branch) }
119+
}
120+
105121
private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Configuration {
106122
TempDirSystemGetPropertyToCreateConfig() { this = "TempDirSystemGetPropertyToCreateConfig" }
107123

@@ -129,6 +145,10 @@ private class TempDirSystemGetPropertyToCreateConfig extends TaintTracking::Conf
129145
sanitizer.asExpr() = sanitisingMethodAccess.getArgument(0)
130146
)
131147
}
148+
149+
override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
150+
guard instanceof OsBarrierGuard
151+
}
132152
}
133153

134154
/**

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
import java.io.File;
22
import java.io.IOException;
3+
import java.io.UncheckedIOException;
34
import java.nio.file.Files;
5+
import java.nio.file.Path;
46
import java.nio.file.attribute.PosixFilePermission;
57
import java.nio.file.attribute.PosixFilePermissions;
8+
69
import java.util.EnumSet;
710

11+
812
public class TempDirUsageSafe {
913
void exampleSafe() throws IOException {
1014
Path temp1 = Files.createTempFile("random", ".txt"); // GOOD: File has permissions `-rw-------`
@@ -30,7 +34,7 @@ void exampleSafeWithWindowsSupportFile() {
3034
createTempFile(tempChildFile.toPath()); // GOOD: Good has permissions `-rw-------`
3135
}
3236

33-
static void createTempFile(Path tempDir) {
37+
static void createTempFile(Path tempDirChild) {
3438
try {
3539
if (tempDirChild.getFileSystem().supportedFileAttributeViews().contains("posix")) {
3640
// Explicit permissions setting is only required on unix-like systems because

‎java/ql/src/change-notes/2022-02-14-os-guards.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* Add new guards `IsWindowsGuard` and `IsUnixGuard` to detect OS specific guards.
5+
* 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.
6+

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,10 @@ edges
5252
| Test.java:258:38:258:73 | getProperty(...) : String | Test.java:258:29:258:109 | new File(...) : File |
5353
| Test.java:258:38:258:73 | getProperty(...) : String | Test.java:261:33:261:44 | tempDirChild : File |
5454
| Test.java:261:33:261:44 | tempDirChild : File | Test.java:261:33:261:53 | toPath(...) |
55+
| Test.java:292:29:292:101 | new File(...) : File | Test.java:296:35:296:46 | tempDirChild : File |
56+
| Test.java:292:38:292:73 | getProperty(...) : String | Test.java:292:29:292:101 | new File(...) : File |
57+
| Test.java:292:38:292:73 | getProperty(...) : String | Test.java:296:35:296:46 | tempDirChild : File |
58+
| Test.java:296:35:296:46 | tempDirChild : File | Test.java:296:35:296:55 | toPath(...) |
5559
nodes
5660
| Files.java:10:24:10:69 | new File(...) : File | semmle.label | new File(...) : File |
5761
| Files.java:10:33:10:68 | getProperty(...) : String | semmle.label | getProperty(...) : String |
@@ -109,6 +113,10 @@ nodes
109113
| Test.java:261:33:261:44 | tempDirChild : File | semmle.label | tempDirChild : File |
110114
| Test.java:261:33:261:53 | toPath(...) | semmle.label | toPath(...) |
111115
| Test.java:268:25:268:63 | createTempFile(...) | semmle.label | createTempFile(...) |
116+
| Test.java:292:29:292:101 | new File(...) : File | semmle.label | new File(...) : File |
117+
| Test.java:292:38:292:73 | getProperty(...) : String | semmle.label | getProperty(...) : String |
118+
| Test.java:296:35:296:46 | tempDirChild : File | semmle.label | tempDirChild : File |
119+
| Test.java:296:35:296:55 | toPath(...) | semmle.label | toPath(...) |
112120
subpaths
113121
#select
114122
| 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
128136
| 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 |
129137
| 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 |
130138
| 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 |
139+
| 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 |

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -279,4 +279,21 @@ void notVulnerableCreateOnSystemPropertyDirs() throws IOException {
279279
File tempDir = new File(System.getProperty("java.io.tmpdir"));
280280
tempDir.mkdirs();
281281
}
282+
283+
void safeBecauseWindows() {
284+
File tempDir = new File(System.getProperty("java.io.tmpdir"), "child");
285+
if (System.getProperty("os.name").toLowerCase().contains("windows")) {
286+
tempDir.mkdir(); // Safe on windows
287+
}
288+
}
289+
290+
void vulnerableBecauseInvertedPosixCheck() throws IOException {
291+
// GIVEN:
292+
File tempDirChild = new File(System.getProperty("java.io.tmpdir"), "/child-create-directory");
293+
294+
// Oops, this check should be inverted
295+
if (tempDirChild.toPath().getFileSystem().supportedFileAttributeViews().contains("posix")) {
296+
Files.createDirectory(tempDirChild.toPath()); // Creates with permissions 'drwxr-xr-x'
297+
}
298+
}
282299
}

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

Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
import java.io.File;
2+
import java.io.IOException;
3+
import java.io.UncheckedIOException;
4+
import java.nio.file.Files;
5+
import java.nio.file.Path;
6+
import java.nio.file.attribute.PosixFilePermission;
7+
import java.nio.file.attribute.PosixFilePermissions;
8+
9+
import java.util.EnumSet;
10+
11+
public class TestSafe {
12+
/*
13+
* An example of a safe use of createFile or createDirectory if your code must support windows and unix-like systems.
14+
*/
15+
void exampleSafeWithWindowsSupportFile() {
16+
// Creating a temporary file with a non-randomly generated name
17+
File tempChildFile = new File(System.getProperty("java.io.tmpdir"), "/child-create-file.txt");
18+
createTempFile(tempChildFile.toPath()); // GOOD: Good has permissions `-rw-------`
19+
}
20+
21+
static void createTempFile(Path tempDirChild) {
22+
try {
23+
if (tempDirChild.getFileSystem().supportedFileAttributeViews().contains("posix")) {
24+
// Explicit permissions setting is only required on unix-like systems because
25+
// the temporary directory is shared between all users.
26+
// This is not necessary on Windows, each user has their own temp directory
27+
final EnumSet<PosixFilePermission> posixFilePermissions =
28+
EnumSet.of(
29+
PosixFilePermission.OWNER_READ,
30+
PosixFilePermission.OWNER_WRITE
31+
);
32+
if (!Files.exists(tempDirChild)) {
33+
Files.createFile(
34+
tempDirChild,
35+
PosixFilePermissions.asFileAttribute(posixFilePermissions)
36+
); // GOOD: Directory has permissions `-rw-------`
37+
} else {
38+
Files.setPosixFilePermissions(
39+
tempDirChild,
40+
posixFilePermissions
41+
); // GOOD: Good has permissions `-rw-------`, or will throw an exception if this fails
42+
}
43+
} else if (!Files.exists(tempDirChild)) {
44+
// On Windows, we still need to create the directory, when it doesn't already exist.
45+
Files.createDirectory(tempDirChild); // GOOD: Windows doesn't share the temp directory between users
46+
}
47+
} catch (IOException exception) {
48+
throw new UncheckedIOException("Failed to create temp file", exception);
49+
}
50+
}
51+
52+
void exampleSafeWithWindowsSupportDirectory() {
53+
File tempDirChildDir = new File(System.getProperty("java.io.tmpdir"), "/child-dir");
54+
createTempDirectories(tempDirChildDir.toPath()); // GOOD: Directory has permissions `drwx------`
55+
}
56+
57+
static void createTempDirectories(Path tempDirChild) {
58+
try {
59+
if (tempDirChild.getFileSystem().supportedFileAttributeViews().contains("posix")) {
60+
// Explicit permissions setting is only required on unix-like systems because
61+
// the temporary directory is shared between all users.
62+
// This is not necessary on Windows, each user has their own temp directory
63+
final EnumSet<PosixFilePermission> posixFilePermissions =
64+
EnumSet.of(
65+
PosixFilePermission.OWNER_READ,
66+
PosixFilePermission.OWNER_WRITE,
67+
PosixFilePermission.OWNER_EXECUTE
68+
);
69+
if (!Files.exists(tempDirChild)) {
70+
Files.createDirectories(
71+
tempDirChild,
72+
PosixFilePermissions.asFileAttribute(posixFilePermissions)
73+
); // GOOD: Directory has permissions `drwx------`
74+
} else {
75+
Files.setPosixFilePermissions(
76+
tempDirChild,
77+
posixFilePermissions
78+
); // GOOD: Good has permissions `drwx------`, or will throw an exception if this fails
79+
}
80+
} else if (!Files.exists(tempDirChild)) {
81+
// On Windows, we still need to create the directory, when it doesn't already exist.
82+
Files.createDirectories(tempDirChild); // GOOD: Windows doesn't share the temp directory between users
83+
}
84+
} catch (IOException exception) {
85+
throw new UncheckedIOException("Failed to create temp dir", exception);
86+
}
87+
}
88+
}

0 commit comments

Comments
 (0)