feat(crypto): harden ECKey validation - #46
Open
Federico2014 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughECKey now uses Tron Bouncy Castle types, validates private scalars and public points, and returns defensive cache copies. Witness keystore loading validates EC private keys and clears key bytes after processing. Tests cover invalid keys, key pairing, equality, caching, and cleanup. ChangesCrypto and witness key validation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
framework/src/test/java/org/tron/common/crypto/ECKeyTest.java (1)
78-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the oversized private-key case so it isolates the length rule.
new byte[33]is rejected by two independent rules: the 32-byte length limit and the zero-scalar check. The assertion therefore does not prove that the length limit works. Add a 33-byte array that holds a valid nonzero scalar. This case also documents the sign-paddedBigInteger.toByteArray()encoding thatisValidPrivateKeynow rejects.♻️ Proposed additional assertion
assertFalse(ECKey.isValidPrivateKey(new byte[33])); + // 33-byte sign-padded encoding of a valid scalar is rejected by the length rule. + byte[] signPadded = new byte[33]; + System.arraycopy(Hex.decode(privString), 0, signPadded, 1, 32); + assertFalse(ECKey.isValidPrivateKey(signPadded)); assertFalse(ECKey.isValidPrivateKey(BigInteger.ZERO));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/test/java/org/tron/common/crypto/ECKeyTest.java` around lines 78 - 96, Update shouldValidatePrivateKeyRange so the oversized byte-array assertion uses a 33-byte value containing a valid nonzero scalar, such as the sign-padded encoding of the existing valid private key, isolating rejection by length rather than the zero-scalar check. Preserve the current null, empty, boundary, and fromPrivate assertions.framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java (1)
124-125: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse a canonical 32-byte encoding for the curve-order boundary.
ECKey.isValidPrivateKey(...bytes...)checksprivateKey.length <= 32before converting bytes withnew BigInteger(1, privateKey)and checking< N.ECKey.CURVE.getN().toByteArray()is 33 bytes because the secp256k1 order starts withff, so this test covers oversized input rather than the scalar-boundary check.Use
ByteArray.fromHexString(ECKey.CURVE.getN().toString(16))so the invalid scalar has the canonical 32-byte magnitude.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java` around lines 124 - 125, Update the invalidKey initialization in WitnessInitializerTest to use ByteArray.fromHexString(ECKey.CURVE.getN().toString(16)), ensuring the curve-order boundary is represented as a canonical 32-byte value and exercises the scalar-boundary validation rather than the oversized-input check.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@framework/src/test/java/org/tron/common/crypto/ECKeyTest.java`:
- Around line 78-96: Update shouldValidatePrivateKeyRange so the oversized
byte-array assertion uses a 33-byte value containing a valid nonzero scalar,
such as the sign-padded encoding of the existing valid private key, isolating
rejection by length rather than the zero-scalar check. Preserve the current
null, empty, boundary, and fromPrivate assertions.
In
`@framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java`:
- Around line 124-125: Update the invalidKey initialization in
WitnessInitializerTest to use
ByteArray.fromHexString(ECKey.CURVE.getN().toString(16)), ensuring the
curve-order boundary is represented as a canonical 32-byte value and exercises
the scalar-boundary validation rather than the oversized-input check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a2fe83d0-29a8-4bd2-b2e2-2a5f81219a67
📒 Files selected for processing (4)
crypto/src/main/java/org/tron/common/crypto/ECKey.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/test/java/org/tron/common/crypto/ECKeyTest.javaframework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crypto/src/main/java/org/tron/common/crypto/ECKey.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/ECKey.java:220">
P2: Private keys serialized with `BigInteger.toByteArray()` gain a leading `0x00` when their high bit is set, so valid keys now fail `fromPrivate` and keystore validation. Allow exactly one zero sign-padding byte while retaining the scalar-range check.
(Based on your team's feedback about accepting BigInteger sign-padded private-key arrays.)</violation>
</file>
<file name="framework/src/test/java/org/tron/common/crypto/ECKeyTest.java">
<violation number="1" location="framework/src/test/java/org/tron/common/crypto/ECKeyTest.java:84">
P2: Java's `BigInteger.toByteArray()` can encode a valid secp256k1 scalar in 33 bytes with a leading `0x00`, but this case only tests scalar zero and lets rejection of valid sign-padded private keys go unnoticed. Add a valid sign-padded 33-byte scalar that must be accepted, with separate cases for nonzero-leading and oversized arrays.
(Based on your team's feedback about preserving Java sign-padded private-key bytes.)</violation>
<violation number="2" location="framework/src/test/java/org/tron/common/crypto/ECKeyTest.java:286">
P3: This test can pass even when `getAddress()` or `getNodeId()` exposes the mutable cache, so it does not verify the defensive-copy contract it claims to test. Snapshot the expected values with `Arrays.copyOf` before mutating the returned arrays.</violation>
</file>
<file name="framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java">
<violation number="1" location="framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java:88">
P2: Malformed EC keystores fail before this validation runs and escape as `IllegalArgumentException`, rather than the expected `TronError(WITNESS_KEYSTORE_LOAD)`. Wrap this failure at the keystore boundary (or convert it in `Wallet.decrypt`) so witness startup reports the documented load error.</violation>
<violation number="2" location="framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java:115">
P3: The cleanup wipes a re-encoded copy, not the decrypted key buffer allocated by `Wallet.decrypt`, leaving an unnecessary plaintext private key in memory. Clear that source buffer in `Wallet.decrypt` after key construction, including failure paths.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| */ | ||
| public static boolean isValidPrivateKey(byte[] privateKey) { | ||
| return !ByteArray.isEmpty(privateKey) | ||
| && privateKey.length <= MAX_PRIVATE_KEY_LENGTH |
There was a problem hiding this comment.
P2: Private keys serialized with BigInteger.toByteArray() gain a leading 0x00 when their high bit is set, so valid keys now fail fromPrivate and keystore validation. Allow exactly one zero sign-padding byte while retaining the scalar-range check.
(Based on your team's feedback about accepting BigInteger sign-padded private-key arrays.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crypto/src/main/java/org/tron/common/crypto/ECKey.java, line 220:
<comment>Private keys serialized with `BigInteger.toByteArray()` gain a leading `0x00` when their high bit is set, so valid keys now fail `fromPrivate` and keystore validation. Allow exactly one zero sign-padding byte while retaining the scalar-range check.
(Based on your team's feedback about accepting BigInteger sign-padded private-key arrays.) </comment>
<file context>
@@ -245,6 +212,51 @@ private static PrivateKey privateKeyFromBigInteger(BigInteger priv) {
+ */
+ public static boolean isValidPrivateKey(byte[] privateKey) {
+ return !ByteArray.isEmpty(privateKey)
+ && privateKey.length <= MAX_PRIVATE_KEY_LENGTH
+ && isValidPrivateKey(new BigInteger(1, privateKey));
+ }
</file context>
Suggested change
| && privateKey.length <= MAX_PRIVATE_KEY_LENGTH | |
| && (privateKey.length <= MAX_PRIVATE_KEY_LENGTH | |
| || (privateKey.length == MAX_PRIVATE_KEY_LENGTH + 1 && privateKey[0] == 0)) |
| assertFalse(ECKey.isValidPrivateKey((BigInteger) null)); | ||
| assertFalse(ECKey.isValidPrivateKey((byte[]) null)); | ||
| assertFalse(ECKey.isValidPrivateKey(new byte[0])); | ||
| assertFalse(ECKey.isValidPrivateKey(new byte[33])); |
There was a problem hiding this comment.
P2: Java's BigInteger.toByteArray() can encode a valid secp256k1 scalar in 33 bytes with a leading 0x00, but this case only tests scalar zero and lets rejection of valid sign-padded private keys go unnoticed. Add a valid sign-padded 33-byte scalar that must be accepted, with separate cases for nonzero-leading and oversized arrays.
(Based on your team's feedback about preserving Java sign-padded private-key bytes.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/common/crypto/ECKeyTest.java, line 84:
<comment>Java's `BigInteger.toByteArray()` can encode a valid secp256k1 scalar in 33 bytes with a leading `0x00`, but this case only tests scalar zero and lets rejection of valid sign-padded private keys go unnoticed. Add a valid sign-padded 33-byte scalar that must be accepted, with separate cases for nonzero-leading and oversized arrays.
(Based on your team's feedback about preserving Java sign-padded private-key bytes.) </comment>
<file context>
@@ -69,10 +68,55 @@ public void testFromPrivateKey() {
+ 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()));
</file context>
| SignInterface sign = credentials.getSignInterface(); | ||
| String prikey = ByteArray.toHexString(sign.getPrivateKey()); | ||
| privateKeys.add(prikey); | ||
| privateKeyBytes = sign.getPrivateKey(); |
There was a problem hiding this comment.
P2: Malformed EC keystores fail before this validation runs and escape as IllegalArgumentException, rather than the expected TronError(WITNESS_KEYSTORE_LOAD). Wrap this failure at the keystore boundary (or convert it in Wallet.decrypt) so witness startup reports the documented load error.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java, line 88:
<comment>Malformed EC keystores fail before this validation runs and escape as `IllegalArgumentException`, rather than the expected `TronError(WITNESS_KEYSTORE_LOAD)`. Wrap this failure at the keystore boundary (or convert it in `Wallet.decrypt`) so witness startup reports the documented load error.</comment>
<file context>
@@ -78,12 +80,19 @@ public static LocalWitnesses initFromKeystore(
SignInterface sign = credentials.getSignInterface();
- String prikey = ByteArray.toHexString(sign.getPrivateKey());
- privateKeys.add(prikey);
+ privateKeyBytes = sign.getPrivateKey();
+ if (Args.getInstance().isECKeyCryptoEngine()
+ && !ECKey.isValidPrivateKey(privateKeyBytes)) {
</file context>
Comment on lines
+286
to
+294
| byte[] expectedAddress = key.getAddress(); | ||
| byte[] returnedAddress = key.getAddress(); | ||
| returnedAddress[0] ^= 1; | ||
| assertArrayEquals(expectedAddress, key.getAddress()); | ||
|
|
||
| byte[] expectedNodeId = key.getNodeId(); | ||
| byte[] returnedNodeId = key.getNodeId(); | ||
| returnedNodeId[0] ^= 1; | ||
| assertArrayEquals(expectedNodeId, key.getNodeId()); |
There was a problem hiding this comment.
P3: This test can pass even when getAddress() or getNodeId() exposes the mutable cache, so it does not verify the defensive-copy contract it claims to test. Snapshot the expected values with Arrays.copyOf before mutating the returned arrays.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/test/java/org/tron/common/crypto/ECKeyTest.java, line 286:
<comment>This test can pass even when `getAddress()` or `getNodeId()` exposes the mutable cache, so it does not verify the defensive-copy contract it claims to test. Snapshot the expected values with `Arrays.copyOf` before mutating the returned arrays.</comment>
<file context>
@@ -215,10 +270,28 @@ public void testEqualsObject() {
+ @Test
+ public void shouldReturnDefensiveCopiesOfCachedValues() {
+ ECKey key = ECKey.fromPrivate(privateKey);
+ byte[] expectedAddress = key.getAddress();
+ byte[] returnedAddress = key.getAddress();
+ returnedAddress[0] ^= 1;
</file context>
Suggested change
| byte[] expectedAddress = key.getAddress(); | |
| byte[] returnedAddress = key.getAddress(); | |
| returnedAddress[0] ^= 1; | |
| assertArrayEquals(expectedAddress, key.getAddress()); | |
| byte[] expectedNodeId = key.getNodeId(); | |
| byte[] returnedNodeId = key.getNodeId(); | |
| returnedNodeId[0] ^= 1; | |
| assertArrayEquals(expectedNodeId, key.getNodeId()); | |
| 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()); |
| throw new TronError(e, TronError.ErrCode.WITNESS_KEYSTORE_LOAD); | ||
| } finally { | ||
| if (privateKeyBytes != null) { | ||
| Arrays.fill(privateKeyBytes, (byte) 0); |
There was a problem hiding this comment.
P3: The cleanup wipes a re-encoded copy, not the decrypted key buffer allocated by Wallet.decrypt, leaving an unnecessary plaintext private key in memory. Clear that source buffer in Wallet.decrypt after key construction, including failure paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java, line 115:
<comment>The cleanup wipes a re-encoded copy, not the decrypted key buffer allocated by `Wallet.decrypt`, leaving an unnecessary plaintext private key in memory. Clear that source buffer in `Wallet.decrypt` after key construction, including failure paths.</comment>
<file context>
@@ -101,6 +110,10 @@ public static LocalWitnesses initFromKeystore(
throw new TronError(e, TronError.ErrCode.WITNESS_KEYSTORE_LOAD);
+ } finally {
+ if (privateKeyBytes != null) {
+ Arrays.fill(privateKeyBytes, (byte) 0);
+ }
}
</file context>
Federico2014
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR strengthens ECKey input validation, state consistency, and private-key handling.
Why are these changes required?
ECKey previously accepted malformed or inconsistent key material, including unbounded private-key byte arrays and mismatched private/public key pairs. It also exposed mutable cached arrays and advertised configurable provider support that the signing implementation did not actually honor. These changes make validation predictable, reduce unnecessary resource consumption from invalid input, and keep key handling consistent with the Bouncy Castle signing implementation.
This PR has been tested by:
ECKeyTest,SignatureInterfaceTest,BouncyCastleTest, andWitnessInitializerTest./gradlew compileJava :framework:checkstyleTestFollow up
None.
Extra details
This change removes the public constructors that accept an arbitrary
Provider, removesfromPrivateAndPrecalculatedPublicandtoStringWithPrivate, and changes null or empty private-key input from returningnullto throwingIllegalArgumentException. No SM2, signature verification, signature recovery, or consensus behavior is changed.Summary by cubic
Hardens
ECKeyvalidation and key handling to reject malformed keys and prevent misuse. Also restricts signing to the Bouncy Castle provider and validates witness keystore keys.Bug Fixes
WitnessInitializer.Migration
ECKey.fromPrivate(...)now throwsIllegalArgumentExceptionfor null, empty, or out-of-range input (instead of returning null).Provider; key gen and signing use Bouncy Castle only.fromPrivateAndPrecalculatedPublic(...)andtoStringWithPrivate().Written for commit 0a22ffd. Summary will update on new commits.