Skip to content

Java: CWE-378: Temp Directory Hijacking Race Condition Vulnerability #4473

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

Open
JLLeitschuh wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Java: CWE-378: Temp Directory Hijacking Race Condition Vulnerability #4473

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
ba727af
Java: CWE-378: Temp Directory Hijacking Race Condition Vulnerability
JLLeitschuh Oct 14, 2020
aea0170
Better temp file deletion and file creation tracking
JLLeitschuh Oct 14, 2020
545eb2c
Fix inverted predicate logic and add additional test cases
JLLeitschuh Oct 15, 2020
253c96f
Improve TempDir hijacking detection with Guard
JLLeitschuh Jan 19, 2022
d9d5b67
Improve warning in TempDirHijackingVulnerability
JLLeitschuh Jan 19, 2022
1fc7629
Add documentation and additional test cases
JLLeitschuh Jan 20, 2022
b42ff13
Improve TempDirHijackingVulnerability message
JLLeitschuh Jan 20, 2022
20bd05b
Add predicate handling `isDirectory` case
JLLeitschuh Mar 9, 2022
442ef83
Add deleteOnExit as safe usage
JLLeitschuh Mar 10, 2022
fbecfdd
Start taint hijacking tracking with `java.io.tmpdir`
JLLeitschuh Mar 15, 2022
8a7d64d
Refactor common logic into TempFileLib
JLLeitschuh Mar 15, 2022
884db9e
Refactor more logic to TempFileLib
JLLeitschuh Mar 15, 2022
6f4ed4b
Apply suggestions from code review
JLLeitschuh Mar 16, 2022
03983f1
Refactor TempDirHijacking to show complete path
JLLeitschuh Mar 16, 2022
37b1e1d
Update to use new getSystemProperty predicate
JLLeitschuh Mar 18, 2022
ac8e1cc
Add additional test cases
JLLeitschuh Mar 18, 2022
84003c1
Fix some false positive paths with FlowState
JLLeitschuh Mar 18, 2022
4b6d1a4
Finalize and document FlowState usage
JLLeitschuh Mar 18, 2022
325d0e1
Add `NullLiteral` flow check for `File.createTempFile`
JLLeitschuh Mar 18, 2022
71f5fc5
Add additional tests and better tracking of 'unsafe use'
JLLeitschuh Mar 29, 2022
140c66e
Add additional tests cases for mkdir wrapper method checking
JLLeitschuh Mar 30, 2022
21bef99
Add release notes
JLLeitschuh Mar 30, 2022
407dd05
Rename localExprFlowPlusInitializers to localExprOrInitializerFlow
JLLeitschuh Mar 30, 2022
0f5a1e7
Expand isDeleteFileExpr to include delete method wrappers
JLLeitschuh Mar 31, 2022
e7f016e
Apply suggestions from code review
JLLeitschuh Apr 25, 2022
3a50253
Fix implicit 'this' use in TempFileLib
JLLeitschuh Apr 28, 2022
cd3662c
Cleanup after rebase on `main`
JLLeitschuh May 3, 2022
a2a7c73
Clean up function naming, documentation, and to some degree code with…
smowton May 9, 2022
b412c7f
Merge pull request #8 from smowton/feat/JLL/java/CWE-378
JLLeitschuh Jun 1, 2022
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
Prev Previous commit
Next Next commit
Fix some false positive paths with FlowState
  • Loading branch information
@JLLeitschuh
JLLeitschuh committed May 3, 2022
commit 84003c1e44291ba98d447a0fa8dd58170598d501
62 changes: 54 additions & 8 deletions java/ql/src/Security/CWE/CWE-378/TempDirHijackingVulnerability.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,29 @@ predicate isDeleteFileExpr(Expr expr) {
expr = any(MethodFileDelete m).getAReference().getQualifier()
}

