fix(plugins): secure keystore file creation and improve robustness

Barbatos · Barbatos · commit 88df0ff91639 · 2026-04-16T23:34:14.000+08:00
- Fix TOCTOU race: keystore files were world-readable between creation
  and permission setting. Now use temp-file + setOwnerOnly + atomic-rename
  pattern (already used by update command) for new and import commands
- Fix KeystoreUpdate password file parsing: apply stripLineEndings to
  both old and new passwords to handle old-Mac line endings
- Warn on unreadable JSON files during keystore lookup instead of
  silently skipping, so users are aware of corrupted files
- Make WalletUtils.getWalletFileName public for reuse
- Extract atomicMove utility to KeystoreCliUtils for shared use
diff --git a/crypto/src/main/java/org/tron/keystore/WalletUtils.java b/crypto/src/main/java/org/tron/keystore/WalletUtils.java
@@ -81,7 +81,7 @@ public static Credentials loadCredentials(String password, File source, boolean
     return Credentials.create(Wallet.decrypt(password, walletFile, ecKey));
   }
 
-  private static String getWalletFileName(WalletFile walletFile) {
+  public static String getWalletFileName(WalletFile walletFile) {
     DateTimeFormatter format = DateTimeFormatter.ofPattern(
         "'UTC--'yyyy-MM-dd'T'HH-mm-ss.nVV'--'");
     ZonedDateTime now = ZonedDateTime.now(ZoneOffset.UTC);
diff --git a/plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java b/plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java
@@ -8,12 +8,17 @@
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.StandardCopyOption;
 import java.nio.file.attribute.PosixFilePermission;
 import java.util.Arrays;
 import java.util.EnumSet;
 import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Set;
+import org.tron.common.crypto.SignInterface;
+import org.tron.core.exception.CipherException;
+import org.tron.keystore.Wallet;
+import org.tron.keystore.WalletFile;
 import org.tron.keystore.WalletUtils;
 
 /**
@@ -29,6 +34,55 @@ final class KeystoreCliUtils {
   private KeystoreCliUtils() {
   }
 
+  /**
+   * Generate a keystore file using temp-file + atomic-rename to avoid
+   * a TOCTOU window where the file is world-readable before permissions are set.
+   *
+   * @return the final keystore file name (not the full path)
+   */
+  static String generateKeystoreFile(String password, SignInterface keyPair,
+      File destDir, boolean useFullScrypt, PrintWriter err)
+      throws CipherException, IOException {
+
+    WalletFile walletFile;
+    if (useFullScrypt) {
+      walletFile = Wallet.createStandard(password, keyPair);
+    } else {
+      walletFile = Wallet.createLight(password, keyPair);
+    }
+
+    String fileName = WalletUtils.getWalletFileName(walletFile);
+    File destination = new File(destDir, fileName);
+
+    File tempFile = File.createTempFile("keystore-", ".tmp", destDir);
+    try {
+      setOwnerOnly(tempFile, err);
+      MAPPER.writeValue(tempFile, walletFile);
+      atomicMove(tempFile, destination);
+    } catch (Exception e) {
+      if (!tempFile.delete()) {
+        err.println("Warning: could not delete temp file: " + tempFile.getName());
+      }
+      throw e;
+    }
+
+    return fileName;
+  }
+
+  /**
+   * Atomic move with fallback for filesystems that don't support it.
+   */
+  static void atomicMove(File source, File target) throws IOException {
+    try {
+      Files.move(source.toPath(), target.toPath(),
+          StandardCopyOption.REPLACE_EXISTING,
+          StandardCopyOption.ATOMIC_MOVE);
+    } catch (java.nio.file.AtomicMoveNotSupportedException e) {
+      Files.move(source.toPath(), target.toPath(),
+          StandardCopyOption.REPLACE_EXISTING);
+    }
+  }
+
   static String readPassword(File passwordFile, PrintWriter err) throws IOException {
     if (passwordFile != null) {
       if (!passwordFile.exists()) {
@@ -171,7 +225,7 @@ static void setOwnerOnly(File file, PrintWriter err) {
       Files.setPosixFilePermissions(file.toPath(), OWNER_ONLY);
     } catch (UnsupportedOperationException | IOException e) {
       err.println("Warning: could not set file permissions on " + file.getName()
-          + ". Please manually restrict access (e.g. chmod 600).");
+          + ". Please manually restrict access to this file.");
     }
   }
 }
diff --git a/plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java b/plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java
@@ -14,7 +14,7 @@
 import org.tron.common.utils.ByteArray;
 import org.tron.core.exception.CipherException;
 import org.tron.keystore.Credentials;
-import org.tron.keystore.WalletUtils;
+import org.tron.keystore.WalletFile;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Model.CommandSpec;
 import picocli.CommandLine.Option;
@@ -92,15 +92,15 @@ public Integer call() {
         return 1;
       }
       String address = Credentials.create(keyPair).getAddress();
-      String existingFile = findExistingKeystore(keystoreDir, address);
+      String existingFile = findExistingKeystore(keystoreDir, address, err);
       if (existingFile != null && !force) {
         err.println("Keystore for address " + address
             + " already exists: " + existingFile
             + ". Use --force to import anyway.");
         return 1;
       }
-      String fileName = WalletUtils.generateWalletFile(password, keyPair, keystoreDir, true);
-      KeystoreCliUtils.setOwnerOnly(new File(keystoreDir, fileName), err);
+      String fileName = KeystoreCliUtils.generateKeystoreFile(
+          password, keyPair, keystoreDir, true, err);
       if (json) {
         KeystoreCliUtils.printJson(out, err, KeystoreCliUtils.jsonMap(
             "address", address, "file", fileName));
@@ -159,7 +159,7 @@ private boolean isValidPrivateKey(String key) {
     return !StringUtils.isEmpty(key) && HEX_PATTERN.matcher(key).matches();
   }
 
-  private String findExistingKeystore(File dir, String address) {
+  private String findExistingKeystore(File dir, String address, PrintWriter err) {
     if (!dir.exists() || !dir.isDirectory()) {
       return null;
     }
@@ -171,13 +171,12 @@ private String findExistingKeystore(File dir, String address) {
         KeystoreCliUtils.mapper();
     for (File file : files) {
       try {
-        org.tron.keystore.WalletFile wf =
-            mapper.readValue(file, org.tron.keystore.WalletFile.class);
+        WalletFile wf = mapper.readValue(file, WalletFile.class);
         if (address.equals(wf.getAddress())) {
           return file.getName();
         }
       } catch (Exception e) {
-        // Skip invalid files
+        err.println("Warning: skipping unreadable file: " + file.getName());
       }
     }
     return null;
diff --git a/plugins/src/main/java/common/org/tron/plugins/KeystoreList.java b/plugins/src/main/java/common/org/tron/plugins/KeystoreList.java
@@ -71,7 +71,7 @@ public Integer call() {
         entry.put("file", file.getName());
         entries.add(entry);
       } catch (Exception e) {
-        // Skip files that aren't valid keystore JSON
+        err.println("Warning: skipping unreadable file: " + file.getName());
       }
     }
 
diff --git a/plugins/src/main/java/common/org/tron/plugins/KeystoreNew.java b/plugins/src/main/java/common/org/tron/plugins/KeystoreNew.java
@@ -8,7 +8,6 @@
 import org.tron.common.utils.Utils;
 import org.tron.core.exception.CipherException;
 import org.tron.keystore.Credentials;
-import org.tron.keystore.WalletUtils;
 import picocli.CommandLine.Command;
 import picocli.CommandLine.Model.CommandSpec;
 import picocli.CommandLine.Option;
@@ -53,8 +52,8 @@ public Integer call() {
 
       boolean ecKey = !sm2;
       SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), ecKey);
-      String fileName = WalletUtils.generateWalletFile(password, keyPair, keystoreDir, true);
-      KeystoreCliUtils.setOwnerOnly(new File(keystoreDir, fileName), err);
+      String fileName = KeystoreCliUtils.generateKeystoreFile(
+          password, keyPair, keystoreDir, true, err);
 
       String address = Credentials.create(keyPair).getAddress();
       if (json) {
diff --git a/plugins/src/main/java/common/org/tron/plugins/KeystoreUpdate.java b/plugins/src/main/java/common/org/tron/plugins/KeystoreUpdate.java
@@ -7,7 +7,6 @@
 import java.io.PrintWriter;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
-import java.nio.file.StandardCopyOption;
 import java.util.Arrays;
 import java.util.concurrent.Callable;
 import org.tron.common.crypto.SignInterface;
@@ -90,8 +89,8 @@ public Integer call() {
                     + " on separate lines.");
             return 1;
           }
-          oldPassword = lines[0];
-          newPassword = lines[1];
+          oldPassword = KeystoreCliUtils.stripLineEndings(lines[0]);
+          newPassword = KeystoreCliUtils.stripLineEndings(lines[1]);
         } finally {
           Arrays.fill(bytes, (byte) 0);
         }
@@ -153,15 +152,7 @@ public Integer call() {
       try {
         KeystoreCliUtils.setOwnerOnly(tempFile, err);
         MAPPER.writeValue(tempFile, newWalletFile);
-        try {
-          Files.move(tempFile.toPath(), keystoreFile.toPath(),
-              StandardCopyOption.REPLACE_EXISTING,
-              StandardCopyOption.ATOMIC_MOVE);
-        } catch (java.nio.file.AtomicMoveNotSupportedException e) {
-          // Fallback for NFS, FAT32, cross-partition
-          Files.move(tempFile.toPath(), keystoreFile.toPath(),
-              StandardCopyOption.REPLACE_EXISTING);
-        }
+        KeystoreCliUtils.atomicMove(tempFile, keystoreFile);
       } catch (Exception e) {
         if (!tempFile.delete()) {
           err.println("Warning: could not delete temp file: "
@@ -204,7 +195,7 @@ private File findKeystoreByAddress(String targetAddress, PrintWriter err) {
           matches.add(file);
         }
       } catch (Exception e) {
-        // Skip invalid files
+        err.println("Warning: skipping unreadable file: " + file.getName());
       }
     }
     if (matches.size() > 1) {

Original file line number	Diff line number	Diff line change
`@@ -81,7 +81,7 @@ public static Credentials loadCredentials(String password, File source, boolean`
`81`	`81`	`return Credentials.create(Wallet.decrypt(password, walletFile, ecKey));`
`82`	`82`	`}`
`83`	`83`
`84`		`- private static String getWalletFileName(WalletFile walletFile) {`
	`84`	`+ public static String getWalletFileName(WalletFile walletFile) {`
`85`	`85`	`DateTimeFormatter format = DateTimeFormatter.ofPattern(`
`86`	`86`	`"'UTC--'yyyy-MM-dd'T'HH-mm-ss.nVV'--'");`
`87`	`87`	`ZonedDateTime now = ZonedDateTime.now(ZoneOffset.UTC);`
Original file line number	Diff line number	Diff line change
`@@ -71,7 +71,7 @@ public Integer call() {`
`71`	`71`	`entry.put("file", file.getName());`
`72`	`72`	`entries.add(entry);`
`73`	`73`	`} catch (Exception e) {`
`74`		`- // Skip files that aren't valid keystore JSON`
	`74`	`+ err.println("Warning: skipping unreadable file: " + file.getName());`
`75`	`75`	`}`
`76`	`76`	`}`
`77`	`77`