From 6be6e71bb528fe60fd7f7d02e00695d9ded55f98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Augusto=20Elesb=C3=A3o?= Date: Thu, 15 Aug 2024 23:30:43 -0300 Subject: [PATCH 1/2] fix: apply patch for CWA-2024-003 --- x/wasm/types/params.go | 21 ++------------- x/wasm/types/tx.go | 38 ++++++--------------------- x/wasm/types/tx_test.go | 54 ++++++++++++++++++++++++++++++++++++++ x/wasm/types/validation.go | 32 ++++++++++++++++++++++ 4 files changed, 96 insertions(+), 49 deletions(-) diff --git a/x/wasm/types/params.go b/x/wasm/types/params.go index 4b923b1bd3..c5cfb67229 100644 --- a/x/wasm/types/params.go +++ b/x/wasm/types/params.go @@ -29,7 +29,7 @@ func (a AccessType) With(addrs ...sdk.AccAddress) AccessConfig { for i, v := range addrs { bech32Addrs[i] = v.String() } - if err := assertValidAddresses(bech32Addrs); err != nil { + if err := validateBech32Addresses(bech32Addrs); err != nil { panic(errorsmod.Wrap(err, "addresses")) } return AccessConfig{Permission: AccessTypeAnyOfAddresses, Addresses: bech32Addrs} @@ -129,28 +129,11 @@ func (a AccessConfig) ValidateBasic() error { case AccessTypeNobody, AccessTypeEverybody: return nil case AccessTypeAnyOfAddresses: - return errorsmod.Wrap(assertValidAddresses(a.Addresses), "addresses") + return errorsmod.Wrap(validateBech32Addresses(a.Addresses), "addresses") } return errorsmod.Wrapf(ErrInvalid, "unknown type: %q", a.Permission) } -func assertValidAddresses(addrs []string) error { - if len(addrs) == 0 { - return ErrEmpty - } - idx := make(map[string]struct{}, len(addrs)) - for _, a := range addrs { - if _, err := sdk.AccAddressFromBech32(a); err != nil { - return errorsmod.Wrapf(err, "address: %s", a) - } - if _, exists := idx[a]; exists { - return ErrDuplicate.Wrapf("address: %s", a) - } - idx[a] = struct{}{} - } - return nil -} - // Allowed returns if permission includes the actor. // Actor address must be valid and not nil func (a AccessConfig) Allowed(actor sdk.AccAddress) bool { diff --git a/x/wasm/types/tx.go b/x/wasm/types/tx.go index ecd68bddf4..057f77ff0b 100644 --- a/x/wasm/types/tx.go +++ b/x/wasm/types/tx.go @@ -12,6 +12,8 @@ import ( sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) +const maxCodeIDCount = 50 + // RawContractMessage defines a json message that is sent or returned by a wasm contract. // This type can hold any type of bytes. Until validateBasic is called there should not be // any assumptions made that the data is valid syntax or semantic. @@ -356,15 +358,14 @@ func (msg MsgPinCodes) ValidateBasic() error { return validateCodeIDs(msg.CodeIDs) } -const maxCodeIDTotal = 50 - -// ensure not empty, not duplicates and not exceeding max number +// validateCodeIDs ensures the list is not empty, has no duplicates +// and does not exceed the max number of code IDs func validateCodeIDs(codeIDs []uint64) error { switch n := len(codeIDs); { case n == 0: return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "empty code ids") - case n > maxCodeIDTotal: - return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "total number of code ids is greater than %d", maxCodeIDTotal) + case n > maxCodeIDCount: + return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "total number of code ids is greater than %d", maxCodeIDCount) } if hasDuplicates(codeIDs) { return errorsmod.Wrap(sdkerrors.ErrInvalidRequest, "duplicate code ids") @@ -468,11 +469,7 @@ func (msg MsgAddCodeUploadParamsAddresses) ValidateBasic() error { return errorsmod.Wrap(err, "authority") } - if len(msg.Addresses) == 0 { - return errorsmod.Wrap(ErrEmpty, "addresses") - } - - return checkDuplicatedAddresses(msg.Addresses) + return validateBech32Addresses(msg.Addresses) } func (msg MsgRemoveCodeUploadParamsAddresses) Route() string { @@ -488,26 +485,7 @@ func (msg MsgRemoveCodeUploadParamsAddresses) ValidateBasic() error { return errorsmod.Wrap(err, "authority") } - if len(msg.Addresses) == 0 { - return errorsmod.Wrap(ErrEmpty, "addresses") - } - - return checkDuplicatedAddresses(msg.Addresses) -} - -func checkDuplicatedAddresses(addresses []string) error { - index := map[string]struct{}{} - for _, addr := range addresses { - addr = strings.ToUpper(addr) - if _, err := sdk.AccAddressFromBech32(addr); err != nil { - return errorsmod.Wrap(err, "addresses") - } - if _, found := index[addr]; found { - return errorsmod.Wrap(ErrInvalid, "duplicate addresses") - } - index[addr] = struct{}{} - } - return nil + return validateBech32Addresses(msg.Addresses) } func (msg MsgStoreAndMigrateContract) Route() string { diff --git a/x/wasm/types/tx_test.go b/x/wasm/types/tx_test.go index 1045b95028..6f237b11a6 100644 --- a/x/wasm/types/tx_test.go +++ b/x/wasm/types/tx_test.go @@ -763,6 +763,13 @@ func TestMsgAddCodeUploadParamsAddressesValidation(t *testing.T) { badAddress := "abcd" // proper address size goodAddress := sdk.AccAddress(make([]byte, 20)).String() + goodAddress2 := strings.ToUpper(goodAddress) + require.NotEqual(t, goodAddress, goodAddress2) // sanity check + + tooManyAddresses := make([]string, MaxAddressCount+1) + for i := range tooManyAddresses { + tooManyAddresses[i] = sdk.AccAddress(bytes.Repeat([]byte{byte(i)}, 20)).String() + } specs := map[string]struct { src MsgAddCodeUploadParamsAddresses @@ -774,6 +781,12 @@ func TestMsgAddCodeUploadParamsAddressesValidation(t *testing.T) { Addresses: []string{goodAddress}, }, }, + "all good, uppercase": { + src: MsgAddCodeUploadParamsAddresses{ + Authority: goodAddress, + Addresses: []string{goodAddress2}, + }, + }, "bad authority": { src: MsgAddCodeUploadParamsAddresses{ Authority: badAddress, @@ -807,6 +820,20 @@ func TestMsgAddCodeUploadParamsAddressesValidation(t *testing.T) { }, expErr: true, }, + "duplicate addresses, different casing": { + src: MsgAddCodeUploadParamsAddresses{ + Authority: goodAddress, + Addresses: []string{goodAddress, goodAddress2}, + }, + expErr: true, + }, + "too many addresses": { + src: MsgAddCodeUploadParamsAddresses{ + Authority: goodAddress, + Addresses: tooManyAddresses, + }, + expErr: true, + }, } for msg, spec := range specs { t.Run(msg, func(t *testing.T) { @@ -823,6 +850,13 @@ func TestMsgAddCodeUploadParamsAddressesValidation(t *testing.T) { func TestMsgRemoveCodeUploadParamsAddressesValidation(t *testing.T) { // proper address size goodAddress := sdk.AccAddress(make([]byte, 20)).String() + goodAddress2 := strings.ToUpper(goodAddress) + require.NotEqual(t, goodAddress, goodAddress2) // sanity check + + tooManyAddresses := make([]string, MaxAddressCount+1) + for i := range tooManyAddresses { + tooManyAddresses[i] = sdk.AccAddress(bytes.Repeat([]byte{byte(i)}, 20)).String() + } specs := map[string]struct { src MsgRemoveCodeUploadParamsAddresses @@ -834,6 +868,12 @@ func TestMsgRemoveCodeUploadParamsAddressesValidation(t *testing.T) { Addresses: []string{goodAddress}, }, }, + "all good, uppercase": { + src: MsgRemoveCodeUploadParamsAddresses{ + Authority: goodAddress, + Addresses: []string{goodAddress2}, + }, + }, "bad authority": { src: MsgRemoveCodeUploadParamsAddresses{ Authority: badAddress, @@ -867,6 +907,20 @@ func TestMsgRemoveCodeUploadParamsAddressesValidation(t *testing.T) { }, expErr: true, }, + "duplicate addresses, different casing": { + src: MsgRemoveCodeUploadParamsAddresses{ + Authority: goodAddress, + Addresses: []string{goodAddress, goodAddress2}, + }, + expErr: true, + }, + "too many addresses": { + src: MsgRemoveCodeUploadParamsAddresses{ + Authority: goodAddress, + Addresses: tooManyAddresses, + }, + expErr: true, + }, } for msg, spec := range specs { t.Run(msg, func(t *testing.T) { diff --git a/x/wasm/types/validation.go b/x/wasm/types/validation.go index bd67e3bc26..f10de3d410 100644 --- a/x/wasm/types/validation.go +++ b/x/wasm/types/validation.go @@ -9,6 +9,9 @@ import ( "github.com/distribution/reference" errorsmod "cosmossdk.io/errors" + + sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" ) // MaxSaltSize is the longest salt that can be used when instantiating a contract @@ -23,6 +26,9 @@ var ( // MaxProposalWasmSize is the largest a gov proposal compiled contract code can be when storing code on chain MaxProposalWasmSize = 3 * 1024 * 1024 // extension point for chains to customize via compile flag. + + // MaxAddressCount is the maximum number of addresses allowed within a message + MaxAddressCount = 50 ) func validateWasmCode(s []byte, maxSize int) error { @@ -92,3 +98,29 @@ func ValidateVerificationInfo(source, builder string, codeHash []byte) error { } return nil } + +// validateBech32Addresses ensures the list is not empty, has no duplicates +// and does not exceed the max number of addresses +func validateBech32Addresses(addresses []string) error { + switch n := len(addresses); { + case n == 0: + return errorsmod.Wrap(ErrEmpty, "addresses") + case n > MaxAddressCount: + return errorsmod.Wrapf(sdkerrors.ErrInvalidRequest, "total number of addresses is greater than %d", MaxAddressCount) + } + + index := map[string]struct{}{} + for _, addr := range addresses { + if _, err := sdk.AccAddressFromBech32(addr); err != nil { + return errorsmod.Wrapf(err, "address: %s", addr) + } + // Bech32 addresses are case-insensitive, i.e. the same address can have multiple representations, + // so we normalize here to avoid duplicates. + addr = strings.ToUpper(addr) + if _, found := index[addr]; found { + return errorsmod.Wrap(ErrDuplicate, "duplicate addresses") + } + index[addr] = struct{}{} + } + return nil +} From 91c89083f2b226c2a3d9f1c5ea1030b1b7cda9dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Augusto=20Elesb=C3=A3o?= Date: Fri, 16 Aug 2024 00:07:02 -0300 Subject: [PATCH 2/2] fix: apply patch for CWA-2024-004 --- go.mod | 2 +- go.sum | 4 ++-- x/wasm/keeper/keeper_test.go | 4 ++-- x/wasm/keeper/recurse_test.go | 16 ++++++++-------- x/wasm/keeper/submsg_test.go | 4 ++-- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index 2938af4151..52bf81f6a3 100644 --- a/go.mod +++ b/go.mod @@ -3,7 +3,7 @@ module github.com/CosmWasm/wasmd go 1.21 require ( - github.com/CosmWasm/wasmvm v1.5.0 + github.com/CosmWasm/wasmvm v1.5.4 github.com/cosmos/cosmos-proto v1.0.0-beta.3 github.com/cosmos/cosmos-sdk v0.50.1 github.com/cosmos/gogogateway v1.2.0 // indirect diff --git a/go.sum b/go.sum index e65d3b01ae..7660e1d0f4 100644 --- a/go.sum +++ b/go.sum @@ -228,8 +228,8 @@ github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161 h1:L/gRVlceqvL25 github.com/Azure/go-ansiterm v0.0.0-20230124172434-306776ec8161/go.mod h1:xomTg63KZ2rFqZQzSB4Vz2SUXa1BpHTVz9L5PTmPC4E= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= -github.com/CosmWasm/wasmvm v1.5.0 h1:3hKeT9SfwfLhxTGKH3vXaKFzBz1yuvP8SlfwfQXbQfw= -github.com/CosmWasm/wasmvm v1.5.0/go.mod h1:fXB+m2gyh4v9839zlIXdMZGeLAxqUdYdFQqYsTha2hc= +github.com/CosmWasm/wasmvm v1.5.4 h1:Opqy65ubJ8bMsT08dn85VjRdsLJVPIAgIXif92qOMGc= +github.com/CosmWasm/wasmvm v1.5.4/go.mod h1:Q0bSEtlktzh7W2hhEaifrFp1Erx11ckQZmjq8FLCyys= github.com/DataDog/datadog-go v3.2.0+incompatible/go.mod h1:LButxg5PwREeZtORoXG3tL4fMGNddJ+vMq1mwgfaqoQ= github.com/DataDog/zstd v1.5.5 h1:oWf5W7GtOLgp6bciQYDmhHHjdhYkALu6S/5Ni9ZgSvQ= github.com/DataDog/zstd v1.5.5/go.mod h1:g4AWEaM3yOg3HYfnJ3YIawPnVdXJh9QME85blwSAmyw= diff --git a/x/wasm/keeper/keeper_test.go b/x/wasm/keeper/keeper_test.go index 213291edf1..bcd86e526e 100644 --- a/x/wasm/keeper/keeper_test.go +++ b/x/wasm/keeper/keeper_test.go @@ -419,7 +419,7 @@ func TestInstantiate(t *testing.T) { gasAfter := ctx.GasMeter().GasConsumed() if types.EnableGasVerification { - require.Equal(t, uint64(0x1e018), gasAfter-gasBefore) + require.Equal(t, uint64(0x1e043), gasAfter-gasBefore) } // ensure it is stored properly @@ -876,7 +876,7 @@ func TestExecute(t *testing.T) { // make sure gas is properly deducted from ctx gasAfter := ctx.GasMeter().GasConsumed() if types.EnableGasVerification { - require.Equal(t, uint64(0x1b31c), gasAfter-gasBefore) + require.Equal(t, uint64(0x1b366), gasAfter-gasBefore) } // ensure bob now exists and got both payments released bobAcct = accKeeper.GetAccount(ctx, bob) diff --git a/x/wasm/keeper/recurse_test.go b/x/wasm/keeper/recurse_test.go index 831d909cca..633c31d7c0 100644 --- a/x/wasm/keeper/recurse_test.go +++ b/x/wasm/keeper/recurse_test.go @@ -56,12 +56,12 @@ func initRecurseContract(t *testing.T) (contract sdk.AccAddress, ctx sdk.Context func TestGasCostOnQuery(t *testing.T) { const ( - GasNoWork uint64 = 63_950 + GasNoWork uint64 = 63_983 // Note: about 100 SDK gas (10k CosmWasm gas) for each round of sha256 - GasWork50 uint64 = 64_218 // this is a little shy of 50k gas - to keep an eye on the limit + GasWork50 uint64 = 64_222 // this is a little shy of 50k gas - to keep an eye on the limit - GasReturnUnhashed uint64 = 32 - GasReturnHashed uint64 = 26 + GasReturnUnhashed uint64 = 55 + GasReturnHashed uint64 = 46 ) cases := map[string]struct { @@ -211,9 +211,9 @@ func TestLimitRecursiveQueryGas(t *testing.T) { const ( // Note: about 100 SDK gas (10k CosmWasm gas) for each round of sha256 - GasWork2k uint64 = 77_161 // = NewContractInstanceCosts + x // we have 6x gas used in cpu than in the instance + GasWork2k uint64 = 76_279 // = NewContractInstanceCosts + x // we have 6x gas used in cpu than in the instance // This is overhead for calling into a sub-contract - GasReturnHashed uint64 = 27 + GasReturnHashed uint64 = 48 ) cases := map[string]struct { @@ -263,8 +263,8 @@ func TestLimitRecursiveQueryGas(t *testing.T) { }, expectQueriesFromContract: 10, expectOutOfGas: false, - expectError: "query wasm contract failed", // Error we get from the contract instance doing the failing query, not wasmd - expectedGas: 10*(GasWork2k+GasReturnHashed) - 249 + 901, // NOTE: investigation on this is required, higher consumption than vanila wasmd, also needs quantification instead of arbitrary + expectError: "query wasm contract failed", // Error we get from the contract instance doing the failing query, not wasmd + expectedGas: 10*(GasWork2k+GasReturnHashed) + 631, // NOTE: investigation on this is required, higher consumption than vanila wasmd, also needs quantification instead of arbitrary }, } diff --git a/x/wasm/keeper/submsg_test.go b/x/wasm/keeper/submsg_test.go index b1f709fcdf..00de4c485f 100644 --- a/x/wasm/keeper/submsg_test.go +++ b/x/wasm/keeper/submsg_test.go @@ -243,14 +243,14 @@ func TestDispatchSubMsgErrorHandling(t *testing.T) { "send tokens": { submsgID: 5, msg: validBankSend, - resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(111000, 112000)}, + resultAssertions: []assertion{assertReturnedEvents(0), assertGasUsed(110_000, 114_000)}, }, "not enough tokens": { submsgID: 6, msg: invalidBankSend, subMsgError: true, // uses less gas than the send tokens (cost of bank transfer) - resultAssertions: []assertion{assertGasUsed(83000, 84000), assertErrorString("codespace: sdk, code: 5")}, + resultAssertions: []assertion{assertGasUsed(83000, 85000), assertErrorString("codespace: sdk, code: 5")}, }, "out of gas panic with no gas limit": { submsgID: 7,