Use an Allow list for StargateQuery instead of completely disabling it

[nicolaslara]

I was discussing the need to disable StargateQuery in the discord channel to better understand why they're completely disabled (https://discord.com/channels/737637324434833438/737643344712171600/998624457188987030).

I would like to suggest replacing the current handler with one that contains an allow list. This way any chain that uses wasmd and has the stargate feature enabled, can just customize the allow list for the queries. If the chain takes no action, the behaviour would be equivalent to the current one (

wasmd/x/wasm/keeper/query_plugins.go

Line 275 in 7c7840c

return nil, wasmvmtypes.UnsupportedRequest{Kind: "Stargate queries are disabled."}

). While chains can customize this manually by replacing the stargate querier, I think it feels inconsistent to have StargateQuery on cosmwasm and have it be useless in wasmd.

As a reference, here is the previous DenyList implementation: https://github.com/CosmWasm/wasmd/blob/v0.25.0/x/wasm/keeper/query_plugins.go#L281 (thanks @jhernandezb!)

My interest in this comes from the decision from osmosis to use StargateMsg/StargateQuery for cosmwasm<->osmosis communication instead of custom bindings (osmosis-labs/osmosis#1484 (comment)).

If this is of interest, I can provide a PR.

[jhernandezb]

Also worth to mention the amount of work that requires adding a binding in upstream for external teams
see: #903

[mattverse]

I'm currently working on this in the osmosis repo for wasmd, can get this upstreamed as I get this finished for further reviews! (hopefully wouldn't take so long)

[ethanfrey]

Allowing the StargateQuery was the cause of 2 critical security issues, introducing non-determinism. One of them was used in the Juno halt... 6 weeks after it was patched in a wasmd release, showing that we need to plan time to roll out fixes on multiple chains and need to be very conservative about allowing this again.

The SDK team said they give no guarantees on the stability and determinism of any grpc query. Even though this would be a requirement when implementing ADR 33, which they do plan to implement.

That means, exposing this may require a deep review if we consider even a patch upgrade of the Cosmos SDK dependency without a hard fork. Did any queries change? Did they introduce maps or other randomness in those?

I am sure an allow list could work, but having been burnt twice and not getting upstream support for this, means we are very hesitant about adding this.

[ethanfrey]

TL;DR it is not about the work to do so, it is about the danger of introducing a chain halt.

[mattverse]

yeah it makes sense to have each separate downstream fork implement it on their own forks, but not support it in upstream level! thanks for the explanation :)

[ethanfrey]

I mean, adding common hooks to wasmd to allow downstream to create an allow list, but keeping it empty by default could work.

I hesitate to enable any by default, but if the downstream wants to, we can make it easier

[nicolaslara]

Thanks for the detailed explanation Ethan!

That means, exposing this may require a deep review if we consider even a patch upgrade of the Cosmos SDK dependency without a hard fork. Did any queries change? Did they introduce maps or other randomness in those?

☝️ This is the part I was missing. We still run the same risks when exposing those modules via Custom queries, though, but maybe the extra work needed to write those queries ensures that chain devs do their due diligence here.

I'm starting to think the actual solution here is having a guide of what to look for when exposing modules to cosmwasm. For example, I would expect randomness and arch dependent implementations (i.e.: floats) to be an issue, but I was surprised maps also introduced non-determinism.

[ethanfrey]

We still run the same risks when exposing those modules via Custom queries

Generally not so, as you parse the data from the sdk and produce a limited subset to the contract. If you control that subset, it should be fine. eg. if you have a list, ensure to sort it in your custom querier. Any extra fields would be filtered out.

[ethanfrey]

but I was surprised maps also introduced non-determinism.

This is a joy of Golang. And they use maps quite a lot in the sdk. If fact, committing multi-store iterates over maps, so the order in which it commits substores is different on each node (a fact I discovered when debugging). Seems it doesn't affect the final state / app hash, but rather annoying on debug.