-
Notifications
You must be signed in to change notification settings - Fork 128
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
base: main
Are you sure you want to change the base?
Conversation
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. |
This is the thing: they don't.
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 occurThat'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 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.... |
I think I could have been wrong, though I still disagree with design here. Also, I really am quite bad at comms. Cheers! |
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:
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.