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.

Sign up for GitHub

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

Reject wasm code exceeding limit #302

Merged
merged 3 commits into from
Nov 9, 2020
Merged

Reject wasm code exceeding limit #302

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e55cc9f
Reject wasm zips exceeding limit
alpe Nov 5, 2020
d4824f3
Parameter for max wasm code size
alpe Nov 6, 2020
2efd962
Add test to very limit on uncompressed data
alpe Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions x/wasm/internal/keeper/genesis_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ func TestGenesisExportImport(t *testing.T) {
srcKeeper.importContractState(srcCtx, contractAddr, stateModels)
}
var wasmParams types.Params
f.Fuzz(&wasmParams)
f.NilChance(0).Fuzz(&wasmParams)
srcKeeper.setParams(srcCtx, wasmParams)

// export
Expand Down Expand Up @@ -369,7 +369,8 @@ func TestImportContractWithCodeHistoryReset(t *testing.T) {
"code_upload_access": {
"permission": "Everybody"
},
"instantiate_default_permission": "Everybody"
"instantiate_default_permission": "Everybody",
"max_wasm_code_size": 500000
},
"codes": [
{
Expand Down
32 changes: 26 additions & 6 deletions x/wasm/internal/keeper/ioutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,20 +5,22 @@ import (
"compress/gzip"
"io"
"io/ioutil"

"github.com/CosmWasm/wasmd/x/wasm/internal/types"
)

// magic bytes to identify gzip.
// See https://www.ietf.org/rfc/rfc1952.txt
// and https://github.com/golang/go/blob/master/src/net/http/sniff.go#L186
var gzipIdent = []byte("\x1F\x8B\x08")

// limit max bytes read to prevent gzip bombs
const maxSize = 400 * 1024

// uncompress returns gzip uncompressed content or given src when not gzip.
func uncompress(src []byte) ([]byte, error) {
if len(src) < 3 {
func uncompress(src []byte, limit uint64) ([]byte, error) {
switch n := uint64(len(src)); {
case n < 3:
return src, nil
case n > limit:
return nil, types.ErrLimit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check also rejects compressed data > limit, which is smaller than limit when uncompressed. Is this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
if !bytes.Equal(gzipIdent, src[0:3]) {
return src, nil
Expand All @@ -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)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// LimitReader returns a Reader that reads from r
// but stops with types.ErrLimit after n bytes.
// The underlying implementation is a *io.LimitedReader.
func LimitReader(r io.Reader, n int64) io.Reader {
return &LimitedReader{r: &io.LimitedReader{R: r, N: n}}
}

type LimitedReader struct {
r *io.LimitedReader
}

return ioutil.ReadAll(io.LimitReader(zr, maxSize))
func (l *LimitedReader) Read(p []byte) (n int, err error) {
if l.r.N <= 0 {
return 0, types.ErrLimit
}
return l.r.Read(p)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe, so the read, then check if it overflowed? Or maybe I don't fully understand how LimitedReader works.

}
32 changes: 21 additions & 11 deletions x/wasm/internal/keeper/ioutil_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"strings"
"testing"

"github.com/CosmWasm/wasmd/x/wasm/internal/types"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand All @@ -20,6 +21,8 @@ func TestUncompress(t *testing.T) {
wasmGzipped, err := ioutil.ReadFile("./testdata/hackatom.wasm.gzip")
require.NoError(t, err)

const maxSize = 400_000

specs := map[string]struct {
src []byte
expError error
Expand All @@ -42,8 +45,8 @@ func TestUncompress(t *testing.T) {
expResult: []byte{0x1, 0x2},
},
"handle big input slice": {
src: []byte(strings.Repeat("a", maxSize+1)),
expResult: []byte(strings.Repeat("a", maxSize+1)),
src: []byte(strings.Repeat("a", maxSize+1)),
expError: types.ErrLimit,
},
"handle gzip identifier only": {
src: gzipIdent,
Expand All @@ -57,31 +60,38 @@ func TestUncompress(t *testing.T) {
src: wasmGzipped[:len(wasmGzipped)-5],
expError: io.ErrUnexpectedEOF,
},
"handle limit gzip output": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src: asGzip(bytes.Repeat([]byte{0x1}, maxSize)),
expResult: bytes.Repeat([]byte{0x1}, maxSize),
},
"handle big gzip output": {
src: asGzip(strings.Repeat("a", maxSize+1)),
expError: io.ErrUnexpectedEOF,
src: asGzip(bytes.Repeat([]byte{0x1}, maxSize+1)),
expError: types.ErrLimit,
},
"handle other big gzip output": {
src: asGzip(strings.Repeat("a", 2*maxSize)),
expError: io.ErrUnexpectedEOF,
src: asGzip(bytes.Repeat([]byte{0x1}, 2*maxSize)),
expError: types.ErrLimit,
},
}
for msg, spec := range specs {
t.Run(msg, func(t *testing.T) {
r, err := uncompress(spec.src)
require.True(t, errors.Is(spec.expError, err), "exp %+v got %+v", spec.expError, err)
r, err := uncompress(spec.src, maxSize)
require.True(t, errors.Is(spec.expError, err), "exp %v got %+v", spec.expError, err)
if spec.expError != nil {
return
}
assert.Equal(t, spec.expResult, r)
})
}

}

func asGzip(src string) []byte {
func asGzip(src []byte) []byte {
var buf bytes.Buffer
if _, err := io.Copy(gzip.NewWriter(&buf), strings.NewReader(src)); err != nil {
zipper := gzip.NewWriter(&buf)
if _, err := io.Copy(zipper, bytes.NewReader(src)); err != nil {
panic(err)
}
if err := zipper.Close(); err != nil {
panic(err)
}
return buf.Bytes()
Expand Down
10 changes: 8 additions & 2 deletions x/wasm/internal/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ func (k Keeper) getInstantiateAccessConfig(ctx sdk.Context) types.AccessType {
return a
}

func (k Keeper) GetMaxWasmCodeSize(ctx sdk.Context) uint64 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, nice to add the parameter here - this is set in genesis, right?

var a uint64
k.paramSpace.Get(ctx, types.ParamStoreKeyMaxWasmCodeSize, &a)
return a
}

// GetParams returns the total set of wasm parameters.
func (k Keeper) GetParams(ctx sdk.Context) types.Params {
var params types.Params
Expand All @@ -130,7 +136,7 @@ func (k Keeper) create(ctx sdk.Context, creator sdk.AccAddress, wasmCode []byte,
if !authZ.CanCreateCode(k.getUploadAccessConfig(ctx), creator) {
return 0, sdkerrors.Wrap(sdkerrors.ErrUnauthorized, "can not create code")
}
wasmCode, err = uncompress(wasmCode)
wasmCode, err = uncompress(wasmCode, k.GetMaxWasmCodeSize(ctx))
if err != nil {
return 0, sdkerrors.Wrap(types.ErrCreateFailed, err.Error())
}
Expand All @@ -155,7 +161,7 @@ func (k Keeper) create(ctx sdk.Context, creator sdk.AccAddress, wasmCode []byte,
}

func (k Keeper) importCode(ctx sdk.Context, codeID uint64, codeInfo types.CodeInfo, wasmCode []byte) error {
wasmCode, err := uncompress(wasmCode)
wasmCode, err := uncompress(wasmCode, k.GetMaxWasmCodeSize(ctx))
if err != nil {
return sdkerrors.Wrap(types.ErrCreateFailed, err.Error())
}
Expand Down
1 change: 1 addition & 0 deletions x/wasm/internal/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ func TestCreateStoresInstantiatePermission(t *testing.T) {
keeper.setParams(ctx, types.Params{
CodeUploadAccess: types.AllowEverybody,
InstantiateDefaultPermission: spec.srcPermission,
MaxWasmCodeSize: types.DefaultMaxWasmCodeSize,
})
fundAccounts(t, ctx, accKeeper, bankKeeper, myAddr, deposit)

Expand Down
24 changes: 20 additions & 4 deletions x/wasm/internal/keeper/proposal_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@ import (
func TestStoreCodeProposal(t *testing.T) {
ctx, keepers := CreateTestInput(t, false, "staking", nil, nil)
govKeeper, wasmKeeper := keepers.GovKeeper, keepers.WasmKeeper
wasmKeeper.setParams(ctx, types.Params{CodeUploadAccess: types.AllowNobody, InstantiateDefaultPermission: types.AccessTypeNobody})
wasmKeeper.setParams(ctx, types.Params{
CodeUploadAccess: types.AllowNobody,
InstantiateDefaultPermission: types.AccessTypeNobody,
MaxWasmCodeSize: types.DefaultMaxWasmCodeSize,
})
wasmCode, err := ioutil.ReadFile("./testdata/hackatom.wasm")
require.NoError(t, err)

Expand Down Expand Up @@ -55,7 +59,11 @@ func TestStoreCodeProposal(t *testing.T) {
func TestInstantiateProposal(t *testing.T) {
ctx, keepers := CreateTestInput(t, false, "staking", nil, nil)
govKeeper, wasmKeeper := keepers.GovKeeper, keepers.WasmKeeper
wasmKeeper.setParams(ctx, types.Params{CodeUploadAccess: types.AllowNobody, InstantiateDefaultPermission: types.AccessTypeNobody})
wasmKeeper.setParams(ctx, types.Params{
CodeUploadAccess: types.AllowNobody,
InstantiateDefaultPermission: types.AccessTypeNobody,
MaxWasmCodeSize: types.DefaultMaxWasmCodeSize,
})

wasmCode, err := ioutil.ReadFile("./testdata/hackatom.wasm")
require.NoError(t, err)
Expand Down Expand Up @@ -107,7 +115,11 @@ func TestInstantiateProposal(t *testing.T) {
func TestMigrateProposal(t *testing.T) {
ctx, keepers := CreateTestInput(t, false, "staking", nil, nil)
govKeeper, wasmKeeper := keepers.GovKeeper, keepers.WasmKeeper
wasmKeeper.setParams(ctx, types.Params{CodeUploadAccess: types.AllowNobody, InstantiateDefaultPermission: types.AccessTypeNobody})
wasmKeeper.setParams(ctx, types.Params{
CodeUploadAccess: types.AllowNobody,
InstantiateDefaultPermission: types.AccessTypeNobody,
MaxWasmCodeSize: types.DefaultMaxWasmCodeSize,
})

wasmCode, err := ioutil.ReadFile("./testdata/hackatom.wasm")
require.NoError(t, err)
Expand Down Expand Up @@ -236,7 +248,11 @@ func TestAdminProposals(t *testing.T) {
t.Run(msg, func(t *testing.T) {
ctx, keepers := CreateTestInput(t, false, "staking", nil, nil)
govKeeper, wasmKeeper := keepers.GovKeeper, keepers.WasmKeeper
wasmKeeper.setParams(ctx, types.Params{CodeUploadAccess: types.AllowNobody, InstantiateDefaultPermission: types.AccessTypeNobody})
wasmKeeper.setParams(ctx, types.Params{
CodeUploadAccess: types.AllowNobody,
InstantiateDefaultPermission: types.AccessTypeNobody,
MaxWasmCodeSize: types.DefaultMaxWasmCodeSize,
})

codeInfoFixture := types.CodeInfoFixture(types.WithSHA256CodeHash(wasmCode))
require.NoError(t, wasmKeeper.importCode(ctx, 1, codeInfoFixture, wasmCode))
Expand Down
19 changes: 19 additions & 0 deletions x/wasm/internal/types/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,13 @@ import (
const (
// DefaultParamspace for params keeper
DefaultParamspace = ModuleName
// DefaultMaxWasmCodeSize limit max bytes read to prevent gzip bombs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice updates here.
You have thought more angles than I did.

💯

DefaultMaxWasmCodeSize = 600 * 1024
)

var ParamStoreKeyUploadAccess = []byte("uploadAccess")
var ParamStoreKeyInstantiateAccess = []byte("instantiateAccess")
var ParamStoreKeyMaxWasmCodeSize = []byte("maxWasmCodeSize")

var AllAccessTypes = []AccessType{
AccessTypeNobody,
Expand Down Expand Up @@ -95,6 +98,7 @@ func DefaultParams() Params {
return Params{
CodeUploadAccess: AllowEverybody,
InstantiateDefaultPermission: AccessTypeEverybody,
MaxWasmCodeSize: DefaultMaxWasmCodeSize,
}
}

Expand All @@ -108,6 +112,7 @@ func (p *Params) ParamSetPairs() paramtypes.ParamSetPairs {
return paramtypes.ParamSetPairs{
paramtypes.NewParamSetPair(ParamStoreKeyUploadAccess, &p.CodeUploadAccess, validateAccessConfig),
paramtypes.NewParamSetPair(ParamStoreKeyInstantiateAccess, &p.InstantiateDefaultPermission, validateAccessType),
paramtypes.NewParamSetPair(ParamStoreKeyMaxWasmCodeSize, &p.MaxWasmCodeSize, validateMaxWasmCodeSize),
}
}

Expand All @@ -119,6 +124,9 @@ func (p Params) ValidateBasic() error {
if err := validateAccessConfig(p.CodeUploadAccess); err != nil {
return errors.Wrap(err, "upload access")
}
if err := validateMaxWasmCodeSize(p.MaxWasmCodeSize); err != nil {
return errors.Wrap(err, "max wasm code size")
}
return nil
}

Expand Down Expand Up @@ -146,6 +154,17 @@ func validateAccessType(i interface{}) error {
return sdkerrors.Wrapf(ErrInvalid, "unknown type: %q", a)
}

func validateMaxWasmCodeSize(i interface{}) error {
a, ok := i.(uint64)
if !ok {
return sdkerrors.Wrapf(ErrInvalid, "type: %T", i)
}
if a == 0 {
return sdkerrors.Wrap(ErrInvalid, "must be greater 0")
}
return nil
}

func (v AccessConfig) ValidateBasic() error {
switch v.Permission {
case AccessTypeUndefined:
Expand Down
Loading