From 3bfd1adff9028038ec77a8b3a46ebf7b2e1939fd Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 21 Oct 2019 12:43:13 -0700 Subject: [PATCH 1/4] add validation functions --- x/ibc/24-validation/validate.go | 47 +++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 x/ibc/24-validation/validate.go diff --git a/x/ibc/24-validation/validate.go b/x/ibc/24-validation/validate.go new file mode 100644 index 000000000000..d523389262c7 --- /dev/null +++ b/x/ibc/24-validation/validate.go @@ -0,0 +1,47 @@ +package validation + +import ( + "regexp" + "strings" +) + +// regular expression to check string is lowercase alphabetic characters only +var isAlphaLower = regexp.MustCompile(`^[a-z]+$`).MatchString + +// regular expression to check string is alphanumeric +var isAlphaNumeric = regexp.MustCompile(`^[a-zA-Z0-9]+$`).MatchString + +// Validator function type to validate path and identifier bytestrings +type Validator func([]byte) bool + +// Default validator function for Client, Connection, and Channel +// identifiers +// Valid Identifier must be between 10-20 characters and only +// contain lowercase alphabetic characters +func DefaultIdentifierValidator(id []byte) bool { + // valid id must be between 10 and 20 characters + if len(id) < 10 || len(id) > 20 { + return false + } + // valid id must contain only lower alphabetic characters + if !isAlphaLower(string(id)) { + return false + } + return true +} + +// NewPathValidator takes in a Identifier Validator function and returns +// a Path Validator function which requires path only has valid identifiers +// alphanumeric character strings, and "/" separators +func NewPathValidator(idValidator Validator) Validator { + return func(path []byte) bool { + pathArr := strings.Split(string(path), "/") + for _, p := range pathArr { + // Each path element must either be valid identifier or alphanumeric + if !idValidator([]byte(p)) && !isAlphaNumeric(p) { + return false + } + } + return true + } +} From 87522311c918d51be4eedb307f0503b6a447aef0 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 21 Oct 2019 14:18:35 -0700 Subject: [PATCH 2/4] validate path in ics-23 --- x/ibc/23-commitment/types/merkle.go | 5 +++++ x/ibc/24-validation/validate.go | 24 +++++++++++++++++++----- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/x/ibc/23-commitment/types/merkle.go b/x/ibc/23-commitment/types/merkle.go index 0a89c58e682b..6d8c644561b4 100644 --- a/x/ibc/23-commitment/types/merkle.go +++ b/x/ibc/23-commitment/types/merkle.go @@ -1,12 +1,14 @@ package types import ( + "fmt" "strings" "github.com/tendermint/tendermint/crypto/merkle" "github.com/cosmos/cosmos-sdk/store/rootmulti" "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/exported" + validation "github.com/cosmos/cosmos-sdk/x/ibc/24-validation" ) // ICS 023 Merkle Types Implementation @@ -104,6 +106,9 @@ func (p Path) String() string { // CONTRACT: provided path string MUST be a well formated path. See ICS24 for // reference. func ApplyPrefix(prefix exported.PrefixI, path string) Path { + if !validation.DefaultPathValidator(path) { + panic(fmt.Sprintf("Path: %s is malformed", path)) + } // Split paths by the separator pathSlice := strings.Split(path, "/") commitmentPath := NewPath(pathSlice) diff --git a/x/ibc/24-validation/validate.go b/x/ibc/24-validation/validate.go index d523389262c7..abba268828cc 100644 --- a/x/ibc/24-validation/validate.go +++ b/x/ibc/24-validation/validate.go @@ -12,13 +12,13 @@ var isAlphaLower = regexp.MustCompile(`^[a-z]+$`).MatchString var isAlphaNumeric = regexp.MustCompile(`^[a-zA-Z0-9]+$`).MatchString // Validator function type to validate path and identifier bytestrings -type Validator func([]byte) bool +type Validator func(string) bool // Default validator function for Client, Connection, and Channel // identifiers // Valid Identifier must be between 10-20 characters and only // contain lowercase alphabetic characters -func DefaultIdentifierValidator(id []byte) bool { +func DefaultIdentifierValidator(id string) bool { // valid id must be between 10 and 20 characters if len(id) < 10 || len(id) > 20 { return false @@ -34,14 +34,28 @@ func DefaultIdentifierValidator(id []byte) bool { // a Path Validator function which requires path only has valid identifiers // alphanumeric character strings, and "/" separators func NewPathValidator(idValidator Validator) Validator { - return func(path []byte) bool { - pathArr := strings.Split(string(path), "/") + return func(path string) bool { + pathArr := strings.Split(path, "/") for _, p := range pathArr { // Each path element must either be valid identifier or alphanumeric - if !idValidator([]byte(p)) && !isAlphaNumeric(p) { + if !idValidator(p) && !isAlphaNumeric(p) { return false } } return true } } + +// Default Path Validator takes in path string and validates +// with default identifier rules. This is optimized by simply +// checking that all path elements are alphanumeric +func DefaultPathValidator(path string) bool { + pathArr := strings.Split(path, "/") + for _, p := range pathArr { + // Each path element must either be alphanumeric + if !isAlphaNumeric(p) { + return false + } + } + return true +} From 0134e0bdac73bec6275e4719411914f311091614 Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Mon, 21 Oct 2019 19:42:24 -0700 Subject: [PATCH 3/4] address @fede comments --- types/errors/errors.go | 6 +++ x/ibc/23-commitment/keeper/keeper.go | 10 ++++- x/ibc/23-commitment/types/merkle.go | 10 ++--- x/ibc/{24-validation => 24-host}/validate.go | 39 +++++++++++++------- 4 files changed, 45 insertions(+), 20 deletions(-) rename x/ibc/{24-validation => 24-host}/validate.go (53%) diff --git a/types/errors/errors.go b/types/errors/errors.go index 642c433d90b2..370e6c591469 100644 --- a/types/errors/errors.go +++ b/types/errors/errors.go @@ -65,6 +65,12 @@ var ( // ErrNoSignatures to doc ErrNoSignatures = Register(RootCodespace, 16, "no signatures supplied") + // ErrInvalidId to doc + ErrInvalidID = Register(RootCodespace, 17, "invalid identifier") + + // ErrInvalidPath to doc + ErrInvalidPath = Register(RootCodespace, 18, "invalid path") + // ErrPanic is only set when we recover from a panic, so we know to // redact potentially sensitive system info ErrPanic = Register(UndefinedCodespace, 111222, "panic") diff --git a/x/ibc/23-commitment/keeper/keeper.go b/x/ibc/23-commitment/keeper/keeper.go index 62790e6b83cb..eb1a2dfb8c60 100644 --- a/x/ibc/23-commitment/keeper/keeper.go +++ b/x/ibc/23-commitment/keeper/keeper.go @@ -45,7 +45,10 @@ func (k Keeper) BatchVerifyMembership(ctx sdk.Context, proof exported.ProofI, it continue } - path := types.ApplyPrefix(k.prefix, pathStr) + path, err := types.ApplyPrefix(k.prefix, pathStr) + if err != nil { + return false + } ok = proof.VerifyMembership(root, path, value) if !ok { return false @@ -69,7 +72,10 @@ func (k Keeper) BatchVerifyNonMembership(ctx sdk.Context, proof exported.ProofI, continue } - path := types.ApplyPrefix(k.prefix, pathStr) + path, err := types.ApplyPrefix(k.prefix, pathStr) + if err != nil { + return false + } ok = proof.VerifyNonMembership(root, path) if !ok { return false diff --git a/x/ibc/23-commitment/types/merkle.go b/x/ibc/23-commitment/types/merkle.go index 6d8c644561b4..5f671304ae29 100644 --- a/x/ibc/23-commitment/types/merkle.go +++ b/x/ibc/23-commitment/types/merkle.go @@ -1,7 +1,6 @@ package types import ( - "fmt" "strings" "github.com/tendermint/tendermint/crypto/merkle" @@ -105,16 +104,17 @@ func (p Path) String() string { // // CONTRACT: provided path string MUST be a well formated path. See ICS24 for // reference. -func ApplyPrefix(prefix exported.PrefixI, path string) Path { - if !validation.DefaultPathValidator(path) { - panic(fmt.Sprintf("Path: %s is malformed", path)) +func ApplyPrefix(prefix exported.PrefixI, path string) (Path, error) { + err := validation.DefaultPathValidator(path) + if err != nil { + return Path{}, err } // Split paths by the separator pathSlice := strings.Split(path, "/") commitmentPath := NewPath(pathSlice) commitmentPath.KeyPath = commitmentPath.KeyPath.AppendKey(prefix.Bytes(), merkle.KeyEncodingHex) - return commitmentPath + return commitmentPath, nil } var _ exported.ProofI = Proof{} diff --git a/x/ibc/24-validation/validate.go b/x/ibc/24-host/validate.go similarity index 53% rename from x/ibc/24-validation/validate.go rename to x/ibc/24-host/validate.go index abba268828cc..06622048d1fb 100644 --- a/x/ibc/24-validation/validate.go +++ b/x/ibc/24-host/validate.go @@ -1,48 +1,61 @@ -package validation +package host import ( "regexp" "strings" + + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) +// ICS 024 Identifier and Path Validation Implementation +// +// This file defines ValidateFn to validate identifier and path strings +// The spec for ICS 024 can be located here: +// https://github.com/cosmos/ics/tree/master/spec/ics-024-host-requirements + // regular expression to check string is lowercase alphabetic characters only var isAlphaLower = regexp.MustCompile(`^[a-z]+$`).MatchString // regular expression to check string is alphanumeric var isAlphaNumeric = regexp.MustCompile(`^[a-zA-Z0-9]+$`).MatchString -// Validator function type to validate path and identifier bytestrings -type Validator func(string) bool +// ValidateFn function type to validate path and identifier bytestrings +type ValidateFn func(string) error // Default validator function for Client, Connection, and Channel // identifiers // Valid Identifier must be between 10-20 characters and only // contain lowercase alphabetic characters -func DefaultIdentifierValidator(id string) bool { +func DefaultIdentifierValidator(id string) error { + // valid id MUST NOT contain "/" separator + if strings.Contains(id, "/") { + return sdkerrors.Wrap(sdkerrors.ErrInvalidID, "identifier cannot contain separator: /") + } // valid id must be between 10 and 20 characters if len(id) < 10 || len(id) > 20 { - return false + return sdkerrors.Wrapf(sdkerrors.ErrInvalidID, "identifier has invalid length: %d, must be between 10-20 characters", len(id)) } // valid id must contain only lower alphabetic characters - if !isAlphaLower(string(id)) { - return false + if !isAlphaLower(id) { + return sdkerrors.Wrap(sdkerrors.ErrInvalidID, "identifier must contain only lowercase alphabetic characters") } - return true + return nil } // NewPathValidator takes in a Identifier Validator function and returns // a Path Validator function which requires path only has valid identifiers // alphanumeric character strings, and "/" separators -func NewPathValidator(idValidator Validator) Validator { - return func(path string) bool { +func NewPathValidator(idValidator ValidateFn) ValidateFn { + return func(path string) error { pathArr := strings.Split(path, "/") for _, p := range pathArr { // Each path element must either be valid identifier or alphanumeric - if !idValidator(p) && !isAlphaNumeric(p) { - return false + err := idValidator(p) + if err != nil && !isAlphaNumeric(p) { + return sdkerrors.Wrapf(sdkerrors.ErrInvalidPath, "path contains invalid identifier or non-alphanumeric path element: %s", p) } } - return true + return nil } } From bd0ff2c295ef9c1cee85e13247b7198cb17bab3e Mon Sep 17 00:00:00 2001 From: Aditya Sripal Date: Tue, 22 Oct 2019 13:38:01 -0700 Subject: [PATCH 4/4] move errors into host package --- x/ibc/23-commitment/types/merkle.go | 4 ++-- x/ibc/24-host/errors.go | 16 ++++++++++++++++ x/ibc/24-host/validate.go | 14 +++++++------- 3 files changed, 25 insertions(+), 9 deletions(-) create mode 100644 x/ibc/24-host/errors.go diff --git a/x/ibc/23-commitment/types/merkle.go b/x/ibc/23-commitment/types/merkle.go index 5f671304ae29..730fb2496f3b 100644 --- a/x/ibc/23-commitment/types/merkle.go +++ b/x/ibc/23-commitment/types/merkle.go @@ -7,7 +7,7 @@ import ( "github.com/cosmos/cosmos-sdk/store/rootmulti" "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/exported" - validation "github.com/cosmos/cosmos-sdk/x/ibc/24-validation" + host "github.com/cosmos/cosmos-sdk/x/ibc/24-host" ) // ICS 023 Merkle Types Implementation @@ -105,7 +105,7 @@ func (p Path) String() string { // CONTRACT: provided path string MUST be a well formated path. See ICS24 for // reference. func ApplyPrefix(prefix exported.PrefixI, path string) (Path, error) { - err := validation.DefaultPathValidator(path) + err := host.DefaultPathValidator(path) if err != nil { return Path{}, err } diff --git a/x/ibc/24-host/errors.go b/x/ibc/24-host/errors.go new file mode 100644 index 000000000000..e1fa5e0e7a08 --- /dev/null +++ b/x/ibc/24-host/errors.go @@ -0,0 +1,16 @@ +package host + +import ( + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" +) + +// IBCCodeSpace is the codespace for all errors defined in the ibc module +const IBCCodeSpace = "ibc" + +var ( + // ErrInvalidID is returned if identifier string is invalid + ErrInvalidID = sdkerrors.Register(IBCCodeSpace, 1, "invalid identifier") + + // ErrInvalidPath is returned if path string is invalid + ErrInvalidPath = sdkerrors.Register(IBCCodeSpace, 2, "invalid path") +) diff --git a/x/ibc/24-host/validate.go b/x/ibc/24-host/validate.go index 06622048d1fb..00dd5f8ba99e 100644 --- a/x/ibc/24-host/validate.go +++ b/x/ibc/24-host/validate.go @@ -29,15 +29,15 @@ type ValidateFn func(string) error func DefaultIdentifierValidator(id string) error { // valid id MUST NOT contain "/" separator if strings.Contains(id, "/") { - return sdkerrors.Wrap(sdkerrors.ErrInvalidID, "identifier cannot contain separator: /") + return sdkerrors.Wrap(ErrInvalidID, "identifier cannot contain separator: /") } // valid id must be between 10 and 20 characters if len(id) < 10 || len(id) > 20 { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidID, "identifier has invalid length: %d, must be between 10-20 characters", len(id)) + return sdkerrors.Wrapf(ErrInvalidID, "identifier has invalid length: %d, must be between 10-20 characters", len(id)) } // valid id must contain only lower alphabetic characters if !isAlphaLower(id) { - return sdkerrors.Wrap(sdkerrors.ErrInvalidID, "identifier must contain only lowercase alphabetic characters") + return sdkerrors.Wrap(ErrInvalidID, "identifier must contain only lowercase alphabetic characters") } return nil } @@ -52,7 +52,7 @@ func NewPathValidator(idValidator ValidateFn) ValidateFn { // Each path element must either be valid identifier or alphanumeric err := idValidator(p) if err != nil && !isAlphaNumeric(p) { - return sdkerrors.Wrapf(sdkerrors.ErrInvalidPath, "path contains invalid identifier or non-alphanumeric path element: %s", p) + return sdkerrors.Wrapf(ErrInvalidPath, "path contains invalid identifier or non-alphanumeric path element: %s", p) } } return nil @@ -62,13 +62,13 @@ func NewPathValidator(idValidator ValidateFn) ValidateFn { // Default Path Validator takes in path string and validates // with default identifier rules. This is optimized by simply // checking that all path elements are alphanumeric -func DefaultPathValidator(path string) bool { +func DefaultPathValidator(path string) error { pathArr := strings.Split(path, "/") for _, p := range pathArr { // Each path element must either be alphanumeric if !isAlphaNumeric(p) { - return false + return sdkerrors.Wrapf(ErrInvalidPath, "invalid path element containing non-alphanumeric characters: %s", p) } } - return true + return nil }