openrewrite / rewrite-java-security

OpenRewrite recipes for patching Java security vulnerabilities.
Apache License 2.0
16 stars 14 forks source link

Issue discovered with `distributedlog-core/src/main/java/com/twitter/distributedlog/LocalDLMEmulator.java` #7

Closed JLLeitschuh closed 2 years ago

JLLeitschuh commented 2 years ago

Problem

SecureTempFileCreation fixes wrong problem in the presence of insecure temporary directory creation.

Example diff

 import java.io.IOException;
 import java.net.BindException;
 import java.net.URI;
+import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.UUID;

NOTE: This is NOT temporary directory hijacking. However, the end-result directory would be world-readable, thus the change made would not have fixed the underlying information disclosure security issue.

         bookieConf.setZkTimeout(zkTimeoutSec * 1000);
         bookieConf.setBookiePort(0);
         bookieConf.setAllowLoopback(true);
-        File tmpdir = File.createTempFile("bookie" + UUID.randomUUID() + "_",
-            "test");
+        File tmpdir = Files.createTempFile("bookie" + UUID.randomUUID() + "_", "test").toFile();
          if (!tmpdir.delete()) {
              LOG.debug("Fail to delete tmpdir " + tmpdir);
          }
          if (!tmpdir.mkdir()) {
              throw new IOException("Fail to create tmpdir " + tmpdir);
          }
          tmpDirs.add(tmpdir);

Expected Diff

         bookieConf.setZkTimeout(zkTimeoutSec * 1000);
         bookieConf.setBookiePort(0);
         bookieConf.setAllowLoopback(true);
-        File tmpdir = File.createTempFile("bookie" + UUID.randomUUID() + "_",
-            "test");
+        File tmpdir = Files.createTempDirectory("bookie" + UUID.randomUUID() + "_" + "test").toFile();
-         if (!tmpdir.delete()) {
-             LOG.debug("Fail to delete tmpdir " + tmpdir);
-         }
-         if (!tmpdir.mkdir()) {
-             throw new IOException("Fail to create tmpdir " + tmpdir);
-         }
          tmpDirs.add(tmpdir);

Recipes in example diff:

References:

pway99 commented 2 years ago

Hi @JLLeitschuh, thanks for submitting this request; I'll take a look

JLLeitschuh commented 2 years ago

Thanks! Added the following to the top issue:

NOTE: This is NOT temporary directory hijacking. However, the end-result directory would be world-readable, thus the change made would not have fixed the underlying information disclosure security issue.

pway99 commented 2 years ago

Fixed by https://github.com/openrewrite/rewrite-java-security/commit/5fef3338fc8af783ef6dae794975587507068db3

JLLeitschuh commented 2 years ago

https://github.com/eclipse/jetty.project/security/advisories/GHSA-g3wg-6mcf-8jj6

pway99 commented 2 years ago

@JLLeitschuh This looks like a more appropriate recipe for this security vulnerability UseFilesCreateTempDirectory UseFilesCreateTempDirectory Tests

JLLeitschuh commented 2 years ago

Fixed by 5fef333

This is accurate