Skip to content

feat(crypto): harden ECKey validation - #6996

Open
Federico2014 wants to merge 11 commits into
tronprotocol:release_v4.8.3from
Federico2014:feature/ec-key-validation-hardening
Open

Federico2014 wants to merge 11 commits into
tronprotocol:release_v4.8.3from
Federico2014:feature/ec-key-validation-hardening

Conversation

@Federico2014

@Federico2014 Federico2014 commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator

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.

  • Validates private-key size and scalar range (1 <= d < n) before curve operations. Rejects null, empty, zero, out-of-range, and oversized encodings, while accepting the single leading sign byte used by BigInteger for valid high-bit scalars.
  • Validates public-key encoding, secp256k1 curve membership, point validity, and non-infinity state; rejects mismatched private/public key pairs.
  • Removes provider-based constructors, unchecked key-pair factories, and private-key string output. Uses public-key identity for equality and returns defensive copies of cached addresses and node IDs.
  • Gives TronCastleProvider its own Bouncy Castle instance. ECKey and Hash use that instance directly, without trusting or modifying an existing globally registered provider named BC.
  • Rejects invalid configured witness private-key scalars with WITNESS_INIT. Wraps invalid decrypted keystore keys in CipherException. A finally block covers all code after key derivation and clears derivedKey plus any allocated encryptKey and privateKey arrays 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:

  • Isolated JDK 8 compilation of the current crypto sources and JUnit execution of 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.
  • Standalone Checkstyle 8.7 passed for WalletAddressValidationTest; git diff --check passed.
  • Gradle validation remains blocked by offline dependency resolution. Full-node manual testing was not performed.

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:checkstyleTest

Additional 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 ECKey objects 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. LocalWitnesses applies the secp256k1 scalar check before witness initialization.

Compatibility impact

  • Source and binary API changes: provider-based ECKey constructors, fromPrivateAndPrecalculatedPublic, and toStringWithPrivate are removed. Use ECKey(SecureRandom) for generation and the validated ECKey(BigInteger, ECPoint) constructor for supplied pairs; private-key string output has no replacement.
  • Key behavior: null/empty private-key input now throws IllegalArgumentException instead of returning null; 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 as CipherException.
  • Provider behavior: callers needing TRON digest mappings should use TronCastleProvider.getInstance() explicitly; an already registered global BC provider 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.

@github-actions
github-actions Bot requested a review from 3for September 23, 2026 10:22
@Federico2014 Federico2014 linked an issue Sep 23, 2026 that may be closed by this pull request
@halibobo1205 halibobo1205 added this to the GreatVoyage-v4.8.3 milestone Sep 23, 2026
Comment thread crypto/src/main/java/org/tron/common/crypto/ECKey.java
Comment thread crypto/src/main/java/org/tron/common/crypto/ECKey.java
@3for

3for commented Sep 28, 2026

Copy link
Copy Markdown
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 3for left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@github-actions
github-actions Bot requested a review from 3for October 11, 2026 03:25
Comment thread crypto/src/main/java/org/tron/common/crypto/cryptohash/Digest.java Outdated
Comment thread crypto/src/main/java/org/tron/keystore/Wallet.java Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

[Feature] Validate ECKey inputs and improve key handling

5 participants