fix(crypto): harden ECKey and signature validation#13
fix(crypto): harden ECKey and signature validation#13Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds defensive validation for private/public key material and signatures across crypto and framework modules, rejects invalid inputs at API boundaries, ensures keystore-derived private bytes are validated and zeroed, and updates tests to expect exceptions for malformed keys. 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 Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
|
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
Federico2014
left a comment
Federico2014
left a comment
There was a problem hiding this comment.
Changes requested (1 [SHOULD] blocker on double-decode; 1 [SHOULD] advisory on zeroization).
Federico2014
left a comment
Federico2014
left a comment
There was a problem hiding this comment.
Changes requested (1 [SHOULD] blocker on double-decode; 1 [SHOULD] advisory on zeroization).
| */ | ||
| //The parameters of the secp256k1 curve. | ||
| public static final ECDomainParameters CURVE; | ||
| public static final ECParameterSpec CURVE_SPEC; |
There was a problem hiding this comment.
[NIT] CURVE and CURVE_SPEC had their /** */ Javadoc converted to a // inline comment. Public static final fields should keep /** */ so IDEs and javadoc tooling pick them up. HALF_CURVE_ORDER now has no comment at all after this PR, which worsens the inconsistency.
There was a problem hiding this comment.
已修复：将 CURVE 和 CURVE_SPEC 的 // 注释恢复为 /** */ Javadoc。
| } | ||
| this.privKey = null; | ||
| this.pub = CURVE.getCurve().decodePoint(key); | ||
| } |
There was a problem hiding this comment.
[SHOULD] Double decodePoint in the isPrivateKey=false path. isValidPublicKey(key) already calls CURVE.getCurve().decodePoint(keyBytes) internally, and this line calls it again to assign this.pub. Decode once, validate, then assign:
} else {
ECPoint point;
try {
point = CURVE.getCurve().decodePoint(key);
} catch (RuntimeException e) {
throw new IllegalArgumentException("Invalid public key.", e);
}
if (point.isInfinity() || !point.isValid()) {
throw new IllegalArgumentException("Invalid public key.");
}
this.privKey = null;
this.pub = point;
}This also distinguishes malformed encoding errors from off-curve point errors.
There was a problem hiding this comment.
已修复：重构 isPrivateKey=false 路径，改为一次 decodePoint + 手动 isInfinity/isValid 检查，消除重复解码。同时区分了编码格式错误和曲线外点两种错误类型。
| * Creates a verify-only ECKey from the given encoded public key bytes. | ||
| */ | ||
| public static ECKey fromPublicOnly(byte[] pub) { | ||
| return new ECKey(null, CURVE.getCurve().decodePoint(pub)); |
There was a problem hiding this comment.
[QUESTION] fromPublicOnly(ECPoint pub) and toStringWithPrivate() were both removed. No remaining callers appear in this repo. Were either of these part of a public SDK contract that downstream integrators depend on? If so, a deprecation cycle would be safer than hard removal.
There was a problem hiding this comment.
两处删除均属有意为之，此 repo 内确认无残留调用者。两个方法均不属于外部 SDK 契约：fromPublicOnly(ECPoint) 是内部工厂方法，已被构造函数层的 EC 点校验取代；toStringWithPrivate() 会将私钥明文写入 String（反模式），主动删除是安全改进。无需走 deprecation 流程。
| @@ -672,7 +625,7 @@ public byte[] getAddress() { | |||
| if (pubKeyHash == null) { | |||
| pubKeyHash = Hash.computeAddress(this.pub); | |||
There was a problem hiding this comment.
[NIT] volatile ensures write visibility but doesn't prevent two threads from both observing null, both computing, and one overwriting. Since computeAddress is deterministic the race is benign (same result either way), but it contradicts the thread-safety intent implied by adding volatile. Either use synchronized or add a comment documenting that the benign double-computation is intentional.
There was a problem hiding this comment.
已在字段声明处补充注释，明确说明 volatile 保证可见性、竞态属于 benign race（两个线程计算结果相同，最后一次写入生效，正确性不受影响）。
| } finally { | ||
| if (privKeyBytes != null) { | ||
| Arrays.fill(privKeyBytes, (byte) 0); | ||
| } |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
|
|
||
| @Test | ||
| public void testInvalidPublicKeyEncoding() { | ||
| byte[] invalidPublicKey = new byte[33]; |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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).
Federico2014
force-pushed
the
fix/eckey-signature-validation
branch
from
65537c7 to
ba307e9
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
framework/src/test/java/org/tron/common/crypto/SM2KeyTest.java (1)
295-296: "Oversized" case is actually a zero-value case.
new byte[34]is all-zero, soBigIntegerdecodes to0and is rejected for being below the minimum — identical in effect to the all-zero 32-byte assertion on Line 290. To actually exercise length-based rejection (vs. value-based), seed a non-zero 34-byte array (e.g. set the last byte to1). Per retrieved learning, short arrays with a leading0x00byte are intentionally accepted as scalar data, so the exact semantics for an oversized non-zero input are worth pinning down in the test.🧪 Proposed fix
- // oversized input (34 bytes) - assertFalse(SM2.isValidPrivateKey(new byte[34])); + // oversized input (34 bytes, non-zero value to distinguish from the all-zero case) + byte[] oversized = new byte[34]; + oversized[33] = 1; + assertFalse(SM2.isValidPrivateKey(oversized));🤖 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/SM2KeyTest.java` around lines 295 - 296, The test currently passes an all-zero 34-byte array which checks the zero-value rejection rather than length-based rejection; change the oversized-case assertion to use a non-zero 34-byte array (e.g., create a byte[34] and set at least one byte to 1) so SM2.isValidPrivateKey(...) is exercised for an actually oversized non-zero input; keep the assertion as assertFalse and reference SM2.isValidPrivateKey to ensure the test distinguishes length-based rejection from zero-value rejection (note that shorter arrays with a leading 0x00 are intentionally accepted, so ensure the 34-byte array is non-zero).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@framework/src/test/java/org/tron/common/crypto/SM2KeyTest.java`:
- Around line 295-296: The test currently passes an all-zero 34-byte array which
checks the zero-value rejection rather than length-based rejection; change the
oversized-case assertion to use a non-zero 34-byte array (e.g., create a
byte[34] and set at least one byte to 1) so SM2.isValidPrivateKey(...) is
exercised for an actually oversized non-zero input; keep the assertion as
assertFalse and reference SM2.isValidPrivateKey to ensure the test distinguishes
length-based rejection from zero-value rejection (note that shorter arrays with
a leading 0x00 are intentionally accepted, so ensure the 34-byte array is
non-zero).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dca9306d-4e83-4f1a-b379-eb0025d01c76
📒 Files selected for processing (8)
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/common/crypto/SM2KeyTest.javaframework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java
✅ Files skipped from review due to trivial changes (1)
- crypto/src/main/java/org/tron/common/crypto/Rsv.java
🚧 Files skipped from review as they are similar to previous changes (5)
- crypto/src/main/java/org/tron/common/crypto/SignUtils.java
- framework/src/test/java/org/tron/common/crypto/ECKeyTest.java
- crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java
- framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java
- crypto/src/main/java/org/tron/common/crypto/ECKey.java
1 participant
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,SM2KeyTest,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
Summary by CodeRabbit
Release Notes
Security Improvements
Breaking Changes
null