Secp256k1 Curve-Order Validation Bug

[ckeshava]

Vulnerability Overview

• Repository: XRPLF/xrpl-rust
• Type: Cryptographic Weakness
• Severity: LOW
• Confidence: HIGH
• Location: src/core/keypairs/algorithms.rs (lines 70–74)

Description

An off-by-one check accepts the secp256k1 curve order n as a valid private key. If that exact value is derived, keypair creation fails when SecretKey::from_slice rejects it, causing wallet generation or import to abort instead of retrying.

Detailed Analysis

Code Context

Wallet::new and other secp256k1 key-derivation paths call derive_keypair(seed, false) after decoding the seed. The secp256k1 implementation hashes the seed and sequence bytes with SHA-512Half, then checks whether the 32-byte candidate is a valid private key.

Vulnerability Details

The code should accept secp256k1 private keys only in the range [1, n-1]. Instead, _is_secret_valid uses <= n, which incorrectly treats the curve order itself as valid. That invalid scalar is later rejected by SecretKey::from_slice, breaking derivation.

Impact

Affected wallet creation or key import can fail for one derivation attempt. The issue does not reveal secrets, change signatures, or impact other accounts, but it can block a legitimate wallet setup flow if triggered.

Exploit Scenario

A user or caller invokes Wallet::new(seed, sequence) or another secp256k1 derivation API with a seed whose SHA-512Half candidate equals the curve order exactly. _is_secret_valid accepts it, SecretKey::from_slice rejects it, and wallet/keypair derivation fails.

Reachability Evidence

Verdict: REACHABLE
Summary: The code hashes secp256k1 candidates with SHA-512Half and only misclassifies one extra scalar as valid: the curve order itself. Reaching the bug therefore requires an exact 256-bit collision with that constant, making practical exploitation computationally infeasible.

Condition Assessments

MET: A SHA-512Half output over the key candidate inputs must collide exactly with the secp256k1 curve order, which has probability approximately 1/2^256 and is computationally infeasible in practice.

The secp256k1 derivation path uses sha512_first_half to generate a 32-byte candidate secret from the input and sequence bytes (src/core/keypairs/utils.rs:58-64, src/core/keypairs/algorithms.rs:132-136). That candidate is accepted only if _is_secret_valid returns true. _is_secret_valid converts the 32-byte value to U256 and checks key_bytes >= U256::ONE && key_bytes <= U256::from_be_bytes(secp256k1::constants::CURVE_ORDER) (src/core/keypairs/algorithms.rs:70-73). Because valid secp256k1 private keys should be in [1, n-1], the only extra value admitted by this off-by-one check is exactly n, the curve order itself. Values 0 or > n are still rejected by the code. Therefore this bug is reachable only when a SHA-512Half result equals that single fixed 256-bit constant exactly. Under normal cryptographic assumptions for SHA-512, that is approximately a 1-in-2^256 event per candidate and is computationally infeasible in practice; the loop over SECP256K1_SEQUENCE_MAX does not change that practical conclusion because it merely tests additional hashes and usually exits on the first in-range candidate.

src/core/keypairs/algorithms.rs:70-74

    fn _is_secret_valid(key: [u8; u32::BITS as usize]) -> bool {
        let key_bytes = U256::from_be_bytes(key);
        key_bytes >= U256::ONE
            && key_bytes <= U256::from_be_bytes(secp256k1::constants::CURVE_ORDER)
    }

[satyakwok]

Fix submitted in #302.

One-character change: <= U256::from_be_bytes(CURVE_ORDER) → < U256::from_be_bytes(CURVE_ORDER). Boundary test covers 0 / 1 / n / n-1.