Maybe I should use CWE-379 instead? https://cwe.mitre.org/data/definitions/379.html

Would it make sense to cover java.nio.file.Files.createDirectories(...) as well? That method does not throw an exception when the directories already exists.

Edit: Though I guess the use case you had in mind was the creation of a temporary directory using java.io.File. However, java.nio.file.Files provides createTempDir so I guess that pattern of deleting a temp file and then creating a directory with its path is unlikely to be used when the java.nio.file package is used.

Edit: Though I guess the use case you had in mind was the creation of a temporary directory using java.io.File. However, java.nio.file.Files provides createTempDir so I guess that pattern of deleting a temp file and then creating a directory with its path is unlikely to be used when the java.nio.file package is used.

This is accurate. I'm happy to add that use case, but I don't think that it will occur all that often.

When you are adding the query help, could you then direct the user to java.nio.file.Files.createTempDirectory​(...) as alternative to their unsafe code? That would probably be the easiest and least error-prone solution.

There are some outstanding disclosures and CVES from this query. Hence why development is kinda frozen. I'm still leveraging it to report vulns.

FYI, Spring boot was vulnerable to this bug and it had been fixed just 1 day before you created this pull request.
spring-projects/spring-boot@667ccda
There's no CVE assigned so I guess the maintainer was refactoring the code and didn't realize he just fixed a security vulnerability. As far as I know, the vulnerable method is used to create a work directory for embedded web servers such as Tomcat and Jetty. The directory contains configuration files, JSP/class files, etc. If a local attacker got the permission to write in this directory, they could completely take over the application.

Hey @trungPa,
Much appreciated for bringing this to my attention. Out of curiosity, how did you spot this?

GH Security Advisory created here to track the above. @trungPa you should have access! Thanks!
GHSA-cm59-pr5q-cw85

@JLLeitschuh I just happened to see the commit last year but didn't give it much thought before I see your PR.

This PR is waiting on some stuff from #8032 but a review at this point would be appreciated

#8032 is running tests + a performance check right now, so will wait til that lands and this is rebased

I've made a bunch of cleanups at JLLeitschuh#8, but the key points that absolutely must be attended to are:

1. Do not use data-flow configurations like isSink(DataFlow::Node n) { not someHeuristics(n) }; this is very likely to explode on certain projects where your (appropriately narrowly constrained) sources happen to be able to flow to a lot of possible sinks. If you want to look for any use not matching a heuristic (use in an Exception constructor, use in a string concatenation...) then you really should restrict the uses considered to at most local flow.
2. There are places (flagged in the cleanup PR as TODOs and FIXMEs) where it looks like heuristics are being employed that would be very hard for a user to comprehend without reading the query source code.

Two examples of those heuristics:

• Any guarding boolean check that surrounds a if(!f.mkdir()) is treated as sanitising, if and only if the check passes (regardless of what was checked). A user wondering why surrounding their whole function in if(something) { ... } (but not if(!something) { ... }) caused all alerts to disappear would not be happy. This looks like it has been added because this is a pattern common to various false-positives, but it isn't a reason to exclude something; this sort of fuzzy relationship with false positives is best left to machine learning approaches, where at least inscrutable behaviour is a feature not a bug.
• The safe-use predicate treats the LHS and RHS of a string concatenation asymmetrically, which I don't think any user would expect, and makes dubious use of localFlow and ad-hoc tracking across variable uses.

I suspect for some of these heuristics we should accept slightly lower accuracy in exchange for a more predictable, explicable query.

So I looked over your test cases, and based on them would like to suggest a simpler structure for this:

Bits to keep: file created in global temp dir -> delete() -> mkdir[s]() -- like it so far, this is a clear indication of the pattern we're looking for, and is an ideal global dataflow problem.

Then flag:

• The mkdir() result discarded
• The mkdir() is controlled by !exists() or !isDirectory()
• The failing path of !mkdir() controls exists() or isDirectory()

Then we could discard all the logic relating to throw statements (there's no reason to prefer a throw over a boolean return as an error signal) and safe/unsafe uses of the file that is mkdir'd and just flag the pattern as hazardous.

In terms of the current implementation, that would mean keeping the dataflow configs that look for the temp dir -> delete -> recreate pattern, but removing the throwsIf family of predicates, the safeUse concept and the constrainedByIfCheck test, and instead just check for those conditions -- is the mkdir result discarded, or is it in a dangerous-looking local control-flow relationship with exists or isDirectory in either direction?

What do you think, would that achieve decent results? Could you test such a simpler query against your test suite and see if it produces acceptable results?