rewrite-java-security

rewrite-java-security copied to clipboard

Problem

The following two diffs violate the underlying principle of Rewrite that it will keep the formatting of the rest of the file and modify the minimum required.

The other diff doesn't technically fix a security issue, and as such, is most likely just noise. However, I'd opt-not to fix that because in tests, it's technically used in a vulnerable way. And I'd argue tests actually demonstrate potentially valid end-user use cases.

Example diff

 import java.math.BigInteger;
 import java.math.RoundingMode;
 import java.nio.ByteBuffer;
+import java.nio.file.Files;
 import java.util.*;
 import java.util.concurrent.*;
 import java.util.concurrent.atomic.*;

This diff is a formatting change, which shouldn't happen.

  * for more information about this.</p>
  */
 public class Wallet extends BaseTaggableObject
-    implements NewBestBlockListener, TransactionReceivedInBlockListener, PeerFilterProvider, KeyBag, TransactionBag, ReorganizeListener {
+        implements NewBestBlockListener, TransactionReceivedInBlockListener, PeerFilterProvider, KeyBag, TransactionBag, ReorganizeListener {
     private static final Logger log = LoggerFactory.getLogger(Wallet.class);
     // Ordering: lock > keyChainGroupLock. KeyChainGroup is protected separately to allow fast querying of current receive address
     // even if the wallet itself is busy e.g. saving or processing a big reorg. Useful for reducing UI latency.

There is no File f here that has dataflow from a temporary directory in production code. However in test code, it is called with an insecure temporary directory. https://github.com/bitcoinj/bitcoinj/blob/2c714a604859c60c56e3e18eb684f179402b293c/core/src/test/java/org/bitcoinj/wallet/WalletTest.java#L1773

      */
     public void saveToFile(File f) throws IOException {
         File directory = f.getAbsoluteFile().getParentFile();
-        File temp = File.createTempFile("wallet", null, directory);
+        File temp = Files.createTempFile(directory.toPath(), "wallet", null).toFile();
         saveToFile(temp, f);
     }
 

Blank space diff that shouldn't be generated by Rewrite

             super(throwable);
         }
     }
+
     /**
      * Thrown if the resultant transaction would violate the dust rules (an output that's too small to be worthwhile).
      */
     public static class DustySendRequested extends CompletionException {}
+
     /**
      * Thrown if there is more than one OP_RETURN output for the resultant transaction.
      */
     public static class MultipleOpReturnRequested extends CompletionException {}
+
     /**
      * Thrown when we were trying to empty the wallet, and the total amount of money we were trying to empty after
      * being reduced for the fee was smaller than the min payment. Note that the missing field will be null in this
      * case.
      */
     public static class CouldNotAdjustDownwards extends CompletionException {}
+
     /**
      * Thrown if the resultant transaction is too big for Bitcoin to process. Try breaking up the amounts of value.
      */
     public static class ExceededMaxTransactionSize extends CompletionException {}
+
     /**
      * Thrown if the private keys and seed of this wallet cannot be decrypted due to the supplied encryption
      * key or password being wrong.

Recipes in example diff:

• org.openrewrite.java.security.SecureTempFileCreation

References:

• Recipe ID: vkjRi
• Recipe Name: "Use secure temporary file creation"
• Repository: bitcoinj/bitcoinj/master
• Created at Thu Jan 27 2022 20:55:51 GMT-0500 (Eastern Standard Time)