fix(crypto): harden crypto module security and robustness#7
Open
Federico2014 wants to merge 6 commits intodevelopfrom
Open
fix(crypto): harden crypto module security and robustness#7Federico2014 wants to merge 6 commits intodevelopfrom
Federico2014 wants to merge 6 commits intodevelopfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughTightens crypto key validation and error handling (ECKey, SM2), makes SM2 signing deterministic, deprecates and rejects the Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 27 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/Rsv.java">
<violation number="1" location="crypto/src/main/java/org/tron/common/crypto/Rsv.java:18">
P2: Signature validation is incomplete: it accepts oversized signatures and silently ignores trailing bytes. Require exact length (65) instead of only checking `< 65`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Federico2014
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
framework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.java (1)
21-28:⚠️ Potential issue | 🟡 MinorAvoid global test-state leakage from static init.
Line 27 mutates global
Argsstate for the whole JVM with no local restore, which can make unrelated tests order-dependent.Suggested fix
static { Args.setParam( new String[]{ "--output-directory", dbPath(), }, TestConstants.TEST_CONF ); - Args.getInstance().allowShieldedTransactionApi = true; } `@Test` public void testCreateSpendAuthSig() { + boolean old = Args.getInstance().isAllowShieldedTransactionApi(); + Args.getInstance().setAllowShieldedTransactionApi(true); + try { String jsonParam = "{" + " \"ask\": \"e3ebcba1531f6d9158d9c162660c5d7c04dadf77d" + "85d7436a9c98b291ff69a09\"," + " \"tx_hash\": \"3b78fee6e956f915ffe082284c5f18640edca9" + "c57a5f227e5f7d7eb65ad61502\"," + " \"alpha\": \"2608999c3a97d005a879ecdaa16fd29ae434fb67" + "b177c5e875b0c829e6a1db04\"" + "}"; ... -} + } finally { + Args.getInstance().setAllowShieldedTransactionApi(old); + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.java` around lines 21 - 28, Static test initialization mutates global Args state (Args.setParam and Args.getInstance().allowShieldedTransactionApi) without restoring it, leaking state across tests; change the static block into explicit setup/teardown that saves previous Args state, applies the test-specific params and allowShieldedTransactionApi=true in a `@Before` or `@BeforeClass` method, and restores the original params and allowShieldedTransactionApi value in an `@After` or `@AfterClass` method so global Args is not permanently mutated by CreateSpendAuthSigServletTest.crypto/src/main/java/org/tron/common/crypto/ECKey.java (1)
191-209:⚠️ Potential issue | 🟠 MajorValidate
ECPointinputs here as well.Line 205 only checks
null.fromPublicOnly(byte[])and any other caller that already has a decoded point can still pass the point at infinity and bypassisValidPublicKey(byte[]).🔒 Suggested fix
- if (pub == null) { - throw new IllegalArgumentException("Public key should not be null"); - } else { - this.pub = pub; - } + if (pub == null || pub.isInfinity() || !pub.isValid()) { + throw new IllegalArgumentException("Invalid public key."); + } + this.pub = pub;🤖 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 191 - 209, The ECKey constructor currently only checks pub for null; add validation to reject the curve's point-at-infinity and any invalid ECPoint values so callers like fromPublicOnly(byte[]) can't bypass isValidPublicKey(byte[]). In the ECKey(Provider, PrivateKey, ECPoint) constructor, after the null check for pub, throw an IllegalArgumentException if pub.isInfinity() (and/or if the ECPoint invalidity check provided by the EC library exists, e.g., a pub.isValid() or equivalent curve validation) so only bona fide public points are accepted.crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java (1)
535-543:⚠️ Potential issue | 🟠 Major
userIDis still ignored on both sign and verify paths.Lines 535-543 and Lines 752-755 never use the caller-supplied
userID, so non-default SM2 identifiers are silently treated as the default ZA hash. That makes this API incorrect for any caller that expects custom IDs to round-trip.Also applies to: 747-755
🤖 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 535 - 543, The signMsg method ignores the caller-supplied userID (and likewise the verify path), so update signMsg to pass the userID into the SM2 signer (instead of calling signer.generateSignature(msg) with no ID): obtain an SM2Signer via getSigner(), initialize or set its user ID (use the caller userID or fall back to the SM2 default ID if null), then call the signer method that uses the user ID to produce the signature (e.g., SM2Signer.generateSignature(msg, userID) or call signer.setUserID(userID) before generateSignature). Do the symmetric change in the verification method (the method that currently calls signer.verifySignature(...)) so it uses the same userID handling (use the provided userID or default) to ensure custom IDs are honored during sign and verify.
🧹 Nitpick comments (4)
framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java (1)
112-113: Make the stub input-sensitive to avoid masking regressions.Right now Line 113 always decodes
privateKey, regardless of the actual argument passed toByteArray.fromHexString(...).Suggested fix
mockedByteArray.when(() -> ByteArray.fromHexString(anyString())) - .thenAnswer(inv -> Hex.decode(privateKey)); + .thenAnswer(inv -> Hex.decode(inv.getArgument(0, String.class)));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java` around lines 112 - 113, The stub for mockedByteArray should be input-sensitive: instead of always decoding the fixed privateKey, change the thenAnswer on ByteArray.fromHexString to decode the actual argument passed—e.g. in the thenAnswer lambda, get the first argument (cast to String) from the InvocationHandler (inv.getArgument(0)) and call Hex.decode on that value so ByteArray.fromHexString(...) returns based on its input rather than always using privateKey; update the mockedByteArray.when(...) -> thenAnswer(...) accordingly.framework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.java (2)
1160-1163: Narrow the expected overflow assertion.Lines 1160-1162 will pass on any
ArithmeticExceptionthrown anywhere in the arrange/act block, not just the overflow you want to document.assertThrowsaround the exact overflowing call would avoid hiding unrelated arithmetic regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.java` around lines 1160 - 1163, Replace the broad try/catch that catches any ArithmeticException in ShieldedTransferActuatorTest with a focused assertion around the specific overflowing call (StrictMathWrapper.subtractExact) using Assert.assertThrows (or JUnit's assertThrows) so only an overflow from that exact operation is accepted; remove the generic catch block and wrap the exact subtractExact invocation in assertThrows to narrowly validate the overflow behavior.
1353-1404: Scope this test to the actual API gate or rename it.
framework/src/main/java/org/tron/core/actuator/ShieldedTransferActuator.java'svalidate()path never readsallowShieldedTransactionApi, so this only proves core validation is unchanged. It will not catch a regression where the wallet/gRPC/HTTP entry points ignore the disabled flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.java` around lines 1353 - 1404, The test shieldedTransferValidationWorksWhenApiDisabled wrongly asserts that the API flag affects core validation even though ShieldedTransferActuator.validate() does not read allowShieldedTransactionApi; either rename the test to reflect it only verifies core validation (e.g., shieldedTransferCoreValidationUnaffectedByApiFlag) or change the test to exercise the actual API gate (call the wallet/gRPC/HTTP entry point that checks Args.getInstance().isAllowShieldedTransactionApi()) so it fails when the flag is false; locate references to ShieldedTransferActuator.validate() and the test method name shieldedTransferValidationWorksWhenApiDisabled to apply the rename or to replace the direct actuator.validate() call with a call through the real API entry path that enforces allowShieldedTransactionApi.framework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.java (1)
76-80: Restore the global shielded-API flag after this class.Line 79 mutates the singleton
Argsstate in@BeforeClassand never puts it back. Since the new default isfalse, later tests can inherittrueand silently miss disabled-path coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.java` around lines 76 - 80, The test sets Args.getInstance().allowShieldedTransactionApi = true in ShieldedTRC20BuilderTest.initZksnarkParams and never restores it; capture the previous value before mutating (or assume default false) and add an `@AfterClass` method (e.g., tearDownZksnarkParams) that restores Args.getInstance().allowShieldedTransactionApi to its original value so other tests don't inherit the mutated singleton state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java`:
- Around line 152-168: The SM2 constructor SM2(`@Nullable` PrivateKey privKey,
ECPoint pub) currently only rejects null pub but allows the point-at-infinity;
update this constructor to validate the ECPoint by checking pub.isInfinity() (or
equivalent) and throw an IllegalArgumentException when the public point is the
point-at-infinity so callers like fromPublicOnly(byte[]) cannot bypass
public-key validation; keep the existing null check and error message style
while referencing the pub parameter and the SM2 constructor.
- Around line 986-1005: The decodeFromDER method currently reads a single ASN.1
object via ASN1InputStream which permits trailing garbage; replace that logic in
decodeFromDER to parse strictly using ASN1Primitive.fromByteArray(signature) (or
equivalent strict parser) to ensure trailing bytes cause an IOException and are
rejected, then validate the returned ASN1Primitive is an ASN1Sequence of size 2
and extract r and s to construct the SM2Signature; keep the same
SignatureException behavior by catching IO/IllegalArgumentException and
rethrowing with a clear message.
In `@crypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.java`:
- Around line 188-194: verifyHashSignature currently only checks for empty hash
but must enforce the same 32-byte contract as generateHashSignature; update
verifyHashSignature to throw IllegalArgumentException if hash.length != 32 (and
keep the existing null/empty checks), referencing the verifyHashSignature method
and matching the validation behavior of generateHashSignature so malformed
hashes are rejected consistently before any verify logic runs.
In `@framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java`:
- Around line 86-94: Check for a null return from sign.getPrivateKey() in
WitnessInitializer before calling SignUtils.isValidPrivateKey or Arrays.fill; if
null, throw the same TronError (TronError.ErrCode.WITNESS_KEYSTORE_LOAD). Also
move the zeroing of the privKeyBytes buffer into a finally block (guarded by
privKeyBytes != null) so Arrays.fill is always called on exit paths (both
success path where you call privateKeys.add(ByteArray.toHexString(privKeyBytes))
and error/throw paths).
In `@framework/src/main/java/org/tron/core/services/RpcApiService.java`:
- Around line 724-727: The code currently calls getRunTimeException(...) which
yields Status.INTERNAL for deprecated 'events' input; replace those calls in
RpcApiService (the branches that check request.getEventsList().isEmpty() and
similar deprecated-events checks) with a gRPC INVALID_ARGUMENT runtime
exception: use io.grpc.Status.INVALID_ARGUMENT.withDescription("'events' field
is deprecated and no longer supported").asRuntimeException() and pass that to
responseObserver.onError(...). Update every occurrence (the other
deprecated-events branches in RpcApiService) so client input errors return
Status.INVALID_ARGUMENT instead of INTERNAL.
In `@framework/src/main/java/org/tron/core/Wallet.java`:
- Around line 2343-2361: Before iterating shieldedSpends in Wallet where you
currently check ask/nsk/ovk, add an explicit validation: if shieldedSpends is
non-empty and any of ask, nsk or ovk is missing, throw a ZksnarkException (with
a clear message like "missing shielded key material") instead of skipping the
loop; apply the same check in the other builder location that also handles
shieldedSpends (the second occurrence around builder.addSpend) so malformed
requests containing shieldedSpends cannot be silently accepted.
In `@framework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.java`:
- Around line 41-44: The static block mutates the global flag
Args.getInstance().allowShieldedTransactionApi which can make tests flaky;
remove that permanent change and instead set and restore the flag per test by
capturing the original value from Args.getInstance() in a `@Before` (or setup)
method, set allowShieldedTransactionApi = true before each test, and restore the
saved original value in an `@After` (or teardown) method so the global state is
not left mutated; update any references to the existing static initializer that
set allowShieldedTransactionApi to rely on these setup/teardown methods instead.
---
Outside diff comments:
In `@crypto/src/main/java/org/tron/common/crypto/ECKey.java`:
- Around line 191-209: The ECKey constructor currently only checks pub for null;
add validation to reject the curve's point-at-infinity and any invalid ECPoint
values so callers like fromPublicOnly(byte[]) can't bypass
isValidPublicKey(byte[]). In the ECKey(Provider, PrivateKey, ECPoint)
constructor, after the null check for pub, throw an IllegalArgumentException if
pub.isInfinity() (and/or if the ECPoint invalidity check provided by the EC
library exists, e.g., a pub.isValid() or equivalent curve validation) so only
bona fide public points are accepted.
In `@crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java`:
- Around line 535-543: The signMsg method ignores the caller-supplied userID
(and likewise the verify path), so update signMsg to pass the userID into the
SM2 signer (instead of calling signer.generateSignature(msg) with no ID): obtain
an SM2Signer via getSigner(), initialize or set its user ID (use the caller
userID or fall back to the SM2 default ID if null), then call the signer method
that uses the user ID to produce the signature (e.g.,
SM2Signer.generateSignature(msg, userID) or call signer.setUserID(userID) before
generateSignature). Do the symmetric change in the verification method (the
method that currently calls signer.verifySignature(...)) so it uses the same
userID handling (use the provided userID or default) to ensure custom IDs are
honored during sign and verify.
In
`@framework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.java`:
- Around line 21-28: Static test initialization mutates global Args state
(Args.setParam and Args.getInstance().allowShieldedTransactionApi) without
restoring it, leaking state across tests; change the static block into explicit
setup/teardown that saves previous Args state, applies the test-specific params
and allowShieldedTransactionApi=true in a `@Before` or `@BeforeClass` method, and
restores the original params and allowShieldedTransactionApi value in an `@After`
or `@AfterClass` method so global Args is not permanently mutated by
CreateSpendAuthSigServletTest.
---
Nitpick comments:
In
`@framework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.java`:
- Around line 1160-1163: Replace the broad try/catch that catches any
ArithmeticException in ShieldedTransferActuatorTest with a focused assertion
around the specific overflowing call (StrictMathWrapper.subtractExact) using
Assert.assertThrows (or JUnit's assertThrows) so only an overflow from that
exact operation is accepted; remove the generic catch block and wrap the exact
subtractExact invocation in assertThrows to narrowly validate the overflow
behavior.
- Around line 1353-1404: The test shieldedTransferValidationWorksWhenApiDisabled
wrongly asserts that the API flag affects core validation even though
ShieldedTransferActuator.validate() does not read allowShieldedTransactionApi;
either rename the test to reflect it only verifies core validation (e.g.,
shieldedTransferCoreValidationUnaffectedByApiFlag) or change the test to
exercise the actual API gate (call the wallet/gRPC/HTTP entry point that checks
Args.getInstance().isAllowShieldedTransactionApi()) so it fails when the flag is
false; locate references to ShieldedTransferActuator.validate() and the test
method name shieldedTransferValidationWorksWhenApiDisabled to apply the rename
or to replace the direct actuator.validate() call with a call through the real
API entry path that enforces allowShieldedTransactionApi.
In
`@framework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.java`:
- Around line 112-113: The stub for mockedByteArray should be input-sensitive:
instead of always decoding the fixed privateKey, change the thenAnswer on
ByteArray.fromHexString to decode the actual argument passed—e.g. in the
thenAnswer lambda, get the first argument (cast to String) from the
InvocationHandler (inv.getArgument(0)) and call Hex.decode on that value so
ByteArray.fromHexString(...) returns based on its input rather than always using
privateKey; update the mockedByteArray.when(...) -> thenAnswer(...) accordingly.
In `@framework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.java`:
- Around line 76-80: The test sets
Args.getInstance().allowShieldedTransactionApi = true in
ShieldedTRC20BuilderTest.initZksnarkParams and never restores it; capture the
previous value before mutating (or assume default false) and add an `@AfterClass`
method (e.g., tearDownZksnarkParams) that restores
Args.getInstance().allowShieldedTransactionApi to its original value so other
tests don't inherit the mutated singleton state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f89f6773-898a-4ab4-a492-4fad47232535
📒 Files selected for processing (27)
common/src/main/java/org/tron/common/parameter/CommonParameter.javacrypto/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.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.javaframework/src/main/java/org/tron/core/Wallet.javaframework/src/main/java/org/tron/core/config/args/Args.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByIvkServlet.javaframework/src/main/java/org/tron/core/services/http/ScanShieldedTRC20NotesByOvkServlet.javaframework/src/main/java/org/tron/core/zen/ShieldedTRC20ParametersBuilder.javaframework/src/main/java/org/tron/core/zen/ZenTransactionBuilder.javaframework/src/main/resources/config.confframework/src/test/java/org/tron/common/crypto/ECKeyTest.javaframework/src/test/java/org/tron/common/crypto/SM2KeyTest.javaframework/src/test/java/org/tron/core/ShieldedTRC20BuilderTest.javaframework/src/test/java/org/tron/core/WalletMockTest.javaframework/src/test/java/org/tron/core/actuator/ShieldedTransferActuatorTest.javaframework/src/test/java/org/tron/core/config/args/ArgsTest.javaframework/src/test/java/org/tron/core/config/args/WitnessInitializerTest.javaframework/src/test/java/org/tron/core/services/RpcApiServicesTest.javaframework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.javaframework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.javaprotocol/src/main/protos/api/api.protosprout-verifying.key
framework/src/main/java/org/tron/core/config/args/WitnessInitializer.java
Outdated
Show resolved
Hide resolved
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 (2)
crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java (2)
334-358:⚠️ Potential issue | 🟠 MajorEnforce exact compact-signature length before key recovery
Line 347 only rejects signatures shorter than 65 bytes. Longer payloads are accepted and trailing bytes are silently ignored at Lines 355–357, which weakens malformed-input hardening.
Suggested 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); + }🤖 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 334 - 358, The method signatureToKeyBytes currently only rejects signatures shorter than 65 bytes but accepts longer inputs and ignores trailing data; update the validation to require signatureEncoded.length == 65 and throw a SignatureException if it's not exactly 65 bytes, so malformed or padded payloads are rejected before calling SM2Signature.fromComponents (referencing signatureToKeyBytes, signatureEncoded and SM2Signature.fromComponents to locate the code).
517-541:⚠️ Potential issue | 🟠 Major
userIDparameter is ignored in both signing and verification pathsLine 517 / Line 533 and Line 745 expose
userID, but current logic hashes/signs/verifies without applying it. This breaks expected behavior for callers using non-default SM2 IDs.Suggested direction
- public SM2Signature signMsg(byte[] msg, `@Nullable` String userID) { + public SM2Signature signMsg(byte[] msg, `@Nullable` String userID) { if (msg == null) { throw new IllegalArgumentException("Expected signature message of " + "SM2 is null"); } - SM2Signer signer = getSigner(); + SM2Signer signer = getSigner(userID); BigInteger[] componets = signer.generateSignature(msg); return new SM2Signature(componets[0], componets[1]); } - public static boolean verifyMessage(byte[] message, byte[] signature, byte[] pub, + public static boolean verifyMessage(byte[] message, byte[] signature, byte[] pub, `@Nullable` String userID) { if (message == null || signature == null || pub == null) { return false; } try { SM2 key = SM2.fromPublicOnly(pub); - byte[] messageHash = key.getSM2SignerForHash().generateSM3Hash(message); + byte[] messageHash = key.getSM2SignerForHash(userID).generateSM3Hash(message); return verify(messageHash, SM2Signature.decodeFromDER(signature), pub); } catch (IllegalArgumentException | SignatureException e) { return false; } }(Implement
getSigner(userID)/getSM2SignerForHash(userID)withParametersWithIDwhereuserID != null.)Also applies to: 745-756
🤖 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 517 - 541, The signMessage and signMsg flows ignore the userID parameter: update signing (signMsg and signMessage) to obtain an SM2Signer that incorporates the userID (e.g., add or use getSigner(userID) / getSM2SignerForHash(userID>) so the signer uses ParametersWithID when userID != null), ensure generateSM3Hash and generateSignature are invoked on that userID-aware SM2Signer, and apply the same fix in the verification path (the methods around the verify logic currently referencing getSigner/SM2Signer) so both signing and verification consistently include ParametersWithID when a non-null userID is provided.
🤖 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/sm2/SM2.java`:
- Around line 334-358: The method signatureToKeyBytes currently only rejects
signatures shorter than 65 bytes but accepts longer inputs and ignores trailing
data; update the validation to require signatureEncoded.length == 65 and throw a
SignatureException if it's not exactly 65 bytes, so malformed or padded payloads
are rejected before calling SM2Signature.fromComponents (referencing
signatureToKeyBytes, signatureEncoded and SM2Signature.fromComponents to locate
the code).
- Around line 517-541: The signMessage and signMsg flows ignore the userID
parameter: update signing (signMsg and signMessage) to obtain an SM2Signer that
incorporates the userID (e.g., add or use getSigner(userID) /
getSM2SignerForHash(userID>) so the signer uses ParametersWithID when userID !=
null), ensure generateSM3Hash and generateSignature are invoked on that
userID-aware SM2Signer, and apply the same fix in the verification path (the
methods around the verify logic currently referencing getSigner/SM2Signer) so
both signing and verification consistently include ParametersWithID when a
non-null userID is provided.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 912282c9-ce91-4499-96bc-1c94dbd3447b
📒 Files selected for processing (5)
crypto/src/main/java/org/tron/common/crypto/sm2/SM2.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/main/java/org/tron/core/services/RpcApiService.java
- crypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.java
Federico2014
force-pushed
the
feature/optimize_crypto
branch
from
2637d0b to
61d6723
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.java (1)
4-4: Unused import:SecureRandomis no longer needed.After switching to
HMacDSAKCalculator, theSecureRandomimport is dead code.🧹 Suggested cleanup
-import java.security.SecureRandom;🤖 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/SM2Signer.java` at line 4, Remove the unused SecureRandom import from SM2Signer.java: locate the import line "import java.security.SecureRandom;" in the SM2Signer class and delete it (the code now uses HMacDSAKCalculator for nonce generation so SecureRandom is unused); ensure no other code in SM2Signer references SecureRandom and run a quick build to confirm no other usages remain.
🤖 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/sm2/SM2Signer.java`:
- Line 4: Remove the unused SecureRandom import from SM2Signer.java: locate the
import line "import java.security.SecureRandom;" in the SM2Signer class and
delete it (the code now uses HMacDSAKCalculator for nonce generation so
SecureRandom is unused); ensure no other code in SM2Signer references
SecureRandom and run a quick build to confirm no other usages remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36723aa9-f8a0-4d54-a1fb-f8b43dc542a2
📒 Files selected for processing (6)
crypto/src/main/java/org/tron/common/crypto/sm2/SM2.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.javaframework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.java
✅ Files skipped from review due to trivial changes (1)
- framework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java
Federico2014
force-pushed
the
feature/optimize_crypto
branch
from
61d6723 to
0379022
Compare
Owner
Author
|
Regarding the two outside-diff findings from the latest review: 1. SM2 2. 3. Unused |
Federico2014
force-pushed
the
feature/optimize_crypto
branch
from
0379022 to
23625fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crypto/src/main/java/org/tron/common/crypto/sm2/SM2.java`:
- Around line 681-689: The verify(byte[] data, SM2Signature signature, byte[]
pub) method currently only checks for non-empty data but should enforce a
32-byte hash like the signing path; update verify to validate that data.length
== 32 and throw an IllegalArgumentException with a clear message if not, so
callers (and the ByteArray wrapper) see malformed hashes fail fast; keep the
rest of the method (SM2Signer init, ECPublicKeyParameters creation) unchanged
and reference the verify(...) method and SM2Signer initialization when making
the change.
- Around line 299-301: The publicKeyFromPrivate method must validate the scalar
before deriving the EC point: ensure privKey is strictly > 0 and strictly < the
curve order (use eccParam.getN() or the order accessor) and throw an
IllegalArgumentException for invalid values; after multiplying
(eccParam.getG().multiply(privKey)) check point.isInfinity() and throw if true
to avoid returning an invalid/aliasing public key, otherwise return
point.getEncoded(compressed). Reference publicKeyFromPrivate,
eccParam.getG().multiply(privKey), eccParam.getN(), and
point.getEncoded(compressed).
- Around line 583-591: The added strict check(...) calls in SM2.java convert
malformed recovery inputs into unchecked exceptions; revert this behavior so
recoverAddressFromSignature, recoverFromSignature, and signatureToKeyBytes keep
the original null/SignatureException failure path: replace the check(...)
assertions for recId, sig.r, sig.s, and messageHash with conditional validations
that, on invalid input, either return null (for the recovery methods) or throw a
SignatureException where the API previously did so, preserving the previous
control flow and not propagating IllegalArgumentException.
- Around line 286-287: The fromPublicOnly factory currently constructs SM2 by
decoding the point directly (new SM2((PrivateKey) null,
eccParam.getCurve().decodePoint(pub))) which bypasses the new public-key
validation in SM2(byte[], false); change fromPublicOnly to route through the
hardened public-key path by calling the SM2 constructor/factory that accepts the
raw public byte array with validation (e.g., new SM2(pub, false) or the existing
SM2(byte[] pub, boolean validate) path) so malformed inputs hit the explicit
validation instead of provider decode errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1de3be2-1c95-44e1-bd84-b6d9a2a98f46
📒 Files selected for processing (6)
crypto/src/main/java/org/tron/common/crypto/sm2/SM2.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.javaframework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.java
- crypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.java
Federico2014
force-pushed
the
feature/optimize_crypto
branch
from
23625fc to
6211729
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)
framework/src/main/java/org/tron/core/services/RpcApiService.java (1)
722-745:⚠️ Potential issue | 🟠 MajorReturn after
onError(...)in the solidity-node scan handlers.The exception handling path violates the gRPC
StreamObservercontract:onError(Throwable)is terminal and must be the last method called on aStreamObserver. CallingonCompleted()afteronError()is not allowed. The fullnode variants below already handle this correctly by returning afteronError().Suggested fix
} catch (Exception e) { responseObserver.onError(getRunTimeException(e)); + return; } responseObserver.onCompleted();This applies to both
scanShieldedTRC20NotesByIvk(lines 722-745) andscanShieldedTRC20NotesByOvk(lines 748-767).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@framework/src/main/java/org/tron/core/services/RpcApiService.java` around lines 722 - 745, The scan handlers scanShieldedTRC20NotesByIvk and scanShieldedTRC20NotesByOvk violate the StreamObserver contract by calling responseObserver.onCompleted() after responseObserver.onError(...); modify both methods in RpcApiService so that after calling responseObserver.onError(getRunTimeException(e)) you immediately return (same as the fullnode variants) to ensure onError is terminal and onCompleted is not invoked afterwards.
🤖 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 `@framework/src/main/java/org/tron/core/services/RpcApiService.java`:
- Around line 722-745: The scan handlers scanShieldedTRC20NotesByIvk and
scanShieldedTRC20NotesByOvk violate the StreamObserver contract by calling
responseObserver.onCompleted() after responseObserver.onError(...); modify both
methods in RpcApiService so that after calling
responseObserver.onError(getRunTimeException(e)) you immediately return (same as
the fullnode variants) to ensure onError is terminal and onCompleted is not
invoked afterwards.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9a04893-9bbe-4a76-b8bc-e73c96f652bd
📒 Files selected for processing (6)
crypto/src/main/java/org/tron/common/crypto/sm2/SM2.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.javaframework/src/main/java/org/tron/core/config/args/WitnessInitializer.javaframework/src/main/java/org/tron/core/services/RpcApiService.javaframework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.javaframework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.java
✅ Files skipped from review due to trivial changes (1)
- crypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.java
🚧 Files skipped from review as they are similar to previous changes (2)
- framework/src/test/java/org/tron/core/zksnark/MerkleContainerTest.java
- framework/src/test/java/org/tron/core/services/http/CreateSpendAuthSigServletTest.java
Owner
Author
|
Regarding the Valid finding — the missing Will address this in a separate follow-up PR to keep the scope of this one focused. |
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 the security and robustness of the
cryptomodule across five areas:Remove unused
topicsListfrom shielded TRC20 log APIstopicsListfield fromScanShieldedTRC20NotesByIvkandScanShieldedTRC20NotesByOvkAPI responses, as it was never populated and exposed misleading empty data.api.proto,RpcApiService, HTTP servlet handlers, andWalletaccordingly.api.protoremoves thetopicsListfield from shielded TRC20 scan responses. This field was never populated by the server (always returned empty), so existing clients will see no behavioral difference.RpcApiServicesTest.Shielded transaction API security enhancement
Walletfor shielded transaction builders (ZenTransactionBuilder,ShieldedTRC20ParametersBuilder).sprout-verifying.keyremoved from the repository root; the file was not referenced by any code and its presence in source control was unnecessary.allowShieldedTransactionApifromtruetofalseto reduce exposure by default, since some shielded transaction APIs require passing private keys as parameters. Updatedconfig.confwith an explicit warning advising users to only invoke these APIs locally.ShieldedTransferActuatorTestandArgsTestto cover new validation paths.SM2 deterministic signing and robustness (
SM2Signer.java)RandomDSAKCalculatorwithHMacDSAKCalculatorfor deterministic nonce generation (RFC 6979 equivalent), eliminating nonce-reuse risk from the random K path.init,generateSM3Hash,generateHashSignature,verifySignature,verifyHashSignature,calculateE).SM2KeyTestwith additional edge-input cases.ECKey input validation (
ECKey.java)fromPrivate,fromPublicOnly, and recovery APIs, preventing silent truncation of oversized key material.ECKeyTestwith negative test cases for invalid inputs.Signature and private key validation (
Rsv.java,SignUtils.java,WitnessInitializer.java)r,s,vcomponents inRsv.SignUtilsbefore signing.WitnessInitializerto reject malformed private keys on node startup.Upgraders should review the following user-visible behavior changes:
ECKey.fromPrivate(byte[])/SM2.fromPrivate(byte[])— null/empty input now throws instead of returning nullnullor empty byte array returnednullsilently.IllegalArgumentException.nullreturn for invalid input must add a pre-check or catch the exception. All internal callers and tests have been updated.allowShieldedTransactionApidefault changed fromtruetofalseallowShieldedTransactionApi = trueinconfig.confif needed.scanShieldedTRC20NotesByIvk/Ovk— deprecatedeventsfield now rejectedeventsfield was silently accepted and ignored.eventsfield returnINVALID_ARGUMENTerror (gRPC) or HTTP 400.Why are these changes required?
The existing crypto code lacked defensive validation at API and library boundaries, creating exposure to:
r,s,v)These issues were identified during an internal security review of the
cryptomodule.This PR has been tested by:
ECKeyTest,SM2KeyTest,WalletMockTest,RpcApiServicesTest,ShieldedTransferActuatorTest,ArgsTest,WitnessInitializerTest./gradlew clean build -x test)Extra details
verifyMessage(byte[], byte[], byte[])andverifyMessage(byte[], byte[], byte[], String)fromSM2.java— no callers exist in the codebase. This also eliminates auserIDparameter regression where the refactored implementation silently ignoreduserID, diverging from the original behavior that passed it toSM2Signer.verifySignature().fromPrivateAndPrecalculatedPublic(),fromPublicOnly(ECPoint), andtoStringWithPrivate()fromSM2.java— no callers exist. SM2 is an internal module; these removals do not affect any external API surface.Summary by CodeRabbit
Release Notes
Bug Fixes
Deprecations
eventsfield in shielded TRC20 note scanning APIs; requests including this field are now rejected.Security