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.

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

Return error if wasm to big #287

Closed
ethanfrey opened this issue Oct 11, 2020 · 5 comments · Fixed by #302
Closed

Return error if wasm to big #287

ethanfrey opened this issue Oct 11, 2020 · 5 comments · Fixed by #302
Assignees
Milestone

Comments

@ethanfrey
Copy link
Member

Currently it cuts of the wasm at 400kb, but doesn't return an error, leading to odd error messages.

We can raise the size limit a bit, maybe 600kb. But mainly, we need to return an error on the limit. Not sure how to do it with this interface.

https://github.com/CosmWasm/cosmwasm/issues/576#issuecomment-706766984

@webmaster128
Copy link
Member

Not sure how to do it with this interface.

If there is no fancy API for that, you can set maxSize to the max Wasm size that is allowed and read up to maxSize + 1 bytes. Now if the result length is > maxSize, then the Wasm exceeded the limit. Like

decompressed = ioutil.ReadAll(io.LimitReader(zr, maxSize +1))
if len(decompressed) > maxSize {
  // error
}
return decompressed

@webmaster128
Copy link
Member

Since this is probably censensus breaking, I suggest adding this to 0.12.0.

@webmaster128 webmaster128 added this to the v0.12.0 milestone Oct 12, 2020
@webmaster128
Copy link
Member

webmaster128 commented Oct 12, 2020

Changing the value if the limit is now tracked separately: #289

@ethanfrey
Copy link
Member Author

I think leaving the same limit and returning a better error message is not consensus breaking. As long as it still succeeds at 400kb and fails at 400kb+1. The error message is not part of consensus (the code is however, so we make sure we return the same error code).

The further improvements (adjustable limit, new code, etc) should definitely be in 0.12. I would like to pull a non breaking part into 0.11 to avoid confusion from new devs

@webmaster128
Copy link
Member

webmaster128 commented Oct 12, 2020

The error code is one thing to check. In addition to that, I am realtively sure you can create a large Wasm file that is perfectly valid when truncated after 400 KB. This way cou can attack consesus. Would be a nice side-challenge for the hacaktom.

We can do this in a patch release, but add a big warning. Since there is no large 0.11 network yet, I assume we don't have to expect 0.11.1 and 0.11.2 mixed networks.

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 a pull request may close this issue.

3 participants