fix dep and sec issues#7
Merged
Merged
Conversation
Signed-off-by: Dave Grantham <dave@cryptid.tech>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
multi-sigsilently swallows malformedThresholdDatamulti-sig/src/views/bls12381.rs:554,645,688:If
ThresholdDataparsing fails (corruption, truncation, tampering),unwrap_or_default()returns an emptyThresholdData. The caller then sees zero shares and may proceed with an empty threshold set. This is a fail-open pattern — corrupted security-critical data is silently replaced with a safe-looking default rather than surfacing an error.fix: Propagate the error with
?or return aThresholdError::InvalidThresholdData.multi-sigduplicate share identifiers silently overwritemulti-sig/src/views/bls12381.rs:314andmulti-key/src/views/bls12381.rs(analogous) —shares.insert(identifier, sdata)silently overwrites an existing share with the same identifier. In threshold reconstruction, a duplicate identifier means one participant's share is lost, potentially dropping below the threshold without any error.fix: Check
contains_keybefore insert and returnDuplicateShareerror.multi-sigBLS codec inferred from signature lengthmulti-sig/src/views/bls12381.rs:419-429andmulti-key/src/views/bls12381.rs:459-465— The BLS curve (G1 vs G2) is selected by signature byte length (48 → G1, 96 → G2). This is a heuristic, not cryptographic binding. A 48-byte G2 signature or 96-byte G1 signature would be misclassified. If downstream code trusts the codec tag for curve selection, this could cause verification on the wrong curve.fix: Require explicit codec specification rather than length-based inference, or document that this heuristic is a known limitation.
multi-siginconsistent error handling between G1/G2 combinemulti-sig/src/views/bls12381.rs:685-688(G2 path) usesav.payload_encoding().ok()(optional — silently proceeds if missing), while line 740 (G1 path) usesav.payload_encoding()?(required — returns error). This asymmetry means G2 combine silently accepts missing payload encoding while G1 rejects it.multi-sigtypo in error messagemulti-sig/src/error.rs:74—"Signature missing limi"should be"Signature missing limit".Signed-off-by: Dave Grantham dave@cryptid.tech