Skip to content

Commit 6b66b40

Browse files
BarbatosBarbatos
Barbatos
authored and
Barbatos
committed
refactor(crypto): centralize secure keystore file writing in WalletUtils
Move the temp-file + atomic-rename + POSIX 0600 permissions pattern from plugins into WalletUtils.generateWalletFile and a new public WalletUtils.writeWalletFile method. Use Files.createTempFile with PosixFilePermissions.asFileAttribute to atomically create the temp file with owner-only permissions, eliminating the brief world-readable window present in the previous File.createTempFile + setOwnerOnly sequence. All callers benefit automatically, including framework/KeystoreFactory (which previously wrote 0644 files) and any third-party project that consumes the crypto artifact. Removes ~68 lines of duplicated plumbing from KeystoreCliUtils. - Windows/non-POSIX fallback is best-effort (DOS attributes) and documented as such in JavaDoc - Error path suppresses secondary IOException via addSuppressed so the original cause is preserved - OWNER_ONLY wrapped with Collections.unmodifiableSet for defense in depth - WalletUtilsWriteTest covers permissions, replacement, temp cleanup, parent-directory creation, and a failure-path test that forces move to fail and asserts no temp file remains - KeystoreUpdateTest adversarially pre-loosens perms to 0644 and verifies update narrows back to 0600
1 parent f519a91 commit 6b66b40

8 files changed

Lines changed: 298 additions & 179 deletions

File tree

‎crypto/src/main/java/org/tron/keystore/WalletUtils.java‎

