Repository navigation
fix: reject undersized legacy BLS messages - #7832
PastaPastaPasta wants to merge 1 commit into
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 42 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 6af5c33) · triage: normal |
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the complete diff at 6af5c33 and found no actionable in-scope defects. The native guards reject undersized legacy messages before mapping, the affected C signing exports contain exceptions, and Go/Rust wrappers handle null results while preserving longer-message semantics and Core's fixed-width hash paths. This was static verification only: the supplied CI snapshot shows formatting and preliminary checks passing, four container builds queued, and conflict prediction cancelled; build and test results remain unconfirmed by that snapshot.
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: dash-core-commit-history); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
normalbygpt-6.1-sol(effort low) — The diff changes legacy BLS signing and verification boundaries and propagates failures through Rust and Go bindings, but the length guards and error handling are sufficiently contained that it does not meet the large-or-intricate requirement for critical. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort high); agentphase2-reviewer,gpt-6.1-sol— dash-core-commit-history (completed, effort high); agentphase2-reviewer
|
Moved this library-only change to dashpay/bls-signatures#136, following the vendor-ownership review on #7813 (review). The 13 original library files match the reviewed patch byte-for-byte. Upstream integration review also caught the shipped Rust example signing a 10-byte legacy message; it now signs and verifies the SHA256 digest of the same message, with key derivation unchanged. The upstream native suite (1,661 assertions), actual Go tests/vet and legacy C-dispatch harness, all 24 safe Rust tests, and the shipped example pass. Independent transfer/integration review approved the final adaptation. Closing this direct vendor-edit PR as superseded. Dash should receive the accepted upstream change through its normal vendor import. The upstream PR is linked and watched here. 🤖 Posted autonomously by Codex on behalf of pasta. |
Issue being fixed or feature implemented
Sized legacy BLS signing and verification APIs ignore caller message lengths below 32 and still map 32 bytes. Safe Rust forwards slice lengths to these APIs, so short allocations can violate the native read contract. Longer legacy messages already use only their first 32 bytes. Core supplies fixed-width
uint256hashes on the inspected paths; no short-message daemon input path is established.What was done?
Reject declared legacy extents below 32 before native signing/mapping, and return false from supported legacy verification paths before mapping any short message. Preserve the existing first-32 mapping for longer inputs, Basic/Aug arbitrary messages, aggregate empty/count behavior, and
AggregateSecureignoring its message.Contain the new native signing exception in both generic and legacy Rust C exports and the generic Go C dispatcher. Check nullable signing results before constructing managed elements. Rust adds recoverable
LegacySchemeMPL::try_sign; the existing traitsignsafely panics on failure. GoSignreturns nil and retains key/scheme lifetimes on that path. Document these contracts.Add native and Rust public-boundary regressions. Existing Rust aggregate and native HD signing fixtures now supply legitimate 32-byte legacy messages; HD key derivation retains its original seeds and derives the signing digest with SHA256.
How Has This Been Tested?
On Apple Silicon/macOS, using the existing Dash depends prefix and one build job:
make -C src/dashbls -j1,runtest Schemes(306 assertions), and corrected fullruntest(1,661 assertions across 17 cases) passed, including existing serialized legacy vectors.go test -p 1 -count=1 ./...andgo vet -p 1 ./...passed. A separate C++ harness compiled the actual Go C dispatcher and verified native Legacy dispatch returns null for declared lengths 0..31 and preserves results at 32/33/64. Go exposes no public legacy factory; wrapper nil handling was inspected.make -j1, Corebls_tests, whitespace lint, Go/Rust formatting and diff checks passed.The first full native run identified two pre-existing HD fixtures that signed short derivation seeds; those fixtures were corrected without changing key derivation. The first standalone harness link omitted the existing
utils.cpp; the corrected harness passed. The removed Rust secure-verification assertion used an invalid different-message fixture; the native regression supplies a valid same-message secure aggregate with positive length controls.No sanitizer read-overrun result, secret disclosure, deployed consumer reachability, or daemon exploit is claimed. The pre-existing binding error-channel concurrency defect is handled separately in #7825.
Breaking Changes
Unsupported short legacy messages now throw from native signing/mapping, return false from supported native verification, and return null from the affected C signing exports. Rust callers can use
try_signto recover; the existingsignsignature is retained with a documented panic. Exported C signatures remain unchanged. Existing messages of at least 32 bytes, historical Core fixed-width hashes, and nonlegacy message handling retain their behavior.Checklist:
This pull request was created by Codex.