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

[JLLeitschuh]

On Unix-like systems, the temporary directory is shared with all users on the system. As such, improperly writing to the system temporary directory can allow attackers to hijack temporary directory resources.

    static File vulnerable() {
		// Attacker sees file creation in temporary directory
        File temp = File.createTempFile("test", "directory");
		// Attacker observes temporary file deletion
        temp.delete();
 		// 🏁 Race condition here
		// Attacker races to create the directory with more permissive permissions
        temp.mkdir(); // mkdir response code not checked. If attacker created directory, would return `false` not fail.
        return temp; // File could be hijacked
    }

[JLLeitschuh]

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

[Marcono1234]

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.

[smowton]

Nitpick: because the two configs are independent (no methods in this class depend on the flows found in TempDirHijackingToDeleteConfig), this can legally use DataFlow::Configuration.

[JLLeitschuh]

Sounds like playing with fire? One mistake and I end up with the two configurations artificially restricting each other.
Historically, I've ended up with some really wacky & hard to debug issues when extending DataFlow::Configuration multiple times.

[smowton]

Note you might get use within an &&, ||, == true and so on, does this work for those cases?

[JLLeitschuh]

It does not, I'd like to expand this so that it uses Guard. I was chatting with a few people about this in the slack channel.

https://ghsecuritylab.slack.com/archives/CQJU6RN49/p1602785245063800

[smowton]

This might be a good example to work from: https://github.com/github/codeql/blob/main/java/ql/src/Security/CWE/CWE-022/TaintedPath.ql#L21

Alternatively you could quite closely approximate by just checking the return value has any use -- it's probably a small minority of users that call mkdir, store its return value, but then don't properly check it (compared to simply throwing it away altogether)

[aschackmull]

To follow up on how to use Guard for this: There is in general not a single recipe for this as there are many ways to mix control flow and data flow in a query.
To cover the linked TaintedPath example first: That's using a BarrierGuard, which is actually just a convenience wrapper that defines a number of data-flow nodes as barriers/sanitizers (which means that data flow is prohibited from using those nodes); it does this by selecting all those data-flow nodes that are expressions whose corresponding control-flow nodes are guarded by the condition evaluating to false ("guarded" is a technical term with the definition: C evaluates to false guards N if and only if every control-flow path reaching N must have passed through the false-edge leaving C).
BarrierGuards are nice, but doesn't fit the use-case in this query, as the flow path stops at the mkdir qualifier. They are however implemented in terms of Guard, which allows us to reason about such "guarding" relationships between control-flow nodes.
So what we want is to see whether there's a (problematic) use of temp that isn't guarded by temp.mkdir() evaluating to true. We can approximate problematic uses by, for instance, all subsequent local uses that are not safe and approximate "safe" by saying that the safe uses are the ones that are used in string concatenations (e.g. for error reporting).
So I'd write this predicate something like this:

predicate isSinkConstrainedByIfCheck(DataFlow::Node sink) {
  exists(Guard g, MethodAccess ma, Expr unsafeUse |
    any(TempDirHijackingFromDeleteConfig c).isSink(sink) and
    sink.asExpr() = ma.getQualifier() and
    g = ma and
    DataFlow::localExprFlow(sink.asExpr(), unsafeUse) and
    unsafeUse != sink.asExpr() and
    not safeUse(unsafeUse) and
    not g.controls(unsafeUse.getBasicBlock(), true)
  )
}

predicate safeUse(Expr e) {
  exists(AddExpr concat |
    concat.getType() instanceof TypeString and
    concat.getAnOperand() = e
  )
}

[aschackmull]

Btw. "controls" and "guards" are synonymous verbs in this context.

[JLLeitschuh]

I had to make some modifications to your proposal above but this ended up working:

predicate isUnsafeUseUnconstrainedByIfCheck(DataFlow::Node sink, Expr unsafeUse) {
  exists(Guard g, MethodAccess ma |
    any(TempDirHijackingFromDeleteConfig c).isSink(sink) and // Sink is a call to delete
    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
    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)
  )
}

private predicate safeUse(Expr e) {
  exists(AndLogicalExpr andExp |
    andExp.getType() instanceof BooleanType and andExp.getAnOperand() = e
  )
  or
  exists(AssignAndExpr assignAndExp |
    assignAndExp.getType() instanceof BooleanType and assignAndExp.getSource() = e
  )
}

However, this false-positives on the following two:

   static File safe4() throws IOException {
        boolean success = true;
        File temp = File.createTempFile("test", "directory");
        success &= temp.delete();
        success &= temp.mkdir();
        if (!success) {
            throw new RuntimeException("Failed to create directory");
        }
        return temp;
    }

    static File safe5() throws IOException {
        boolean success = true;
        File temp = File.createTempFile("test", "directory");
        success &= temp.delete();
        success &= temp.mkdir();
        if (success) {
            return temp;
        } else {
            throw new RuntimeException("Failed to create directory");
        }
    }

Is Guard not handling AssignAndExpr correctly for controls?

[aschackmull]

Is Guard not handling AssignAndExpr correctly for controls?

It is now: #7698

[JLLeitschuh]

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.

[Marcono1234]

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.

[JLLeitschuh]

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

[JLLeitschuh]

Files.createDirectories should also be considered here.
Files.createDirectory is safe, but Files.createDirectories is not.

[JLLeitschuh]

I decided not to do this, because this would be taint flow, instead of data. flow. I don't think the added overhead of taint flow is worth adding these two methods that are probably not commonly leveraged

[trgpa]

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.

[JLLeitschuh]

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

[JLLeitschuh]

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

[trgpa]

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

[JLLeitschuh]

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

[Marcono1234]

Hopefully these mostly documentation related comments are useful.
Feel free to consider them only as suggestions since I am not a member of this project.

[Marcono1234]

Should createDirectories really be recommended (especially in the context of custom file permissions)? As mentioned in this comment that method does not fail when the directories already exist.

[JLLeitschuh]

It is safe when the file attribute is explicitly passed IIRC

[Marcono1234]

Might be necessary to change these QLDoc comments to "A ... method ..." to comply with the style guide.

[Marcono1234]

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

[JLLeitschuh]

Done!

[Marcono1234]

Is this taint step correct in case the source is already a MethodFileCreateTempFile?
It looks like this taint step is only intended for the case where File.createTempFile is called with java.io.tmpdir as parent directory.

(Though apparently the tests cover this and pass?)

[smowton]

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

[JLLeitschuh]

These two still false-positive due to #8490

@smowton any thought as to how to fix this?

[smowton]

As mentioned in #8490 safe11 is an uninteresting case since it's practically certainly a bug when a local variable is constant and controlling a condition. Safe12 is not safe unless this is the only possible write to that field, which we can only know if the field is private and the assignment on line 170 is the only possible write.

[JLLeitschuh]

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.

[smowton]

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.

[smowton]

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?