Lines changed: 72 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,22 @@
66
import java.io.Console;
77
import java.io.File;
88
import java.io.IOException;
9+
import java.nio.file.AtomicMoveNotSupportedException;
10+
import java.nio.file.Files;
11+
import java.nio.file.Path;
12+
import java.nio.file.StandardCopyOption;
13+
import java.nio.file.attribute.PosixFilePermission;
14+
import java.nio.file.attribute.PosixFilePermissions;
915
import java.security.InvalidAlgorithmParameterException;
1016
import java.security.NoSuchAlgorithmException;
1117
import java.security.NoSuchProviderException;
1218
import java.time.ZoneOffset;
1319
import java.time.ZonedDateTime;
1420
import java.time.format.DateTimeFormatter;
21+
import java.util.Collections;
22+
import java.util.EnumSet;
1523
import java.util.Scanner;
24+
import java.util.Set;
1625
import org.apache.commons.lang3.StringUtils;
1726
import org.tron.common.crypto.SignInterface;
1827
import org.tron.common.crypto.SignUtils;
@@ -26,6 +35,10 @@ public class WalletUtils {
2635

2736
private static final ObjectMapper objectMapper = new ObjectMapper();
2837

38+
private static final Set<PosixFilePermission> OWNER_ONLY =
39+
Collections.unmodifiableSet(EnumSet.of(
40+
PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE));
41+
2942
static {
3043
objectMapper.configure(JsonParser.Feature.ALLOW_UNQUOTED_FIELD_NAMES, true);
3144
objectMapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
@@ -69,12 +82,69 @@ public static String generateWalletFile(
6982

7083
String fileName = getWalletFileName(walletFile);
7184
File destination = new File(destinationDirectory, fileName);
72-
73-
objectMapper.writeValue(destination, walletFile);
85+
writeWalletFile(walletFile, destination);
7486

7587
return fileName;
7688
}
7789

90+
/**
91+
* Write a WalletFile to the given destination path with owner-only (0600)
92+
* permissions, using a temp file + atomic rename.
93+
*
94+
* <p>On POSIX filesystems, the temp file is created atomically with 0600
95+
* permissions via {@link Files#createTempFile(Path, String, String,
96+
* java.nio.file.attribute.FileAttribute[])}, avoiding any window where the
97+
* file is world-readable.
98+
*
99+
* <p>On non-POSIX filesystems (e.g. Windows) the fallback uses
100+
* {@link File#setReadable(boolean, boolean)} /
101+
* {@link File#setWritable(boolean, boolean)} which is best-effort — these
102+
* methods manipulate only DOS-style attributes on Windows and may not update
103+
* file ACLs. The sensitive keystore JSON is written only after the narrowing
104+
* calls, so no confidential data is exposed during the window, but callers
105+
* on Windows should not infer strict owner-only ACL enforcement from this.
106+
*
107+
* @param walletFile the keystore to serialize
108+
* @param destination the final target file (existing file will be replaced)
109+
*/
110+
public static void writeWalletFile(WalletFile walletFile, File destination)
111+
throws IOException {
112+
Path dir = destination.getAbsoluteFile().getParentFile().toPath();
113+
Files.createDirectories(dir);
114+
115+
Path tmp;
116+
try {
117+
tmp = Files.createTempFile(dir, "keystore-", ".tmp",
118+
PosixFilePermissions.asFileAttribute(OWNER_ONLY));
119+
} catch (UnsupportedOperationException e) {
120+
// Windows / non-POSIX fallback — best-effort narrowing only (see JavaDoc)
121+
tmp = Files.createTempFile(dir, "keystore-", ".tmp");
122+
File tf = tmp.toFile();
123+
tf.setReadable(false, false);
124+
tf.setReadable(true, true);
125+
tf.setWritable(false, false);
126+
tf.setWritable(true, true);
127+
}
128+
129+
try {
130+
objectMapper.writeValue(tmp.toFile(), walletFile);
131+
try {
132+
Files.move(tmp, destination.toPath(),
133+
StandardCopyOption.REPLACE_EXISTING,
134+
StandardCopyOption.ATOMIC_MOVE);
135+
} catch (AtomicMoveNotSupportedException e) {
136+
Files.move(tmp, destination.toPath(), StandardCopyOption.REPLACE_EXISTING);
137+
}
138+
} catch (Exception e) {
139+
try {
140+
Files.deleteIfExists(tmp);
141+
} catch (IOException suppress) {
142+
e.addSuppressed(suppress);
143+
}
144+
throw e;
145+
}
146+
}
147+
78148
public static Credentials loadCredentials(String password, File source, boolean ecKey)
79149
throws IOException, CipherException {
80150
WalletFile walletFile = objectMapper.readValue(source, WalletFile.class);

‎crypto/src/test/java/org/tron/keystore/WalletUtilsWriteTest.java‎

Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
package org.tron.keystore;
2+
3+
import static org.junit.Assert.assertEquals;
4+
import static org.junit.Assert.assertFalse;
5+
import static org.junit.Assert.assertNotNull;
6+
import static org.junit.Assert.assertTrue;
7+
import static org.junit.Assert.fail;
8+
9+
import java.io.File;
10+
import java.io.IOException;
11+
import java.nio.file.Files;
12+
import java.nio.file.attribute.PosixFilePermission;
13+
import java.util.EnumSet;
14+
import java.util.Set;
15+
import org.junit.Assume;
16+
import org.junit.Rule;
17+
import org.junit.Test;
18+
import org.junit.rules.TemporaryFolder;
19+
import org.tron.common.crypto.SignInterface;
20+
import org.tron.common.crypto.SignUtils;
21+
import org.tron.common.utils.Utils;
22+
23+
/**
24+
* Verifies that {@link WalletUtils#generateWalletFile} and
25+
* {@link WalletUtils#writeWalletFile} produce keystore files with
26+
* owner-only permissions (0600) atomically, leaving no temp files behind.
27+
*
28+
* <p>Tests use light scrypt (useFullScrypt=false) where possible because
29+
* they validate filesystem behavior, not the KDF parameters.
30+
*/
31+
public class WalletUtilsWriteTest {
32+
33+
@Rule
34+
public TemporaryFolder tempFolder = new TemporaryFolder();
35+
36+
private static WalletFile lightWalletFile(String password) throws Exception {
37+
SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true);
38+
return Wallet.createLight(password, keyPair);
39+
}
40+
41+
@Test
42+
public void testGenerateWalletFileCreatesOwnerOnlyFile() throws Exception {
43+
Assume.assumeTrue("POSIX permissions test",
44+
!System.getProperty("os.name").toLowerCase().contains("win"));
45+
46+
File dir = tempFolder.newFolder("gen-perms");
47+
SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true);
48+
49+
String fileName = WalletUtils.generateWalletFile("password123", keyPair, dir, false);
50+
51+
File created = new File(dir, fileName);
52+
assertTrue(created.exists());
53+
54+
Set<PosixFilePermission> perms = Files.getPosixFilePermissions(created.toPath());
55+
assertEquals("Keystore must have owner-only permissions (rw-------)",
56+
EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE),
57+
perms);
58+
}
59+
60+
@Test
61+
public void testGenerateWalletFileLeavesNoTempFile() throws Exception {
62+
File dir = tempFolder.newFolder("gen-no-temp");
63+
SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true);
64+
65+
WalletUtils.generateWalletFile("password123", keyPair, dir, false);
66+
67+
File[] tempFiles = dir.listFiles((d, name) -> name.startsWith("keystore-")
68+
&& name.endsWith(".tmp"));
69+
assertNotNull(tempFiles);
70+
assertEquals("No temp files should remain after generation", 0, tempFiles.length);
71+
}
72+
73+
@Test
74+
public void testGenerateWalletFileLightScrypt() throws Exception {
75+
File dir = tempFolder.newFolder("gen-light");
76+
SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true);
77+
78+
String fileName = WalletUtils.generateWalletFile("password123", keyPair, dir, false);
79+
assertNotNull(fileName);
80+
assertTrue(fileName.endsWith(".json"));
81+
assertTrue(new File(dir, fileName).exists());
82+
}
83+
84+
@Test
85+
public void testWriteWalletFileOwnerOnly() throws Exception {
86+
Assume.assumeTrue("POSIX permissions test",
87+
!System.getProperty("os.name").toLowerCase().contains("win"));
88+
89+
File dir = tempFolder.newFolder("write-perms");
90+
WalletFile wf = lightWalletFile("password123");
91+
File destination = new File(dir, "out.json");
92+
93+
WalletUtils.writeWalletFile(wf, destination);
94+
95+
assertTrue(destination.exists());
96+
Set<PosixFilePermission> perms = Files.getPosixFilePermissions(destination.toPath());
97+
assertEquals("Keystore must have owner-only permissions (rw-------)",
98+
EnumSet.of(PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE),
99+
perms);
100+
}
101+
102+
@Test
103+
public void testWriteWalletFileReplacesExisting() throws Exception {
104+
File dir = tempFolder.newFolder("write-replace");
105+
WalletFile wf1 = lightWalletFile("password123");
106+
WalletFile wf2 = lightWalletFile("password123");
107+
File destination = new File(dir, "out.json");
108+
109+
WalletUtils.writeWalletFile(wf1, destination);
110+
WalletUtils.writeWalletFile(wf2, destination);
111+
112+
assertTrue("Destination exists after replace", destination.exists());
113+
WalletFile reread = new com.fasterxml.jackson.databind.ObjectMapper()
114+
.readValue(destination, WalletFile.class);
115+
assertEquals("Replaced file should have wf2's address",
116+
wf2.getAddress(), reread.getAddress());
117+
}
118+
119+
@Test
120+
public void testWriteWalletFileLeavesNoTempFile() throws Exception {
121+
File dir = tempFolder.newFolder("write-no-temp");
122+
WalletFile wf = lightWalletFile("password123");
123+
File destination = new File(dir, "final.json");
124+
125+
WalletUtils.writeWalletFile(wf, destination);
126+
127+
File[] tempFiles = dir.listFiles((d, name) -> name.startsWith("keystore-")
128+
&& name.endsWith(".tmp"));
129+
assertNotNull(tempFiles);
130+
assertEquals("No temp files should remain", 0, tempFiles.length);
131+
}
132+
133+
@Test
134+
public void testWriteWalletFileCreatesParentDirectories() throws Exception {
135+
File base = tempFolder.newFolder("write-nested");
136+
File destination = new File(base, "a/b/c/out.json");
137+
assertFalse("Parent dir does not exist yet", destination.getParentFile().exists());
138+
139+
WalletFile wf = lightWalletFile("password123");
140+
WalletUtils.writeWalletFile(wf, destination);
141+
142+
assertTrue("Destination written", destination.exists());
143+
}
144+
145+
@Test
146+
public void testWriteWalletFileCleansUpTempOnFailure() throws Exception {
147+
// Force failure by making the destination a directory — Files.move will fail
148+
// because the source is a file. The temp file must be cleaned up.
149+
File dir = tempFolder.newFolder("write-fail");
150+
File destinationAsDir = new File(dir, "blocking-dir");
151+
assertTrue("Setup: blocking dir created", destinationAsDir.mkdir());
152+
// Put a file inside so Files.move with REPLACE_EXISTING fails (non-empty dir).
153+
assertTrue("Setup: block file", new File(destinationAsDir, "blocker").createNewFile());
154+
155+
WalletFile wf = lightWalletFile("password123");
156+
157+
try {
158+
WalletUtils.writeWalletFile(wf, destinationAsDir);
159+
fail("Expected IOException because destination is a non-empty directory");
160+
} catch (IOException expected) {
161+
// Expected
162+
}
163+
164+
File[] tempFiles = dir.listFiles((d, name) -> name.startsWith("keystore-")
165+
&& name.endsWith(".tmp"));
166+
assertNotNull(tempFiles);
167+
assertEquals("Temp file must be cleaned up on failure", 0, tempFiles.length);
168+
}
169+
}

