Skip to content

feat: tests, grpc server #676

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.

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

Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft

feat: tests, grpc server #676

wants to merge 26 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented May 25, 2025

Avoid ffi by making a contract rpc but also add tons of new tests, including tests that it was previously impossible to write because we've now got a .proto definition for a contract virtual machine, and from there we can actually see where issues are.

So it's also in some way a security disclosure.

Reported by me to Mag and Barry of Interchain Labs in October 2024. Back then I was under the impression that things like this were going to get fixed silently -- as they should be. Unfortunately, I was wrong in my impression that security items would get fixed in a clear and timely manner, and full disclosure is better than partial disclosure, so here it is.

Claude got a little carried away with emojis in the readme, but I am just as confident as claude is that there are some serious oopsies here, mainly of the "you put invalid data in field, chain die now" variety, but possibly others. As I explained repeatedly to Mag and Barry, it's a broad set of issues with varying impacts here, because wasmvm is very loose. If wasmvm were tightened, and made testable, then Cosmos would have a safe, secure, modern smart contract platform. The CW language is really pretty good.

So there's stuff that you can do that should be impossible for you to do, and the result of you doing that stuff on mainnets is unknown, but I reckon that they'd just crash and not come back up easy.

To be clear, the only reason this is being reported in public is that reporting it in private repeatedly failed due to the issue having actual complexity, and no one having time / desire to deal with difficult/complex issues.

In short, wasmvm fails to validate many seemingly consequential items:

  • Addresses
  • Checksums
  • Sizes
  • Gas (it doesn't get checked much)

The service definition adds a layer of validation before anything can reach the virtual machine, and aids in making problem areas show clearly.

Note that thus work is being done entirely unpaid and unguided. What I know for sure is that we need to improve things. If a specific team using CosmWasm would like to sponsor the work, I'm happy to license it exclusively to that team.

@faddat faddat marked this pull request as draft May 25, 2025 12:44
@faddat faddat changed the title faddat/grpc feat: tests, grpc server May 25, 2025
@chipshort
Copy link
Collaborator

In short, wasmvm fails to validate many seemingly consequential items:

* Addresses

* Checksums

* Sizes

* Gas (it doesn't get checked much)

The service definition adds a layer of validation before anything can reach the virtual machine, and aids in making problem areas show clearly.

WasmVM is strictly just a binding layer providing a Go API for cosmwasm-vm. It gives no additional security guarantees on top of that. All checks like this need to be done in either cosmwasm-vm or wasmd.

@faddat
Copy link
Contributor Author

faddat commented May 27, 2025

All checks like this need to be done in either cosmwasm-vm or wasmd.

This is the thing: they don't.

All checks like this need to be done in either cosmwasm-vm or wasmd.

doesn't this library depend on cosmwasm-vm? So if the checks existed, they'd work here?

Why would it ever let them through?

We have no definition of what's valid. The .proto file provides that definition and then we can have security at each layer.

We shouldn't have any deliberately insecure layer, that is where bad stuff can happen.

Additionally, we both know well that if those tests were added, they'd fail without changes to code, because we don't validate some of those things some of the time. And this could of course result in chain halts. So basically, for the issue that we are both aware of, the fix should actually occur here because by having strict definitions here we are able to ensure that an entire class of issues is made impossible.

I would like to change the design from one that permits a very large class of issues to one that ensures that these issues never occur

That's why we should adopt a standard, because then the expectations for each piece of software are clear.

Every layer should enforce security, and no layer should blindly pass on invalid data. We can do that by having a clear definition of what is acceptable and sharing it across the layers. That's the purpose of the .proto file.

In terms of the process separation, it's not ideal for speed, but I figure that the safety that we get by separating the processes is a worthy trade-off. I don't think we take more than a 10% hit, and we actually are able to gain a number of performance features and plan features.

I mean think of the implications of this on the IBC WASM VM.

in short, I am arguing that no component of a secure system should ever pass along invalid state.

If you're suggesting that I write an RPC server for cosmwasm in vm package in the cosmwasm repo, that might make sense? Is that what you mean?

That might let us stop using this repository altogether and that might be good.

@chipshort - basically if I'm totally wrong, please just lmk, something like "you're totally wrong", because....

@faddat faddat closed this May 27, 2025
@faddat
Copy link
Contributor Author

faddat commented May 28, 2025

I think I could have been wrong, though I still disagree with design here.

Also, I really am quite bad at comms.

Cheers!

@faddat faddat reopened this Jun 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants