feat(crypto): harden ECKey validation - #46
Open
Federico2014 wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthrough
ChangesCrypto and witness key validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Wallet
participant SignUtils
participant WitnessInitializer
Wallet->>SignUtils: reconstruct decrypted private key
SignUtils-->>Wallet: ECKey or IllegalArgumentException
Wallet->>Wallet: wrap error and clear private-key bytes
WitnessInitializer->>Wallet: load witness keystore
Wallet-->>WitnessInitializer: key or CipherException
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Federico2014
changed the title
Federico2014
force-pushed
the
feature/ec-key-validation-hardening
branch
3 times, most recently
from
August 4, 2026 07:48
bf45f6b to
07283db
Compare
Federico2014
force-pushed
the
feature/ec-key-validation-hardening
branch
from
August 4, 2026 09:19
07283db to
2caafc9
Compare
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 hardens secp256k1
ECKeyconstruction, state handling, provider usage, and keystore decryption.BigIntegerfor valid high-bit scalars.CipherExceptionand clears the raw decrypted private-key buffer on both success and failure paths.Why are these changes required?
ECKeypreviously 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 consistently honor. In addition, malformed decrypted keys could escape the keystore boundary asIllegalArgumentException, and the raw decrypted private-key buffer remained in memory unnecessarily.These changes make validation predictable, reject invalid input before expensive curve operations, keep key generation and signing aligned with the Bouncy Castle implementation, preserve cache encapsulation, and normalize keystore failures while reducing plaintext private-key lifetime.
This PR has been tested by:
./gradlew :framework:test --tests org.tron.common.crypto.ECKeyTest./gradlew :framework:test --tests org.tron.keystore.WalletAddressValidationTest --tests org.tron.core.config.args.WitnessInitializerKeystoreTest./gradlew :framework:checkstyleMain :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. Invalid decrypted keystore keys are now reported asCipherException. No SM2, signature verification, signature recovery, configuration, or consensus behavior is changed.Compatibility impact
As part of this hardening, consumers using the following public
ECKeyAPIs may be affected by source and binary compatibility changes:ECKey(SecureRandom)for key generation.fromPrivateAndPrecalculatedPublicis removed. Use the validatedECKey(BigInteger, ECPoint)constructor instead.toStringWithPrivateis removed without replacement to prevent accidental private-key exposure.Downstream consumers must update their code and recompile when upgrading. This does not change node protocol, consensus, transaction, or on-chain behavior.