Skip to content

PoS: bitmap bounds-check bugs in multi_bls.rs #3513

Description

@peilun-conflux

Found during review of #3505. Four bounds-check bugs in crates/pos/crypto/crypto/src/multi_bls.rs. Filing rather than patching directly: none of these is currently exploitable, and correcting the verification-path ones is a consensus-rule change that needs a coordinated rollout.

The issue

1. bitmap_last_set_bit returns Option<u8> and truncates (multi_bls.rs:521). The bitmap is [u8; 40] so the last-set-bit index can be 0–319, but the result is cast as u8, wrapping any index ≥ 256. The truncated value is only used in <= / >= guards (in get_signers and verify_arbitrary_msg), never to index, so there is no panic — but the guards are weakened. Return type should be Option<usize>.

2. verify_arbitrary_msg uses <= instead of < (multi_bls.rs:448). Some(last_bit) if last_bit as usize <= public_keys().len() accepts last_bit == len(), one past the last valid index. get_signers's sibling check correctly uses >= len to reject.

3. verify_arbitrary_msg is missing the bitmap-capacity guard that get_signers has (multi_bls.rs:463-464). Its loop for i in 0..public_keys().len() calls bitmap_get_bit(bitmap, i), which indexes [u8; 40] at i / 8 — a panic if a committee ever exceeds 320. get_signers guards exactly this at line 350; verify_arbitrary_msg does not.

4. Stale comments on bitmap_set_bit / bitmap_get_bit (multi_bls.rs:505, :514). Both say "always invoked with index < 32, thus there is no need to check range" — the real bound is MAX_NUM_OF_KEYS = 300 < BITMAP_NUM_OF_BYTES * 8 = 320.

Why this is not a security issue

The bitmap guards weakened by #1 and #2 are redundant with the cryptographic check. verify_arbitrary_msg collects pubkeys only for set bits in 0..committee_len and passes them to bls_signatures::verify_same_message, which rejects an empty key set (signature.rs:218, active blst build) and rejects identity public keys. A signature with only out-of-range bits therefore yields an empty key set and is rejected at the crypto layer regardless of the guard — no invalid signature can pass.

The crypto also binds the aggregate to exactly the in-range claimed pubkeys (e(Σ pk, H(m)) == e(g1, S)), so the claimed signer set is always a subset of validators who actually signed — no vote inflation. check_voting_power in validate_pivot_decision then sums per-address over the same in-range set.

#3 is currently unreachable: the validator committee is bounded ≤ 300 by design (TERM_LIST_LEN 6 × TERM_ELECTED_SIZE 50 = MAX_NUM_OF_KEYS), and the 320-bit bitmap is sized with deliberate slack above that.

The only behavioural gap is that a cryptographically valid signature carrying spurious out-of-range bitmap bits is currently accepted with those bits ignored.

What fixing it changes

#1 and #2 change which MultiBLSSignature bitmaps are accepted: a cryptographically valid signature carrying spurious out-of-range bits, accepted today, would be rejected. Old and fixed nodes can therefore disagree on such an object — a consensus-rule tightening that must ship as a coordinated upgrade, not a routine patch.

#3 is non-behavior-changing while the committee stays ≤ 320 (turns a would-be panic into a clean Err if that bound is ever exceeded by misconfiguration).

#4 is comment-only.

Suggested resolution

Bundle #1#3 into the next PoS hardfork. #4 can land anytime.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions