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
Refactor more logic to TempFileLib
  • Loading branch information
@JLLeitschuh
JLLeitschuh committed May 3, 2022
commit 884db9ee334a92531df2aff20bb29c0b258f1b6f
10 changes: 10 additions & 0 deletions java/ql/lib/semmle/code/java/security/TempFileLib.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,16 @@ class MethodFileCreateTempFile extends Method {
}
}

/**
* All methods on the class `java.io.File` that create directories.
Comment thread
JLLeitschuh marked this conversation as resolved.
Outdated
*/
class MethodFileCreatesDirs extends Method {
MethodFileCreatesDirs() {
getDeclaringType() instanceof TypeFile and

Check notice

Code scanning

Using implicit `this`

Use of implicit `this`.
Copy link
Copy Markdown
Contributor

@aschackmull aschackmull Apr 27, 2022

Choose a reason for hiding this comment

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

please fix

hasName(["mkdir", "mkdirs"])

Check notice

Code scanning

Using implicit `this`

Use of implicit `this`.
Copy link
Copy Markdown
Contributor

@aschackmull aschackmull Apr 27, 2022

Choose a reason for hiding this comment

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

please fix

}
}

private class TemporaryFileFlow extends SummaryModelCsv {
Copy link
Copy Markdown
Contributor

@smowton smowton Apr 20, 2022

Choose a reason for hiding this comment

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

Any file that defines model rows like this needs a private import at https://github.com/github/codeql/blob/main/java/ql/lib/semmle/code/java/dataflow/ExternalFlow.qll#L79

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Apr 20, 2022

Choose a reason for hiding this comment

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

I intentionally don't want this to be defined at a global scope, yet. I don't know what the impact of this would be on any of the path normalization (path traversal) based queries and I don't really want to deal with that rn

Copy link
Copy Markdown
Contributor

@aschackmull aschackmull Apr 26, 2022

Choose a reason for hiding this comment

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

If it's only imported in some queries and not in others then it will lead to a whole bunch of cache invalidation and reevaluation of major parts of the libraries, which isn't acceptable. So if these steps only needs to be active for certain queries on an opt-in basis, then they can't use the csv format and must be defined fully in QL as pairs of data-flow nodes and directly mentioned by the configurations that wants to opt-in.

Copy link
Copy Markdown
Contributor Author

@JLLeitschuh JLLeitschuh Apr 26, 2022

Choose a reason for hiding this comment

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

I've broken this into an independent PR to allow this change to be tested independently. Thus, if it introduces FP into other queries, that can be analyzed independently: #8884

override predicate row(string row) {
// qualifier to return
Expand Down
3 changes: 1 addition & 2 deletions java/ql/src/Security/CWE/CWE-200/TempDirLocalInformationDisclosure.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ abstract private class MethodFileSystemFileCreation extends Method {
MethodFileSystemFileCreation() { this.getDeclaringType() instanceof TypeFile }
}

private class MethodFileDirectoryCreation extends MethodFileSystemFileCreation {
MethodFileDirectoryCreation() { this.hasName(["mkdir", "mkdirs"]) }
private class MethodFileDirectoryCreation extends MethodFileSystemFileCreation instanceof MethodFileCreatesDirs {
}

private class MethodFileFileCreation extends MethodFileSystemFileCreation {
Expand Down
21 changes: 8 additions & 13 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 @@ -13,20 +13,11 @@ import semmle.code.java.dataflow.FlowSources
import semmle.code.java.security.TempFileLib
import DataFlow::PathGraph

private class MethodFileMkdir extends Method {
MethodFileMkdir() {
getDeclaringType() instanceof TypeFile and
hasName("mkdir")
or
hasName("mkdirs")
}
}

/**
* An expression that will create a directory without throwing an exception if a file/directory already exists.
*/
private predicate isNonThrowingDirectoryCreationExpression(Expr expr, MethodAccess creationCall) {
creationCall.getMethod() instanceof MethodFileMkdir and creationCall.getQualifier() = expr
creationCall.getMethod() instanceof MethodFileCreatesDirs and creationCall.getQualifier() = expr
}
Comment thread
JLLeitschuh marked this conversation as resolved.

private class MethodFileDelete extends Method {
Expand All @@ -47,9 +38,13 @@ private class TempDirHijackingToDeleteConfig extends TaintTracking::Configuratio
source.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
// TODO: Replace with getSystemProperty("java.io.tmpdir")
source.asExpr() = any(MethodAccessSystemGetProperty maSgp | maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir"))
)
or
// TODO: Replace with getSystemProperty("java.io.tmpdir")
source.asExpr() =
any(MethodAccessSystemGetProperty maSgp |
maSgp.hasCompileTimeConstantGetPropertyName("java.io.tmpdir")
)
}

override predicate isAdditionalTaintStep(DataFlow::Node node1, DataFlow::Node node2) {
Expand Down