From 516ff45392bb7fcde604a34971e1dcb3d350796f Mon Sep 17 00:00:00 2001 From: Thane Thomson Date: Thu, 28 Apr 2022 13:53:44 -0400 Subject: [PATCH] abci++: Vote extension cleanup (#8402) * Split vote verification/validation based on vote extensions Some parts of the code need vote extensions to be verified and validated (mostly in consensus), and other parts of the code don't because its possible that, in some cases (as per RFC 017), we won't have vote extensions. This explicitly facilitates that split. Signed-off-by: Thane Thomson * Only sign extensions in precommits, not prevotes Signed-off-by: Thane Thomson * Update privval/file.go Co-authored-by: M. J. Fromberger * Apply suggestions from code review Co-authored-by: M. J. Fromberger * Temporarily disable extension requirement again for E2E testing Signed-off-by: Thane Thomson * Reorganize comment for clarity Signed-off-by: Thane Thomson * Leave vote validation and pre-call nil check up to caller of VoteToProto Signed-off-by: Thane Thomson * Split complex vote validation test into multiple tests Signed-off-by: Thane Thomson * Universally enforce no vote extensions on any vote type but precommits Signed-off-by: Thane Thomson * Make error messages more generic Signed-off-by: Thane Thomson * Verify with vote extensions when constructing a VoteSet Signed-off-by: Thane Thomson * Expand comment for clarity Signed-off-by: Thane Thomson * Add extension check for prevotes prior to signing votes Signed-off-by: Thane Thomson * Fix supporting test code to only inject extensions into precommits Signed-off-by: Thane Thomson * Separate vote malleation from signing in vote tests for clarity Signed-off-by: Thane Thomson * Add extension signature length check and corresponding test Signed-off-by: Thane Thomson * Perform basic vote validation in CommitToVoteSet Signed-off-by: Thane Thomson Co-authored-by: M. J. Fromberger --- internal/consensus/common_test.go | 6 +- internal/consensus/msgs.go | 10 +- internal/test/factory/vote.go | 1 + privval/file.go | 22 ++-- types/block.go | 6 +- types/evidence.go | 26 +++-- types/priv_validator.go | 13 ++- types/vote.go | 130 ++++++++++++++++-------- types/vote_set.go | 2 +- types/vote_test.go | 162 +++++++++++++++++++++++------- 10 files changed, 278 insertions(+), 100 deletions(-) diff --git a/internal/consensus/common_test.go b/internal/consensus/common_test.go index 515a74d1a..ca1db8425 100644 --- a/internal/consensus/common_test.go +++ b/internal/consensus/common_test.go @@ -159,7 +159,11 @@ func signVote( chainID string, blockID types.BlockID) *types.Vote { - v, err := vs.signVote(ctx, voteType, chainID, blockID, []byte("extension")) + var ext []byte + if voteType == tmproto.PrecommitType { + ext = []byte("extension") + } + v, err := vs.signVote(ctx, voteType, chainID, blockID, ext) require.NoError(t, err, "failed to sign vote") vs.lastVote = v diff --git a/internal/consensus/msgs.go b/internal/consensus/msgs.go index 8b19db423..c59c06a41 100644 --- a/internal/consensus/msgs.go +++ b/internal/consensus/msgs.go @@ -220,9 +220,13 @@ type VoteMessage struct { func (*VoteMessage) TypeTag() string { return "tendermint/Vote" } -// ValidateBasic performs basic validation. +// ValidateBasic checks whether the vote within the message is well-formed. func (m *VoteMessage) ValidateBasic() error { - return m.Vote.ValidateBasic() + // Here we validate votes with vote extensions, since we require vote + // extensions to be sent in precommit messages during consensus. Prevote + // messages should never have vote extensions, and this is also validated + // here. + return m.Vote.ValidateWithExtension() } // String returns a string representation. @@ -534,6 +538,8 @@ func MsgFromProto(msg *tmcons.Message) (Message, error) { Part: parts, } case *tmcons.Message_Vote: + // Vote validation will be handled in the vote message ValidateBasic + // call below. vote, err := types.VoteFromProto(msg.Vote.Vote) if err != nil { return nil, fmt.Errorf("vote msg to proto error: %w", err) diff --git a/internal/test/factory/vote.go b/internal/test/factory/vote.go index fc63e8d68..4e77473c2 100644 --- a/internal/test/factory/vote.go +++ b/internal/test/factory/vote.go @@ -40,5 +40,6 @@ func MakeVote( } v.Signature = vpb.Signature + v.ExtensionSignature = vpb.ExtensionSignature return v, nil } diff --git a/privval/file.go b/privval/file.go index 1e31b8c78..bf5803632 100644 --- a/privval/file.go +++ b/privval/file.go @@ -372,11 +372,20 @@ func (pv *FilePV) signVote(chainID string, vote *tmproto.Vote) error { } signBytes := types.VoteSignBytes(chainID, vote) - extSignBytes := types.VoteExtensionSignBytes(chainID, vote) - // We always sign the vote extension. See below for details. - extSig, err := pv.Key.PrivKey.Sign(extSignBytes) - if err != nil { - return err + + // Vote extensions are non-deterministic, so it is possible that an + // application may have created a different extension. We therefore always + // re-sign the vote extensions of precommits. For prevotes, the extension + // signature will always be empty. + var extSig []byte + if vote.Type == tmproto.PrecommitType { + extSignBytes := types.VoteExtensionSignBytes(chainID, vote) + extSig, err = pv.Key.PrivKey.Sign(extSignBytes) + if err != nil { + return err + } + } else if len(vote.Extension) > 0 { + return errors.New("unexpected vote extension - extensions are only allowed in precommits") } // We might crash before writing to the wal, @@ -402,9 +411,6 @@ func (pv *FilePV) signVote(chainID string, vote *tmproto.Vote) error { vote.Signature = lss.Signature } - // Vote extensions are non-deterministic, so it's possible that an - // application may have created a different extension. We therefore - // always re-sign the vote extension. vote.ExtensionSignature = extSig return nil diff --git a/types/block.go b/types/block.go index 578ac43ee..cb1b43ea5 100644 --- a/types/block.go +++ b/types/block.go @@ -780,7 +780,11 @@ func CommitToVoteSet(chainID string, commit *Commit, vals *ValidatorSet) *VoteSe if commitSig.Absent() { continue // OK, some precommits can be missing. } - added, err := voteSet.AddVote(commit.GetVote(int32(idx))) + vote := commit.GetVote(int32(idx)) + if err := vote.ValidateBasic(); err != nil { + panic(fmt.Errorf("failed to validate vote reconstructed from LastCommit: %w", err)) + } + added, err := voteSet.AddVote(vote) if !added || err != nil { panic(fmt.Errorf("failed to reconstruct LastCommit: %w", err)) } diff --git a/types/evidence.go b/types/evidence.go index c8ea967cf..dc66a6fcf 100644 --- a/types/evidence.go +++ b/types/evidence.go @@ -211,14 +211,28 @@ func DuplicateVoteEvidenceFromProto(pb *tmproto.DuplicateVoteEvidence) (*Duplica return nil, errors.New("nil duplicate vote evidence") } - vA, err := VoteFromProto(pb.VoteA) - if err != nil { - return nil, err + var vA *Vote + if pb.VoteA != nil { + var err error + vA, err = VoteFromProto(pb.VoteA) + if err != nil { + return nil, err + } + if err = vA.ValidateBasic(); err != nil { + return nil, err + } } - vB, err := VoteFromProto(pb.VoteB) - if err != nil { - return nil, err + var vB *Vote + if pb.VoteB != nil { + var err error + vB, err = VoteFromProto(pb.VoteB) + if err != nil { + return nil, err + } + if err = vB.ValidateBasic(); err != nil { + return nil, err + } } dve := &DuplicateVoteEvidence{ diff --git a/types/priv_validator.go b/types/priv_validator.go index 29620f829..72027c622 100644 --- a/types/priv_validator.go +++ b/types/priv_validator.go @@ -96,9 +96,16 @@ func (pv MockPV) SignVote(ctx context.Context, chainID string, vote *tmproto.Vot return err } vote.Signature = sig - extSig, err := pv.PrivKey.Sign(extSignBytes) - if err != nil { - return err + + var extSig []byte + // We only sign vote extensions for precommits + if vote.Type == tmproto.PrecommitType { + extSig, err = pv.PrivKey.Sign(extSignBytes) + if err != nil { + return err + } + } else if len(vote.Extension) > 0 { + return errors.New("unexpected vote extension - vote extensions are only allowed in precommits") } vote.ExtensionSignature = extSig return nil diff --git a/types/vote.go b/types/vote.go index e66f40396..f20ee491e 100644 --- a/types/vote.go +++ b/types/vote.go @@ -61,6 +61,30 @@ type Vote struct { ExtensionSignature []byte `json:"extension_signature"` } +// VoteFromProto attempts to convert the given serialization (Protobuf) type to +// our Vote domain type. No validation is performed on the resulting vote - +// this is left up to the caller to decide whether to call ValidateBasic or +// ValidateWithExtension. +func VoteFromProto(pv *tmproto.Vote) (*Vote, error) { + blockID, err := BlockIDFromProto(&pv.BlockID) + if err != nil { + return nil, err + } + + return &Vote{ + Type: pv.Type, + Height: pv.Height, + Round: pv.Round, + BlockID: *blockID, + Timestamp: pv.Timestamp, + ValidatorAddress: pv.ValidatorAddress, + ValidatorIndex: pv.ValidatorIndex, + Signature: pv.Signature, + Extension: pv.Extension, + ExtensionSignature: pv.ExtensionSignature, + }, nil +} + // CommitSig converts the Vote to a CommitSig. func (vote *Vote) CommitSig() CommitSig { if vote == nil { @@ -164,24 +188,49 @@ func (vote *Vote) String() string { ) } -func (vote *Vote) Verify(chainID string, pubKey crypto.PubKey) error { +func (vote *Vote) verifyAndReturnProto(chainID string, pubKey crypto.PubKey) (*tmproto.Vote, error) { if !bytes.Equal(pubKey.Address(), vote.ValidatorAddress) { - return ErrVoteInvalidValidatorAddress + return nil, ErrVoteInvalidValidatorAddress } v := vote.ToProto() if !pubKey.VerifySignature(VoteSignBytes(chainID, v), vote.Signature) { - return ErrVoteInvalidSignature + return nil, ErrVoteInvalidSignature } - extSignBytes := VoteExtensionSignBytes(chainID, v) - // TODO: Remove extension signature nil check to enforce vote extension - // signing once we resolve https://github.com/tendermint/tendermint/issues/8272 - if vote.ExtensionSignature != nil && !pubKey.VerifySignature(extSignBytes, vote.ExtensionSignature) { - return ErrVoteInvalidSignature + return v, nil +} + +// Verify checks whether the signature associated with this vote corresponds to +// the given chain ID and public key. This function does not validate vote +// extension signatures - to do so, use VerifyWithExtension instead. +func (vote *Vote) Verify(chainID string, pubKey crypto.PubKey) error { + _, err := vote.verifyAndReturnProto(chainID, pubKey) + return err +} + +// VerifyWithExtension performs the same verification as Verify, but +// additionally checks whether the vote extension signature corresponds to the +// given chain ID and public key. We only verify vote extension signatures for +// precommits. +func (vote *Vote) VerifyWithExtension(chainID string, pubKey crypto.PubKey) error { + v, err := vote.verifyAndReturnProto(chainID, pubKey) + if err != nil { + return err + } + // We only verify vote extension signatures for precommits. + if vote.Type == tmproto.PrecommitType { + extSignBytes := VoteExtensionSignBytes(chainID, v) + // TODO: Remove extension signature nil check to enforce vote extension + // signing once we resolve https://github.com/tendermint/tendermint/issues/8272 + if vote.ExtensionSignature != nil && !pubKey.VerifySignature(extSignBytes, vote.ExtensionSignature) { + return ErrVoteInvalidSignature + } } return nil } -// ValidateBasic performs basic validation. +// ValidateBasic checks whether the vote is well-formed. It does not, however, +// check vote extensions - for vote validation with vote extension validation, +// use ValidateWithExtension. func (vote *Vote) ValidateBasic() error { if !IsVoteTypeValid(vote.Type) { return errors.New("invalid Type") @@ -224,10 +273,38 @@ func (vote *Vote) ValidateBasic() error { return fmt.Errorf("signature is too big (max: %d)", MaxSignatureSize) } - // TODO: Remove the extension length check such that we always require - // extension signatures to be present. - if len(vote.Extension) > 0 && len(vote.ExtensionSignature) == 0 { - return errors.New("vote extension signature is missing") + // We should only ever see vote extensions in precommits. + if vote.Type != tmproto.PrecommitType { + if len(vote.Extension) > 0 { + return errors.New("unexpected vote extension") + } + if len(vote.ExtensionSignature) > 0 { + return errors.New("unexpected vote extension signature") + } + } + + return nil +} + +// ValidateWithExtension performs the same validations as ValidateBasic, but +// additionally checks whether a vote extension signature is present. This +// function is used in places where vote extension signatures are expected. +func (vote *Vote) ValidateWithExtension() error { + if err := vote.ValidateBasic(); err != nil { + return err + } + + // We should always see vote extension signatures in precommits + if vote.Type == tmproto.PrecommitType { + // TODO(thane): Remove extension length check once + // https://github.com/tendermint/tendermint/issues/8272 is + // resolved. + if len(vote.Extension) > 0 && len(vote.ExtensionSignature) == 0 { + return errors.New("vote extension signature is missing") + } + if len(vote.ExtensionSignature) > MaxSignatureSize { + return fmt.Errorf("vote extension signature is too big (max: %d)", MaxSignatureSize) + } } return nil @@ -269,30 +346,3 @@ func VotesToProto(votes []*Vote) []*tmproto.Vote { } return res } - -// FromProto converts a proto generetad type to a handwritten type -// return type, nil if everything converts safely, otherwise nil, error -func VoteFromProto(pv *tmproto.Vote) (*Vote, error) { - if pv == nil { - return nil, errors.New("nil vote") - } - - blockID, err := BlockIDFromProto(&pv.BlockID) - if err != nil { - return nil, err - } - - vote := new(Vote) - vote.Type = pv.Type - vote.Height = pv.Height - vote.Round = pv.Round - vote.BlockID = *blockID - vote.Timestamp = pv.Timestamp - vote.ValidatorAddress = pv.ValidatorAddress - vote.ValidatorIndex = pv.ValidatorIndex - vote.Signature = pv.Signature - vote.Extension = pv.Extension - vote.ExtensionSignature = pv.ExtensionSignature - - return vote, vote.ValidateBasic() -} diff --git a/types/vote_set.go b/types/vote_set.go index 142a4130c..b4d149576 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -194,7 +194,7 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { } // Check signature. - if err := vote.Verify(voteSet.chainID, val.PubKey); err != nil { + if err := vote.VerifyWithExtension(voteSet.chainID, val.PubKey); err != nil { return false, fmt.Errorf("failed to verify vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) } diff --git a/types/vote_test.go b/types/vote_test.go index 949c996d0..684422b97 100644 --- a/types/vote_test.go +++ b/types/vote_test.go @@ -24,7 +24,9 @@ func examplePrevote(t *testing.T) *Vote { func examplePrecommit(t testing.TB) *Vote { t.Helper() - return exampleVote(t, byte(tmproto.PrecommitType)) + vote := exampleVote(t, byte(tmproto.PrecommitType)) + vote.ExtensionSignature = []byte("signature") + return vote } func exampleVote(tb testing.TB, t byte) *Vote { @@ -48,6 +50,7 @@ func exampleVote(tb testing.TB, t byte) *Vote { ValidatorIndex: 56789, } } + func TestVoteSignable(t *testing.T) { vote := examplePrecommit(t) v := vote.ToProto() @@ -221,8 +224,8 @@ func TestVoteExtension(t *testing.T) { includeSignature: true, expectError: false, }, - // TODO: Re-enable once - // https://github.com/tendermint/tendermint/issues/8272 is resolved. + // TODO(thane): Re-enable once + // https://github.com/tendermint/tendermint/issues/8272 is resolved //{ // name: "no extension signature", // extension: []byte("extension"), @@ -269,7 +272,7 @@ func TestVoteExtension(t *testing.T) { if tc.includeSignature { vote.ExtensionSignature = v.ExtensionSignature } - err = vote.Verify("test_chain_id", pk) + err = vote.VerifyWithExtension("test_chain_id", pk) if tc.expectError { require.Error(t, err) } else { @@ -336,39 +339,115 @@ func TestVoteString(t *testing.T) { } } -func TestVoteValidateBasic(t *testing.T) { +func signVote(ctx context.Context, t *testing.T, pv PrivValidator, chainID string, vote *Vote) { + t.Helper() + + v := vote.ToProto() + require.NoError(t, pv.SignVote(ctx, chainID, v)) + vote.Signature = v.Signature + vote.ExtensionSignature = v.ExtensionSignature +} + +func TestValidVotes(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() privVal := NewMockPV() testCases := []struct { - testName string + name string + vote *Vote malleateVote func(*Vote) - expectErr bool }{ - {"Good Vote", func(v *Vote) {}, false}, - {"Negative Height", func(v *Vote) { v.Height = -1 }, true}, - {"Negative Round", func(v *Vote) { v.Round = -1 }, true}, - {"Invalid BlockID", func(v *Vote) { - v.BlockID = BlockID{[]byte{1, 2, 3}, PartSetHeader{111, []byte("blockparts")}} - }, true}, - {"Invalid Address", func(v *Vote) { v.ValidatorAddress = make([]byte, 1) }, true}, - {"Invalid ValidatorIndex", func(v *Vote) { v.ValidatorIndex = -1 }, true}, - {"Invalid Signature", func(v *Vote) { v.Signature = nil }, true}, - {"Too big Signature", func(v *Vote) { v.Signature = make([]byte, MaxSignatureSize+1) }, true}, + {"good prevote", examplePrevote(t), func(v *Vote) {}}, + {"good precommit without vote extension", examplePrecommit(t), func(v *Vote) { v.Extension = nil }}, + {"good precommit with vote extension", examplePrecommit(t), func(v *Vote) { v.Extension = []byte("extension") }}, } for _, tc := range testCases { - tc := tc - t.Run(tc.testName, func(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + signVote(ctx, t, privVal, "test_chain_id", tc.vote) + tc.malleateVote(tc.vote) + require.NoError(t, tc.vote.ValidateBasic(), "ValidateBasic for %s", tc.name) + require.NoError(t, tc.vote.ValidateWithExtension(), "ValidateWithExtension for %s", tc.name) + } +} - vote := examplePrecommit(t) - v := vote.ToProto() - err := privVal.SignVote(ctx, "test_chain_id", v) - vote.Signature = v.Signature - require.NoError(t, err) - tc.malleateVote(vote) - assert.Equal(t, tc.expectErr, vote.ValidateBasic() != nil, "Validate Basic had an unexpected result") - }) +func TestInvalidVotes(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + privVal := NewMockPV() + + testCases := []struct { + name string + malleateVote func(*Vote) + }{ + {"negative height", func(v *Vote) { v.Height = -1 }}, + {"negative round", func(v *Vote) { v.Round = -1 }}, + {"invalid block ID", func(v *Vote) { v.BlockID = BlockID{[]byte{1, 2, 3}, PartSetHeader{111, []byte("blockparts")}} }}, + {"invalid address", func(v *Vote) { v.ValidatorAddress = make([]byte, 1) }}, + {"invalid validator index", func(v *Vote) { v.ValidatorIndex = -1 }}, + {"invalid signature", func(v *Vote) { v.Signature = nil }}, + {"oversized signature", func(v *Vote) { v.Signature = make([]byte, MaxSignatureSize+1) }}, + } + for _, tc := range testCases { + prevote := examplePrevote(t) + signVote(ctx, t, privVal, "test_chain_id", prevote) + tc.malleateVote(prevote) + require.Error(t, prevote.ValidateBasic(), "ValidateBasic for %s in invalid prevote", tc.name) + require.Error(t, prevote.ValidateWithExtension(), "ValidateWithExtension for %s in invalid prevote", tc.name) + + precommit := examplePrecommit(t) + signVote(ctx, t, privVal, "test_chain_id", precommit) + tc.malleateVote(precommit) + require.Error(t, precommit.ValidateBasic(), "ValidateBasic for %s in invalid precommit", tc.name) + require.Error(t, precommit.ValidateWithExtension(), "ValidateWithExtension for %s in invalid precommit", tc.name) + } +} + +func TestInvalidPrevotes(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + privVal := NewMockPV() + + testCases := []struct { + name string + malleateVote func(*Vote) + }{ + {"vote extension present", func(v *Vote) { v.Extension = []byte("extension") }}, + {"vote extension signature present", func(v *Vote) { v.ExtensionSignature = []byte("signature") }}, + } + for _, tc := range testCases { + prevote := examplePrevote(t) + signVote(ctx, t, privVal, "test_chain_id", prevote) + tc.malleateVote(prevote) + require.Error(t, prevote.ValidateBasic(), "ValidateBasic for %s", tc.name) + require.Error(t, prevote.ValidateWithExtension(), "ValidateWithExtension for %s", tc.name) + } +} + +func TestInvalidPrecommitExtensions(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + privVal := NewMockPV() + + testCases := []struct { + name string + malleateVote func(*Vote) + }{ + {"vote extension present without signature", func(v *Vote) { + v.Extension = []byte("extension") + v.ExtensionSignature = nil + }}, + // TODO(thane): Re-enable once https://github.com/tendermint/tendermint/issues/8272 is resolved + //{"missing vote extension signature", func(v *Vote) { v.ExtensionSignature = nil }}, + {"oversized vote extension signature", func(v *Vote) { v.ExtensionSignature = make([]byte, MaxSignatureSize+1) }}, + } + for _, tc := range testCases { + precommit := examplePrecommit(t) + signVote(ctx, t, privVal, "test_chain_id", precommit) + tc.malleateVote(precommit) + // We don't expect an error from ValidateBasic, because it doesn't + // handle vote extensions. + require.NoError(t, precommit.ValidateBasic(), "ValidateBasic for %s", tc.name) + require.Error(t, precommit.ValidateWithExtension(), "ValidateWithExtension for %s", tc.name) } } @@ -384,21 +463,28 @@ func TestVoteProtobuf(t *testing.T) { require.NoError(t, err) testCases := []struct { - msg string - v1 *Vote - expPass bool + msg string + vote *Vote + convertsOk bool + passesValidateBasic bool }{ - {"success", vote, true}, - {"fail vote validate basic", &Vote{}, false}, - {"failure nil", nil, false}, + {"success", vote, true, true}, + {"fail vote validate basic", &Vote{}, true, false}, } for _, tc := range testCases { - protoProposal := tc.v1.ToProto() + protoProposal := tc.vote.ToProto() v, err := VoteFromProto(protoProposal) - if tc.expPass { + if tc.convertsOk { require.NoError(t, err) - require.Equal(t, tc.v1, v, tc.msg) + } else { + require.Error(t, err) + } + + err = v.ValidateBasic() + if tc.passesValidateBasic { + require.NoError(t, err) + require.Equal(t, tc.vote, v, tc.msg) } else { require.Error(t, err) }