refactor(crypto): enhance SM2 security and simplify signing model#12
refactor(crypto): enhance SM2 security and simplify signing model#12Federico2014 wants to merge 1 commit intodevelopfrom
Conversation
📝 WalkthroughWalkthroughThe PR refactors the SM2 elliptic curve cryptography implementation with stricter input validation, removal of message-level signing APIs (moving hash responsibility to callers), deterministic signature generation via HMAC-based k derivation, and improved public key recovery validation. Field naming is standardized, defensive copying added for sensitive data, and DER signature decoding is rewritten using ASN.1 primitives. Changes
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 679-686: The verify(byte[] hash, byte[] signature, byte[] pub)
method currently returns false when any argument is null; change this to treat
nulls as programmer errors by replacing the initial null-check and false-return
with the codebase's precondition check (call check(...) or the project's
null-assert helper) for hash, signature, and pub so a thrown
assertion/IllegalArgumentException surfaces instead of a silent false; keep the
try block that calls SM2Signature.decodeFromDER(signature) and the existing
catch for decoding/signature errors so only decoding/signature failures still
return false while null-argument bugs throw.
- Around line 269-280: The isValidPrivateKey method currently allows
non-canonical leading-zero padding; update SM2.isValidPrivateKey to reject any
multi-byte array that begins with 0x00 (i.e., if keyBytes.length > 1 &&
keyBytes[0] == 0x00 return false) so redundant leading-zero prefixes are not
accepted for any length; keep the empty check and the numeric-range check
against SM2_N using new BigInteger(1, keyBytes) to validate 1 <= key < SM2_N.
In `@crypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.java`:
- Around line 34-39: The init(CipherParameters param) method must validate param
and its runtime type before casting: check param != null and that it is an
instance of ECKeyParameters (use the project's check() pattern) and initialize
ecKey and ecParams only after those checks; in generateHashSignature() validate
that ecKey is non-null and an ECPrivateKeyParameters instance and ecParams is
non-null before casting/using them (throw a clear IllegalArgumentException via
check() if not); likewise in verifyHashSignature() validate ecKey is non-null
and an ECPublicKeyParameters instance and ecParams is initialized before
casting/using; ensure all error messages identify the expected type
(ECPrivateKeyParameters/ECPublicKeyParameters) and the offending 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: f29d7f74-8d7d-4aa7-bb1a-8e0be8b2d7e6
📒 Files selected for processing (4)
crypto/src/main/java/org/tron/common/crypto/sm2/SM2.javacrypto/src/main/java/org/tron/common/crypto/sm2/SM2Signer.javaframework/src/test/java/org/tron/common/crypto/SM2KeyTest.javaframework/src/test/java/org/tron/common/utils/PublicMethod.java
💤 Files with no reviewable changes (1)
- framework/src/test/java/org/tron/common/utils/PublicMethod.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Federico2014
changed the title
Federico2014
changed the title
Federico2014
changed the title
Federico2014
left a comment
Federico2014
left a comment
There was a problem hiding this comment.
Thanks for the review. Responses to each comment:
-
isValidPrivateKeyaccepts shorter zero-padded encodings: By design.{0x00, 0x01}represents the value1, which is a valid private key.ECKey.isValidPrivateKeyuses the exact same logic — only rejecting 33-byte arrays with non-zero leading byte (BigInteger sign-byte padding). Keeping SM2 and ECKey consistent. Won't fix. -
verify()returns false on null instead of throwing: This is a verification utility — returningfalsefor null input is semantically consistent with "verification failed". The method already catchesIllegalArgumentException | SignatureExceptionand returnsfalse, so null handling follows the same pattern. Changing to throw would be an API behavior change. Won't fix. -
SM2Signer.init()lacks instanceof type checks:init()already validates null. A wrong type will throwClassCastExceptionwith a clear stack trace.SM2Signeris an internal class only called bySM2.sign()andSM2.getVerifier(), which always pass the correct type. Adding instanceof guards on internal classes adds code bloat without practical benefit. Won't fix.
1 participant
What does this PR do?
Hardens and simplifies the SM2 cryptographic implementation to align with how ECDSA is used on TRON.
RandomDSAKCalculatorwithHMacDSAKCalculator(RFC 6979 equivalent), eliminating nonce-reuse riskisValidPrivateKey(byte[])andisValidPrivateKey(BigInteger), enforcing[1, n)range check on all key construction paths (SM2(byte[], boolean),fromPrivate(BigInteger),fromPrivate(byte[]),privateKeyFromBigInteger(),publicKeyFromPrivate())recoverPubBytesFromSignature()to enforcer ∈ (0, n),s ∈ (0, n),recId ∈ [0, 3], andmessageHash.length == 32sign(),verify(),signatureToKeyBytes(),fromNodeId(),fromPublicOnly()Z_Apre-hash step to keep SM2 and ECDSA interchangeable at theSignInterfaceleveluserIDchain (userIDfield,ParametersWithIDhandling,addUserID(),forSigningparameter), message-level signing path (generateSignature,generateSM3Hash,getZ,addFieldElement,verifySignature,verifyMessage), and unused methods (hash(),signMessage(),signMsg(),fromPrivateAndPrecalculatedPublic())getSM2SignerForHash()→getVerifier()SM2andSM2Signerstating the non-standard design and non-interoperability with GB/T 32918Why are these changes required?
r >= 0ands >= 0is insufficient — zero or out-of-range values cause incorrect public key recoveryuserIDcode path acceptedParametersWithIDbut silently ignored them (addUserIDwas commented out), creating a false sense of GB/T 32918 complianceSM2Signer.init()signature changed —forSigningparameter removedIllegalArgumentExceptioninstead of silently proceedinghash(),signMessage(),signMsg(),generateSM3Hash(),generateSignature(),verifySignature())SM2.fromPrivateAndPrecalculatedPublic()removedgetSM2SignerForHash()renamed togetVerifier()This PR has been tested by:
SM2KeyTest./gradlew clean build -x test)Follow up
SM2 has never been enabled on the TRON network — all production nodes use ECKey (SECP256K1) exclusively. The
isECKeyCryptoEngineflag is alwaystrueat runtime. Therefore, these changes have zero impact on the consensus mechanism and do not affect any on-chain transaction processing or block validation.Extra details
Split from #7
Closes #18