From 2caafc96820506b4eb10b9873286135eeacf105a Mon Sep 17 00:00:00 2001 From: federico Date: Tue, 4 Aug 2026 15:00:51 +0800 Subject: [PATCH 1/8] fix(crypto): harden ECKey validation --- .../java/org/tron/common/crypto/ECKey.java | 265 ++++++++---------- .../main/java/org/tron/keystore/Wallet.java | 37 ++- .../org/tron/common/crypto/ECKeyTest.java | 120 +++++++- .../args/WitnessInitializerKeystoreTest.java | 22 ++ .../keystore/WalletAddressValidationTest.java | 61 ++++ 5 files changed, 332 insertions(+), 173 deletions(-) diff --git a/crypto/src/main/java/org/tron/common/crypto/ECKey.java b/crypto/src/main/java/org/tron/common/crypto/ECKey.java index d0a6048aca1..ddb25f6afa1 100644 --- a/crypto/src/main/java/org/tron/common/crypto/ECKey.java +++ b/crypto/src/main/java/org/tron/common/crypto/ECKey.java @@ -23,15 +23,11 @@ import java.security.KeyPair; import java.security.KeyPairGenerator; import java.security.PrivateKey; -import java.security.Provider; import java.security.PublicKey; import java.security.SecureRandom; import java.security.SignatureException; -import java.security.interfaces.ECPrivateKey; -import java.security.interfaces.ECPublicKey; import java.security.spec.InvalidKeySpecException; import java.util.Arrays; -import java.util.Objects; import javax.annotation.Nullable; import lombok.extern.slf4j.Slf4j; import org.bouncycastle.asn1.sec.SECNamedCurves; @@ -58,6 +54,14 @@ import org.tron.common.utils.ByteArray; import org.tron.common.utils.ByteUtil; +/** + * A secp256k1 key pair and ECDSA signing utility. + * + *

ECDSA signatures are malleable: for a valid {@code (r, s)} pair, {@code (r, n - s)} is + * valid as well. New signatures are canonicalized to low-S form, but verification accepts both + * forms because transaction identifiers do not include signatures. See BIP-62 for background on + * low-S signatures. + */ @Slf4j(topic = "crypto") public class ECKey implements Serializable, SignInterface { @@ -77,6 +81,9 @@ public class ECKey implements Serializable, SignInterface { */ public static final BigInteger HALF_CURVE_ORDER; + private static final int MAX_PRIVATE_KEY_LENGTH = 32; + private static final int COMPRESSED_PUBLIC_KEY_LENGTH = 33; + private static final int UNCOMPRESSED_PUBLIC_KEY_LENGTH = 65; private static final BigInteger SECP256K1N = new BigInteger("fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141", 16); private static final SecureRandom secureRandom; @@ -100,16 +107,10 @@ public class ECKey implements Serializable, SignInterface { // TODO: Redesign this class to use consistent internals and more // efficient serialization. private final PrivateKey privKey; - // the Java Cryptographic Architecture provider to use for Signature - // this is set along with the PrivateKey privKey and must be compatible - // this provider will be used when selecting a Signature instance - // https://docs.oracle.com/javase/8/docs/technotes/guides/security - // /SunProviders.html - private final Provider provider; // Transient because it's calculated on demand. - private transient byte[] pubKeyHash; - private transient byte[] nodeId; + private transient volatile byte[] pubKeyHash; + private transient volatile byte[] nodeId; /** * Generates an entirely new keypair. @@ -121,78 +122,58 @@ public ECKey() { } /** - * Generate a new keypair using the given Java Security Provider. + * Generates an entirely new keypair with the given {@link SecureRandom} object.

BouncyCastle + * will be used as the Java Security Provider * - *

All private key operations will use the provider. + * @param secureRandom - */ - public ECKey(Provider provider, SecureRandom secureRandom) { - this.provider = provider; - - final KeyPairGenerator keyPairGen = ECKeyPairGenerator.getInstance(provider, secureRandom); + public ECKey(SecureRandom secureRandom) { + final KeyPairGenerator keyPairGen = ECKeyPairGenerator.getInstance( + TronCastleProvider.getInstance(), secureRandom); final KeyPair keyPair = keyPairGen.generateKeyPair(); this.privKey = keyPair.getPrivate(); final PublicKey pubKey = keyPair.getPublic(); - if (pubKey instanceof BCECPublicKey) { - pub = ((BCECPublicKey) pubKey).getQ(); - } else if (pubKey instanceof ECPublicKey) { - pub = extractPublicKey((ECPublicKey) pubKey); - } else { + if (!(pubKey instanceof BCECPublicKey)) { throw new AssertionError( - "Expected Provider " + provider.getName() - + " to produce a subtype of ECPublicKey, found " - + pubKey.getClass()); + "Expected Bouncy Castle EC public key, found " + pubKey.getClass()); } + this.pub = ((BCECPublicKey) pubKey).getQ(); } - /** - * Generates an entirely new keypair with the given {@link SecureRandom} object.

BouncyCastle - * will be used as the Java Security Provider - * - * @param secureRandom - - */ - public ECKey(SecureRandom secureRandom) { - this(TronCastleProvider.getInstance(), secureRandom); - } - - /** - * Pair a private key with a public EC point. - * - *

All private key operations will use the provider. - */ - public ECKey(byte[] key, boolean isPrivateKey) { if (isPrivateKey) { + check(isValidPrivateKey(key), "Invalid private key"); BigInteger pk = new BigInteger(1, key); this.privKey = privateKeyFromBigInteger(pk); this.pub = CURVE.getG().multiply(pk); } else { this.privKey = null; - this.pub = CURVE.getCurve().decodePoint(key); + this.pub = decodePublicKey(key); + check(isValidPublicPoint(this.pub), "Invalid public key"); } - this.provider = TronCastleProvider.getInstance(); } - public ECKey(Provider provider, @Nullable PrivateKey privKey, ECPoint pub) { - this.provider = provider; + private ECKey(BigInteger privateKey) { + this.privKey = privateKeyFromBigInteger(privateKey); + this.pub = CURVE.getG().multiply(privateKey); + } - if (privKey == null || isECPrivateKey(privKey)) { + private ECKey(@Nullable PrivateKey privKey, ECPoint pub) { + if (privKey == null || privKey instanceof BCECPrivateKey) { this.privKey = privKey; } else { throw new IllegalArgumentException( - "Expected EC private key, given a private key object with" + - " class " + "Expected Bouncy Castle EC private key, given a private key object with class " + privKey.getClass().toString() + " and algorithm " + privKey.getAlgorithm()); } - if (pub == null) { - throw new IllegalArgumentException("Public key may not be null"); - } else { - this.pub = pub; - } + check(isValidPublicPoint(pub), "Invalid public key"); + checkPrivateKeyMatchesPublic(privKey, pub); + this.pub = pub; } /** @@ -200,32 +181,17 @@ public ECKey(Provider provider, @Nullable PrivateKey privKey, ECPoint pub) { * Security Provider */ public ECKey(@Nullable BigInteger priv, ECPoint pub) { - this( - TronCastleProvider.getInstance(), - privateKeyFromBigInteger(priv), - pub - ); + this(privateKeyFromBigInteger(priv), pub); } - /* Convert a Java JCE ECPublicKey into a BouncyCastle ECPoint - */ - private static ECPoint extractPublicKey(final ECPublicKey ecPublicKey) { - final java.security.spec.ECPoint publicPointW = ecPublicKey.getW(); - final BigInteger xCoord = publicPointW.getAffineX(); - final BigInteger yCoord = publicPointW.getAffineY(); - - return CURVE.getCurve().createPoint(xCoord, yCoord); - } - - /* Test if a generic private key is an EC private key - * - * it is not sufficient to check that privKey is a subtype of ECPrivateKey - * as the SunPKCS11 Provider will return a generic PrivateKey instance - * a fallback that covers this case is to check the key algorithm - */ - private static boolean isECPrivateKey(PrivateKey privKey) { - return privKey instanceof ECPrivateKey || privKey.getAlgorithm() - .equals("EC"); + private static void checkPrivateKeyMatchesPublic( + @Nullable PrivateKey privateKey, ECPoint publicPoint) { + if (privateKey instanceof BCECPrivateKey) { + BigInteger privateScalar = ((BCECPrivateKey) privateKey).getD(); + check(isValidPrivateKey(privateScalar), "Invalid private key"); + check(CURVE.getG().multiply(privateScalar).equals(publicPoint), + "Private key does not match public key"); + } } /* Convert a BigInteger into a PrivateKey object @@ -234,6 +200,7 @@ private static PrivateKey privateKeyFromBigInteger(BigInteger priv) { if (priv == null) { return null; } else { + check(isValidPrivateKey(priv), "Invalid private key"); try { return ECKeyFactory .getInstance(TronCastleProvider.getInstance()) @@ -245,6 +212,57 @@ private static PrivateKey privateKeyFromBigInteger(BigInteger priv) { } } + /** + * Returns whether the supplied key bytes represent a valid secp256k1 private scalar. + * Accepts unsigned encodings up to 32 bytes and Java {@link BigInteger} encodings with one + * leading zero sign byte. + */ + public static boolean isValidPrivateKey(byte[] privateKey) { + // BigInteger.toByteArray() prepends a zero sign byte only when the magnitude's high bit is set. + return !ByteArray.isEmpty(privateKey) + && (privateKey.length <= MAX_PRIVATE_KEY_LENGTH + || (privateKey.length == MAX_PRIVATE_KEY_LENGTH + 1 + && privateKey[0] == 0 + && (privateKey[1] & 0x80) != 0)) + && isValidPrivateKey(new BigInteger(1, privateKey)); + } + + /** + * Returns whether the supplied value is in the secp256k1 private-key range {@code [1, n)}. + */ + public static boolean isValidPrivateKey(BigInteger privateKey) { + return privateKey != null + && privateKey.signum() > 0 + && privateKey.compareTo(SECP256K1N) < 0; + } + + /** + * Returns whether the encoded key is a non-infinity point on its elliptic curve. + */ + public static boolean isValidPublicKey(byte[] publicKey) { + try { + return isValidPublicPoint(decodePublicKey(publicKey)); + } catch (RuntimeException e) { + return false; + } + } + + private static boolean isValidPublicPoint(ECPoint publicPoint) { + return publicPoint != null + && CURVE.getCurve().equals(publicPoint.getCurve()) + && !publicPoint.isInfinity() + && publicPoint.isValid(); + } + + private static ECPoint decodePublicKey(byte[] publicKey) { + check(isPubKeyCanonical(publicKey), "Invalid public key"); + try { + return CURVE.getCurve().decodePoint(publicKey); + } catch (RuntimeException e) { + throw new IllegalArgumentException("Invalid public key", e); + } + } + /** * Utility for compressing an elliptic curve point. Returns the same point if it's already * compressed. See the ECKey class docs for a discussion of point compression. @@ -276,7 +294,8 @@ public static ECPoint decompressPoint(ECPoint compressed) { * @return - */ public static ECKey fromPrivate(BigInteger privKey) { - return new ECKey(privKey, CURVE.getG().multiply(privKey)); + check(isValidPrivateKey(privKey), "Invalid private key"); + return new ECKey(privKey); } /** @@ -286,41 +305,8 @@ public static ECKey fromPrivate(BigInteger privKey) { * @return - */ public static ECKey fromPrivate(byte[] privKeyBytes) { - if (ByteArray.isEmpty(privKeyBytes)) { - return null; - } - return fromPrivate(new BigInteger(1, privKeyBytes)); - } - - /** - * Creates an ECKey that simply trusts the caller to ensure that point is really the result of - * multiplying the generator point by the private key. This is used to speed things up when you - * know you have the right values already. The compression state of pub will be preserved. - * - * @param priv - - * @param pub - - * @return - - */ - public static ECKey fromPrivateAndPrecalculatedPublic(BigInteger priv, - ECPoint pub) { - return new ECKey(priv, pub); - } - - /** - * Creates an ECKey that simply trusts the caller to ensure that point is really the result of - * multiplying the generator point by the private key. This is used to speed things up when you - * know you have the right values already. The compression state of the point will be preserved. - * - * @param priv - - * @param pub - - * @return - - */ - public static ECKey fromPrivateAndPrecalculatedPublic(byte[] priv, byte[] - pub) { - check(priv != null, "Private key must not be null"); - check(pub != null, "Public key must not be null"); - return new ECKey(new BigInteger(1, priv), CURVE.getCurve() - .decodePoint(pub)); + check(isValidPrivateKey(privKeyBytes), "Invalid private key"); + return new ECKey(new BigInteger(1, privKeyBytes)); } /** @@ -331,7 +317,7 @@ public static ECKey fromPrivateAndPrecalculatedPublic(byte[] priv, byte[] * @return - */ public static ECKey fromPublicOnly(ECPoint pub) { - return new ECKey(null, pub); + return new ECKey((PrivateKey) null, pub); } /** @@ -342,7 +328,7 @@ public static ECKey fromPublicOnly(ECPoint pub) { * @return - */ public static ECKey fromPublicOnly(byte[] pub) { - return new ECKey(null, CURVE.getCurve().decodePoint(pub)); + return new ECKey((PrivateKey) null, decodePublicKey(pub)); } /** @@ -355,6 +341,7 @@ public static ECKey fromPublicOnly(byte[] pub) { */ public static byte[] publicKeyFromPrivate(BigInteger privKey, boolean compressed) { + check(isValidPrivateKey(privKey), "Invalid private key"); ECPoint point = CURVE.getG().multiply(privKey); return point.getEncoded(compressed); } @@ -481,12 +468,15 @@ public static ECKey signatureToKey(byte[] messageHash, String * @return - */ public static boolean isPubKeyCanonical(byte[] pubkey) { + if (ByteArray.isEmpty(pubkey)) { + return false; + } if (pubkey[0] == 0x04) { // Uncompressed pubkey - return pubkey.length == 65; + return pubkey.length == UNCOMPRESSED_PUBLIC_KEY_LENGTH; } else if (pubkey[0] == 0x02 || pubkey[0] == 0x03) { // Compressed pubkey - return pubkey.length == 33; + return pubkey.length == COMPRESSED_PUBLIC_KEY_LENGTH; } else { return false; } @@ -669,10 +659,12 @@ public boolean hasPrivKey() { * @return 21-byte address */ public byte[] getAddress() { - if (pubKeyHash == null) { - pubKeyHash = Hash.computeAddress(this.pub); + byte[] address = pubKeyHash; + if (address == null) { + address = Hash.computeAddress(this.pub); + pubKeyHash = address; } - return pubKeyHash; + return Arrays.copyOf(address, address.length); } @Override @@ -691,10 +683,12 @@ public byte[] Base64toBytes(String signature) { * Generates the NodeID based on this key, that is the public key without first format byte */ public byte[] getNodeId() { - if (nodeId == null) { - nodeId = pubBytesWithoutFormat(this.pub); + byte[] id = nodeId; + if (id == null) { + id = pubBytesWithoutFormat(this.pub); + nodeId = id; } - return nodeId; + return Arrays.copyOf(id, id.length); } @@ -744,22 +738,6 @@ public String toString() { return b.toString(); } - /** - * Produce a string rendering of the ECKey INCLUDING the private key. Unless you absolutely need - * the private key it is better for security reasons to just use toString(). - * - * @return - - */ - public String toStringWithPrivate() { - StringBuilder b = new StringBuilder(); - b.append(toString()); - if (privKey != null && privKey instanceof BCECPrivateKey) { - b.append(" priv:").append(Hex.toHexString(((BCECPrivateKey) - privKey).getD().toByteArray())); - } - return b.toString(); - } - /** * Signs the given hash and returns the R and S components as BigIntegers and putData them in * ECDSASignature @@ -841,8 +819,6 @@ public byte[] getPrivKeyBytes() { return null; } else if (privKey instanceof BCECPrivateKey) { return ByteUtil.bigIntegerToBytes(((BCECPrivateKey) privKey).getD(), 32); - } else if (privKey instanceof ECPrivateKey) { - return ByteUtil.bigIntegerToBytes(((ECPrivateKey) privKey).getS(), 32); } else { return null; } @@ -860,10 +836,7 @@ public boolean equals(Object o) { ECKey ecKey = (ECKey) o; - if (privKey != null && !privKey.equals(ecKey.privKey)) { - return false; - } - return pub == null || pub.equals(ecKey.pub); + return pub.equals(ecKey.pub); } @Override diff --git a/crypto/src/main/java/org/tron/keystore/Wallet.java b/crypto/src/main/java/org/tron/keystore/Wallet.java index d63525b1e4d..127d724a567 100644 --- a/crypto/src/main/java/org/tron/keystore/Wallet.java +++ b/crypto/src/main/java/org/tron/keystore/Wallet.java @@ -216,22 +216,31 @@ public static SignInterface decrypt(String password, WalletFile walletFile, byte[] encryptKey = Arrays.copyOfRange(derivedKey, 0, 16); byte[] privateKey = performCipherOperation(Cipher.DECRYPT_MODE, iv, encryptKey, cipherText); - SignInterface keyPair = SignUtils.fromPrivate(privateKey, ecKey); - - // Enforce address consistency: if the keystore declares an address, it MUST match - // the address derived from the decrypted private key. Prevents address spoofing - // where a crafted keystore displays one address but encrypts a different key. - String declared = walletFile.getAddress(); - if (declared != null && !declared.isEmpty()) { - String derived = StringUtil.encode58Check(keyPair.getAddress()); - if (!declared.equals(derived)) { - throw new CipherException( - "Keystore address mismatch: file declares " + declared - + " but private key derives " + derived); + try { + SignInterface keyPair; + try { + keyPair = SignUtils.fromPrivate(privateKey, ecKey); + } catch (IllegalArgumentException e) { + throw new CipherException("Invalid private key in keystore", e); } - } - return keyPair; + // Enforce address consistency: if the keystore declares an address, it MUST match + // the address derived from the decrypted private key. Prevents address spoofing + // where a crafted keystore displays one address but encrypts a different key. + String declared = walletFile.getAddress(); + if (declared != null && !declared.isEmpty()) { + String derived = StringUtil.encode58Check(keyPair.getAddress()); + if (!declared.equals(derived)) { + throw new CipherException( + "Keystore address mismatch: file declares " + declared + + " but private key derives " + derived); + } + } + + return keyPair; + } finally { + Arrays.fill(privateKey, (byte) 0); + } } /** diff --git a/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java b/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java index 273672e8342..6f983f9e66d 100644 --- a/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java +++ b/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java @@ -5,17 +5,16 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.tron.common.utils.client.utils.AbiUtil.generateOccupationConstantPrivateKey; import java.math.BigInteger; -import java.security.KeyPairGenerator; -import java.security.Security; import java.security.SignatureException; import java.util.Arrays; import lombok.extern.slf4j.Slf4j; +import org.bouncycastle.asn1.sec.SECNamedCurves; import org.bouncycastle.util.encoders.Hex; import org.junit.Test; import org.tron.common.crypto.ECKey.ECDSASignature; @@ -69,10 +68,76 @@ public void testFromPrivateKey() { assertTrue(key.hasPrivKey()); assertArrayEquals(pubKey, key.getPubKey()); - key = ECKey.fromPrivate((byte[]) null); - assertNull(key); - key = ECKey.fromPrivate(new byte[0]); - assertNull(key); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate((byte[]) null)); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate(new byte[0])); + } + + @Test + public void shouldValidatePrivateKeyRange() { + assertTrue(ECKey.isValidPrivateKey(privateKey)); + assertTrue(ECKey.isValidPrivateKey(Hex.decode(privString))); + assertFalse(ECKey.isValidPrivateKey((BigInteger) null)); + assertFalse(ECKey.isValidPrivateKey((byte[]) null)); + assertFalse(ECKey.isValidPrivateKey(new byte[0])); + assertFalse(ECKey.isValidPrivateKey(new byte[33])); + assertFalse(ECKey.isValidPrivateKey(BigInteger.ZERO)); + assertFalse(ECKey.isValidPrivateKey(ECKey.CURVE.getN())); + + BigInteger highBitPrivateKey = ECKey.CURVE.getN().subtract(BigInteger.ONE); + byte[] signPaddedPrivateKey = highBitPrivateKey.toByteArray(); + assertEquals(33, signPaddedPrivateKey.length); + assertEquals(0, signPaddedPrivateKey[0]); + assertTrue(ECKey.isValidPrivateKey(signPaddedPrivateKey)); + assertEquals(highBitPrivateKey, ECKey.fromPrivate(signPaddedPrivateKey).getPrivKey()); + + byte[] redundantSignPaddedPrivateKey = new byte[33]; + redundantSignPaddedPrivateKey[32] = 1; + assertFalse(ECKey.isValidPrivateKey(redundantSignPaddedPrivateKey)); + + byte[] nonZeroLeadingPrivateKey = Arrays.copyOf(signPaddedPrivateKey, + signPaddedPrivateKey.length); + nonZeroLeadingPrivateKey[0] = 1; + assertFalse(ECKey.isValidPrivateKey(nonZeroLeadingPrivateKey)); + + byte[] doubleSignPaddedPrivateKey = new byte[34]; + System.arraycopy(signPaddedPrivateKey, 0, doubleSignPaddedPrivateKey, 1, + signPaddedPrivateKey.length); + assertFalse(ECKey.isValidPrivateKey(doubleSignPaddedPrivateKey)); + + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate(BigInteger.ZERO)); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate(ECKey.CURVE.getN())); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate(new byte[1024 * 1024])); + assertThrows(IllegalArgumentException.class, + () -> ECKey.publicKeyFromPrivate(null, false)); + } + + @Test + public void shouldRejectInvalidPublicKeys() { + assertTrue(ECKey.isValidPublicKey(pubKey)); + assertTrue(ECKey.isValidPublicKey(compressedPubKey)); + assertFalse(ECKey.isValidPublicKey(null)); + assertFalse(ECKey.isValidPublicKey(new byte[0])); + assertFalse(ECKey.isValidPublicKey(new byte[]{0})); + assertFalse(ECKey.isValidPublicKey(new byte[66])); + + byte[] oversizedPublicKey = new byte[1024 * 1024]; + oversizedPublicKey[0] = 0x04; + assertFalse(ECKey.isValidPublicKey(oversizedPublicKey)); + + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPublicOnly((byte[]) null)); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPublicOnly(ECKey.CURVE.getCurve().getInfinity())); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPublicOnly( + ECKey.CURVE.getCurve().createPoint(BigInteger.ZERO, BigInteger.ZERO))); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPublicOnly(SECNamedCurves.getByName("secp256r1").getG())); } @Test(expected = IllegalArgumentException.class) @@ -81,12 +146,21 @@ public void testPrivatePublicKeyBytesNoArg() { fail("Expecting an IllegalArgumentException for using only null-parameters"); } - @Test(expected = IllegalArgumentException.class) - public void testInvalidPrivateKey() throws Exception { - new ECKey(Security.getProvider("SunEC"), - KeyPairGenerator.getInstance("RSA").generateKeyPair().getPrivate(), - ECKey.fromPublicOnly(pubKey).getPubKeyPoint()); - fail("Expecting an IllegalArgumentException for using an non EC private key"); + @Test + public void shouldRejectMismatchedPrivateAndPublicKeys() { + BigInteger otherPrivateKey = privateKey.add(BigInteger.ONE); + ECKey otherKey = ECKey.fromPrivate(otherPrivateKey); + + assertThrows(IllegalArgumentException.class, + () -> new ECKey(privateKey, otherKey.getPubKeyPoint())); + } + + @Test + public void shouldAcceptMatchingPrivateAndPublicKeys() { + ECKey key = new ECKey(privateKey, ECKey.CURVE.getG().multiply(privateKey)); + + assertArrayEquals(pubKey, key.getPubKey()); + assertTrue(key.hasPrivKey()); } @Test @@ -201,6 +275,8 @@ public void testIsPubKeyCanonicalWrongPrefix() { // Test wrong prefix 3, right length 33 byte[] nonCanonicalPubkey6 = new byte[33]; assertFalse(ECKey.isPubKeyCanonical(nonCanonicalPubkey6)); + assertFalse(ECKey.isPubKeyCanonical(null)); + assertFalse(ECKey.isPubKeyCanonical(new byte[0])); } @Test @@ -215,10 +291,28 @@ public void testEqualsObject() { ECKey key0 = new ECKey(); ECKey key1 = ECKey.fromPrivate(privateKey); ECKey key2 = ECKey.fromPrivate(privateKey); + ECKey publicOnlyKey = ECKey.fromPublicOnly(key1.getPubKey()); assertFalse(key0.equals(key1)); assertTrue(key1.equals(key1)); assertTrue(key1.equals(key2)); + assertTrue(key1.equals(publicOnlyKey)); + assertTrue(publicOnlyKey.equals(key1)); + assertEquals(key1.hashCode(), publicOnlyKey.hashCode()); + } + + @Test + public void shouldReturnDefensiveCopiesOfCachedValues() { + ECKey key = ECKey.fromPrivate(privateKey); + byte[] expectedAddress = Arrays.copyOf(key.getAddress(), key.getAddress().length); + byte[] returnedAddress = key.getAddress(); + returnedAddress[0] ^= 1; + assertArrayEquals(expectedAddress, key.getAddress()); + + byte[] expectedNodeId = Arrays.copyOf(key.getNodeId(), key.getNodeId().length); + byte[] returnedNodeId = key.getNodeId(); + returnedNodeId[0] ^= 1; + assertArrayEquals(expectedNodeId, key.getNodeId()); } diff --git a/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java b/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java index 80d8287682b..386ac7c90a9 100644 --- a/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java +++ b/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java @@ -5,6 +5,9 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mockStatic; import ch.qos.logback.classic.Logger; import ch.qos.logback.classic.spi.ILoggingEvent; @@ -18,11 +21,13 @@ import org.junit.ClassRule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.mockito.MockedStatic; import org.slf4j.LoggerFactory; import org.tron.common.crypto.SignInterface; import org.tron.common.crypto.SignUtils; import org.tron.common.utils.ByteArray; import org.tron.common.utils.LocalWitnesses; +import org.tron.core.exception.CipherException; import org.tron.core.exception.TronError; import org.tron.keystore.Credentials; import org.tron.keystore.WalletFile; @@ -183,6 +188,23 @@ public void testTamperedKeystoreRejectedAtSrLoading() throws Exception { } } + @Test + public void testInvalidPrivateKeyLoadError() { + java.util.List keystores = + java.util.Collections.singletonList(keystoreFileName); + + try (MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { + mockedSignUtils.when(() -> SignUtils.fromPrivate(any(byte[].class), eq(true))) + .thenThrow(new IllegalArgumentException("Invalid private key")); + + TronError err = assertThrows(TronError.class, + () -> WitnessInitializer.initFromKeystore(keystores, PASSWORD, null)); + assertEquals(TronError.ErrCode.WITNESS_KEYSTORE_LOAD, err.getErrCode()); + assertTrue(err.getCause() instanceof CipherException); + assertTrue(err.getCause().getCause() instanceof IllegalArgumentException); + } + } + private static ListAppender attachAppender() { ListAppender appender = new ListAppender<>(); appender.start(); diff --git a/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java b/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java index 82008988b6e..75cce11328e 100644 --- a/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java +++ b/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java @@ -1,11 +1,19 @@ package org.tron.keystore; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mockStatic; +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; +import org.mockito.MockedStatic; import org.tron.common.crypto.SignInterface; import org.tron.common.crypto.SignUtils; import org.tron.common.utils.Utils; @@ -90,4 +98,57 @@ public void testDecryptRejectsSpoofedAddressSm2() throws Exception { assertTrue(e.getMessage().contains("address mismatch")); } } + + @Test + public void testDecryptClearsPrivateKey() throws Exception { + String password = "test123456"; + SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true); + WalletFile walletFile = Wallet.createLight(password, keyPair); + byte[] expectedPrivateKey = keyPair.getPrivateKey(); + AtomicReference decryptedPrivateKey = new AtomicReference<>(); + + try (MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { + mockedSignUtils.when(() -> SignUtils.fromPrivate(any(byte[].class), eq(true))) + .thenAnswer(invocation -> { + byte[] privateKey = invocation.getArgument(0); + assertArrayEquals(expectedPrivateKey, privateKey); + decryptedPrivateKey.set(privateKey); + return keyPair; + }); + + assertSame(keyPair, Wallet.decrypt(password, walletFile, true)); + } + + byte[] clearedPrivateKey = decryptedPrivateKey.get(); + assertNotNull(clearedPrivateKey); + assertArrayEquals(new byte[clearedPrivateKey.length], clearedPrivateKey); + } + + @Test + public void testDecryptWrapsInvalidPrivateKey() throws Exception { + String password = "test123456"; + SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true); + WalletFile walletFile = Wallet.createLight(password, keyPair); + byte[] expectedPrivateKey = keyPair.getPrivateKey(); + AtomicReference decryptedPrivateKey = new AtomicReference<>(); + + try (MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { + mockedSignUtils.when(() -> SignUtils.fromPrivate(any(byte[].class), eq(true))) + .thenAnswer(invocation -> { + byte[] privateKey = invocation.getArgument(0); + assertArrayEquals(expectedPrivateKey, privateKey); + decryptedPrivateKey.set(privateKey); + throw new IllegalArgumentException("Invalid private key"); + }); + + CipherException exception = assertThrows(CipherException.class, + () -> Wallet.decrypt(password, walletFile, true)); + assertTrue(exception.getMessage().contains("Invalid private key")); + assertTrue(exception.getCause() instanceof IllegalArgumentException); + } + + byte[] clearedPrivateKey = decryptedPrivateKey.get(); + assertNotNull(clearedPrivateKey); + assertArrayEquals(new byte[clearedPrivateKey.length], clearedPrivateKey); + } } From 28d5aafae344dbe893b36f10043125eb42bbcea9 Mon Sep 17 00:00:00 2001 From: federico Date: Thu, 13 Aug 2026 17:40:35 +0800 Subject: [PATCH 2/8] test(crypto): simplify ECKey test names --- .../test/java/org/tron/common/crypto/ECKeyTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java b/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java index 6f983f9e66d..f847ef30bd8 100644 --- a/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java +++ b/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java @@ -75,7 +75,7 @@ public void testFromPrivateKey() { } @Test - public void shouldValidatePrivateKeyRange() { + public void testValidatePrivateKey() { assertTrue(ECKey.isValidPrivateKey(privateKey)); assertTrue(ECKey.isValidPrivateKey(Hex.decode(privString))); assertFalse(ECKey.isValidPrivateKey((BigInteger) null)); @@ -117,7 +117,7 @@ public void shouldValidatePrivateKeyRange() { } @Test - public void shouldRejectInvalidPublicKeys() { + public void testRejectInvalidPublicKey() { assertTrue(ECKey.isValidPublicKey(pubKey)); assertTrue(ECKey.isValidPublicKey(compressedPubKey)); assertFalse(ECKey.isValidPublicKey(null)); @@ -147,7 +147,7 @@ public void testPrivatePublicKeyBytesNoArg() { } @Test - public void shouldRejectMismatchedPrivateAndPublicKeys() { + public void testRejectMismatchedKeyPair() { BigInteger otherPrivateKey = privateKey.add(BigInteger.ONE); ECKey otherKey = ECKey.fromPrivate(otherPrivateKey); @@ -156,7 +156,7 @@ public void shouldRejectMismatchedPrivateAndPublicKeys() { } @Test - public void shouldAcceptMatchingPrivateAndPublicKeys() { + public void testAcceptMatchingKeyPair() { ECKey key = new ECKey(privateKey, ECKey.CURVE.getG().multiply(privateKey)); assertArrayEquals(pubKey, key.getPubKey()); @@ -302,7 +302,7 @@ public void testEqualsObject() { } @Test - public void shouldReturnDefensiveCopiesOfCachedValues() { + public void testDefensiveCopy() { ECKey key = ECKey.fromPrivate(privateKey); byte[] expectedAddress = Arrays.copyOf(key.getAddress(), key.getAddress().length); byte[] returnedAddress = key.getAddress(); From 749d937deda21f2686b39fdfc0e24751cdf37eaa Mon Sep 17 00:00:00 2001 From: federico Date: Tue, 22 Sep 2026 22:03:33 +0800 Subject: [PATCH 3/8] fix(crypto): harden ECKey validation --- .../java/org/tron/common/crypto/ECKey.java | 265 ++++++++---------- .../main/java/org/tron/keystore/Wallet.java | 37 ++- .../org/tron/common/crypto/ECKeyTest.java | 120 +++++++- .../args/WitnessInitializerKeystoreTest.java | 22 ++ .../keystore/WalletAddressValidationTest.java | 61 ++++ 5 files changed, 332 insertions(+), 173 deletions(-) diff --git a/crypto/src/main/java/org/tron/common/crypto/ECKey.java b/crypto/src/main/java/org/tron/common/crypto/ECKey.java index d0a6048aca1..ddb25f6afa1 100644 --- a/crypto/src/main/java/org/tron/common/crypto/ECKey.java +++ b/crypto/src/main/java/org/tron/common/crypto/ECKey.java @@ -23,15 +23,11 @@ import java.security.KeyPair; import java.security.KeyPairGenerator; import java.security.PrivateKey; -import java.security.Provider; import java.security.PublicKey; import java.security.SecureRandom; import java.security.SignatureException; -import java.security.interfaces.ECPrivateKey; -import java.security.interfaces.ECPublicKey; import java.security.spec.InvalidKeySpecException; import java.util.Arrays; -import java.util.Objects; import javax.annotation.Nullable; import lombok.extern.slf4j.Slf4j; import org.bouncycastle.asn1.sec.SECNamedCurves; @@ -58,6 +54,14 @@ import org.tron.common.utils.ByteArray; import org.tron.common.utils.ByteUtil; +/** + * A secp256k1 key pair and ECDSA signing utility. + * + *

ECDSA signatures are malleable: for a valid {@code (r, s)} pair, {@code (r, n - s)} is + * valid as well. New signatures are canonicalized to low-S form, but verification accepts both + * forms because transaction identifiers do not include signatures. See BIP-62 for background on + * low-S signatures. + */ @Slf4j(topic = "crypto") public class ECKey implements Serializable, SignInterface { @@ -77,6 +81,9 @@ public class ECKey implements Serializable, SignInterface { */ public static final BigInteger HALF_CURVE_ORDER; + private static final int MAX_PRIVATE_KEY_LENGTH = 32; + private static final int COMPRESSED_PUBLIC_KEY_LENGTH = 33; + private static final int UNCOMPRESSED_PUBLIC_KEY_LENGTH = 65; private static final BigInteger SECP256K1N = new BigInteger("fffffffffffffffffffffffffffffffebaaedce6af48a03bbfd25e8cd0364141", 16); private static final SecureRandom secureRandom; @@ -100,16 +107,10 @@ public class ECKey implements Serializable, SignInterface { // TODO: Redesign this class to use consistent internals and more // efficient serialization. private final PrivateKey privKey; - // the Java Cryptographic Architecture provider to use for Signature - // this is set along with the PrivateKey privKey and must be compatible - // this provider will be used when selecting a Signature instance - // https://docs.oracle.com/javase/8/docs/technotes/guides/security - // /SunProviders.html - private final Provider provider; // Transient because it's calculated on demand. - private transient byte[] pubKeyHash; - private transient byte[] nodeId; + private transient volatile byte[] pubKeyHash; + private transient volatile byte[] nodeId; /** * Generates an entirely new keypair. @@ -121,78 +122,58 @@ public ECKey() { } /** - * Generate a new keypair using the given Java Security Provider. + * Generates an entirely new keypair with the given {@link SecureRandom} object.

BouncyCastle + * will be used as the Java Security Provider * - *

All private key operations will use the provider. + * @param secureRandom - */ - public ECKey(Provider provider, SecureRandom secureRandom) { - this.provider = provider; - - final KeyPairGenerator keyPairGen = ECKeyPairGenerator.getInstance(provider, secureRandom); + public ECKey(SecureRandom secureRandom) { + final KeyPairGenerator keyPairGen = ECKeyPairGenerator.getInstance( + TronCastleProvider.getInstance(), secureRandom); final KeyPair keyPair = keyPairGen.generateKeyPair(); this.privKey = keyPair.getPrivate(); final PublicKey pubKey = keyPair.getPublic(); - if (pubKey instanceof BCECPublicKey) { - pub = ((BCECPublicKey) pubKey).getQ(); - } else if (pubKey instanceof ECPublicKey) { - pub = extractPublicKey((ECPublicKey) pubKey); - } else { + if (!(pubKey instanceof BCECPublicKey)) { throw new AssertionError( - "Expected Provider " + provider.getName() - + " to produce a subtype of ECPublicKey, found " - + pubKey.getClass()); + "Expected Bouncy Castle EC public key, found " + pubKey.getClass()); } + this.pub = ((BCECPublicKey) pubKey).getQ(); } - /** - * Generates an entirely new keypair with the given {@link SecureRandom} object.

BouncyCastle - * will be used as the Java Security Provider - * - * @param secureRandom - - */ - public ECKey(SecureRandom secureRandom) { - this(TronCastleProvider.getInstance(), secureRandom); - } - - /** - * Pair a private key with a public EC point. - * - *

All private key operations will use the provider. - */ - public ECKey(byte[] key, boolean isPrivateKey) { if (isPrivateKey) { + check(isValidPrivateKey(key), "Invalid private key"); BigInteger pk = new BigInteger(1, key); this.privKey = privateKeyFromBigInteger(pk); this.pub = CURVE.getG().multiply(pk); } else { this.privKey = null; - this.pub = CURVE.getCurve().decodePoint(key); + this.pub = decodePublicKey(key); + check(isValidPublicPoint(this.pub), "Invalid public key"); } - this.provider = TronCastleProvider.getInstance(); } - public ECKey(Provider provider, @Nullable PrivateKey privKey, ECPoint pub) { - this.provider = provider; + private ECKey(BigInteger privateKey) { + this.privKey = privateKeyFromBigInteger(privateKey); + this.pub = CURVE.getG().multiply(privateKey); + } - if (privKey == null || isECPrivateKey(privKey)) { + private ECKey(@Nullable PrivateKey privKey, ECPoint pub) { + if (privKey == null || privKey instanceof BCECPrivateKey) { this.privKey = privKey; } else { throw new IllegalArgumentException( - "Expected EC private key, given a private key object with" + - " class " + "Expected Bouncy Castle EC private key, given a private key object with class " + privKey.getClass().toString() + " and algorithm " + privKey.getAlgorithm()); } - if (pub == null) { - throw new IllegalArgumentException("Public key may not be null"); - } else { - this.pub = pub; - } + check(isValidPublicPoint(pub), "Invalid public key"); + checkPrivateKeyMatchesPublic(privKey, pub); + this.pub = pub; } /** @@ -200,32 +181,17 @@ public ECKey(Provider provider, @Nullable PrivateKey privKey, ECPoint pub) { * Security Provider */ public ECKey(@Nullable BigInteger priv, ECPoint pub) { - this( - TronCastleProvider.getInstance(), - privateKeyFromBigInteger(priv), - pub - ); + this(privateKeyFromBigInteger(priv), pub); } - /* Convert a Java JCE ECPublicKey into a BouncyCastle ECPoint - */ - private static ECPoint extractPublicKey(final ECPublicKey ecPublicKey) { - final java.security.spec.ECPoint publicPointW = ecPublicKey.getW(); - final BigInteger xCoord = publicPointW.getAffineX(); - final BigInteger yCoord = publicPointW.getAffineY(); - - return CURVE.getCurve().createPoint(xCoord, yCoord); - } - - /* Test if a generic private key is an EC private key - * - * it is not sufficient to check that privKey is a subtype of ECPrivateKey - * as the SunPKCS11 Provider will return a generic PrivateKey instance - * a fallback that covers this case is to check the key algorithm - */ - private static boolean isECPrivateKey(PrivateKey privKey) { - return privKey instanceof ECPrivateKey || privKey.getAlgorithm() - .equals("EC"); + private static void checkPrivateKeyMatchesPublic( + @Nullable PrivateKey privateKey, ECPoint publicPoint) { + if (privateKey instanceof BCECPrivateKey) { + BigInteger privateScalar = ((BCECPrivateKey) privateKey).getD(); + check(isValidPrivateKey(privateScalar), "Invalid private key"); + check(CURVE.getG().multiply(privateScalar).equals(publicPoint), + "Private key does not match public key"); + } } /* Convert a BigInteger into a PrivateKey object @@ -234,6 +200,7 @@ private static PrivateKey privateKeyFromBigInteger(BigInteger priv) { if (priv == null) { return null; } else { + check(isValidPrivateKey(priv), "Invalid private key"); try { return ECKeyFactory .getInstance(TronCastleProvider.getInstance()) @@ -245,6 +212,57 @@ private static PrivateKey privateKeyFromBigInteger(BigInteger priv) { } } + /** + * Returns whether the supplied key bytes represent a valid secp256k1 private scalar. + * Accepts unsigned encodings up to 32 bytes and Java {@link BigInteger} encodings with one + * leading zero sign byte. + */ + public static boolean isValidPrivateKey(byte[] privateKey) { + // BigInteger.toByteArray() prepends a zero sign byte only when the magnitude's high bit is set. + return !ByteArray.isEmpty(privateKey) + && (privateKey.length <= MAX_PRIVATE_KEY_LENGTH + || (privateKey.length == MAX_PRIVATE_KEY_LENGTH + 1 + && privateKey[0] == 0 + && (privateKey[1] & 0x80) != 0)) + && isValidPrivateKey(new BigInteger(1, privateKey)); + } + + /** + * Returns whether the supplied value is in the secp256k1 private-key range {@code [1, n)}. + */ + public static boolean isValidPrivateKey(BigInteger privateKey) { + return privateKey != null + && privateKey.signum() > 0 + && privateKey.compareTo(SECP256K1N) < 0; + } + + /** + * Returns whether the encoded key is a non-infinity point on its elliptic curve. + */ + public static boolean isValidPublicKey(byte[] publicKey) { + try { + return isValidPublicPoint(decodePublicKey(publicKey)); + } catch (RuntimeException e) { + return false; + } + } + + private static boolean isValidPublicPoint(ECPoint publicPoint) { + return publicPoint != null + && CURVE.getCurve().equals(publicPoint.getCurve()) + && !publicPoint.isInfinity() + && publicPoint.isValid(); + } + + private static ECPoint decodePublicKey(byte[] publicKey) { + check(isPubKeyCanonical(publicKey), "Invalid public key"); + try { + return CURVE.getCurve().decodePoint(publicKey); + } catch (RuntimeException e) { + throw new IllegalArgumentException("Invalid public key", e); + } + } + /** * Utility for compressing an elliptic curve point. Returns the same point if it's already * compressed. See the ECKey class docs for a discussion of point compression. @@ -276,7 +294,8 @@ public static ECPoint decompressPoint(ECPoint compressed) { * @return - */ public static ECKey fromPrivate(BigInteger privKey) { - return new ECKey(privKey, CURVE.getG().multiply(privKey)); + check(isValidPrivateKey(privKey), "Invalid private key"); + return new ECKey(privKey); } /** @@ -286,41 +305,8 @@ public static ECKey fromPrivate(BigInteger privKey) { * @return - */ public static ECKey fromPrivate(byte[] privKeyBytes) { - if (ByteArray.isEmpty(privKeyBytes)) { - return null; - } - return fromPrivate(new BigInteger(1, privKeyBytes)); - } - - /** - * Creates an ECKey that simply trusts the caller to ensure that point is really the result of - * multiplying the generator point by the private key. This is used to speed things up when you - * know you have the right values already. The compression state of pub will be preserved. - * - * @param priv - - * @param pub - - * @return - - */ - public static ECKey fromPrivateAndPrecalculatedPublic(BigInteger priv, - ECPoint pub) { - return new ECKey(priv, pub); - } - - /** - * Creates an ECKey that simply trusts the caller to ensure that point is really the result of - * multiplying the generator point by the private key. This is used to speed things up when you - * know you have the right values already. The compression state of the point will be preserved. - * - * @param priv - - * @param pub - - * @return - - */ - public static ECKey fromPrivateAndPrecalculatedPublic(byte[] priv, byte[] - pub) { - check(priv != null, "Private key must not be null"); - check(pub != null, "Public key must not be null"); - return new ECKey(new BigInteger(1, priv), CURVE.getCurve() - .decodePoint(pub)); + check(isValidPrivateKey(privKeyBytes), "Invalid private key"); + return new ECKey(new BigInteger(1, privKeyBytes)); } /** @@ -331,7 +317,7 @@ public static ECKey fromPrivateAndPrecalculatedPublic(byte[] priv, byte[] * @return - */ public static ECKey fromPublicOnly(ECPoint pub) { - return new ECKey(null, pub); + return new ECKey((PrivateKey) null, pub); } /** @@ -342,7 +328,7 @@ public static ECKey fromPublicOnly(ECPoint pub) { * @return - */ public static ECKey fromPublicOnly(byte[] pub) { - return new ECKey(null, CURVE.getCurve().decodePoint(pub)); + return new ECKey((PrivateKey) null, decodePublicKey(pub)); } /** @@ -355,6 +341,7 @@ public static ECKey fromPublicOnly(byte[] pub) { */ public static byte[] publicKeyFromPrivate(BigInteger privKey, boolean compressed) { + check(isValidPrivateKey(privKey), "Invalid private key"); ECPoint point = CURVE.getG().multiply(privKey); return point.getEncoded(compressed); } @@ -481,12 +468,15 @@ public static ECKey signatureToKey(byte[] messageHash, String * @return - */ public static boolean isPubKeyCanonical(byte[] pubkey) { + if (ByteArray.isEmpty(pubkey)) { + return false; + } if (pubkey[0] == 0x04) { // Uncompressed pubkey - return pubkey.length == 65; + return pubkey.length == UNCOMPRESSED_PUBLIC_KEY_LENGTH; } else if (pubkey[0] == 0x02 || pubkey[0] == 0x03) { // Compressed pubkey - return pubkey.length == 33; + return pubkey.length == COMPRESSED_PUBLIC_KEY_LENGTH; } else { return false; } @@ -669,10 +659,12 @@ public boolean hasPrivKey() { * @return 21-byte address */ public byte[] getAddress() { - if (pubKeyHash == null) { - pubKeyHash = Hash.computeAddress(this.pub); + byte[] address = pubKeyHash; + if (address == null) { + address = Hash.computeAddress(this.pub); + pubKeyHash = address; } - return pubKeyHash; + return Arrays.copyOf(address, address.length); } @Override @@ -691,10 +683,12 @@ public byte[] Base64toBytes(String signature) { * Generates the NodeID based on this key, that is the public key without first format byte */ public byte[] getNodeId() { - if (nodeId == null) { - nodeId = pubBytesWithoutFormat(this.pub); + byte[] id = nodeId; + if (id == null) { + id = pubBytesWithoutFormat(this.pub); + nodeId = id; } - return nodeId; + return Arrays.copyOf(id, id.length); } @@ -744,22 +738,6 @@ public String toString() { return b.toString(); } - /** - * Produce a string rendering of the ECKey INCLUDING the private key. Unless you absolutely need - * the private key it is better for security reasons to just use toString(). - * - * @return - - */ - public String toStringWithPrivate() { - StringBuilder b = new StringBuilder(); - b.append(toString()); - if (privKey != null && privKey instanceof BCECPrivateKey) { - b.append(" priv:").append(Hex.toHexString(((BCECPrivateKey) - privKey).getD().toByteArray())); - } - return b.toString(); - } - /** * Signs the given hash and returns the R and S components as BigIntegers and putData them in * ECDSASignature @@ -841,8 +819,6 @@ public byte[] getPrivKeyBytes() { return null; } else if (privKey instanceof BCECPrivateKey) { return ByteUtil.bigIntegerToBytes(((BCECPrivateKey) privKey).getD(), 32); - } else if (privKey instanceof ECPrivateKey) { - return ByteUtil.bigIntegerToBytes(((ECPrivateKey) privKey).getS(), 32); } else { return null; } @@ -860,10 +836,7 @@ public boolean equals(Object o) { ECKey ecKey = (ECKey) o; - if (privKey != null && !privKey.equals(ecKey.privKey)) { - return false; - } - return pub == null || pub.equals(ecKey.pub); + return pub.equals(ecKey.pub); } @Override diff --git a/crypto/src/main/java/org/tron/keystore/Wallet.java b/crypto/src/main/java/org/tron/keystore/Wallet.java index d63525b1e4d..127d724a567 100644 --- a/crypto/src/main/java/org/tron/keystore/Wallet.java +++ b/crypto/src/main/java/org/tron/keystore/Wallet.java @@ -216,22 +216,31 @@ public static SignInterface decrypt(String password, WalletFile walletFile, byte[] encryptKey = Arrays.copyOfRange(derivedKey, 0, 16); byte[] privateKey = performCipherOperation(Cipher.DECRYPT_MODE, iv, encryptKey, cipherText); - SignInterface keyPair = SignUtils.fromPrivate(privateKey, ecKey); - - // Enforce address consistency: if the keystore declares an address, it MUST match - // the address derived from the decrypted private key. Prevents address spoofing - // where a crafted keystore displays one address but encrypts a different key. - String declared = walletFile.getAddress(); - if (declared != null && !declared.isEmpty()) { - String derived = StringUtil.encode58Check(keyPair.getAddress()); - if (!declared.equals(derived)) { - throw new CipherException( - "Keystore address mismatch: file declares " + declared - + " but private key derives " + derived); + try { + SignInterface keyPair; + try { + keyPair = SignUtils.fromPrivate(privateKey, ecKey); + } catch (IllegalArgumentException e) { + throw new CipherException("Invalid private key in keystore", e); } - } - return keyPair; + // Enforce address consistency: if the keystore declares an address, it MUST match + // the address derived from the decrypted private key. Prevents address spoofing + // where a crafted keystore displays one address but encrypts a different key. + String declared = walletFile.getAddress(); + if (declared != null && !declared.isEmpty()) { + String derived = StringUtil.encode58Check(keyPair.getAddress()); + if (!declared.equals(derived)) { + throw new CipherException( + "Keystore address mismatch: file declares " + declared + + " but private key derives " + derived); + } + } + + return keyPair; + } finally { + Arrays.fill(privateKey, (byte) 0); + } } /** diff --git a/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java b/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java index 273672e8342..f847ef30bd8 100644 --- a/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java +++ b/framework/src/test/java/org/tron/common/crypto/ECKeyTest.java @@ -5,17 +5,16 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.tron.common.utils.client.utils.AbiUtil.generateOccupationConstantPrivateKey; import java.math.BigInteger; -import java.security.KeyPairGenerator; -import java.security.Security; import java.security.SignatureException; import java.util.Arrays; import lombok.extern.slf4j.Slf4j; +import org.bouncycastle.asn1.sec.SECNamedCurves; import org.bouncycastle.util.encoders.Hex; import org.junit.Test; import org.tron.common.crypto.ECKey.ECDSASignature; @@ -69,10 +68,76 @@ public void testFromPrivateKey() { assertTrue(key.hasPrivKey()); assertArrayEquals(pubKey, key.getPubKey()); - key = ECKey.fromPrivate((byte[]) null); - assertNull(key); - key = ECKey.fromPrivate(new byte[0]); - assertNull(key); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate((byte[]) null)); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate(new byte[0])); + } + + @Test + public void testValidatePrivateKey() { + assertTrue(ECKey.isValidPrivateKey(privateKey)); + assertTrue(ECKey.isValidPrivateKey(Hex.decode(privString))); + assertFalse(ECKey.isValidPrivateKey((BigInteger) null)); + assertFalse(ECKey.isValidPrivateKey((byte[]) null)); + assertFalse(ECKey.isValidPrivateKey(new byte[0])); + assertFalse(ECKey.isValidPrivateKey(new byte[33])); + assertFalse(ECKey.isValidPrivateKey(BigInteger.ZERO)); + assertFalse(ECKey.isValidPrivateKey(ECKey.CURVE.getN())); + + BigInteger highBitPrivateKey = ECKey.CURVE.getN().subtract(BigInteger.ONE); + byte[] signPaddedPrivateKey = highBitPrivateKey.toByteArray(); + assertEquals(33, signPaddedPrivateKey.length); + assertEquals(0, signPaddedPrivateKey[0]); + assertTrue(ECKey.isValidPrivateKey(signPaddedPrivateKey)); + assertEquals(highBitPrivateKey, ECKey.fromPrivate(signPaddedPrivateKey).getPrivKey()); + + byte[] redundantSignPaddedPrivateKey = new byte[33]; + redundantSignPaddedPrivateKey[32] = 1; + assertFalse(ECKey.isValidPrivateKey(redundantSignPaddedPrivateKey)); + + byte[] nonZeroLeadingPrivateKey = Arrays.copyOf(signPaddedPrivateKey, + signPaddedPrivateKey.length); + nonZeroLeadingPrivateKey[0] = 1; + assertFalse(ECKey.isValidPrivateKey(nonZeroLeadingPrivateKey)); + + byte[] doubleSignPaddedPrivateKey = new byte[34]; + System.arraycopy(signPaddedPrivateKey, 0, doubleSignPaddedPrivateKey, 1, + signPaddedPrivateKey.length); + assertFalse(ECKey.isValidPrivateKey(doubleSignPaddedPrivateKey)); + + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate(BigInteger.ZERO)); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate(ECKey.CURVE.getN())); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPrivate(new byte[1024 * 1024])); + assertThrows(IllegalArgumentException.class, + () -> ECKey.publicKeyFromPrivate(null, false)); + } + + @Test + public void testRejectInvalidPublicKey() { + assertTrue(ECKey.isValidPublicKey(pubKey)); + assertTrue(ECKey.isValidPublicKey(compressedPubKey)); + assertFalse(ECKey.isValidPublicKey(null)); + assertFalse(ECKey.isValidPublicKey(new byte[0])); + assertFalse(ECKey.isValidPublicKey(new byte[]{0})); + assertFalse(ECKey.isValidPublicKey(new byte[66])); + + byte[] oversizedPublicKey = new byte[1024 * 1024]; + oversizedPublicKey[0] = 0x04; + assertFalse(ECKey.isValidPublicKey(oversizedPublicKey)); + + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPublicOnly((byte[]) null)); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPublicOnly(ECKey.CURVE.getCurve().getInfinity())); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPublicOnly( + ECKey.CURVE.getCurve().createPoint(BigInteger.ZERO, BigInteger.ZERO))); + assertThrows(IllegalArgumentException.class, + () -> ECKey.fromPublicOnly(SECNamedCurves.getByName("secp256r1").getG())); } @Test(expected = IllegalArgumentException.class) @@ -81,12 +146,21 @@ public void testPrivatePublicKeyBytesNoArg() { fail("Expecting an IllegalArgumentException for using only null-parameters"); } - @Test(expected = IllegalArgumentException.class) - public void testInvalidPrivateKey() throws Exception { - new ECKey(Security.getProvider("SunEC"), - KeyPairGenerator.getInstance("RSA").generateKeyPair().getPrivate(), - ECKey.fromPublicOnly(pubKey).getPubKeyPoint()); - fail("Expecting an IllegalArgumentException for using an non EC private key"); + @Test + public void testRejectMismatchedKeyPair() { + BigInteger otherPrivateKey = privateKey.add(BigInteger.ONE); + ECKey otherKey = ECKey.fromPrivate(otherPrivateKey); + + assertThrows(IllegalArgumentException.class, + () -> new ECKey(privateKey, otherKey.getPubKeyPoint())); + } + + @Test + public void testAcceptMatchingKeyPair() { + ECKey key = new ECKey(privateKey, ECKey.CURVE.getG().multiply(privateKey)); + + assertArrayEquals(pubKey, key.getPubKey()); + assertTrue(key.hasPrivKey()); } @Test @@ -201,6 +275,8 @@ public void testIsPubKeyCanonicalWrongPrefix() { // Test wrong prefix 3, right length 33 byte[] nonCanonicalPubkey6 = new byte[33]; assertFalse(ECKey.isPubKeyCanonical(nonCanonicalPubkey6)); + assertFalse(ECKey.isPubKeyCanonical(null)); + assertFalse(ECKey.isPubKeyCanonical(new byte[0])); } @Test @@ -215,10 +291,28 @@ public void testEqualsObject() { ECKey key0 = new ECKey(); ECKey key1 = ECKey.fromPrivate(privateKey); ECKey key2 = ECKey.fromPrivate(privateKey); + ECKey publicOnlyKey = ECKey.fromPublicOnly(key1.getPubKey()); assertFalse(key0.equals(key1)); assertTrue(key1.equals(key1)); assertTrue(key1.equals(key2)); + assertTrue(key1.equals(publicOnlyKey)); + assertTrue(publicOnlyKey.equals(key1)); + assertEquals(key1.hashCode(), publicOnlyKey.hashCode()); + } + + @Test + public void testDefensiveCopy() { + ECKey key = ECKey.fromPrivate(privateKey); + byte[] expectedAddress = Arrays.copyOf(key.getAddress(), key.getAddress().length); + byte[] returnedAddress = key.getAddress(); + returnedAddress[0] ^= 1; + assertArrayEquals(expectedAddress, key.getAddress()); + + byte[] expectedNodeId = Arrays.copyOf(key.getNodeId(), key.getNodeId().length); + byte[] returnedNodeId = key.getNodeId(); + returnedNodeId[0] ^= 1; + assertArrayEquals(expectedNodeId, key.getNodeId()); } diff --git a/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java b/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java index 80d8287682b..386ac7c90a9 100644 --- a/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java +++ b/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java @@ -5,6 +5,9 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mockStatic; import ch.qos.logback.classic.Logger; import ch.qos.logback.classic.spi.ILoggingEvent; @@ -18,11 +21,13 @@ import org.junit.ClassRule; import org.junit.Test; import org.junit.rules.TemporaryFolder; +import org.mockito.MockedStatic; import org.slf4j.LoggerFactory; import org.tron.common.crypto.SignInterface; import org.tron.common.crypto.SignUtils; import org.tron.common.utils.ByteArray; import org.tron.common.utils.LocalWitnesses; +import org.tron.core.exception.CipherException; import org.tron.core.exception.TronError; import org.tron.keystore.Credentials; import org.tron.keystore.WalletFile; @@ -183,6 +188,23 @@ public void testTamperedKeystoreRejectedAtSrLoading() throws Exception { } } + @Test + public void testInvalidPrivateKeyLoadError() { + java.util.List keystores = + java.util.Collections.singletonList(keystoreFileName); + + try (MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { + mockedSignUtils.when(() -> SignUtils.fromPrivate(any(byte[].class), eq(true))) + .thenThrow(new IllegalArgumentException("Invalid private key")); + + TronError err = assertThrows(TronError.class, + () -> WitnessInitializer.initFromKeystore(keystores, PASSWORD, null)); + assertEquals(TronError.ErrCode.WITNESS_KEYSTORE_LOAD, err.getErrCode()); + assertTrue(err.getCause() instanceof CipherException); + assertTrue(err.getCause().getCause() instanceof IllegalArgumentException); + } + } + private static ListAppender attachAppender() { ListAppender appender = new ListAppender<>(); appender.start(); diff --git a/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java b/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java index 82008988b6e..75cce11328e 100644 --- a/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java +++ b/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java @@ -1,11 +1,19 @@ package org.tron.keystore; +import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.mockStatic; +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; +import org.mockito.MockedStatic; import org.tron.common.crypto.SignInterface; import org.tron.common.crypto.SignUtils; import org.tron.common.utils.Utils; @@ -90,4 +98,57 @@ public void testDecryptRejectsSpoofedAddressSm2() throws Exception { assertTrue(e.getMessage().contains("address mismatch")); } } + + @Test + public void testDecryptClearsPrivateKey() throws Exception { + String password = "test123456"; + SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true); + WalletFile walletFile = Wallet.createLight(password, keyPair); + byte[] expectedPrivateKey = keyPair.getPrivateKey(); + AtomicReference decryptedPrivateKey = new AtomicReference<>(); + + try (MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { + mockedSignUtils.when(() -> SignUtils.fromPrivate(any(byte[].class), eq(true))) + .thenAnswer(invocation -> { + byte[] privateKey = invocation.getArgument(0); + assertArrayEquals(expectedPrivateKey, privateKey); + decryptedPrivateKey.set(privateKey); + return keyPair; + }); + + assertSame(keyPair, Wallet.decrypt(password, walletFile, true)); + } + + byte[] clearedPrivateKey = decryptedPrivateKey.get(); + assertNotNull(clearedPrivateKey); + assertArrayEquals(new byte[clearedPrivateKey.length], clearedPrivateKey); + } + + @Test + public void testDecryptWrapsInvalidPrivateKey() throws Exception { + String password = "test123456"; + SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom(), true); + WalletFile walletFile = Wallet.createLight(password, keyPair); + byte[] expectedPrivateKey = keyPair.getPrivateKey(); + AtomicReference decryptedPrivateKey = new AtomicReference<>(); + + try (MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { + mockedSignUtils.when(() -> SignUtils.fromPrivate(any(byte[].class), eq(true))) + .thenAnswer(invocation -> { + byte[] privateKey = invocation.getArgument(0); + assertArrayEquals(expectedPrivateKey, privateKey); + decryptedPrivateKey.set(privateKey); + throw new IllegalArgumentException("Invalid private key"); + }); + + CipherException exception = assertThrows(CipherException.class, + () -> Wallet.decrypt(password, walletFile, true)); + assertTrue(exception.getMessage().contains("Invalid private key")); + assertTrue(exception.getCause() instanceof IllegalArgumentException); + } + + byte[] clearedPrivateKey = decryptedPrivateKey.get(); + assertNotNull(clearedPrivateKey); + assertArrayEquals(new byte[clearedPrivateKey.length], clearedPrivateKey); + } } From e9c5ce4765ff333ee0ac124380e4388df46c0d83 Mon Sep 17 00:00:00 2001 From: federico Date: Thu, 24 Sep 2026 14:57:46 +0800 Subject: [PATCH 4/8] fix(config): validate witness private key range --- .../org/tron/common/utils/LocalWitnesses.java | 3 ++ .../core/config/args/LocalWitnessTest.java | 19 +++++++++ .../args/WitnessInitializerKeystoreTest.java | 40 ++++++++++--------- .../config/args/WitnessInitializerTest.java | 15 +++++++ 4 files changed, 59 insertions(+), 18 deletions(-) diff --git a/chainbase/src/main/java/org/tron/common/utils/LocalWitnesses.java b/chainbase/src/main/java/org/tron/common/utils/LocalWitnesses.java index 7179045ea7e..b1346e870bf 100644 --- a/chainbase/src/main/java/org/tron/common/utils/LocalWitnesses.java +++ b/chainbase/src/main/java/org/tron/common/utils/LocalWitnesses.java @@ -88,6 +88,9 @@ private void validate(String privateKey) { throw new TronError("private key must be hex string", TronError.ErrCode.WITNESS_INIT); } + if (!ECKey.isValidPrivateKey(ByteArray.fromHexString(privateKey))) { + throw new TronError("Invalid ECDSA witness private key", TronError.ErrCode.WITNESS_INIT); + } } public void addPrivateKeys(String privateKey) { diff --git a/framework/src/test/java/org/tron/core/config/args/LocalWitnessTest.java b/framework/src/test/java/org/tron/core/config/args/LocalWitnessTest.java index 1b30518c7e3..8197e904cd4 100644 --- a/framework/src/test/java/org/tron/core/config/args/LocalWitnessTest.java +++ b/framework/src/test/java/org/tron/core/config/args/LocalWitnessTest.java @@ -22,7 +22,9 @@ import com.google.common.collect.Lists; import java.io.IOException; +import java.math.BigInteger; import java.security.SecureRandom; +import java.util.Arrays; import org.junit.AfterClass; import org.junit.Assert; import org.junit.Before; @@ -30,6 +32,7 @@ import org.junit.Test; import org.junit.rules.TemporaryFolder; import org.tron.common.TestConstants; +import org.tron.common.crypto.ECKey; import org.tron.common.utils.ByteArray; import org.tron.common.utils.LocalWitnesses; import org.tron.common.utils.PublicMethod; @@ -132,6 +135,22 @@ public void testInvalidPrivateKey() { assertTronError(localWitnesses, privateKey2, expectedMessage2); } + @Test + public void testPrivateKeyRange() { + for (BigInteger scalar : Arrays.asList(BigInteger.ZERO, ECKey.CURVE.getN())) { + String key = String.format("%064x", scalar); + assertTronError(localWitness, key, "Invalid ECDSA witness private key"); + TronError error = assertThrows(TronError.class, + () -> localWitness.setPrivateKeys(Arrays.asList(PRIVATE_KEY, key))); + assertEquals(ErrCode.WITNESS_INIT, error.getErrCode()); + } + for (BigInteger scalar : Arrays.asList(BigInteger.ONE, + ECKey.CURVE.getN().subtract(BigInteger.ONE))) { + String key = String.format("%064x", scalar); + assertEquals(key, new LocalWitnesses(key).getPrivateKey()); + } + } + private void assertTronError(LocalWitnesses localWitnesses, String privateKey, String expectedMessage) { TronError thrown = assertThrows(TronError.class, diff --git a/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java b/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java index 386ac7c90a9..ee6a7044d13 100644 --- a/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java +++ b/framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java @@ -5,9 +5,8 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import ch.qos.logback.classic.Logger; import ch.qos.logback.classic.spi.ILoggingEvent; @@ -15,14 +14,16 @@ import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; import java.io.File; +import java.math.BigInteger; import java.security.SecureRandom; +import java.util.Collections; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.ClassRule; import org.junit.Test; import org.junit.rules.TemporaryFolder; -import org.mockito.MockedStatic; import org.slf4j.LoggerFactory; +import org.tron.common.crypto.ECKey; import org.tron.common.crypto.SignInterface; import org.tron.common.crypto.SignUtils; import org.tron.common.utils.ByteArray; @@ -30,6 +31,7 @@ import org.tron.core.exception.CipherException; import org.tron.core.exception.TronError; import org.tron.keystore.Credentials; +import org.tron.keystore.Wallet; import org.tron.keystore.WalletFile; import org.tron.keystore.WalletUtils; @@ -189,20 +191,22 @@ public void testTamperedKeystoreRejectedAtSrLoading() throws Exception { } @Test - public void testInvalidPrivateKeyLoadError() { - java.util.List keystores = - java.util.Collections.singletonList(keystoreFileName); - - try (MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { - mockedSignUtils.when(() -> SignUtils.fromPrivate(any(byte[].class), eq(true))) - .thenThrow(new IllegalArgumentException("Invalid private key")); - - TronError err = assertThrows(TronError.class, - () -> WitnessInitializer.initFromKeystore(keystores, PASSWORD, null)); - assertEquals(TronError.ErrCode.WITNESS_KEYSTORE_LOAD, err.getErrCode()); - assertTrue(err.getCause() instanceof CipherException); - assertTrue(err.getCause().getCause() instanceof IllegalArgumentException); - } + public void testInvalidPrivateKeyLoadError() throws Exception { + // Only mock the source of bytes for encryption. Decryption and key validation are real. + SignInterface invalidKey = mock(SignInterface.class); + when(invalidKey.getPrivateKey()).thenReturn(new byte[32]); + when(invalidKey.getAddress()).thenReturn(ECKey.fromPrivate(BigInteger.ONE).getAddress()); + WalletFile walletFile = Wallet.createLight(PASSWORD, invalidKey); + String fileName = DIR_NAME + "/invalid-key.json"; + new ObjectMapper().writeValue(new File(System.getProperty("user.dir"), fileName), walletFile); + + TronError err = assertThrows(TronError.class, + () -> WitnessInitializer.initFromKeystore( + Collections.singletonList(fileName), PASSWORD, null)); + assertEquals(TronError.ErrCode.WITNESS_KEYSTORE_LOAD, err.getErrCode()); + assertTrue(err.getCause() instanceof CipherException); + assertEquals("Invalid private key in keystore", err.getCause().getMessage()); + assertTrue(err.getCause().getCause() instanceof IllegalArgumentException); } private static ListAppender attachAppender() { diff --git a/framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java b/framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java index e0aa2606473..0b945e759a6 100644 --- a/framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java +++ b/framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java @@ -13,6 +13,7 @@ import static org.mockito.Mockito.when; import java.io.File; +import java.math.BigInteger; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -94,6 +95,20 @@ public void testInitFromConfig() { assertEquals(ErrCode.WITNESS_INIT, err.getErrCode()); } + @Test + public void testRejectInvalidPlaintextPrivateKeys() { + String key = String.format("%064x", BigInteger.ZERO); + for (String witnessAddress : Arrays.asList(null, address)) { + TronError cliError = assertThrows(TronError.class, + () -> WitnessInitializer.initFromCLIPrivateKey(key, witnessAddress)); + assertEquals(ErrCode.WITNESS_INIT, cliError.getErrCode()); + TronError configError = assertThrows(TronError.class, + () -> WitnessInitializer.initFromCFGPrivateKey( + Collections.singletonList(key), witnessAddress)); + assertEquals(ErrCode.WITNESS_INIT, configError.getErrCode()); + } + } + @Test public void testInitFromKeystore() { List keystores = Arrays.asList("keystore1.json", "keystore2.json"); From 98a40b62fd067da11c5f3b4ad6ef527de58fdad8 Mon Sep 17 00:00:00 2001 From: federico Date: Wed, 30 Sep 2026 10:29:30 +0800 Subject: [PATCH 5/8] fix(crypto): isolate Bouncy Castle provider --- .../java/org/tron/common/crypto/Hash.java | 4 +- .../common/crypto/jce/TronCastleProvider.java | 6 +- .../crypto/jce/TronCastleProviderTest.java | 98 +++++++++++++++++++ 3 files changed, 102 insertions(+), 6 deletions(-) create mode 100644 framework/src/test/java/org/tron/common/crypto/jce/TronCastleProviderTest.java diff --git a/crypto/src/main/java/org/tron/common/crypto/Hash.java b/crypto/src/main/java/org/tron/common/crypto/Hash.java index eca769a8dad..7af0f2c06fb 100644 --- a/crypto/src/main/java/org/tron/common/crypto/Hash.java +++ b/crypto/src/main/java/org/tron/common/crypto/Hash.java @@ -68,8 +68,8 @@ public class Hash { private static final int SIZE_THRESHOLD = 56; static { - Security.addProvider(TronCastleProvider.getInstance()); - CRYPTO_PROVIDER = Security.getProvider("BC"); + CRYPTO_PROVIDER = TronCastleProvider.getInstance(); + Security.addProvider(CRYPTO_PROVIDER); HASH_256_ALGORITHM_NAME = "TRON-KECCAK-256"; HASH_512_ALGORITHM_NAME = "TRON-KECCAK-512"; EMPTY_TRIE_HASH = sha3(encodeElement(EMPTY_BYTE_ARRAY)); diff --git a/crypto/src/main/java/org/tron/common/crypto/jce/TronCastleProvider.java b/crypto/src/main/java/org/tron/common/crypto/jce/TronCastleProvider.java index ad8e51e65ac..b65317deda9 100644 --- a/crypto/src/main/java/org/tron/common/crypto/jce/TronCastleProvider.java +++ b/crypto/src/main/java/org/tron/common/crypto/jce/TronCastleProvider.java @@ -19,7 +19,6 @@ package org.tron.common.crypto.jce; import java.security.Provider; -import java.security.Security; import org.bouncycastle.jce.provider.BouncyCastleProvider; import org.tron.common.crypto.cryptohash.Keccak256; import org.tron.common.crypto.cryptohash.Keccak512; @@ -35,9 +34,8 @@ private static class Holder { private static final Provider INSTANCE; static { - Provider p = Security.getProvider("BC"); - - INSTANCE = (p != null) ? p : new BouncyCastleProvider(); + // Use our own instance instead of trusting a globally registered provider named BC. + INSTANCE = new BouncyCastleProvider(); INSTANCE.put("MessageDigest.TRON-KECCAK-256", Keccak256.class.getName()); INSTANCE.put("MessageDigest.TRON-KECCAK-512", Keccak512.class.getName()); } diff --git a/framework/src/test/java/org/tron/common/crypto/jce/TronCastleProviderTest.java b/framework/src/test/java/org/tron/common/crypto/jce/TronCastleProviderTest.java new file mode 100644 index 00000000000..81fd0e3b952 --- /dev/null +++ b/framework/src/test/java/org/tron/common/crypto/jce/TronCastleProviderTest.java @@ -0,0 +1,98 @@ +package org.tron.common.crypto.jce; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNotSame; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertSame; + +import java.net.URL; +import java.net.URLClassLoader; +import java.security.MessageDigest; +import java.security.Provider; +import java.security.SecureRandom; +import java.security.Security; +import java.util.Arrays; +import org.bouncycastle.jce.provider.BouncyCastleProvider; +import org.bouncycastle.util.encoders.Hex; +import org.junit.Test; +import org.tron.common.crypto.Hash; +import org.tron.common.crypto.cryptohash.Keccak512; + +public class TronCastleProviderTest { + + @Test + public void testExistingProvider() throws Exception { + assertProviderIsolation(new Provider("BC", 1.0, "Unrelated test provider") { }); + } + + @Test + public void testNoRegisteredProvider() throws Exception { + assertProviderIsolation(null); + } + + private void assertProviderIsolation(Provider existing) throws Exception { + Provider original = Security.getProvider("BC"); + int position = Arrays.asList(Security.getProviders()).indexOf(original) + 1; + Security.removeProvider("BC"); + try (URLClassLoader loader = newCryptoClassLoader()) { + if (existing != null) { + Security.addProvider(existing); + } + Class providerClass = loader.loadClass(TronCastleProvider.class.getName()); + Provider provider = (Provider) providerClass.getMethod("getInstance").invoke(null); + assertEquals(BouncyCastleProvider.class, provider.getClass()); + assertNotSame(existing, provider); + assertSame(provider, providerClass.getMethod("getInstance").invoke(null)); + assertSame(existing, Security.getProvider("BC")); + + byte[] empty = new byte[0]; + byte[] expected = Hex.decode( + "c5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470"); + assertArrayEquals(expected, + MessageDigest.getInstance("TRON-KECCAK-256", provider).digest(empty)); + assertArrayEquals(new Keccak512().digest(empty), + MessageDigest.getInstance("TRON-KECCAK-512", provider).digest(empty)); + assertNotNull(ECKeyPairGenerator.getInstance(provider, new SecureRandom()).generateKeyPair()); + + Class hashClass = loader.loadClass(Hash.class.getName()); + assertArrayEquals(expected, + (byte[]) hashClass.getMethod("sha3", byte[].class).invoke(null, empty)); + if (existing != null) { + assertSame(existing, Security.getProvider("BC")); + assertNull(existing.get("MessageDigest.TRON-KECCAK-256")); + assertNull(existing.get("MessageDigest.TRON-KECCAK-512")); + } + } finally { + Security.removeProvider("BC"); + if (original != null) { + Security.insertProviderAt(original, position); + } + } + } + + private URLClassLoader newCryptoClassLoader() { + URL location = TronCastleProvider.class.getProtectionDomain().getCodeSource().getLocation(); + // Reload static initializers regardless of which crypto tests ran earlier in this JVM. + return new URLClassLoader(new URL[]{location}, getClass().getClassLoader()) { + @Override + protected synchronized Class loadClass(String name, boolean resolve) + throws ClassNotFoundException { + if (name.equals(Hash.class.getName()) + || name.equals(TronCastleProvider.class.getName()) + || name.startsWith(TronCastleProvider.class.getName() + "$")) { + Class loaded = findLoadedClass(name); + if (loaded == null) { + loaded = findClass(name); + } + if (resolve) { + resolveClass(loaded); + } + return loaded; + } + return super.loadClass(name, resolve); + } + }; + } +} From ed0819b77b728da99731305cc70a7d6fcb0b93c6 Mon Sep 17 00:00:00 2001 From: federico Date: Sun, 11 Oct 2026 11:18:46 +0800 Subject: [PATCH 6/8] fix(crypto): correct Keccak digest API behavior --- .../tron/common/crypto/cryptohash/Digest.java | 14 +- .../crypto/cryptohash/DigestEngine.java | 32 +-- .../common/crypto/cryptohash/Keccak256.java | 8 +- .../common/crypto/cryptohash/Keccak512.java | 4 +- .../common/crypto/cryptohash/KeccakTest.java | 229 ++++++++++++++++++ 5 files changed, 261 insertions(+), 26 deletions(-) create mode 100644 crypto/src/test/java/org/tron/common/crypto/cryptohash/KeccakTest.java diff --git a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java index 62362d1d62d..312c3dd9ab9 100644 --- a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java +++ b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java @@ -18,6 +18,8 @@ package org.tron.common.crypto.cryptohash; +import java.security.DigestException; + /** * Copyright (c) 2007-2010 Project RNRT SAPHIR * @@ -82,17 +84,17 @@ public interface Digest { /** * Finalize the current hash computation and store the hash value in the provided output buffer. - * The {@code len} parameter contains the maximum number of bytes that should be written; no more - * bytes than the natural hash function output length will be produced. If {@code len} is smaller - * than the natural hash output length, the hash output is truncated to its first {@code len} - * bytes. The object is resetted. + * The {@code len} parameter contains the number of bytes available for the digest. The complete + * digest is written and the object is reset on success. Partial digests are not returned. * * @param outbuf the output buffer * @param off the output offset within {@code outbuf} - * @param len the requested hash output length (in bytes) + * @param len the available output length (in bytes) * @return the number of bytes actually written in {@code outbuf} + * @throws DigestException if {@code len} is smaller than the digest length + * @throws IllegalArgumentException if the output buffer is null or the output range is invalid */ - int digest(byte[] outbuf, int off, int len); + int digest(byte[] outbuf, int off, int len) throws DigestException; /** * Get the natural hash function output length (in bytes). diff --git a/crypto/src/main/java/org/tron/common/crypto/cryptohash/DigestEngine.java b/crypto/src/main/java/org/tron/common/crypto/cryptohash/DigestEngine.java index c7d013a0804..a27a58ec6a8 100644 --- a/crypto/src/main/java/org/tron/common/crypto/cryptohash/DigestEngine.java +++ b/crypto/src/main/java/org/tron/common/crypto/cryptohash/DigestEngine.java @@ -18,12 +18,13 @@ package org.tron.common.crypto.cryptohash; +import java.security.DigestException; import java.security.MessageDigest; public abstract class DigestEngine extends MessageDigest implements Digest { private int digestLen, blockLen, inputLen; - private byte[] inputBuf, outputBuf; + private byte[] inputBuf; private long blockCount; /** @@ -35,7 +36,6 @@ public DigestEngine(String alg) { digestLen = engineGetDigestLength(); blockLen = getInternalBlockLength(); inputBuf = new byte[blockLen]; - outputBuf = new byte[digestLen]; inputLen = 0; blockCount = 0; } @@ -72,7 +72,6 @@ public DigestEngine(String alg) { private void adjustDigestLen() { if (digestLen == 0) { digestLen = engineGetDigestLength(); - outputBuf = new byte[digestLen]; } } @@ -82,7 +81,8 @@ private void adjustDigestLen() { public byte[] digest() { adjustDigestLen(); byte[] result = new byte[digestLen]; - digest(result, 0, digestLen); + doPadding(result, 0); + reset(); return result; } @@ -97,18 +97,20 @@ public byte[] digest(byte[] input) { /** * @see Digest */ - public int digest(byte[] buf, int offset, int len) { + public int digest(byte[] buf, int offset, int len) throws DigestException { + if (buf == null) { + throw new IllegalArgumentException("Output buffer is null"); + } + if (offset < 0 || len < 0 || offset > buf.length || len > buf.length - offset) { + throw new IllegalArgumentException("Invalid output buffer range"); + } adjustDigestLen(); - if (len >= digestLen) { - doPadding(buf, offset); - reset(); - return digestLen; - } else { - doPadding(outputBuf, 0); - System.arraycopy(outputBuf, 0, buf, offset, len); - reset(); - return len; + if (len < digestLen) { + throw new DigestException("Partial digests are not returned"); } + doPadding(buf, offset); + reset(); + return digestLen; } /** @@ -222,8 +224,6 @@ protected Digest copyState(DigestEngine dest) { inputBuf.length); adjustDigestLen(); dest.adjustDigestLen(); - System.arraycopy(outputBuf, 0, dest.outputBuf, 0, - outputBuf.length); return dest; } } diff --git a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak256.java b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak256.java index 83c0577c789..7ac282e84b5 100644 --- a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak256.java +++ b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak256.java @@ -37,14 +37,16 @@ public int engineGetDigestLength() { @Override protected byte[] engineDigest() { - return null; + return digest(); } @Override - protected void engineUpdate(byte arg0) { + protected void engineUpdate(byte input) { + update(input); } @Override - protected void engineUpdate(byte[] arg0, int arg1, int arg2) { + protected void engineUpdate(byte[] input, int offset, int len) { + update(input, offset, len); } } diff --git a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak512.java b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak512.java index 8283180370e..3846012ec46 100644 --- a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak512.java +++ b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak512.java @@ -37,14 +37,16 @@ public int engineGetDigestLength() { @Override protected byte[] engineDigest() { - return null; + return digest(); } @Override protected void engineUpdate(byte input) { + update(input); } @Override protected void engineUpdate(byte[] input, int offset, int len) { + update(input, offset, len); } } diff --git a/crypto/src/test/java/org/tron/common/crypto/cryptohash/KeccakTest.java b/crypto/src/test/java/org/tron/common/crypto/cryptohash/KeccakTest.java new file mode 100644 index 00000000000..ece07286d68 --- /dev/null +++ b/crypto/src/test/java/org/tron/common/crypto/cryptohash/KeccakTest.java @@ -0,0 +1,229 @@ +package org.tron.common.crypto.cryptohash; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; + +import java.nio.ByteBuffer; +import java.security.DigestException; +import java.security.MessageDigest; +import java.util.Arrays; +import java.util.Collection; +import org.bouncycastle.crypto.digests.KeccakDigest; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.tron.common.crypto.jce.TronCastleProvider; + +@RunWith(Parameterized.class) +public class KeccakTest { + + private final int bits; + + public KeccakTest(int bits) { + this.bits = bits; + } + + @Parameterized.Parameters(name = "Keccak{0}") + public static Collection parameters() { + return Arrays.asList(new Object[][]{{256}, {512}}); + } + + @Test + public void testByteArrayDigest() throws Exception { + byte[] input = inputBytes(); + assertArrayEquals(referenceDigest(input), newDigest().digest(input)); + } + + @Test + public void testHeapByteBuffer() throws Exception { + checkBuffer(ByteBuffer.wrap(inputBytes())); + } + + @Test + public void testDirectByteBuffer() throws Exception { + ByteBuffer buffer = ByteBuffer.allocateDirect(inputBytes().length); + buffer.put(inputBytes()); + buffer.flip(); + checkBuffer(buffer); + } + + @Test + public void testReadOnlyByteBuffer() throws Exception { + checkBuffer(ByteBuffer.wrap(inputBytes()).asReadOnlyBuffer()); + } + + @Test + public void testByteBufferSlice() throws Exception { + byte[] input = inputBytes(); + ByteBuffer buffer = ByteBuffer.allocate(input.length + 10); + buffer.position(5); + buffer.put(input); + buffer.limit(5 + input.length); + buffer.position(5); + checkBuffer(buffer.slice()); + } + + @Test + public void testByteBufferWindow() throws Exception { + ByteBuffer buffer = ByteBuffer.wrap(inputBytes()); + buffer.position(7); + buffer.limit(buffer.limit() - 11); + checkBuffer(buffer); + } + + @Test + public void testEmptyByteBuffer() throws Exception { + ByteBuffer buffer = ByteBuffer.wrap(inputBytes()); + buffer.position(buffer.limit()); + checkBuffer(buffer); + } + + @Test + public void testMixedInput() throws Exception { + byte[] input = inputBytes(); + MessageDigest digest = newDigest(); + digest.update(input[0]); + digest.update(input, 1, 16); + ByteBuffer buffer = ByteBuffer.wrap(input, 17, input.length - 17); + digest.update(buffer); + assertEquals(buffer.limit(), buffer.position()); + assertArrayEquals(referenceDigest(input), digest.digest()); + } + + @Test + public void testDigestOutputRange() throws Exception { + byte[] input = inputBytes(); + byte[] expected = referenceDigest(input); + for (int available : new int[]{expected.length, expected.length + 5}) { + MessageDigest digest = newDigest(); + digest.update(input); + byte[] output = new byte[expected.length + 12]; + Arrays.fill(output, (byte) 0x5a); + byte[] expectedOutput = output.clone(); + System.arraycopy(expected, 0, expectedOutput, 3, expected.length); + assertEquals(expected.length, digest.digest(output, 3, available)); + assertArrayEquals(expectedOutput, output); + assertArrayEquals(referenceDigest(new byte[0]), digest.digest()); + } + } + + @Test + public void testShortDigestPreservesState() throws Exception { + byte[] input = inputBytes(); + for (int available : new int[]{0, 1, bits / 8 - 1}) { + for (int capacity : new int[]{available, bits / 8}) { + MessageDigest digest = newDigest(); + digest.update(input, 0, 17); + byte[] output = new byte[capacity]; + Arrays.fill(output, (byte) 0x5a); + byte[] original = output.clone(); + assertThrows(DigestException.class, () -> digest.digest(output, 0, available)); + assertArrayEquals(original, output); + digest.update(input, 17, input.length - 17); + assertArrayEquals(referenceDigest(input), digest.digest()); + } + } + } + + @Test + public void testInvalidDigestRangePreservesState() throws Exception { + byte[] input = inputBytes(); + int length = bits / 8; + int[][] ranges = { + {-1, length}, {0, -1}, {length + 1, 0}, {1, length}, + {Integer.MAX_VALUE, length}, {1, Integer.MAX_VALUE} + }; + for (int[] range : ranges) { + MessageDigest digest = newDigest(); + digest.update(input); + byte[] output = new byte[length]; + Arrays.fill(output, (byte) 0x5a); + byte[] original = output.clone(); + assertThrows(IllegalArgumentException.class, + () -> digest.digest(output, range[0], range[1])); + assertArrayEquals(original, output); + assertArrayEquals(referenceDigest(input), digest.digest()); + } + MessageDigest digest = newDigest(); + digest.update(input); + assertThrows(IllegalArgumentException.class, () -> digest.digest(null, 0, length)); + assertArrayEquals(referenceDigest(input), digest.digest()); + } + + @Test + public void testDigestInterface() throws Exception { + Digest digest = (Digest) newDigest(); + byte[] input = inputBytes(); + digest.update(input); + assertThrows(DigestException.class, () -> digest.digest(new byte[1], 0, 1)); + byte[] output = new byte[bits / 8]; + assertEquals(output.length, digest.digest(output, 0, output.length)); + assertArrayEquals(referenceDigest(input), output); + } + + @Test + public void testSpiMethods() throws Exception { + byte[] input = inputBytes(); + byte[] actual; + byte[] afterReset; + if (bits == 256) { + Keccak256 digest = (Keccak256) newDigest(); + digest.engineUpdate(input[0]); + digest.engineUpdate(input, 1, input.length - 1); + actual = digest.engineDigest(); + afterReset = digest.engineDigest(); + } else { + Keccak512 digest = (Keccak512) newDigest(); + digest.engineUpdate(input[0]); + digest.engineUpdate(input, 1, input.length - 1); + actual = digest.engineDigest(); + afterReset = digest.engineDigest(); + } + assertArrayEquals(referenceDigest(input), actual); + assertArrayEquals(referenceDigest(new byte[0]), afterReset); + } + + @Test + public void testCopy() throws Exception { + DigestEngine digest = (DigestEngine) newDigest(); + byte[] input = inputBytes(); + digest.update(ByteBuffer.wrap(input, 0, 151)); + Digest copy = digest.copy(); + digest.update(ByteBuffer.wrap(input, 151, input.length - 151)); + assertArrayEquals(referenceDigest(input), digest.digest()); + assertArrayEquals(referenceDigest(Arrays.copyOf(input, 151)), copy.digest()); + } + + private MessageDigest newDigest() throws Exception { + return MessageDigest.getInstance("TRON-KECCAK-" + bits, TronCastleProvider.getInstance()); + } + + private byte[] referenceDigest(byte[] input) { + KeccakDigest digest = new KeccakDigest(bits); + digest.update(input, 0, input.length); + byte[] output = new byte[bits / 8]; + digest.doFinal(output, 0); + return output; + } + + private byte[] inputBytes() { + byte[] input = new byte[8193]; + for (int i = 0; i < input.length; i++) { + input[i] = (byte) i; + } + return input; + } + + private void checkBuffer(ByteBuffer buffer) throws Exception { + byte[] input = new byte[buffer.remaining()]; + buffer.duplicate().get(input); + int limit = buffer.limit(); + MessageDigest digest = newDigest(); + digest.update(buffer); + assertEquals(limit, buffer.position()); + assertEquals(limit, buffer.limit()); + assertArrayEquals(referenceDigest(input), digest.digest()); + assertArrayEquals(referenceDigest(new byte[0]), digest.digest()); + } +} From 3f986f2d7a578c40d6111a1b2d8cd914ea6124af Mon Sep 17 00:00:00 2001 From: federico Date: Mon, 12 Oct 2026 15:06:36 +0800 Subject: [PATCH 7/8] refactor(crypto): move Keccak digest fixes to separate branch --- .../tron/common/crypto/cryptohash/Digest.java | 14 +- .../crypto/cryptohash/DigestEngine.java | 32 +-- .../common/crypto/cryptohash/Keccak256.java | 8 +- .../common/crypto/cryptohash/Keccak512.java | 4 +- .../common/crypto/cryptohash/KeccakTest.java | 229 ------------------ 5 files changed, 26 insertions(+), 261 deletions(-) delete mode 100644 crypto/src/test/java/org/tron/common/crypto/cryptohash/KeccakTest.java diff --git a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java index 312c3dd9ab9..62362d1d62d 100644 --- a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java +++ b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java @@ -18,8 +18,6 @@ package org.tron.common.crypto.cryptohash; -import java.security.DigestException; - /** * Copyright (c) 2007-2010 Project RNRT SAPHIR * @@ -84,17 +82,17 @@ public interface Digest { /** * Finalize the current hash computation and store the hash value in the provided output buffer. - * The {@code len} parameter contains the number of bytes available for the digest. The complete - * digest is written and the object is reset on success. Partial digests are not returned. + * The {@code len} parameter contains the maximum number of bytes that should be written; no more + * bytes than the natural hash function output length will be produced. If {@code len} is smaller + * than the natural hash output length, the hash output is truncated to its first {@code len} + * bytes. The object is resetted. * * @param outbuf the output buffer * @param off the output offset within {@code outbuf} - * @param len the available output length (in bytes) + * @param len the requested hash output length (in bytes) * @return the number of bytes actually written in {@code outbuf} - * @throws DigestException if {@code len} is smaller than the digest length - * @throws IllegalArgumentException if the output buffer is null or the output range is invalid */ - int digest(byte[] outbuf, int off, int len) throws DigestException; + int digest(byte[] outbuf, int off, int len); /** * Get the natural hash function output length (in bytes). diff --git a/crypto/src/main/java/org/tron/common/crypto/cryptohash/DigestEngine.java b/crypto/src/main/java/org/tron/common/crypto/cryptohash/DigestEngine.java index a27a58ec6a8..c7d013a0804 100644 --- a/crypto/src/main/java/org/tron/common/crypto/cryptohash/DigestEngine.java +++ b/crypto/src/main/java/org/tron/common/crypto/cryptohash/DigestEngine.java @@ -18,13 +18,12 @@ package org.tron.common.crypto.cryptohash; -import java.security.DigestException; import java.security.MessageDigest; public abstract class DigestEngine extends MessageDigest implements Digest { private int digestLen, blockLen, inputLen; - private byte[] inputBuf; + private byte[] inputBuf, outputBuf; private long blockCount; /** @@ -36,6 +35,7 @@ public DigestEngine(String alg) { digestLen = engineGetDigestLength(); blockLen = getInternalBlockLength(); inputBuf = new byte[blockLen]; + outputBuf = new byte[digestLen]; inputLen = 0; blockCount = 0; } @@ -72,6 +72,7 @@ public DigestEngine(String alg) { private void adjustDigestLen() { if (digestLen == 0) { digestLen = engineGetDigestLength(); + outputBuf = new byte[digestLen]; } } @@ -81,8 +82,7 @@ private void adjustDigestLen() { public byte[] digest() { adjustDigestLen(); byte[] result = new byte[digestLen]; - doPadding(result, 0); - reset(); + digest(result, 0, digestLen); return result; } @@ -97,20 +97,18 @@ public byte[] digest(byte[] input) { /** * @see Digest */ - public int digest(byte[] buf, int offset, int len) throws DigestException { - if (buf == null) { - throw new IllegalArgumentException("Output buffer is null"); - } - if (offset < 0 || len < 0 || offset > buf.length || len > buf.length - offset) { - throw new IllegalArgumentException("Invalid output buffer range"); - } + public int digest(byte[] buf, int offset, int len) { adjustDigestLen(); - if (len < digestLen) { - throw new DigestException("Partial digests are not returned"); + if (len >= digestLen) { + doPadding(buf, offset); + reset(); + return digestLen; + } else { + doPadding(outputBuf, 0); + System.arraycopy(outputBuf, 0, buf, offset, len); + reset(); + return len; } - doPadding(buf, offset); - reset(); - return digestLen; } /** @@ -224,6 +222,8 @@ protected Digest copyState(DigestEngine dest) { inputBuf.length); adjustDigestLen(); dest.adjustDigestLen(); + System.arraycopy(outputBuf, 0, dest.outputBuf, 0, + outputBuf.length); return dest; } } diff --git a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak256.java b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak256.java index 7ac282e84b5..83c0577c789 100644 --- a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak256.java +++ b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak256.java @@ -37,16 +37,14 @@ public int engineGetDigestLength() { @Override protected byte[] engineDigest() { - return digest(); + return null; } @Override - protected void engineUpdate(byte input) { - update(input); + protected void engineUpdate(byte arg0) { } @Override - protected void engineUpdate(byte[] input, int offset, int len) { - update(input, offset, len); + protected void engineUpdate(byte[] arg0, int arg1, int arg2) { } } diff --git a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak512.java b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak512.java index 3846012ec46..8283180370e 100644 --- a/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak512.java +++ b/crypto/src/main/java/org/tron/common/crypto/cryptohash/Keccak512.java @@ -37,16 +37,14 @@ public int engineGetDigestLength() { @Override protected byte[] engineDigest() { - return digest(); + return null; } @Override protected void engineUpdate(byte input) { - update(input); } @Override protected void engineUpdate(byte[] input, int offset, int len) { - update(input, offset, len); } } diff --git a/crypto/src/test/java/org/tron/common/crypto/cryptohash/KeccakTest.java b/crypto/src/test/java/org/tron/common/crypto/cryptohash/KeccakTest.java deleted file mode 100644 index ece07286d68..00000000000 --- a/crypto/src/test/java/org/tron/common/crypto/cryptohash/KeccakTest.java +++ /dev/null @@ -1,229 +0,0 @@ -package org.tron.common.crypto.cryptohash; - -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertThrows; - -import java.nio.ByteBuffer; -import java.security.DigestException; -import java.security.MessageDigest; -import java.util.Arrays; -import java.util.Collection; -import org.bouncycastle.crypto.digests.KeccakDigest; -import org.junit.Test; -import org.junit.runner.RunWith; -import org.junit.runners.Parameterized; -import org.tron.common.crypto.jce.TronCastleProvider; - -@RunWith(Parameterized.class) -public class KeccakTest { - - private final int bits; - - public KeccakTest(int bits) { - this.bits = bits; - } - - @Parameterized.Parameters(name = "Keccak{0}") - public static Collection parameters() { - return Arrays.asList(new Object[][]{{256}, {512}}); - } - - @Test - public void testByteArrayDigest() throws Exception { - byte[] input = inputBytes(); - assertArrayEquals(referenceDigest(input), newDigest().digest(input)); - } - - @Test - public void testHeapByteBuffer() throws Exception { - checkBuffer(ByteBuffer.wrap(inputBytes())); - } - - @Test - public void testDirectByteBuffer() throws Exception { - ByteBuffer buffer = ByteBuffer.allocateDirect(inputBytes().length); - buffer.put(inputBytes()); - buffer.flip(); - checkBuffer(buffer); - } - - @Test - public void testReadOnlyByteBuffer() throws Exception { - checkBuffer(ByteBuffer.wrap(inputBytes()).asReadOnlyBuffer()); - } - - @Test - public void testByteBufferSlice() throws Exception { - byte[] input = inputBytes(); - ByteBuffer buffer = ByteBuffer.allocate(input.length + 10); - buffer.position(5); - buffer.put(input); - buffer.limit(5 + input.length); - buffer.position(5); - checkBuffer(buffer.slice()); - } - - @Test - public void testByteBufferWindow() throws Exception { - ByteBuffer buffer = ByteBuffer.wrap(inputBytes()); - buffer.position(7); - buffer.limit(buffer.limit() - 11); - checkBuffer(buffer); - } - - @Test - public void testEmptyByteBuffer() throws Exception { - ByteBuffer buffer = ByteBuffer.wrap(inputBytes()); - buffer.position(buffer.limit()); - checkBuffer(buffer); - } - - @Test - public void testMixedInput() throws Exception { - byte[] input = inputBytes(); - MessageDigest digest = newDigest(); - digest.update(input[0]); - digest.update(input, 1, 16); - ByteBuffer buffer = ByteBuffer.wrap(input, 17, input.length - 17); - digest.update(buffer); - assertEquals(buffer.limit(), buffer.position()); - assertArrayEquals(referenceDigest(input), digest.digest()); - } - - @Test - public void testDigestOutputRange() throws Exception { - byte[] input = inputBytes(); - byte[] expected = referenceDigest(input); - for (int available : new int[]{expected.length, expected.length + 5}) { - MessageDigest digest = newDigest(); - digest.update(input); - byte[] output = new byte[expected.length + 12]; - Arrays.fill(output, (byte) 0x5a); - byte[] expectedOutput = output.clone(); - System.arraycopy(expected, 0, expectedOutput, 3, expected.length); - assertEquals(expected.length, digest.digest(output, 3, available)); - assertArrayEquals(expectedOutput, output); - assertArrayEquals(referenceDigest(new byte[0]), digest.digest()); - } - } - - @Test - public void testShortDigestPreservesState() throws Exception { - byte[] input = inputBytes(); - for (int available : new int[]{0, 1, bits / 8 - 1}) { - for (int capacity : new int[]{available, bits / 8}) { - MessageDigest digest = newDigest(); - digest.update(input, 0, 17); - byte[] output = new byte[capacity]; - Arrays.fill(output, (byte) 0x5a); - byte[] original = output.clone(); - assertThrows(DigestException.class, () -> digest.digest(output, 0, available)); - assertArrayEquals(original, output); - digest.update(input, 17, input.length - 17); - assertArrayEquals(referenceDigest(input), digest.digest()); - } - } - } - - @Test - public void testInvalidDigestRangePreservesState() throws Exception { - byte[] input = inputBytes(); - int length = bits / 8; - int[][] ranges = { - {-1, length}, {0, -1}, {length + 1, 0}, {1, length}, - {Integer.MAX_VALUE, length}, {1, Integer.MAX_VALUE} - }; - for (int[] range : ranges) { - MessageDigest digest = newDigest(); - digest.update(input); - byte[] output = new byte[length]; - Arrays.fill(output, (byte) 0x5a); - byte[] original = output.clone(); - assertThrows(IllegalArgumentException.class, - () -> digest.digest(output, range[0], range[1])); - assertArrayEquals(original, output); - assertArrayEquals(referenceDigest(input), digest.digest()); - } - MessageDigest digest = newDigest(); - digest.update(input); - assertThrows(IllegalArgumentException.class, () -> digest.digest(null, 0, length)); - assertArrayEquals(referenceDigest(input), digest.digest()); - } - - @Test - public void testDigestInterface() throws Exception { - Digest digest = (Digest) newDigest(); - byte[] input = inputBytes(); - digest.update(input); - assertThrows(DigestException.class, () -> digest.digest(new byte[1], 0, 1)); - byte[] output = new byte[bits / 8]; - assertEquals(output.length, digest.digest(output, 0, output.length)); - assertArrayEquals(referenceDigest(input), output); - } - - @Test - public void testSpiMethods() throws Exception { - byte[] input = inputBytes(); - byte[] actual; - byte[] afterReset; - if (bits == 256) { - Keccak256 digest = (Keccak256) newDigest(); - digest.engineUpdate(input[0]); - digest.engineUpdate(input, 1, input.length - 1); - actual = digest.engineDigest(); - afterReset = digest.engineDigest(); - } else { - Keccak512 digest = (Keccak512) newDigest(); - digest.engineUpdate(input[0]); - digest.engineUpdate(input, 1, input.length - 1); - actual = digest.engineDigest(); - afterReset = digest.engineDigest(); - } - assertArrayEquals(referenceDigest(input), actual); - assertArrayEquals(referenceDigest(new byte[0]), afterReset); - } - - @Test - public void testCopy() throws Exception { - DigestEngine digest = (DigestEngine) newDigest(); - byte[] input = inputBytes(); - digest.update(ByteBuffer.wrap(input, 0, 151)); - Digest copy = digest.copy(); - digest.update(ByteBuffer.wrap(input, 151, input.length - 151)); - assertArrayEquals(referenceDigest(input), digest.digest()); - assertArrayEquals(referenceDigest(Arrays.copyOf(input, 151)), copy.digest()); - } - - private MessageDigest newDigest() throws Exception { - return MessageDigest.getInstance("TRON-KECCAK-" + bits, TronCastleProvider.getInstance()); - } - - private byte[] referenceDigest(byte[] input) { - KeccakDigest digest = new KeccakDigest(bits); - digest.update(input, 0, input.length); - byte[] output = new byte[bits / 8]; - digest.doFinal(output, 0); - return output; - } - - private byte[] inputBytes() { - byte[] input = new byte[8193]; - for (int i = 0; i < input.length; i++) { - input[i] = (byte) i; - } - return input; - } - - private void checkBuffer(ByteBuffer buffer) throws Exception { - byte[] input = new byte[buffer.remaining()]; - buffer.duplicate().get(input); - int limit = buffer.limit(); - MessageDigest digest = newDigest(); - digest.update(buffer); - assertEquals(limit, buffer.position()); - assertEquals(limit, buffer.limit()); - assertArrayEquals(referenceDigest(input), digest.digest()); - assertArrayEquals(referenceDigest(new byte[0]), digest.digest()); - } -} From c7f0fea0c1cecb95dda38d2bd2cf68b77746a234 Mon Sep 17 00:00:00 2001 From: federico Date: Mon, 12 Oct 2026 15:22:17 +0800 Subject: [PATCH 8/8] fix(crypto): clear temporary keystore keys --- .../main/java/org/tron/keystore/Wallet.java | 24 ++++++---- .../keystore/WalletAddressValidationTest.java | 46 +++++++++++++++++-- 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/crypto/src/main/java/org/tron/keystore/Wallet.java b/crypto/src/main/java/org/tron/keystore/Wallet.java index 12c7a58a22d..d0051764490 100644 --- a/crypto/src/main/java/org/tron/keystore/Wallet.java +++ b/crypto/src/main/java/org/tron/keystore/Wallet.java @@ -207,16 +207,18 @@ public static SignInterface decrypt(String password, WalletFile walletFile) throw new CipherException("Unable to deserialize params: " + crypto.getKdf()); } - byte[] derivedMac = generateMac(derivedKey, cipherText); + byte[] encryptKey = null; + byte[] privateKey = null; + try { + byte[] derivedMac = generateMac(derivedKey, cipherText); - if (!java.security.MessageDigest.isEqual(derivedMac, mac)) { - throw new CipherException("Invalid password provided"); - } + if (!java.security.MessageDigest.isEqual(derivedMac, mac)) { + throw new CipherException("Invalid password provided"); + } - byte[] encryptKey = Arrays.copyOfRange(derivedKey, 0, 16); - byte[] privateKey = performCipherOperation(Cipher.DECRYPT_MODE, iv, encryptKey, cipherText); + encryptKey = Arrays.copyOfRange(derivedKey, 0, 16); + privateKey = performCipherOperation(Cipher.DECRYPT_MODE, iv, encryptKey, cipherText); - try { SignInterface keyPair; try { keyPair = SignUtils.fromPrivate(privateKey); @@ -239,7 +241,13 @@ public static SignInterface decrypt(String password, WalletFile walletFile) return keyPair; } finally { - Arrays.fill(privateKey, (byte) 0); + Arrays.fill(derivedKey, (byte) 0); + if (encryptKey != null) { + Arrays.fill(encryptKey, (byte) 0); + } + if (privateKey != null) { + Arrays.fill(privateKey, (byte) 0); + } } } diff --git a/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java b/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java index c33a22e0bba..1135ac65c19 100644 --- a/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java +++ b/framework/src/test/java/org/tron/keystore/WalletAddressValidationTest.java @@ -1,20 +1,24 @@ package org.tron.keystore; +import static java.nio.charset.StandardCharsets.UTF_8; + import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertSame; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.Mockito.mockStatic; import java.util.concurrent.atomic.AtomicReference; +import org.bouncycastle.crypto.generators.SCrypt; import org.junit.Test; import org.mockito.MockedStatic; import org.tron.common.crypto.SignInterface; import org.tron.common.crypto.SignUtils; +import org.tron.common.utils.ByteArray; import org.tron.common.utils.Utils; import org.tron.core.exception.CipherException; @@ -83,25 +87,32 @@ public void testDecryptAllowsEmptyAddress() throws Exception { } @Test - public void testDecryptClearsPrivateKey() throws Exception { + public void testDecryptClearsTemporaryKeys() throws Exception { String password = "test123456"; SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom()); WalletFile walletFile = Wallet.createLight(password, keyPair); byte[] expectedPrivateKey = keyPair.getPrivateKey(); + byte[] derivedKey = deriveKey(password, walletFile); AtomicReference decryptedPrivateKey = new AtomicReference<>(); - try (MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { + try (MockedStatic mockedScrypt = mockStatic(SCrypt.class); + MockedStatic mockedSignUtils = mockStatic(SignUtils.class)) { + mockedScrypt.when(() -> SCrypt.generate(any(byte[].class), any(byte[].class), + anyInt(), anyInt(), anyInt(), anyInt())).thenReturn(derivedKey); mockedSignUtils.when(() -> SignUtils.fromPrivate(any(byte[].class))) .thenAnswer(invocation -> { byte[] privateKey = invocation.getArgument(0); assertArrayEquals(expectedPrivateKey, privateKey); decryptedPrivateKey.set(privateKey); - return keyPair; + return invocation.callRealMethod(); }); - assertSame(keyPair, Wallet.decrypt(password, walletFile)); + SignInterface recovered = Wallet.decrypt(password, walletFile); + assertArrayEquals(expectedPrivateKey, recovered.getPrivateKey()); + assertArrayEquals(keyPair.getAddress(), recovered.getAddress()); } + assertArrayEquals(new byte[derivedKey.length], derivedKey); byte[] clearedPrivateKey = decryptedPrivateKey.get(); assertNotNull(clearedPrivateKey); assertArrayEquals(new byte[clearedPrivateKey.length], clearedPrivateKey); @@ -134,4 +145,29 @@ public void testDecryptWrapsInvalidPrivateKey() throws Exception { assertNotNull(clearedPrivateKey); assertArrayEquals(new byte[clearedPrivateKey.length], clearedPrivateKey); } + + @Test + public void testDecryptClearsDerivedKeyOnIncorrectPassword() throws Exception { + String password = "test123456"; + SignInterface keyPair = SignUtils.getGeneratedRandomSign(Utils.getRandom()); + WalletFile walletFile = Wallet.createLight(password, keyPair); + byte[] derivedKey = deriveKey("wrong", walletFile); + + try (MockedStatic mockedScrypt = mockStatic(SCrypt.class)) { + mockedScrypt.when(() -> SCrypt.generate(any(byte[].class), any(byte[].class), + anyInt(), anyInt(), anyInt(), anyInt())).thenReturn(derivedKey); + CipherException exception = assertThrows(CipherException.class, + () -> Wallet.decrypt("wrong", walletFile)); + assertEquals("Invalid password provided", exception.getMessage()); + } + + assertArrayEquals(new byte[derivedKey.length], derivedKey); + } + + private static byte[] deriveKey(String password, WalletFile walletFile) { + WalletFile.ScryptKdfParams params = + (WalletFile.ScryptKdfParams) walletFile.getCrypto().getKdfparams(); + return SCrypt.generate(password.getBytes(UTF_8), ByteArray.fromHexString(params.getSalt()), + params.getN(), params.getR(), params.getP(), params.getDklen()); + } }