private class DirHijackingTaintSource extends DataFlow::Node {
DirHijackingTaintSource() {
abstract private class DirHijackingTaintSource extends DataFlow::Node {
abstract DataFlow::FlowState getFlowState();
}

private class FromSystemPropertyFlowState extends DataFlow::FlowState {
FromSystemPropertyFlowState() { this = "FromSystemPropertyFlowState" }
}

private class FileCreateTempFileDirHijackingTaintSource extends DirHijackingTaintSource {
FileCreateTempFileDirHijackingTaintSource() {
this.asExpr() =
any(MethodAccess ma |
ma.getMethod() instanceof MethodFileCreateTempFile and ma.getNumArgument() = 2
Copy link
Copy Markdown
Contributor

@Marcono1234 Marcono1234 Mar 15, 2022

Choose a reason for hiding this comment

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

Might be good to explain why check for = 2, that is, to match the method creating a file in the default OS temp directory.

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Mar 18, 2022

Choose a reason for hiding this comment

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

Done!

)
or
this.asExpr() = getSystemProperty("java.io.tmpdir")
}

override DataFlow::FlowState getFlowState() { result instanceof DataFlow::FlowStateEmpty }
}

private class SystemPropertyDirHijackingTaintSource extends DirHijackingTaintSource {
SystemPropertyDirHijackingTaintSource() { this.asExpr() = getSystemProperty("java.io.tmpdir") }

override DataFlow::FlowState getFlowState() { result instanceof FromSystemPropertyFlowState }
}

private predicate isAdditionalTaintStepCommon(DataFlow::Node node1, DataFlow::Node node2) {
Expand Down Expand Up @@ -76,6 +90,28 @@ private class TempDirHijackingFromDeleteConfig extends DataFlow3::Configuration
}
}

private class TempDirHijackingFromDirectoryCreateToUnsafeUseConfig extends DataFlow4::Configuration {
TempDirHijackingFromDirectoryCreateToUnsafeUseConfig() {
this = "TempDirHijackingFromDirectoryCreateToUnsafeUseConfig"
}

override predicate isSource(DataFlow::Node source) {
isNonThrowingDirectoryCreationExpression(source.asExpr(), _)
}

override predicate isSink(DataFlow::Node sink) { not safeUse(sink.asExpr()) }

override predicate isBarrierGuard(DataFlow::BarrierGuard guard) {
guard instanceof DirectoryCreationBarrierGuard
}
}

private class DirectoryCreationBarrierGuard extends DataFlow::BarrierGuard {
DirectoryCreationBarrierGuard() { isNonThrowingDirectoryCreationExpression(_, this) }

override predicate checks(Expr e, boolean branch) { this.controls(e, branch) }
}

/**
* Holds when there is an expression `unsafeUse` which is an unsafe use of the file that
* is not guarded by a check of the return value of `File::mkdir(s)`.
Expand All @@ -85,7 +121,8 @@ predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse)
any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to `mkdir` or `mkdirs`
sink.asExpr() = ma.getQualifier() and // The method access is on the same object as the sink
g = ma and // The guard is the method access
DataFlow::localExprFlow(sink.asExpr(), unsafeUse) and // There is some flow from the sink to an unsafe use of the File
any(TempDirHijackingFromDirectoryCreateToUnsafeUseConfig c)
.hasFlow(sink, DataFlow::exprNode(unsafeUse)) and // There is some flow from the sink to an unsafe use of the File
unsafeUse != sink.asExpr() and // The unsafe use is not the sink itself
not safeUse(unsafeUse) and // The unsafe use is not a safe use
not g.controls(unsafeUse.getBasicBlock(), true) and
Expand Down Expand Up @@ -172,14 +209,23 @@ private predicate safeUse(Expr e) {
private class TempDirHijackingFullPath extends TaintTracking::Configuration {
TempDirHijackingFullPath() { this = "TempDirHijackingFullPath" }

override predicate isSource(DataFlow::Node source) { source instanceof DirHijackingTaintSource }
override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
exists(DirHijackingTaintSource taintSource |
source = taintSource and state = taintSource.getFlowState()
)
}

override predicate isSink(DataFlow::Node sink) {
isNonThrowingDirectoryCreationExpression(sink.asExpr(), _)
}

override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
isAdditionalTaintStepCommon(node1, node2)
override predicate isAdditionalTaintStep(
DataFlow::Node node1, DataFlow::FlowState state1, DataFlow::Node node2,
DataFlow::FlowState state2
) {
isAdditionalTaintStepCommon(node1, node2) and
state1 instanceof FromSystemPropertyFlowState and
state2 instanceof DataFlow::FlowStateEmpty
}
}

Expand Down
27 changes: 27 additions & 0 deletions .../ql/test/query-tests/security/CWE-378/semmle/tests/TempDirHijackingVulnerability.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,17 @@ edges
| Test.java:146:13:146:58 | new File(...) : File | Test.java:146:13:146:76 | getAbsoluteFile(...) : File |
| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | Test.java:149:9:149:12 | temp |
| Test.java:146:22:146:57 | getProperty(...) : String | Test.java:146:13:146:58 | new File(...) : File |
| Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | temp |
| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | Test.java:171:21:171:30 | this <.field> [safe12temp] : File |
| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | Test.java:171:44:171:53 | this <.field> [safe12temp] : File |
| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File |
| Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp |
| Test.java:171:21:171:30 | safe12temp : File | Test.java:171:44:171:53 | safe12temp |
| Test.java:171:21:171:30 | this <.field> [safe12temp] : File | Test.java:171:21:171:30 | safe12temp : File |
| Test.java:171:44:171:53 | this <.field> [safe12temp] : File | Test.java:171:44:171:53 | safe12temp |
| Test.java:180:21:180:60 | createTempFile(...) : File | Test.java:182:13:182:16 | temp |
| Test.java:190:21:190:60 | createTempFile(...) : File | Test.java:192:13:192:16 | temp |
| Test.java:200:21:200:60 | createTempFile(...) : File | Test.java:202:13:202:16 | temp |
nodes
| Test.java:11:20:11:59 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
| Test.java:13:13:13:16 | temp | semmle.label | temp |
Expand Down Expand Up @@ -48,10 +59,26 @@ nodes
| Test.java:146:13:146:76 | getAbsoluteFile(...) : File | semmle.label | getAbsoluteFile(...) : File |
| Test.java:146:22:146:57 | getProperty(...) : String | semmle.label | getProperty(...) : String |
| Test.java:149:9:149:12 | temp | semmle.label | temp |
| Test.java:157:24:157:63 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
| Test.java:158:38:158:41 | temp | semmle.label | temp |
| Test.java:170:17:170:26 | this <.field> [post update] [safe12temp] : File | semmle.label | this <.field> [post update] [safe12temp] : File |
| Test.java:170:30:170:69 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
| Test.java:171:21:171:30 | safe12temp : File | semmle.label | safe12temp : File |
| Test.java:171:21:171:30 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File |
| Test.java:171:44:171:53 | safe12temp | semmle.label | safe12temp |
| Test.java:171:44:171:53 | this <.field> [safe12temp] : File | semmle.label | this <.field> [safe12temp] : File |
| Test.java:180:21:180:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
| Test.java:182:13:182:16 | temp | semmle.label | temp |
| Test.java:190:21:190:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
| Test.java:192:13:192:16 | temp | semmle.label | temp |
| Test.java:200:21:200:60 | createTempFile(...) : File | semmle.label | createTempFile(...) : File |
| Test.java:202:13:202:16 | temp | semmle.label | temp |
subpaths
#select
| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:9:18:33 | ...=... | here |
| Test.java:13:13:13:16 | temp | Test.java:11:20:11:59 | createTempFile(...) : File | Test.java:13:13:13:16 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:12:13:12:16 | temp | delete here | Test.java:18:30:18:33 | temp | here |
| Test.java:24:9:24:12 | temp | Test.java:22:21:22:60 | createTempFile(...) : File | Test.java:24:9:24:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:23:9:23:12 | temp | delete here | Test.java:25:16:25:19 | temp | here |
| Test.java:131:9:131:12 | temp | Test.java:129:71:129:106 | getProperty(...) : String | Test.java:131:9:131:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:130:9:130:12 | temp | delete here | Test.java:132:16:132:19 | temp | here |
| Test.java:149:9:149:12 | temp | Test.java:146:22:146:57 | getProperty(...) : String | Test.java:149:9:149:12 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:148:9:148:12 | temp | delete here | Test.java:150:16:150:19 | temp | here |
| Test.java:158:38:158:41 | temp | Test.java:157:24:157:63 | createTempFile(...) : File | Test.java:158:38:158:41 | temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:158:21:158:24 | temp | delete here | Test.java:163:16:163:19 | temp | here |
| Test.java:171:44:171:53 | safe12temp | Test.java:170:30:170:69 | createTempFile(...) : File | Test.java:171:44:171:53 | safe12temp | Local temporary directory hijacking race condition between $@ and this directory creation call. As such, the directory usage $@ may have been hijacked by another local user. | Test.java:171:21:171:30 | safe12temp | delete here | Test.java:176:16:176:25 | safe12temp | here |
16 changes: 16 additions & 0 deletions java/ql/test/query-tests/security/CWE-378/semmle/tests/Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,5 +206,21 @@ File safe15() throws IOException {
throw new IOException(absolutePath);
}
}

File vulnerable4() throws IOException {
File temp = new File(System.getProperty("java.io.tmpdir"));
ensureDirectory(temp);
File workDir = File.createTempFile("test", "directory", temp);
if (!workDir.delete()) {
throw new IOException("Could not delete temp file: " + workDir.getAbsolutePath());
}
ensureDirectory(workDir);
return temp;
}

private static void ensureDirectory(File dir) throws IOException {
if (!dir.mkdirs() && !dir.isDirectory()) {
throw new IOException("Mkdirs failed to create " + dir.toString());
}
}
Comment on lines +210 to +225
Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Mar 18, 2022

Choose a reason for hiding this comment

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

Before, when I wasn't using using FlowState, this actually flagged two vulnerabilities instead of one. The first one for the call to ensureDirectory(temp); and the second for the call to ensureDirectory(workDir);. Because I'm now using flow labels, it correctly, identifies the single path that is actually a vulnerability here.

}