Repository navigation
fix: validate threshold identifier lengths - #134
PastaPastaPasta wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
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 6ef9acf) · triage: low |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
The native guards and all three safe Rust recovery guards reject short identifiers before fixed-width reads or FFI calls, while preserving longer-identifier interpretation, scalar reduction, and existing recovery checks. No blocking defects were found; one nonblocking naming nitpick remains. Validation was static only: git diff --check passed, and the supplied exact-head CI snapshot reports successful native and binding checks, with publication jobs skipped.
💬 1 nitpick(s)
Review provenance
Source: reviewer 1: glm-5.3-flash (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: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
lowbygpt-6.1-sol(effort low) — The diff adds small, well-contained identifier-length guards and contract documentation with focused tests, without changing cryptographic algorithms, valid-input interpretation, or public C signatures. - Phase 1 reviewers:
glm-5.3-flash— general (completed, effort high); agentphase1-reviewer - Phase 1 model:
glm-5.3-flash— zai quota: 5h 89% left, weekly 94% left; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left) - Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort medium); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort medium); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `rust-bindings/bls-signatures/src/utils.rs`:
- [NITPICK] rust-bindings/bls-signatures/src/utils.rs:11-18: Name the threshold identifier size constant
The literal 32 represents the fixed-width identifier contract shared with native Poly::nIdSize and the C header's HashSize. Naming it THRESHOLD_ID_SIZE would make that contract explicit and match the Rust crate's existing G1_ELEMENT_SIZE, G2_ELEMENT_SIZE, and PRIVATE_KEY_SIZE conventions. The current validation and error message are correct; this is a readability improvement, not a correctness or security defect.
| pub(crate) fn check_threshold_ids<T>(shares: &[(Vec<u8>, T)]) -> Result<(), BlsError> { | ||
| if shares.iter().any(|(id, _)| id.len() < 32) { | ||
| return Err(BlsError { | ||
| msg: "Threshold identifier must contain at least 32 bytes".to_owned(), | ||
| }); | ||
| } | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
💬 Nitpick: Name the threshold identifier size constant
The literal 32 represents the fixed-width identifier contract shared with native Poly::nIdSize and the C header's HashSize. Naming it THRESHOLD_ID_SIZE would make that contract explicit and match the Rust crate's existing G1_ELEMENT_SIZE, G2_ELEMENT_SIZE, and PRIVATE_KEY_SIZE conventions. The current validation and error message are correct; this is a readability improvement, not a correctness or security defect.
| pub(crate) fn check_threshold_ids<T>(shares: &[(Vec<u8>, T)]) -> Result<(), BlsError> { | |
| if shares.iter().any(|(id, _)| id.len() < 32) { | |
| return Err(BlsError { | |
| msg: "Threshold identifier must contain at least 32 bytes".to_owned(), | |
| }); | |
| } | |
| Ok(()) | |
| } | |
| pub(crate) const THRESHOLD_ID_SIZE: usize = 32; | |
| pub(crate) fn check_threshold_ids<T>(shares: &[(Vec<u8>, T)]) -> Result<(), BlsError> { | |
| if shares.iter().any(|(id, _)| id.len() < THRESHOLD_ID_SIZE) { | |
| return Err(BlsError { | |
| msg: "Threshold identifier must contain at least 32 bytes".to_owned(), | |
| }); | |
| } | |
| Ok(()) | |
| } |
source: glm-5.3-flash (phase1-reviewer: general)
|
Check if not needed when #132 is merged |
Native threshold sharing and recovery read the first 32 identifier bytes without validating the declared length. Safe Rust recovery also forwards arbitrary identifier slices to a raw C API without per-identifier lengths. Reject short identifiers before either boundary; retain the existing first-32-byte interpretation of longer identifiers, scalar reduction, and zero/duplicate recovery checks.
This moves the library-only fix from dashpay/dash#7813 into its owning repository following the maintainer review. All eight changed files are byte-identical to that reviewed patch after removing the vendor path prefix. The base is
developatdbd61e6, the library revision currently imported by Dash Core;mainstill contains the older 1.3.6 release.The patch documents native/Rust contracts and the raw C caller's requirement to supply 32 readable bytes, without changing public C signatures. Native tests cover all sharing/recovery types and identifier positions. Rust tests cover every safe recovery type, short inputs, legitimate 32/33-byte inputs, and recovered key/signature relationships.
Validation on Apple Silicon macOS:
runtest: 1,455 assertions in 17 cases passed.git diff --checkpassed; independent relocation/integration review approved.Unsupported identifiers shorter than 32 bytes now return ordinary errors. Valid identifiers and historical fixed-width Dash identifiers retain their behavior. The raw C ABI still cannot validate a caller's allocation extent. No sanitizer over-read, daemon exploit, or disclosure is claimed.
This pull request was created by Codex.