Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

imp(staking): detect the length of the ed25519 pubkey in CreateValidator to prevent panic #18506

Merged
merged 4 commits into from
Nov 27, 2023
Merged

imp(staking): detect the length of the ed25519 pubkey in CreateValidator to prevent panic #18506

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8a9ebb7
imp(staking): detect the length of the ed25519 pubkey in CreateValida…
luchenqun Nov 19, 2023
78a7467
imp(staking): use default bond denom constant
luchenqun Nov 20, 2023
de5a9dd
imp(staking): use the constant for the ed25519 pubkey type
luchenqun Nov 20, 2023
6cc1bbb
imp(staking): use slices.Contains above instead of manually looping
luchenqun Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [#17733](https://github.com/cosmos/cosmos-sdk/pull/17733) Ensure `buf export` exports all proto dependencies
* (version) [#18063](https://github.com/cosmos/cosmos-sdk/pull/18063) Include additional information in the Info struct. This change enhances the Info struct by adding support for additional information through the ExtraInfo field
* (crypto | x/auth) [#14372](https://github.com/cosmos/cosmos-sdk/pull/18194) Key checks on signatures antehandle.
* (staking) [#18506](https://github.com/cosmos/cosmos-sdk/pull/18506) Detect the length of the ed25519 pubkey in CreateValidator to prevent panic.

### Bug Fixes

Expand Down
3 changes: 3 additions & 0 deletions types/staking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ var (

// DefaultPowerReduction is the default amount of staking tokens required for 1 unit of consensus-engine power
DefaultPowerReduction = sdkmath.NewIntFromUint64(1000000)

// PubKeyEd25519Type is ed25519 for type the consensus params validator pub_key_types params
PubKeyEd25519Type = "ed25519"
)

// TokensToConsensusPower - convert input tokens to potential consensus-engine power
Expand Down
46 changes: 27 additions & 19 deletions x/staking/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"cosmossdk.io/math"
"cosmossdk.io/x/staking/types"

"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
"github.com/cosmos/cosmos-sdk/telemetry"
sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -63,25 +64,6 @@ func (k msgServer) CreateValidator(ctx context.Context, msg *types.MsgCreateVali
return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting cryptotypes.PubKey, got %T", pk)
}
luchenqun marked this conversation as resolved.
Show resolved Hide resolved

if _, err := k.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(pk)); err == nil {
return nil, types.ErrValidatorPubKeyExists
}

bondDenom, err := k.BondDenom(ctx)
if err != nil {
return nil, err
}

if msg.Value.Denom != bondDenom {
return nil, errorsmod.Wrapf(
sdkerrors.ErrInvalidRequest, "invalid coin denomination: got %s, expected %s", msg.Value.Denom, bondDenom,
)
}

if _, err := msg.Description.EnsureLength(); err != nil {
return nil, err
}

sdkCtx := sdk.UnwrapSDKContext(ctx)
cp := sdkCtx.ConsensusParams()
if cp.Validator != nil {
Expand All @@ -99,6 +81,32 @@ func (k msgServer) CreateValidator(ctx context.Context, msg *types.MsgCreateVali
"got: %s, expected: %s", pk.Type(), cp.Validator.PubKeyTypes,
)
}

luchenqun marked this conversation as resolved.
Show resolved Hide resolved
if pkType == sdk.PubKeyEd25519Type && len(pk.Bytes()) != ed25519.PubKeySize {
return nil, errorsmod.Wrapf(
types.ErrConsensusPubKeyLenInvalid,
"got: %d, expected: %d", len(pk.Bytes()), ed25519.PubKeySize,
)
}
}

if _, err := k.GetValidatorByConsAddr(ctx, sdk.GetConsAddress(pk)); err == nil {
return nil, types.ErrValidatorPubKeyExists
}

bondDenom, err := k.BondDenom(ctx)
if err != nil {
return nil, err
}

if msg.Value.Denom != bondDenom {
return nil, errorsmod.Wrapf(
sdkerrors.ErrInvalidRequest, "invalid coin denomination: got %s, expected %s", msg.Value.Denom, bondDenom,
)
}

if _, err := msg.Description.EnsureLength(); err != nil {
return nil, err
luchenqun marked this conversation as resolved.
Show resolved Hide resolved
}

validator, err := types.NewValidator(msg.ValidatorAddress, pk, msg.Description)
Expand Down
52 changes: 42 additions & 10 deletions x/staking/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"
"time"

cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
"github.com/golang/mock/gomock"

"cosmossdk.io/collections"
Expand All @@ -14,6 +15,7 @@ import (
"github.com/cosmos/cosmos-sdk/codec/address"
codectypes "github.com/cosmos/cosmos-sdk/codec/types"
"github.com/cosmos/cosmos-sdk/crypto/keys/ed25519"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
simtestutil "github.com/cosmos/cosmos-sdk/testutil/sims"
sdk "github.com/cosmos/cosmos-sdk/types"
)
Expand All @@ -40,6 +42,16 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
pubkey, err := codectypes.NewAnyWithValue(pk1)
require.NoError(err)

var ed25519pk cryptotypes.PubKey = &ed25519.PubKey{Key: []byte{1, 2, 3, 4, 5, 6}}
pubkeyInvalidLen, err := codectypes.NewAnyWithValue(ed25519pk)
require.NoError(err)

ctx = ctx.WithConsensusParams(cmtproto.ConsensusParams{
Validator: &cmtproto.ValidatorParams{
PubKeyTypes: []string{sdk.PubKeyEd25519Type},
},
})

testCases := []struct {
name string
input *stakingtypes.MsgCreateValidator
Expand All @@ -59,7 +71,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
DelegatorAddress: Addr.String(),
ValidatorAddress: ValAddr.String(),
Pubkey: pubkey,
Value: sdk.NewInt64Coin("stake", 10000),
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000),
},
expErr: true,
expErrMsg: "empty description",
Expand All @@ -79,7 +91,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
DelegatorAddress: Addr.String(),
ValidatorAddress: sdk.AccAddress([]byte("invalid")).String(),
Pubkey: pubkey,
Value: sdk.NewInt64Coin("stake", 10000),
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000),
},
expErr: true,
expErrMsg: "invalid validator address",
Expand All @@ -99,11 +111,31 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
DelegatorAddress: Addr.String(),
ValidatorAddress: ValAddr.String(),
Pubkey: nil,
Value: sdk.NewInt64Coin("stake", 10000),
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000),
},
expErr: true,
expErrMsg: "empty validator public key",
},
{
name: "validator pubkey len is invalid",
input: &stakingtypes.MsgCreateValidator{
Description: stakingtypes.Description{
Moniker: "NewValidator",
},
Commission: stakingtypes.CommissionRates{
Rate: math.LegacyNewDecWithPrec(5, 1),
MaxRate: math.LegacyNewDecWithPrec(5, 1),
MaxChangeRate: math.LegacyNewDec(0),
},
MinSelfDelegation: math.NewInt(1),
DelegatorAddress: Addr.String(),
ValidatorAddress: ValAddr.String(),
Pubkey: pubkeyInvalidLen,
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000),
},
expErr: true,
expErrMsg: "consensus pubkey len is invalid",
},
{
name: "empty delegation amount",
input: &stakingtypes.MsgCreateValidator{
Expand All @@ -119,7 +151,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
DelegatorAddress: Addr.String(),
ValidatorAddress: ValAddr.String(),
Pubkey: pubkey,
Value: sdk.NewInt64Coin("stake", 0),
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 0),
},
expErr: true,
expErrMsg: "invalid delegation amount",
Expand Down Expand Up @@ -159,7 +191,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
DelegatorAddress: Addr.String(),
ValidatorAddress: ValAddr.String(),
Pubkey: pubkey,
Value: sdk.NewInt64Coin("stake", 10000),
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000),
},
expErr: true,
expErrMsg: "minimum self delegation must be a positive integer",
Expand All @@ -179,7 +211,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
DelegatorAddress: Addr.String(),
ValidatorAddress: ValAddr.String(),
Pubkey: pubkey,
Value: sdk.NewInt64Coin("stake", 10000),
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000),
},
expErr: true,
expErrMsg: "minimum self delegation must be a positive integer",
Expand All @@ -199,7 +231,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
DelegatorAddress: Addr.String(),
ValidatorAddress: ValAddr.String(),
Pubkey: pubkey,
Value: sdk.NewInt64Coin("stake", 10),
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10),
},
expErr: true,
expErrMsg: "validator's self delegation must be greater than their minimum self delegation",
Expand All @@ -223,7 +255,7 @@ func (s *KeeperTestSuite) TestMsgCreateValidator() {
DelegatorAddress: Addr.String(),
ValidatorAddress: ValAddr.String(),
Pubkey: pubkey,
Value: sdk.NewInt64Coin("stake", 10000),
Value: sdk.NewInt64Coin(sdk.DefaultBondDenom, 10000),
},
expErr: false,
},
Expand Down Expand Up @@ -253,7 +285,7 @@ func (s *KeeperTestSuite) TestMsgEditValidator() {
require.NotNil(pk)

comm := stakingtypes.NewCommissionRates(math.LegacyNewDec(0), math.LegacyNewDec(0), math.LegacyNewDec(0))
msg, err := stakingtypes.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin("stake", math.NewInt(10)), stakingtypes.Description{Moniker: "NewVal"}, comm, math.OneInt())
msg, err := stakingtypes.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(10)), stakingtypes.Description{Moniker: "NewVal"}, comm, math.OneInt())
require.NoError(err)