‎plugins/src/main/java/common/org/tron/plugins/KeystoreCliUtils.java‎

Lines changed: 0 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -8,16 +8,9 @@
88
import java.nio.charset.StandardCharsets;
99
import java.nio.file.Files;
1010
import java.nio.file.Path;
11-
import java.nio.file.StandardCopyOption;
12-
import java.nio.file.attribute.PosixFilePermission;
1311
import java.util.Arrays;
14-
import java.util.EnumSet;
1512
import java.util.LinkedHashMap;
1613
import java.util.Map;
17-
import java.util.Set;
18-
import org.tron.common.crypto.SignInterface;
19-
import org.tron.core.exception.CipherException;
20-
import org.tron.keystore.Wallet;
2114
import org.tron.keystore.WalletFile;
2215
import org.tron.keystore.WalletUtils;
2316

@@ -26,63 +19,11 @@
2619
*/
2720
final class KeystoreCliUtils {
2821

29-
private static final Set<PosixFilePermission> OWNER_ONLY = EnumSet.of(
30-
PosixFilePermission.OWNER_READ, PosixFilePermission.OWNER_WRITE);
31-
3222
private static final long MAX_FILE_SIZE = 1024;
3323

3424
private KeystoreCliUtils() {
3525
}
3626

37-
/**
38-
* Generate a keystore file using temp-file + atomic-rename to avoid
39-
* a TOCTOU window where the file is world-readable before permissions are set.
40-
*
41-
* @return the final keystore file name (not the full path)
42-
*/
43-
static String generateKeystoreFile(String password, SignInterface keyPair,
44-
File destDir, boolean useFullScrypt, PrintWriter err)
45-
throws CipherException, IOException {
46-
47-
WalletFile walletFile;
48-
if (useFullScrypt) {
49-
walletFile = Wallet.createStandard(password, keyPair);
50-
} else {
51-
walletFile = Wallet.createLight(password, keyPair);
52-
}
53-
54-
String fileName = WalletUtils.getWalletFileName(walletFile);
55-
File destination = new File(destDir, fileName);
56-
57-
File tempFile = File.createTempFile("keystore-", ".tmp", destDir);
58-
try {
59-
setOwnerOnly(tempFile, err);
60-
MAPPER.writeValue(tempFile, walletFile);
61-
atomicMove(tempFile, destination);
62-
} catch (Exception e) {
63-
if (!tempFile.delete()) {
64-
err.println("Warning: could not delete temp file: " + tempFile.getName());
65-
}
66-
throw e;
67-
}
68-
69-
return fileName;
70-
}
71-
72-
/**
73-
* Atomic move with fallback for filesystems that don't support it.
74-
*/
75-
static void atomicMove(File source, File target) throws IOException {
76-
try {
77-
Files.move(source.toPath(), target.toPath(),
78-
StandardCopyOption.REPLACE_EXISTING,
79-
StandardCopyOption.ATOMIC_MOVE);
80-
} catch (java.nio.file.AtomicMoveNotSupportedException e) {
81-
Files.move(source.toPath(), target.toPath(),
82-
StandardCopyOption.REPLACE_EXISTING);
83-
}
84-
}
85-
8627
static String readPassword(File passwordFile, PrintWriter err) throws IOException {
8728
if (passwordFile != null) {
8829
if (!passwordFile.exists()) {
@@ -228,13 +169,4 @@ static boolean isValidKeystoreFile(WalletFile wf) {
228169
&& wf.getCrypto() != null
229170
&& wf.getVersion() == 3;
230171
}
231-
232-
static void setOwnerOnly(File file, PrintWriter err) {
233-
try {
234-
Files.setPosixFilePermissions(file.toPath(), OWNER_ONLY);
235-
} catch (UnsupportedOperationException | IOException e) {
236-
err.println("Warning: could not set file permissions on " + file.getName()
237-
+ ". Please manually restrict access to this file.");
238-
}
239-
}
240172
}

‎plugins/src/main/java/common/org/tron/plugins/KeystoreImport.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
import org.tron.core.exception.CipherException;
1616
import org.tron.keystore.Credentials;
1717
import org.tron.keystore.WalletFile;
18+
import org.tron.keystore.WalletUtils;
1819
import picocli.CommandLine.Command;
1920
import picocli.CommandLine.Model.CommandSpec;
2021
import picocli.CommandLine.Option;
@@ -99,8 +100,8 @@ public Integer call() {
99100
+ ". Use --force to import anyway.");
100101
return 1;
101102
}
102-
String fileName = KeystoreCliUtils.generateKeystoreFile(
103-
password, keyPair, keystoreDir, true, err);
103+
String fileName = WalletUtils.generateWalletFile(
104+
password, keyPair, keystoreDir, true);
104105
if (json) {
105106
KeystoreCliUtils.printJson(out, err, KeystoreCliUtils.jsonMap(
106107
"address", address, "file", fileName));

0 commit comments

Comments
 (0)