feat(crypto): harden ECKey validation - #46
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
ChangesCrypto and witness key validation
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to The change hardens EC key validation and keystore error handling without any supplied evidence of a current correctness, security, or availability issue; no actionable merge-blocking risk remains beyond normal checks and review. Possibly related issues
Possibly related PRs
🚥 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
bf45f6b to
07283db
Compare
07283db to
2caafc9
Compare
1 participant
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.
Summary by CodeRabbit
Security Enhancements
Compatibility