fix(crypto): harden ECKey and signature validation#13
Open
Federico2014 wants to merge 1 commit intodevelopfrom
Open
fix(crypto): harden ECKey and signature validation#13Federico2014 wants to merge 1 commit intodevelopfrom
Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
📝 WalkthroughWalkthroughThis PR hardens cryptographic input validation and defensive handling: ECKey/SM2 gained explicit private/public validators, constructors and factories now reject invalid keys, Rsv validates signatures, SignUtils routes validations, WitnessInitializer validates and zeroes keystore bytes, and tests updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant WI as WitnessInitializer
participant SU as SignUtils
participant ECK as ECKey
participant SM2 as SM2
WI->>SU: isValidPrivateKey(privKeyBytes, isECKeyCryptoEngine)
alt isECKeyCryptoEngine == true
SU->>ECK: isValidPrivateKey(privKeyBytes)
ECK-->>SU: true / false
else isECKeyCryptoEngine == false
SU->>SM2: isValidPrivateKey(privKeyBytes)
SM2-->>SU: true / false
end
SU-->>WI: validation result
alt valid
WI->>WI: convert to hex & add to privateKeys
else invalid
WI-->>WI: throw TronError(WITNESS_KEYSTORE_LOAD)
end
WI->>WI: Arrays.fill(privKeyBytes, 0) (finally)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 7 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/sm2/SM2.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java:261">
P2: `isValidPrivateKey(byte[])` should enforce private-key byte length/padding rules; currently it accepts malformed overlong encodings if the numeric value is in range.
(Based on your team's feedback about handling BigInteger sign-padding for private key byte arrays.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Federico2014
force-pushed
the
fix/eckey-signature-validation
branch
from
f1f5543 to
b4ab742
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java (1)
250-255: Consider aligningSM2.fromPrivate(byte[])withECKey.fromPrivate(byte[]).
ECKey.fromPrivate(byte[])now throwsIllegalArgumentExceptionfor null/empty input, butSM2.fromPrivate(byte[])still returnsnull. This inconsistency could be confusing for callers who expect uniform behavior across crypto engines when usingSignUtils.fromPrivate().♻️ Suggested alignment
public static SM2 fromPrivate(byte[] privKeyBytes) { - if (ByteArray.isEmpty(privKeyBytes)) { - return null; - } + if (!isValidPrivateKey(privKeyBytes)) { + throw new IllegalArgumentException("Invalid private key."); + } return fromPrivate(new BigInteger(1, privKeyBytes)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java` around lines 250 - 255, SM2.fromPrivate(byte[]) currently returns null for null/empty input which is inconsistent with ECKey.fromPrivate(byte[]) that throws IllegalArgumentException; change SM2.fromPrivate(byte[]) to validate privKeyBytes and throw IllegalArgumentException for null/empty inputs (use the same error message style as ECKey.fromPrivate), then delegate to fromPrivate(BigInteger) as before. Ensure callers like SignUtils.fromPrivate() will now receive the exception instead of a null return.crypto/src/main/java/org/tron/common/crypto/ECKey.java (1)
359-369: Minor inefficiency: EC point validated twice.
fromPublicOnly(byte[])decodes and validates the point, then theECKey(BigInteger, ECPoint)constructor (via the provider constructor at line 207) validates the point again. Consider passing the already-validated point directly to avoid redundantisValid()computation.♻️ Suggested optimization
public static ECKey fromPublicOnly(byte[] pub) { if (ByteArray.isEmpty(pub)) { throw new IllegalArgumentException("Public key bytes cannot be null or empty"); } ECPoint point = CURVE.getCurve().decodePoint(pub); if (point.isInfinity() || !point.isValid()) { throw new IllegalArgumentException( "Public key is not a valid point on secp256k1 curve."); } - return new ECKey(null, point); + // Use provider constructor directly to avoid re-validation + return new ECKey(TronCastleProvider.getInstance(), null, point); }Note: This would require adjusting the provider constructor to skip re-validation when called internally, or introducing a private constructor. Given the complexity, the current approach is acceptable for correctness over micro-optimization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/src/main/java/org/tron/common/crypto/ECKey.java` around lines 359 - 369, fromPublicOnly(byte[]) decodes and validates the ECPoint then calls the public ECKey(BigInteger, ECPoint) which re-validates the point; to avoid the redundant isValid() call add a private/internal constructor (e.g., ECKey(ECPoint, boolean skipValidation) or a private ECKeyFromPoint(ECPoint)) that accepts an already-validated ECPoint and bypasses the extra validation, then have fromPublicOnly(byte[]) call that private constructor while leaving the public ECKey(BigInteger, ECPoint) unchanged for external callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crypto/src/main/java/org/tron/common/crypto/ECKey.java`:
- Around line 359-369: fromPublicOnly(byte[]) decodes and validates the ECPoint
then calls the public ECKey(BigInteger, ECPoint) which re-validates the point;
to avoid the redundant isValid() call add a private/internal constructor (e.g.,
ECKey(ECPoint, boolean skipValidation) or a private ECKeyFromPoint(ECPoint))
that accepts an already-validated ECPoint and bypasses the extra validation,
then have fromPublicOnly(byte[]) call that private constructor while leaving the
public ECKey(BigInteger, ECPoint) unchanged for external callers.
In `@crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java`:
- Around line 250-255: SM2.fromPrivate(byte[]) currently returns null for
null/empty input which is inconsistent with ECKey.fromPrivate(byte[]) that
throws IllegalArgumentException; change SM2.fromPrivate(byte[]) to validate
privKeyBytes and throw IllegalArgumentException for null/empty inputs (use the
same error message style as ECKey.fromPrivate), then delegate to
fromPrivate(BigInteger) as before. Ensure callers like SignUtils.fromPrivate()
will now receive the exception instead of a null return.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b835b3ae-5885-4654-81e4-a9dba921e594
📒 Files selected for processing (7)
crypto/src/main/java/org/tron/common/crypto/ECKey.javacrypto/src/main/java/org/tron/common/crypto/Rsv.javacrypto/src/main/java/org/tron/common/crypto/SignUtils.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2.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
Federico2014
force-pushed
the
fix/eckey-signature-validation
branch
from
b4ab742 to
399e6cf
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crypto/src/main/java/org/tron/common/crypto/ECKey.java (1)
378-381:⚠️ Potential issue | 🟠 MajorReject oversized signatures instead of silently truncating.
Line 378 only rejects signatures shorter than 65 bytes. Inputs longer than 65 bytes are currently accepted and tail bytes are ignored (Lines 386-388), which weakens boundary validation.
🔧 Proposed fix
- if (signatureEncoded.length < 65) { - throw new SignatureException("Signature truncated, expected 65 " + - "bytes and got " + signatureEncoded.length); - } + if (signatureEncoded.length != 65) { + throw new SignatureException("Invalid signature length, expected 65 bytes and got " + + signatureEncoded.length); + }Also applies to: 386-388
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crypto/src/main/java/org/tron/common/crypto/ECKey.java` around lines 378 - 381, The code in ECKey that currently only checks if signatureEncoded.length < 65 must also reject inputs > 65 instead of silently truncating: update the validation around signatureEncoded (and the logic that extracts r, s, v from signatureEncoded) to throw a SignatureException when signatureEncoded.length != 65, so oversized signatures are refused; reference the signatureEncoded variable and the existing SignatureException class in ECKey and ensure downstream extraction code (the block that reads the first 65 bytes into r, s, v) no longer ignores extra bytes.
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/common/crypto/ECKeyTest.java (1)
72-74: Add one regression test for oversized (>=66-byte) signatures.You already cover invalid Base64 and truncated signatures; adding an oversized-signature case will guard the strict-length boundary too.
🧪 Suggested test addition
+import java.util.Base64; ... + `@Test`(expected = SignatureException.class) + public void testOversizedSignatureLength() throws SignatureException { + byte[] messageHash = new byte[32]; + byte[] oversized = new byte[66]; + oversized[0] = 27; // valid-ish header domain + String sigBase64 = Base64.getEncoder().encodeToString(oversized); + ECKey.signatureToKey(messageHash, sigBase64); + fail("Expecting a SignatureException for oversized signature length"); + }Also applies to: 134-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/common/crypto/ECKeyTest.java` around lines 72 - 74, Add a regression test in ECKeyTest that asserts parsing an oversized signature (>=66 bytes) fails: create a byte[] of length 66 (or longer), Base64-encode it, and use the same parsing method exercised by the existing invalid Base64/truncated-signature tests to assertThrows(IllegalArgumentException.class). Name the test clearly (e.g., testParseSignature_oversized) and add the same assertion in both relevant locations referenced (around the existing signature-parsing tests and the block at lines ~134-146) so the strict-length boundary is enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crypto/src/main/java/org/tron/common/crypto/ECKey.java`:
- Around line 378-381: The code in ECKey that currently only checks if
signatureEncoded.length < 65 must also reject inputs > 65 instead of silently
truncating: update the validation around signatureEncoded (and the logic that
extracts r, s, v from signatureEncoded) to throw a SignatureException when
signatureEncoded.length != 65, so oversized signatures are refused; reference
the signatureEncoded variable and the existing SignatureException class in ECKey
and ensure downstream extraction code (the block that reads the first 65 bytes
into r, s, v) no longer ignores extra bytes.
---
Nitpick comments:
In `@framework/src/test/java/org/tron/common/crypto/ECKeyTest.java`:
- Around line 72-74: Add a regression test in ECKeyTest that asserts parsing an
oversized signature (>=66 bytes) fails: create a byte[] of length 66 (or
longer), Base64-encode it, and use the same parsing method exercised by the
existing invalid Base64/truncated-signature tests to
assertThrows(IllegalArgumentException.class). Name the test clearly (e.g.,
testParseSignature_oversized) and add the same assertion in both relevant
locations referenced (around the existing signature-parsing tests and the block
at lines ~134-146) so the strict-length boundary is enforced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b5cbf8f-6f47-47ba-ad18-ad4d5fff36e7
📒 Files selected for processing (7)
crypto/src/main/java/org/tron/common/crypto/ECKey.javacrypto/src/main/java/org/tron/common/crypto/Rsv.javacrypto/src/main/java/org/tron/common/crypto/SignUtils.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2.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
🚧 Files skipped from review as they are similar to previous changes (4)
- framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java
- crypto/src/main/java/org/tron/common/crypto/Rsv.java
- crypto/src/main/java/org/tron/common/crypto/SignUtils.java
- framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java
Owner
Author
|
Thanks for the review. 1. Reject oversized signatures ( This code (line 374, 2. Add oversized-signature test Since the code behavior is unchanged in this PR, adding a test for the current |
Federico2014
force-pushed
the
fix/eckey-signature-validation
branch
from
399e6cf to
65537c7
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?
Hardens input validation across ECKey, Rsv, SignUtils, and WitnessInitializer to reject invalid key material at API boundaries.
ECKey.fromPrivate,fromPublicOnly,publicKeyFromPrivate,signatureToKeyBytes, preventing silent truncation of oversized key materialisInfinity(),isValid()) in the ECKey constructor to reject invalid public keys; removed redundant duplicate validation fromfromPublicOnly()ECKey.fromPrivateAndPrecalculatedPublic()(unused factory method)ECKeyabout signature malleability and BIP-62; cleaned up Javadoc acrossECKey.java— removed orphan/duplicate comment blocks, converted/* */to/** */, removed meaningless@param -/@return -tags, and simplified verbose descriptionsisValidPrivateKey()to bothECKeyandSM2for key material validationRsv.fromSignature()(rejects null or < 65 bytes)SignUtilsbefore signingWitnessInitializerto reject malformed private keys on node startup and zeroes key material in afinallyblock after useWhy are these changes required?
ECKey.fromPrivate(null)silently returnednullinstead of failing fast, allowing invalid key material to propagateECKey.fromPublicOnly()accepted infinity points and off-curve points without validationRsv.fromSignature()lacked null/length checks on signature bytesWitnessInitializerdid not zero key material in error paths, risking private key leakage in heap dumpsisValidPrivateKeyutility existed for pre-validation of key materialECKey.fromPrivate(byte[])— null/empty input now throwsIllegalArgumentExceptioninstead of returning nullECKey.fromPrivateAndPrecalculatedPublic()removedECKeyconstructor andfromPublicOnly()now validate EC points — invalid public keys throwIllegalArgumentExceptionThis PR has been tested by:
ECKeyTest,WitnessInitializerTest./gradlew clean build -x test)Follow up
Consensus Safety
These changes do not affect consensus. All modifications are defensive input validation at API boundaries — they reject invalid inputs earlier with clear error messages, without changing the behavior for any valid input.
In particular,
Rsv.fromSignature()now throwsIllegalArgumentExceptionfor null or short (< 65 bytes) signature arrays. Since signatures come from protobuf deserialization of network data, a malicious peer could send a short or empty signature. The callers handle this correctly:TransactionCapsule.validateSignature()(transaction verification): already checkssig.size() < 65before calling, so the new check is redundant here.BlockCapsule.validateSignature()(block witness signature verification): callsgetBase64FromByteString()without a length pre-check. Previously a short signature would cause a rawArrayIndexOutOfBoundsException; now it throws a clearIllegalArgumentException— both are caught by the surroundingtry-catchand result inValidateSignatureException(block rejected), so the outcome is unchanged.PrecompiledContracts.recoverAddrBySign()(TVMecrecoverprecompile): checksisEmpty || length < 65before calling, and wraps intry-catch(Throwable).PbftBaseMessage.analyzeSignature(),RelayService,PbftDataSyncHandler: no length pre-check, but all wrapped in exception handlers. The new validation converts silentArrayIndexOutOfBoundsExceptioninto explicitIllegalArgumentException— same outcome (message rejected).TransactionResult(JSON-RPC layer): read-only API formatting, not in the consensus path.In all cases, the new validation only changes the exception type for invalid inputs — from an opaque
ArrayIndexOutOfBoundsExceptionto a descriptiveIllegalArgumentException. No valid signature that previously passed is now rejected, and no consensus outcome changes.Extra details
Split from #7
Closes #17