Repository navigation
feat(crypto): harden ECKey validation - #6996
Open
Federico2014 wants to merge 11 commits into
Open
Federico2014 wants to merge 11 commits into
Federico2014 wants to merge 11 commits into
Conversation
3for
reviewed
Sep 28, 2026
3for
reviewed
Sep 28, 2026
Collaborator
|
[DISCUSSION] Currently, wallet-cli maintains a separate copy of the crypto source code and has similar issues. Could we consider having wallet-cli reuse the crypto module from this repository to avoid maintaining two separate copies of essentially the same code? |
…ing' into feature/ec-key-validation-hardening # Conflicts: # framework/src/test/java/org/tron/core/config/args/WitnessInitializerKeystoreTest.java
3for
reviewed
Oct 12, 2026
3for
reviewed
Oct 12, 2026
3for
approved these changes
Oct 12, 2026
bladehan1
approved these changes
Oct 12, 2026
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
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 EC key and keystore handling and isolates the cryptographic provider. The Keccak/Digest fixes are tracked separately in #7018.
1 <= d < n) before curve operations. Rejects null, empty, zero, out-of-range, and oversized encodings, while accepting the single leading sign byte used byBigIntegerfor valid high-bit scalars.TronCastleProviderits own Bouncy Castle instance.ECKeyandHashuse that instance directly, without trusting or modifying an existing globally registered provider namedBC.WITNESS_INIT. Wraps invalid decrypted keystore keys inCipherException. Afinallyblock covers all code after key derivation and clearsderivedKeyplus any allocatedencryptKeyandprivateKeyarrays on successful return and exception paths, including incorrect password, decryption failure, invalid private key, and address mismatch. This is best-effort cleanup of temporary arrays.Why are these changes required?
Previously, malformed or inconsistent key material could reach curve operations, callers could modify cached identity arrays, and provider selection depended on the global provider registry. Invalid keystore keys could escape as unchecked exceptions, and temporary key material could remain in arrays after the method returned or failed.
This PR has been tested by:
Latest wallet cleanup at
c7f0fea0c1, after removing the Keccak/Digest changes:WalletAddressValidationTest: 7 tests passed. Extends the existing successful-decryption test to verify derived-key and raw-private-key cleanup and that the returned key remains usable; adds one incorrect-password cleanup test.WalletAddressValidationTest;git diff --checkpassed.Earlier validation recorded in this PR, not rerun against the latest head:
./gradlew :framework:test --tests org.tron.common.crypto.ECKeyTest./gradlew :framework:test --tests org.tron.keystore.WalletAddressValidationTest --tests org.tron.core.config.args.WitnessInitializerKeystoreTest./gradlew :framework:checkstyleMain :framework:checkstyleTestAdditional regression coverage includes provider isolation and witness initialization failures. The isolated wallet results do not establish that the complete PR passes the framework or full-node test suite.
Follow up
Run the changed crypto/framework tests, Checkstyle, and the full test suite on the latest head before merging.
Extra details
Consensus impact
No consensus rule, protocol format, or fork activation is introduced. ECDSA signing/recovery arithmetic, low-S normalization, and the Keccak permutation/padding are unchanged. Transaction, block, and TVM signature-to-address paths use the existing recovery-byte path; stricter construction of recovered
ECKeyobjects affects separate object-returning APIs.These checks do not constitute historical-chain replay or mixed-version consensus testing.
Invalid local witness configuration can now fail earlier during initialization.
LocalWitnessesapplies the secp256k1 scalar check before witness initialization.Compatibility impact
ECKeyconstructors,fromPrivateAndPrecalculatedPublic, andtoStringWithPrivateare removed. UseECKey(SecureRandom)for generation and the validatedECKey(BigInteger, ECPoint)constructor for supplied pairs; private-key string output has no replacement.IllegalArgumentExceptioninstead of returningnull; invalid public keys and mismatched pairs are rejected. Equality compares public-key identity, and modifying returned address/node-ID arrays no longer changes cached values. Invalid decrypted keys are reported asCipherException.TronCastleProvider.getInstance()explicitly; an already registered globalBCprovider is no longer augmented with those mappings.Downstream users of removed APIs must update and recompile. No database migration or configuration-schema change is introduced, but invalid witness-key configuration may be rejected at startup.