res, err := msgServer.CreateValidator(ctx, msg)
Expand Down Expand Up @@ -427,7 +459,7 @@ func (s *KeeperTestSuite) TestMsgDelegate() {

comm := stakingtypes.NewCommissionRates(math.LegacyNewDec(0), math.LegacyNewDec(0), math.LegacyNewDec(0))

msg, err := stakingtypes.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin("stake", math.NewInt(10)), stakingtypes.Description{Moniker: "NewVal"}, comm, math.OneInt())
msg, err := stakingtypes.NewMsgCreateValidator(ValAddr.String(), pk, sdk.NewCoin(sdk.DefaultBondDenom, math.NewInt(10)), stakingtypes.Description{Moniker: "NewVal"}, comm, math.OneInt())
require.NoError(err)

res, err := msgServer.CreateValidator(ctx, msg)
Expand Down
1 change: 1 addition & 0 deletions x/staking/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,5 @@ var (
// consensus key errors
ErrConsensusPubKeyAlreadyUsedForValidator = errors.Register(ModuleName, 46, "consensus pubkey is already used for a validator")
ErrExceedingMaxConsPubKeyRotations = errors.Register(ModuleName, 47, "exceeding maximum consensus pubkey rotations within unbonding period")
ErrConsensusPubKeyLenInvalid = errors.Register(ModuleName, 48, "consensus pubkey len is invalid")
)
Loading