Skip to content

fix(crypto): harden crypto module security and robustness#6652

Closed
Federico2014 wants to merge 5 commits intotronprotocol:developfrom
Federico2014:feature/optimize_crypto
Closed

fix(crypto): harden crypto module security and robustness#6652
Federico2014 wants to merge 5 commits intotronprotocol:developfrom
Federico2014:feature/optimize_crypto

Conversation

@Federico2014
Copy link
Copy Markdown
Collaborator

What does this PR do?

This PR hardens the security and robustness of the crypto module across five areas:

  1. Remove unused topicsList from shielded TRC20 log APIs

    • Removed the topicsList field from ScanShieldedTRC20NotesByIvk and
      ScanShieldedTRC20NotesByOvk API responses, as it was never populated
      and exposed misleading empty data.
    • Updated api.proto, RpcApiService, HTTP servlet handlers, and Wallet
      accordingly.
    • api.proto removes the topicsList field from shielded TRC20 scan
      responses. This field was never populated by the server (always returned
      empty), so existing clients will see no behavioral difference.
    • Added full gRPC service test coverage in RpcApiServicesTest.
  2. Shielded transaction API security enhancement

    • Strengthened parameter validation in Wallet for shielded transaction
      builders (ZenTransactionBuilder, ShieldedTRC20ParametersBuilder).
    • sprout-verifying.key removed from the repository root; the file was
      not referenced by any code and its presence in source control was
      unnecessary.
    • Changed the default value of allowShieldedTransactionApi from true
      to false to reduce exposure by default, since some shielded transaction
      APIs require passing private keys as parameters. Updated config.conf
      with an explicit warning advising users to only invoke these APIs locally.
    • Added ShieldedTransferActuatorTest and ArgsTest to cover new
      validation paths.
  3. SM2 deterministic signing and robustness (SM2Signer.java)

    • Replaced RandomDSAKCalculator with HMacDSAKCalculator for
      deterministic nonce generation (RFC 6979 equivalent), eliminating
      nonce-reuse risk from the random K path.
    • Added null/empty guards on all public entry points (init,
      generateSM3Hash, generateHashSignature, verifySignature,
      verifyHashSignature, calculateE).
    • Fixed misleading error message that referred to "ECDSA" instead of "SM2".
    • Extended SM2KeyTest with additional edge-input cases.
  4. ECKey input validation (ECKey.java)

    • Added strict null and length checks on fromPrivate, fromPublicOnly,
      and recovery APIs, preventing silent truncation of oversized key material.
    • Expanded ECKeyTest with negative test cases for invalid inputs.
  5. Signature and private key validation (Rsv.java, SignUtils.java,
    WitnessInitializer.java)

    • Added range checks for r, s, v components in Rsv.
    • Enforced key material validity in SignUtils before signing.
    • Hardened WitnessInitializer to reject malformed private keys on node
      startup.

Why are these changes required?

The existing crypto code lacked defensive validation at API and library
boundaries, creating exposure to:

  • Nonce reuse in SM2 (non-deterministic random K path)
  • Silent acceptance of invalid or truncated key material in ECKey
  • Missing range checks on signature components (r, s, v)
  • Information leakage through unused protobuf fields in shielded APIs
  • Risk of private key leakage when shielded transaction APIs are enabled
    by default on public-facing nodes

These issues were identified during an internal security review of the
crypto module.

This PR has been tested by:

  • Unit Tests — ECKeyTest, SM2KeyTest, WalletMockTest,
    RpcApiServicesTest, ShieldedTransferActuatorTest, ArgsTest,
    WitnessInitializerTest
  • Build passes (./gradlew clean build -x test)
  • Manual testing on private testnet

Extra details

  • Breaking change: nodes that relied on allowShieldedTransactionApi
    defaulting to true must now explicitly set it in config.conf.
  • No changes to consensus logic or on-chain state transitions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant