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

Jump to bottom

Validation of string length is not present #4492

Closed
ljah8 opened this issue Mar 3, 2023 · 2 comments
Closed

Validation of string length is not present #4492

ljah8 opened this issue Mar 3, 2023 · 2 comments
Labels
informal-audit Informal Systems' audit output

Comments

@ljah8
Copy link
Contributor

ljah8 commented Mar 3, 2023

Involved artifacts

Description

During the testing of memo format and its validation, mainly by using already written ibc integration tests, we were able to insert a very large string into the memo field of the packet data, specifically the string was inserted in the raw_message_data:

{
    //... other ibc fields that we don't care about
    "data":{
    	"denom": "denom on counterparty chain (e.g. uatom)",  // will be transformed to the local denom (ibc/...)
        "amount": "1000",
        "sender": "addr on counterparty chain", // will be transformed
        "receiver": "contract addr or blank",
    	"memo": {
           "wasm": {
              "contract": "osmo1contractAddr",
              "msg": {
                "raw_message_fields": "raw_message_data",
              }
            }
        }
    }
}

The test was run with this newly changed memo, and as a result, a panic with the following trace occurred:

 Error:          Received unexpected error:
                [github.com/cosmos/cosmos-sdk/x/auth/ante.SetUpContextDecorator.AnteHandle.func1](http://github.com/cosmos/cosmos-sdk/x/auth/ante.SetUpContextDecorator.AnteHandle.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/auth/ante/setup.go:57
                runtime.gopanic
                        /home/ivan/.go/src/runtime/panic.go:884
                [github.com/cosmos/cosmos-sdk/store/types.(*basicGasMeter).ConsumeGas](http://github.com/cosmos/cosmos-sdk/store/types.(*basicGasMeter).ConsumeGas)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/store/types/gas.go:99
                [github.com/cosmos/cosmos-sdk/x/auth/ante.ConsumeTxSizeGasDecorator.AnteHandle](http://github.com/cosmos/cosmos-sdk/x/auth/ante.ConsumeTxSizeGasDecorator.AnteHandle)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/auth/ante/basic.go:95
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/cosmos/cosmos-sdk/x/auth/ante.ValidateMemoDecorator.AnteHandle](http://github.com/cosmos/cosmos-sdk/x/auth/ante.ValidateMemoDecorator.AnteHandle)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/auth/ante/basic.go:66
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/cosmos/cosmos-sdk/x/auth/ante.TxTimeoutHeightDecorator.AnteHandle](http://github.com/cosmos/cosmos-sdk/x/auth/ante.TxTimeoutHeightDecorator.AnteHandle)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/auth/ante/basic.go:205
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/cosmos/cosmos-sdk/x/auth/ante.ValidateBasicDecorator.AnteHandle](http://github.com/cosmos/cosmos-sdk/x/auth/ante.ValidateBasicDecorator.AnteHandle)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/auth/ante/basic.go:34
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/osmosis-labs/osmosis/v14/ante.(*SendBlockDecorator).AnteHandle](http://github.com/osmosis-labs/osmosis/v14/ante.(*SendBlockDecorator).AnteHandle)
                        /home/ivan/Projects/osmosis/ante/sendblock.go:61
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/osmosis-labs/osmosis/v14/x/txfees/keeper.MempoolFeeDecorator.AnteHandle](http://github.com/osmosis-labs/osmosis/v14/x/txfees/keeper.MempoolFeeDecorator.AnteHandle)
                        /home/ivan/Projects/osmosis/x/txfees/keeper/feedecorator.go:92
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/osmosis-labs/osmosis/v14/app/upgrades/v9.MsgFilterDecorator.AnteHandle](http://github.com/osmosis-labs/osmosis/v14/app/upgrades/v9.MsgFilterDecorator.AnteHandle)
                        /home/ivan/Projects/osmosis/app/upgrades/v9/msg_filter_ante.go:23
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/cosmos/cosmos-sdk/x/auth/ante.RejectExtensionOptionsDecorator.AnteHandle](http://github.com/cosmos/cosmos-sdk/x/auth/ante.RejectExtensionOptionsDecorator.AnteHandle)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/auth/ante/ext.go:35
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/CosmWasm/wasmd/x/wasm/keeper.CountTXDecorator.AnteHandle](http://github.com/CosmWasm/wasmd/x/wasm/keeper.CountTXDecorator.AnteHandle)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/wasm/keeper/ante.go:44
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/CosmWasm/wasmd/x/wasm/keeper.LimitSimulationGasDecorator.AnteHandle](http://github.com/CosmWasm/wasmd/x/wasm/keeper.LimitSimulationGasDecorator.AnteHandle)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/wasm/keeper/ante.go:83
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/cosmos/cosmos-sdk/x/auth/ante.SetUpContextDecorator.AnteHandle](http://github.com/cosmos/cosmos-sdk/x/auth/ante.SetUpContextDecorator.AnteHandle)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/x/auth/ante/setup.go:64
                [github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1](http://github.com/cosmos/cosmos-sdk/types.ChainAnteDecorators.func1)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/types/handler.go:40
                [github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runTx](http://github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).runTx)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/baseapp/baseapp.go:709
                [github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).Deliver](http://github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).Deliver)
                        /home/ivan/go/pkg/mod/github.com/osmosis-labs/[email protected]/baseapp/test_helpers.go:33
                [github.com/cosmos/ibc-go/v4/testing/simapp.SignAndDeliver](http://github.com/cosmos/ibc-go/v4/testing/simapp.SignAndDeliver)
                        /home/ivan/go/pkg/mod/github.com/cosmos/ibc-go/[email protected]/testing/simapp/test_helpers.go:256
                [github.com/cosmos/ibc-go/v4/testing.(*TestChain).SendMsgs](http://github.com/cosmos/ibc-go/v4/testing.(*TestChain).SendMsgs)
                        /home/ivan/go/pkg/mod/github.com/cosmos/ibc-go/[email protected]/testing/chain.go:312
                [github.com/cosmos/ibc-go/v4/testing.(*Endpoint).RecvPacketWithResult](http://github.com/cosmos/ibc-go/v4/testing.(*Endpoint).RecvPacketWithResult)
                        /home/ivan/go/pkg/mod/github.com/cosmos/ibc-go/[email protected]/testing/endpoint.go:409
                [github.com/osmosis-labs/osmosis/v14/tests/ibc-hooks_test.(*HooksTestSuite).receivePacketWithSequence](http://github.com/osmosis-labs/osmosis/v14/tests/ibc-hooks_test.(*HooksTestSuite).receivePacketWithSequence)
                        /home/ivan/Projects/osmosis/tests/ibc-hooks/ibc_middleware_test.go:195
                out of gas in location: txSize; gasWanted: 1000000, gasUsed: 1340958: out of gas
Test:           TestIBCHooksTestSuite/TestRecvTransferWithMetadata

It is clear that panic was a result of gas usage handling, which is expected, but it seems that it would be more useful and more informative to set up input string validation and return a more exploratory error to inform the user why the transaction failed.

In addition to the case mentioned above, it is noticed that string length validation is rarely used across the module. It could be useful to prevent errors deeper in the execution and inform the user what exactly went wrong with the input.

After consulting with the Osmosis team and their recommendation to apply a limit to length of all the strings in the packet and performed analysis to check if there are any other vulnerable points, it seems that the proposed solution should be enough since all the strings that could be maliciously inserted in such a length to cause panic actually originate from packet.
@ljah8 ljah8 added the informal-audit Informal Systems' audit output label Mar 3, 2023
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Osmosis Chain Development Mar 3, 2023
@nicolaslara
Copy link
Contributor

I think this is behaving as we want it for now. There are use cases where users may need longer memos, and it feels like we would need to keep updating those lengths if they are limited.

@ValarDragon
Copy link
Member

Thanks for the issue! I think we want to not add a length limit, and leave fixing this to the role of fee markets. Thank you!

@github-project-automation github-project-automation bot moved this from Needs Triage 🔍 to Done ✅ in Osmosis Chain Development Aug 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
informal-audit Informal Systems' audit output
Projects
Osmosis Chain Development
Archived in project
Milestone
No milestone
Development

No branches or pull requests
3 participants
@nicolaslara @ValarDragon @ljah8