Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
467 changes: 184 additions & 283 deletions crypto/src/main/java/org/tron/common/crypto/ECKey.java

Large diffs are not rendered by default.

4 changes: 4 additions & 0 deletions crypto/src/main/java/org/tron/common/crypto/Rsv.java
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ public class Rsv {


public static Rsv fromSignature(byte[] sign) {
if (sign == null || sign.length < 65) {
throw new IllegalArgumentException(
"Invalid signature length: " + (sign == null ? "null" : sign.length));
}
byte[] r = Arrays.copyOfRange(sign, 0, 32);
byte[] s = Arrays.copyOfRange(sign, 32, 64);
byte v = sign[64];
Expand Down
7 changes: 7 additions & 0 deletions crypto/src/main/java/org/tron/common/crypto/SignUtils.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ public static SignInterface fromPrivate(byte[] privKeyBytes, boolean isECKeyCryp
return SM2.fromPrivate(privKeyBytes);
}

public static boolean isValidPrivateKey(byte[] privKeyBytes, boolean isECKeyCryptoEngine) {
if (isECKeyCryptoEngine) {
return ECKey.isValidPrivateKey(privKeyBytes);
}
return SM2.isValidPrivateKey(privKeyBytes);
}

public static byte[] signatureToAddress(
byte[] messageHash, String signatureBase64, boolean isECKeyCryptoEngine)
throws SignatureException {
Expand Down
14 changes: 14 additions & 0 deletions crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,20 @@ public static SM2 fromPrivate(byte[] privKeyBytes) {
return fromPrivate(new BigInteger(1, privKeyBytes));
}

public static boolean isValidPrivateKey(byte[] keyBytes) {
if (ByteArray.isEmpty(keyBytes)) {
return false;
}
// Accept a 33-byte array only when the leading byte is 0x00 (BigInteger sign-byte padding);
// reject anything longer or any non-canonical 33-byte encoding.
if (keyBytes.length > 33 || (keyBytes.length == 33 && keyBytes[0] != 0x00)) {
return false;
}

BigInteger key = new BigInteger(1, keyBytes);
Comment thread
Federico2014 marked this conversation as resolved.
return key.compareTo(BigInteger.ONE) >= 0 && key.compareTo(SM2_N) < 0;
}

/**
* Creates an SM2 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@
import java.io.File;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;
import org.tron.common.crypto.SignInterface;
import org.tron.common.crypto.SignUtils;
import org.tron.common.utils.ByteArray;
import org.tron.common.utils.Commons;
import org.tron.common.utils.LocalWitnesses;
Expand Down Expand Up @@ -78,14 +80,27 @@ public static LocalWitnesses initFromKeystore(
}

List<String> privateKeys = new ArrayList<>();
byte[] privKeyBytes = null;
try {
Credentials credentials = WalletUtils.loadCredentials(pwd, new File(fileName));
SignInterface sign = credentials.getSignInterface();
String prikey = ByteArray.toHexString(sign.getPrivateKey());
privateKeys.add(prikey);
privKeyBytes = sign.getPrivateKey();
if (privKeyBytes == null
|| !SignUtils.isValidPrivateKey(privKeyBytes, Args.getInstance().isECKeyCryptoEngine())) {
throw new TronError(
"Keystore contains an invalid private key",
TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
}
privateKeys.add(ByteArray.toHexString(privKeyBytes));
} catch (IOException | CipherException e) {
logger.error("Witness node start failed!");
throw new TronError(e, TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
} finally {
if (privKeyBytes != null) {
// Zeroize the raw key bytes. Note: the hex String already added to
// privateKeys is immutable and cannot be zeroed — a known JVM limitation.
Arrays.fill(privKeyBytes, (byte) 0);
}
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SHOULD] Arrays.fill(privKeyBytes, (byte) 0) clears the source byte array, but ByteArray.toHexString(privKeyBytes) already created an immutable String at line 94 before the finally block runs. The hex representation of the private key remains in the JVM heap until GC and cannot be zeroed. The zeroization intent is correct but doesn't cover the sensitive string. This is a known JVM limitation; consider documenting it explicitly, or avoid materialising the hex string until after the key is loaded into LocalWitnesses.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: added a comment above the Arrays.fill call noting that the hex String already pushed to privateKeys is immutable and cannot be zeroed — a known JVM limitation. This documents the residual risk without overstating the protection.

}

LocalWitnesses witnesses = new LocalWitnesses();
Expand Down
55 changes: 47 additions & 8 deletions framework/src/test/java/org/tron/common/crypto/ECKeyTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
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;
Expand All @@ -19,12 +19,12 @@
import org.bouncycastle.util.encoders.Hex;
import org.junit.Test;
import org.tron.common.crypto.ECKey.ECDSASignature;
import org.tron.common.utils.ByteUtil;
import org.tron.core.Wallet;

/**
* The reason the test case uses the private key plaintext is to ensure that,
* after the ECkey tool or algorithm is upgraded,
* the upgraded differences can be verified.
* The reason the test case uses the private key plaintext is to ensure that, after the ECkey tool
* or algorithm is upgraded, the upgraded differences can be verified.
*/
@Slf4j
public class ECKeyTest {
Expand Down Expand Up @@ -69,10 +69,49 @@ 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 testFromPrivateKeyRejectsZeroValue() {
byte[] zeroPrivateKey = new byte[32];

IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
() -> ECKey.fromPrivate(zeroPrivateKey));
assertEquals("Invalid private key.", exception.getMessage());
}

@Test
public void testFromPrivateKeyRejectsCurveOrder() {
byte[] curveOrder = ByteUtil.bigIntegerToBytes(ECKey.CURVE.getN(), 32);

IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
() -> ECKey.fromPrivate(curveOrder));
assertEquals("Invalid private key.", exception.getMessage());
}

@Test
public void testInvalidPublicKeyEncoding() {
byte[] invalidPublicKey = new byte[33];
Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[NIT] testInvalidPublicKeyEncoding exercises the constructor path but uses a zero-filled compressed-point encoding — this hits decodePoint failure, not isInfinity/isValid. Consider adding a test that passes a validly-encoded but off-curve or infinity point directly to isValidPublicKey(). Also, SM2.isValidPrivateKey() has no test coverage at all in this PR.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: added testIsValidPublicKey to ECKeyTest (null, empty, invalid curve point, valid uncompressed, valid compressed) and testSM2IsValidPrivateKey to SM2KeyTest (null, empty, zero key, curve-order key, oversized key, minimum valid key).

invalidPublicKey[0] = 0x02;

IllegalArgumentException exception = assertThrows(IllegalArgumentException.class,
() -> new ECKey(invalidPublicKey, false));
assertEquals("Invalid public key.", exception.getMessage());
}

@Test
public void testIsValidPublicKey() {
assertFalse(ECKey.isValidPublicKey(null));
assertFalse(ECKey.isValidPublicKey(new byte[0]));
// 0x02 prefix + zero x-coordinate: valid compressed format but no point on secp256k1
byte[] invalidPoint = new byte[33];
invalidPoint[0] = 0x02;
assertFalse(ECKey.isValidPublicKey(invalidPoint));
// valid uncompressed and compressed public keys
assertTrue(ECKey.isValidPublicKey(pubKey));
assertTrue(ECKey.isValidPublicKey(compressedPubKey));
}

@Test(expected = IllegalArgumentException.class)
Expand Down
18 changes: 18 additions & 0 deletions framework/src/test/java/org/tron/common/crypto/SM2KeyTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.junit.Test;
import org.tron.common.crypto.sm2.SM2;
import org.tron.common.crypto.sm2.SM2Signer;
import org.tron.common.utils.ByteUtil;
import org.tron.core.Wallet;

/**
Expand Down Expand Up @@ -281,4 +282,21 @@ public void testSM3() {
assertEquals("b524f552cd82b8b028476e005c377fb19a87e6fc682d48bb5d42e3d9b9effe76",
Hex.toHexString(eHash));
}

@Test
public void testSM2IsValidPrivateKey() {
assertFalse(SM2.isValidPrivateKey(null));
assertFalse(SM2.isValidPrivateKey(new byte[0]));
assertFalse(SM2.isValidPrivateKey(new byte[32])); // all-zero = 0, below minimum
// SM2 curve order n — key must be strictly less than n
BigInteger sm2N = new BigInteger(
"FFFFFFFEFFFFFFFFFFFFFFFFFFFFFFFF7203DF6B21C6052B53BBF40939D54123", 16);
assertFalse(SM2.isValidPrivateKey(ByteUtil.bigIntegerToBytes(sm2N, 32)));
// oversized input (34 bytes)
assertFalse(SM2.isValidPrivateKey(new byte[34]));
// minimum valid value = 1
byte[] minKey = new byte[32];
minKey[31] = 1;
assertTrue(SM2.isValidPrivateKey(minKey));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public void testInitFromKeystore() {
mockedByteArray.when(() -> ByteArray.toHexString(any()))
.thenReturn(privateKey);
mockedByteArray.when(() -> ByteArray.fromHexString(anyString()))
.thenReturn(keyBytes);
.thenAnswer(inv -> Hex.decode(privateKey));

LocalWitnesses result = WitnessInitializer.initFromKeystore(
keystores, "password", null);
Expand Down
Loading