From 72ab03b3ef84be60d2370f34a4328cce8aec464a Mon Sep 17 00:00:00 2001 From: Federico Kunze <31522760+fedekunze@users.noreply.github.com> Date: Tue, 3 Nov 2020 11:07:32 +0100 Subject: [PATCH] ibc: validate signer address (#7781) * ibc: validate signer address * comment * validate channel handshakes and transfer signers --- x/ibc/applications/transfer/types/msgs.go | 12 +++- x/ibc/applications/transfer/types/packet.go | 4 +- x/ibc/core/02-client/types/msgs.go | 15 +++-- x/ibc/core/03-connection/types/msgs.go | 20 +++--- x/ibc/core/04-channel/types/msgs.go | 74 +++++++++++++-------- 5 files changed, 79 insertions(+), 46 deletions(-) diff --git a/x/ibc/applications/transfer/types/msgs.go b/x/ibc/applications/transfer/types/msgs.go index 25ff69e715..0654826713 100644 --- a/x/ibc/applications/transfer/types/msgs.go +++ b/x/ibc/applications/transfer/types/msgs.go @@ -1,6 +1,8 @@ package types import ( + "strings" + sdk "github.com/cosmos/cosmos-sdk/types" sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" clienttypes "github.com/cosmos/cosmos-sdk/x/ibc/core/02-client/types" @@ -42,6 +44,8 @@ func (MsgTransfer) Type() string { // ValidateBasic performs a basic check of the MsgTransfer fields. // NOTE: timeout height or timestamp values can be 0 to disable the timeout. +// NOTE: The recipient addresses format is not validated as the format defined by +// the chain is not known to IBC. func (msg MsgTransfer) ValidateBasic() error { if err := host.PortIdentifierValidator(msg.SourcePort); err != nil { return sdkerrors.Wrap(err, "invalid source port ID") @@ -55,10 +59,12 @@ func (msg MsgTransfer) ValidateBasic() error { if !msg.Token.IsPositive() { return sdkerrors.Wrap(sdkerrors.ErrInsufficientFunds, msg.Token.String()) } - if msg.Sender == "" { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "missing sender address") + // NOTE: sender format must be validated as it is required by the GetSigners function. + _, err := sdk.AccAddressFromBech32(msg.Sender) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } - if msg.Receiver == "" { + if strings.TrimSpace(msg.Receiver) == "" { return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "missing recipient address") } return ValidateIBCDenom(msg.Token.Denom) diff --git a/x/ibc/applications/transfer/types/packet.go b/x/ibc/applications/transfer/types/packet.go index c9d37d2674..d726577f6f 100644 --- a/x/ibc/applications/transfer/types/packet.go +++ b/x/ibc/applications/transfer/types/packet.go @@ -34,7 +34,9 @@ func NewFungibleTokenPacketData( } } -// ValidateBasic is used for validating the token transfer +// ValidateBasic is used for validating the token transfer. +// NOTE: The addresses formats are not validated as the sender and recipient can have different +// formats defined by their corresponding chains that are not known to IBC. func (ftpd FungibleTokenPacketData) ValidateBasic() error { if ftpd.Amount == 0 { return sdkerrors.Wrap(ErrInvalidAmount, "amount cannot be 0") diff --git a/x/ibc/core/02-client/types/msgs.go b/x/ibc/core/02-client/types/msgs.go index 7d3e4af219..04b351eb26 100644 --- a/x/ibc/core/02-client/types/msgs.go +++ b/x/ibc/core/02-client/types/msgs.go @@ -64,8 +64,9 @@ func (msg MsgCreateClient) Type() string { // ValidateBasic implements sdk.Msg func (msg MsgCreateClient) ValidateBasic() error { - if msg.Signer == "" { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "signer address cannot be empty") + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } clientState, err := UnpackClientState(msg.ClientState) if err != nil { @@ -141,8 +142,9 @@ func (msg MsgUpdateClient) Type() string { // ValidateBasic implements sdk.Msg func (msg MsgUpdateClient) ValidateBasic() error { - if msg.Signer == "" { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "signer address cannot be empty") + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } header, err := UnpackHeader(msg.Header) if err != nil { @@ -281,8 +283,9 @@ func (msg MsgSubmitMisbehaviour) Type() string { // ValidateBasic performs basic (non-state-dependant) validation on a MsgSubmitMisbehaviour. func (msg MsgSubmitMisbehaviour) ValidateBasic() error { - if msg.Signer == "" { - return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "signer address cannot be empty") + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } misbehaviour, err := UnpackMisbehaviour(msg.Misbehaviour) if err != nil { diff --git a/x/ibc/core/03-connection/types/msgs.go b/x/ibc/core/03-connection/types/msgs.go index 6df9512e15..a162fb589b 100644 --- a/x/ibc/core/03-connection/types/msgs.go +++ b/x/ibc/core/03-connection/types/msgs.go @@ -53,8 +53,9 @@ func (msg MsgConnectionOpenInit) ValidateBasic() error { return sdkerrors.Wrap(err, "basic validation of the provided version failed") } } - if msg.Signer == "" { - return sdkerrors.ErrInvalidAddress + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } return msg.Counterparty.ValidateBasic() } @@ -157,8 +158,9 @@ func (msg MsgConnectionOpenTry) ValidateBasic() error { if msg.ConsensusHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "consensus height must be non-zero") } - if msg.Signer == "" { - return sdkerrors.ErrInvalidAddress + _, err = sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } return msg.Counterparty.ValidateBasic() } @@ -266,8 +268,9 @@ func (msg MsgConnectionOpenAck) ValidateBasic() error { if msg.ConsensusHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "consensus height must be non-zero") } - if msg.Signer == "" { - return sdkerrors.ErrInvalidAddress + _, err = sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } return nil } @@ -324,8 +327,9 @@ func (msg MsgConnectionOpenConfirm) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - if msg.Signer == "" { - return sdkerrors.ErrInvalidAddress + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } return nil } diff --git a/x/ibc/core/04-channel/types/msgs.go b/x/ibc/core/04-channel/types/msgs.go index ebd9730e01..ed29cc24ff 100644 --- a/x/ibc/core/04-channel/types/msgs.go +++ b/x/ibc/core/04-channel/types/msgs.go @@ -13,7 +13,7 @@ import ( var _ sdk.Msg = &MsgChannelOpenInit{} // NewMsgChannelOpenInit creates a new MsgChannelOpenInit -//nolint:interfacer +// nolint:interfacer func NewMsgChannelOpenInit( portID, channelID string, version string, channelOrder Order, connectionHops []string, counterpartyPortID, counterpartyChannelID string, signer sdk.AccAddress, @@ -46,7 +46,10 @@ func (msg MsgChannelOpenInit) ValidateBasic() error { if err := host.ChannelIdentifierValidator(msg.ChannelId); err != nil { return sdkerrors.Wrap(err, "invalid channel ID") } - // Signer can be empty + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) + } return msg.Channel.ValidateBasic() } @@ -68,7 +71,7 @@ func (msg MsgChannelOpenInit) GetSigners() []sdk.AccAddress { var _ sdk.Msg = &MsgChannelOpenTry{} // NewMsgChannelOpenTry creates a new MsgChannelOpenTry instance -//nolint:interfacer +// nolint:interfacer func NewMsgChannelOpenTry( portID, desiredChannelID, counterpartyChosenChannelID, version string, channelOrder Order, connectionHops []string, counterpartyPortID, counterpartyChannelID, counterpartyVersion string, @@ -115,7 +118,10 @@ func (msg MsgChannelOpenTry) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - // Signer can be empty + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) + } return msg.Channel.ValidateBasic() } @@ -137,7 +143,7 @@ func (msg MsgChannelOpenTry) GetSigners() []sdk.AccAddress { var _ sdk.Msg = &MsgChannelOpenAck{} // NewMsgChannelOpenAck creates a new MsgChannelOpenAck instance -//nolint:interfacer +// nolint:interfacer func NewMsgChannelOpenAck( portID, channelID, counterpartyChannelID string, cpv string, proofTry []byte, proofHeight clienttypes.Height, signer sdk.AccAddress, @@ -180,7 +186,10 @@ func (msg MsgChannelOpenAck) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - // Signer can be empty + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) + } return nil } @@ -202,7 +211,7 @@ func (msg MsgChannelOpenAck) GetSigners() []sdk.AccAddress { var _ sdk.Msg = &MsgChannelOpenConfirm{} // NewMsgChannelOpenConfirm creates a new MsgChannelOpenConfirm instance -//nolint:interfacer +// nolint:interfacer func NewMsgChannelOpenConfirm( portID, channelID string, proofAck []byte, proofHeight clienttypes.Height, signer sdk.AccAddress, @@ -240,7 +249,10 @@ func (msg MsgChannelOpenConfirm) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - // Signer can be empty + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) + } return nil } @@ -262,7 +274,7 @@ func (msg MsgChannelOpenConfirm) GetSigners() []sdk.AccAddress { var _ sdk.Msg = &MsgChannelCloseInit{} // NewMsgChannelCloseInit creates a new MsgChannelCloseInit instance -//nolint:interfacer +// nolint:interfacer func NewMsgChannelCloseInit( portID string, channelID string, signer sdk.AccAddress, ) *MsgChannelCloseInit { @@ -291,7 +303,10 @@ func (msg MsgChannelCloseInit) ValidateBasic() error { if err := host.ChannelIdentifierValidator(msg.ChannelId); err != nil { return sdkerrors.Wrap(err, "invalid channel ID") } - // Signer can be empty + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) + } return nil } @@ -313,7 +328,7 @@ func (msg MsgChannelCloseInit) GetSigners() []sdk.AccAddress { var _ sdk.Msg = &MsgChannelCloseConfirm{} // NewMsgChannelCloseConfirm creates a new MsgChannelCloseConfirm instance -//nolint:interfacer +// nolint:interfacer func NewMsgChannelCloseConfirm( portID, channelID string, proofInit []byte, proofHeight clienttypes.Height, signer sdk.AccAddress, @@ -351,7 +366,10 @@ func (msg MsgChannelCloseConfirm) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - // Signer can be empty + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) + } return nil } @@ -373,7 +391,7 @@ func (msg MsgChannelCloseConfirm) GetSigners() []sdk.AccAddress { var _ sdk.Msg = &MsgRecvPacket{} // NewMsgRecvPacket constructs new MsgRecvPacket -//nolint:interfacer +// nolint:interfacer func NewMsgRecvPacket( packet Packet, proof []byte, proofHeight clienttypes.Height, signer sdk.AccAddress, @@ -399,10 +417,10 @@ func (msg MsgRecvPacket) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - if msg.Signer == "" { - return sdkerrors.ErrInvalidAddress + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } - return msg.Packet.ValidateBasic() } @@ -436,7 +454,7 @@ func (msg MsgRecvPacket) Type() string { var _ sdk.Msg = &MsgTimeout{} // NewMsgTimeout constructs new MsgTimeout -//nolint:interfacer +// nolint:interfacer func NewMsgTimeout( packet Packet, nextSequenceRecv uint64, proof []byte, proofHeight clienttypes.Height, signer sdk.AccAddress, @@ -463,10 +481,10 @@ func (msg MsgTimeout) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - if msg.Signer == "" { - return sdkerrors.ErrInvalidAddress + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } - return msg.Packet.ValidateBasic() } @@ -491,7 +509,7 @@ func (msg MsgTimeout) Type() string { } // NewMsgTimeoutOnClose constructs new MsgTimeoutOnClose -//nolint:interfacer +// nolint:interfacer func NewMsgTimeoutOnClose( packet Packet, nextSequenceRecv uint64, proof, proofClose []byte, @@ -523,10 +541,10 @@ func (msg MsgTimeoutOnClose) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - if msg.Signer == "" { - return sdkerrors.ErrInvalidAddress + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } - return msg.Packet.ValidateBasic() } @@ -553,7 +571,7 @@ func (msg MsgTimeoutOnClose) Type() string { var _ sdk.Msg = &MsgAcknowledgement{} // NewMsgAcknowledgement constructs a new MsgAcknowledgement -//nolint:interfacer +// nolint:interfacer func NewMsgAcknowledgement( packet Packet, ack []byte, proof []byte, proofHeight clienttypes.Height, signer sdk.AccAddress) *MsgAcknowledgement { return &MsgAcknowledgement{ @@ -578,10 +596,10 @@ func (msg MsgAcknowledgement) ValidateBasic() error { if msg.ProofHeight.IsZero() { return sdkerrors.Wrap(sdkerrors.ErrInvalidHeight, "proof height must be non-zero") } - if msg.Signer == "" { - return sdkerrors.ErrInvalidAddress + _, err := sdk.AccAddressFromBech32(msg.Signer) + if err != nil { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "string could not be parsed as address: %v", err) } - return msg.Packet.ValidateBasic() }