fix(multisig): reject zero-member identity/verifier to close anyone-can-spend bypass#1859
Open
adecaro wants to merge 1 commit into
Conversation
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.
Closes the "anyone-can-spend" bypass described in the linked security issue: a
MultiIdentitywith zero constituent identities could be serialised directly (bypassing thehonest-client
WrapIdentitiesguard), committed on-chain as a token output owner, and laterspent by anyone supplying an empty
MultiSignature{}— no real signature required.Two minimal, targeted guards close the two independent paths that allowed this:
token/services/identity/multisig/sig.go— fail closed on degenerate verifier stateThe previous
len(v.Verifiers) != len(sig.Signatures)guard passed vacuously when bothlengths were zero (
0 != 0→false). The loop then iterated zero times and returnednil.The new check fires before any ASN.1 parsing, making the function fail closed regardless of
how a zero-verifier
Verifierwas constructed.token/services/identity/multisig/deserializer.go— reject at the trust boundaryThis is the primary enforcement point: attacker-controlled bytes enter here. The guard ensures
a zero-identity
MultiIdentityis rejected before aVerifierobject is ever allocated.Why two guards
become a typed Go value.
Verifyguard — defence-in-depth: ensures the function fails closed even if azero-verifier
Verifieris constructed through any future programmatic path (e.g. a testhelper, a different deserializer, or a future refactor) that does not go through
DeserializeVerifier.The two guards are independent and complement each other; neither alone is sufficient for
robust defence-in-depth.
Test coverage
Five new tests across the three test files pin the expected behaviour and would catch any
regression:
sig_test.goTestVerifier_Verify_ZeroVerifiersVerifier+ emptyMultiSignature{}bytes → errordeserializer_test.goTestDeserializeVerifier_EmptyIdentitiesasn1.Marshal(MultiIdentity{})attacker path → rejected at deserializationidentity_test.goTestWrapIdentities_SingleIdentityidentity_test.goTestUnwrap_EmptyMultiIdentityUnwrapitself does not reject empty identity (not the enforcement point); empty slice is returned, no erroridentity_test.goTestInfoMatcher_Match_ZeroMatchersInfoMatcher.Match; pins expectation that the mitigation lives at the deserializer, not insideMatchChecklist
go test ./token/services/identity/multisig/...)