Reject wasm code exceeding limit #302
Conversation
Codecov Report
@@ Coverage Diff @@
## master #302 +/- ##
==========================================
+ Coverage 17.23% 17.40% +0.16%
==========================================
Files 35 35
Lines 10463 10471 +8
==========================================
+ Hits 1803 1822 +19
+ Misses 8563 8551 -12
- Partials 97 98 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Looks good. I am scratching my head how it is sure that limit passes and limit+1 fails. But tests pass for those cases and your understanding of the go stdlib is better than mine.
You can merge as is or attempt to add some more defensive code there if you think my worry is justified.
| if l.r.N <= 0 { | ||
| return 0, types.ErrLimit | ||
| } | ||
| return l.r.Read(p) |
There was a problem hiding this comment.
ReadAll keeps reading until one read returns (0, nil) or any return (x, err) correct?
Update: checked ioutil and bytes.Buffer. They only stop when it hits an error. If the error is EOF, then it stops and return data with no error, otherwise, it returns the error.
Seems fine, just note this returns an error if the input is the exact size of n.
There was a problem hiding this comment.
Or maybe, so the read, then check if it overflowed? Or maybe I don't fully understand how LimitedReader works.
| @@ -28,6 +30,24 @@ func uncompress(src []byte) ([]byte, error) { | |||
| return nil, err | |||
| } | |||
| zr.Multistream(false) | |||
| defer zr.Close() | |||
| return ioutil.ReadAll(LimitReader(zr, int64(limit))) | |||
There was a problem hiding this comment.
This means if the total size >= limit we will get the types.ErrLimit error. However, we want total_size > limit.
I would use int64(limit + 1) here, so if there is one byte more than the requested limit, it returns an error
| @@ -57,31 +60,38 @@ func TestUncompress(t *testing.T) { | |||
| src: wasmGzipped[:len(wasmGzipped)-5], | |||
| expError: io.ErrUnexpectedEOF, | |||
| }, | |||
| "handle limit gzip output": { | |||
There was a problem hiding this comment.
Huh, this passes? That means it can return (n, EOF) where n > 0?
The test cases (both limit and limit+1 gzips) pass. So I can approve it. Just seems there is some possible way something can slip through.
| @@ -110,6 +110,12 @@ func (k Keeper) getInstantiateAccessConfig(ctx sdk.Context) types.AccessType { | |||
| return a | |||
| } | |||
|
|
|||
| func (k Keeper) GetMaxWasmCodeSize(ctx sdk.Context) uint64 { | |||
There was a problem hiding this comment.
Okay, nice to add the parameter here - this is set in genesis, right?
| @@ -32,6 +32,7 @@ message Params { | |||
| option (gogoproto.goproto_stringer) = false; | |||
| AccessConfig code_upload_access = 1 [(gogoproto.nullable) = false, (gogoproto.moretags) = "yaml:\"code_upload_access\""]; | |||
| AccessType instantiate_default_permission = 2 [(gogoproto.moretags) = "yaml:\"instantiate_default_permission\""]; | |||
| uint64 max_wasm_code_size = 3 [(gogoproto.moretags) = "yaml:\"max_wasm_code_size\""]; | |||
There was a problem hiding this comment.
Yup. Nice addition here.
Reading the config option (1k gas) on upload (> 1 million gas) is virtually 0 overhead. Happy for a param here.
| @@ -15,10 +15,13 @@ import ( | |||
| const ( | |||
| // DefaultParamspace for params keeper | |||
| DefaultParamspace = ModuleName | |||
| // DefaultMaxWasmCodeSize limit max bytes read to prevent gzip bombs | |||
There was a problem hiding this comment.
Very nice updates here.
You have thought more angles than I did.
💯
|
The underlying (gzip) reader returns |
| return src, nil | ||
| case n > limit: | ||
| return nil, types.ErrLimit |
There was a problem hiding this comment.
This check also rejects compressed data > limit, which is smaller than limit when uncompressed. Is this intended?
There was a problem hiding this comment.
good edge case!
This can only happen when gzip does not compress the data but adds it header on top. I guess that is very unlikely to happen and the workaround would be to send uncompressed data.
There was a problem hiding this comment.
Right. If found the code a bit confusing to review, because it is not very clear which parts deals with compressed and which with uncompressed data. Some checks operate on both.
Summary of changes: