From d8523c7f66744eade61bbe3904d7b29afa2f0823 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 21 Jul 2021 16:58:46 +0200 Subject: [PATCH 1/7] feat: TxRaw must follow ADR 027 --- x/auth/tx/decoder.go | 38 ++++++++++++- x/auth/tx/encode_decode_test.go | 96 ++++++++++++++++++++++++++++++--- 2 files changed, 127 insertions(+), 7 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index a32a118604f9..e739bf97ee72 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -1,6 +1,11 @@ package tx import ( + "errors" + "fmt" + + "google.golang.org/protobuf/encoding/protowire" + "github.com/cosmos/cosmos-sdk/codec" "github.com/cosmos/cosmos-sdk/codec/unknownproto" sdk "github.com/cosmos/cosmos-sdk/types" @@ -11,10 +16,16 @@ import ( // DefaultTxDecoder returns a default protobuf TxDecoder using the provided Marshaler. func DefaultTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { return func(txBytes []byte) (sdk.Tx, error) { + // Make sure txBytes adheres to ADR-027. + err := rejectNonADR027(txBytes) + if err != nil { + return nil, sdkerrors.Wrap(sdkerrors.ErrTxDecode, err.Error()) + } + var raw tx.TxRaw // reject all unknown proto fields in the root TxRaw - err := unknownproto.RejectUnknownFieldsStrict(txBytes, &raw, cdc.InterfaceRegistry()) + err = unknownproto.RejectUnknownFieldsStrict(txBytes, &raw, cdc.InterfaceRegistry()) if err != nil { return nil, sdkerrors.Wrap(sdkerrors.ErrTxDecode, err.Error()) } @@ -79,3 +90,28 @@ func DefaultJSONTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { }, nil } } + +// rejectNonADR027 rejects txBytes that do not adhere to ADR-027. This function +// only checks that field numbers are in ascending order, as all other ADR-027 +// edge cases (e.g. TxRaw fields having default values) will not happen with +// TxRaw. +func rejectNonADR027(txBytes []byte) error { + // Make sure all fields are ordered in ascending order. + prevTagNum := protowire.Number(0) + for len(txBytes) > 0 { + tagNum, _, m := protowire.ConsumeField(txBytes) + if m < 0 { + return errors.New("invalid length") + } + + if tagNum < prevTagNum { + return fmt.Errorf("txRaw must follow ADR-027, got tagNum %d after tagNum %d", tagNum, prevTagNum) + } + prevTagNum = tagNum + + // Skip over the bytes that store fieldNumber and wireType bytes. + txBytes = txBytes[m:] + } + + return nil +} diff --git a/x/auth/tx/encode_decode_test.go b/x/auth/tx/encode_decode_test.go index 55fff38c366f..3b59952aaf7e 100644 --- a/x/auth/tx/encode_decode_test.go +++ b/x/auth/tx/encode_decode_test.go @@ -4,17 +4,16 @@ import ( "fmt" "testing" - sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" - - "github.com/cosmos/cosmos-sdk/types/tx" - signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing" - "github.com/cosmos/cosmos-sdk/x/auth/signing" - "github.com/stretchr/testify/require" + "google.golang.org/protobuf/encoding/protowire" "github.com/cosmos/cosmos-sdk/codec" codectypes "github.com/cosmos/cosmos-sdk/codec/types" "github.com/cosmos/cosmos-sdk/testutil/testdata" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" + "github.com/cosmos/cosmos-sdk/types/tx" + signingtypes "github.com/cosmos/cosmos-sdk/types/tx/signing" + "github.com/cosmos/cosmos-sdk/x/auth/signing" ) func TestDefaultTxDecoderError(t *testing.T) { @@ -159,3 +158,88 @@ func TestUnknownFields(t *testing.T) { _, err = decoder(txBz) require.Error(t, err) } + +func TestRejectNonADR027(t *testing.T) { + registry := codectypes.NewInterfaceRegistry() + cdc := codec.NewProtoCodec(registry) + decoder := DefaultTxDecoder(cdc) + + body := &testdata.TestUpdatedTxBody{Memo: "AAA"} // Look for "65 65 65" when debugging the bytes stream. + bodyBz, err := body.Marshal() + require.NoError(t, err) + authInfo := &testdata.TestUpdatedAuthInfo{Fee: &tx.Fee{GasLimit: 128}} // Look for "128" when debugging the bytes stream. + authInfoBz, err := authInfo.Marshal() + txRaw := &tx.TxRaw{ + BodyBytes: bodyBz, + AuthInfoBytes: authInfoBz, + Signatures: [][]byte{{41}, {42}, {43}}, // Look for "42" when debugging the bytes stream. + } + + // We know these bytes are ADR-027-compliant. + txBz, err := txRaw.Marshal() + + // From the `txBz`, we extract the 3 components: + // bodyBz, authInfoBz, sigsBz. + // In our tests, we will try to decode txs with those 3 components in all + // possible orders. + // + // Consume "BodyBytes" field. + _, _, m := protowire.ConsumeField(txBz) + bodyBz = append([]byte{}, txBz[:m]...) + txBz = txBz[m:] // Skip over "BodyBytes" bytes. + // Consume "AuthInfoBytes" field. + _, _, m = protowire.ConsumeField(txBz) + authInfoBz = append([]byte{}, txBz[:m]...) + txBz = txBz[m:] // Skip over "AuthInfoBytes" bytes. + // Consume "Signature" field, it's the remaining bytes. + sigsBz := append([]byte{}, txBz...) + + tests := []struct { + name string + txBz []byte + shouldErr bool + }{ + { + "authInfo, body, sigs", + append(append(authInfoBz, bodyBz...), sigsBz...), + true, + }, + { + "authInfo, sigs, body", + append(append(authInfoBz, sigsBz...), bodyBz...), + true, + }, + { + "sigs, body, authInfo", + append(append(sigsBz, bodyBz...), authInfoBz...), + true, + }, + { + "sigs, authInfo, body", + append(append(sigsBz, authInfoBz...), bodyBz...), + true, + }, + { + "body, sigs, authInfo", + append(append(bodyBz, sigsBz...), authInfoBz...), + true, + }, + { + "body, authInfo, sigs (valid txRaw)", + append(append(bodyBz, authInfoBz...), sigsBz...), + false, + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + _, err = decoder(tt.txBz) + if tt.shouldErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} From 7dc13859dedaf238674013ee4fa6f9ce27fa781e Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 21 Jul 2021 19:06:10 +0200 Subject: [PATCH 2/7] Add varint checks --- x/auth/tx/decoder.go | 31 ++++++++++++++++++++++++++----- x/auth/tx/encode_decode_test.go | 11 ++++++++++- 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index e739bf97ee72..b9a314ab6641 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -1,6 +1,7 @@ package tx import ( + "encoding/binary" "errors" "fmt" @@ -92,14 +93,17 @@ func DefaultJSONTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { } // rejectNonADR027 rejects txBytes that do not adhere to ADR-027. This function -// only checks that field numbers are in ascending order, as all other ADR-027 -// edge cases (e.g. TxRaw fields having default values) will not happen with -// TxRaw. +// only checks that: +// - field numbers are in ascending order (1, 2, and potentially multiple 3s), +// - and varints as as short as possible. +// All other ADR-027 edge cases (e.g. TxRaw fields having default values) will +// not happen with TxRaw. func rejectNonADR027(txBytes []byte) error { - // Make sure all fields are ordered in ascending order. + // Make sure all fields are ordered in ascending order with this variable. prevTagNum := protowire.Number(0) + for len(txBytes) > 0 { - tagNum, _, m := protowire.ConsumeField(txBytes) + tagNum, _, m := protowire.ConsumeTag(txBytes) if m < 0 { return errors.New("invalid length") } @@ -109,7 +113,24 @@ func rejectNonADR027(txBytes []byte) error { } prevTagNum = tagNum + // All 3 fields of TxRaw have wireType == 2, so their next component + // is a varint. + // We make sure that the varint is as short as possible. + lengthPrefix, m := protowire.ConsumeVarint(txBytes[m:]) + if m < 0 { + return errors.New("invalid length") + } + buf := make([]byte, m) + n := binary.PutUvarint(buf, lengthPrefix) // `n` is the shortest length for varint-encoding `lengthPrefix`. + if n != m { + return fmt.Errorf("length prefix varint for tagNum %d is not as short as possible, read %d, only need %d", tagNum, m, n) + } + // Skip over the bytes that store fieldNumber and wireType bytes. + _, _, m = protowire.ConsumeField(txBytes) + if m < 0 { + return errors.New("invalid length") + } txBytes = txBytes[m:] } diff --git a/x/auth/tx/encode_decode_test.go b/x/auth/tx/encode_decode_test.go index 3b59952aaf7e..b3038ba6d22f 100644 --- a/x/auth/tx/encode_decode_test.go +++ b/x/auth/tx/encode_decode_test.go @@ -167,7 +167,7 @@ func TestRejectNonADR027(t *testing.T) { body := &testdata.TestUpdatedTxBody{Memo: "AAA"} // Look for "65 65 65" when debugging the bytes stream. bodyBz, err := body.Marshal() require.NoError(t, err) - authInfo := &testdata.TestUpdatedAuthInfo{Fee: &tx.Fee{GasLimit: 128}} // Look for "128" when debugging the bytes stream. + authInfo := &testdata.TestUpdatedAuthInfo{Fee: &tx.Fee{GasLimit: 127}} // Look for "127" when debugging the bytes stream. authInfoBz, err := authInfo.Marshal() txRaw := &tx.TxRaw{ BodyBytes: bodyBz, @@ -194,6 +194,10 @@ func TestRejectNonADR027(t *testing.T) { // Consume "Signature" field, it's the remaining bytes. sigsBz := append([]byte{}, txBz...) + // bodyBz's length prefix is 5, with `5` as varint encoding. We also try a + // longer varint encoding for 5: `133 00`. + longVarintBodyBz := append(append([]byte{bodyBz[0]}, byte(133), byte(00)), bodyBz[2:]...) + tests := []struct { name string txBz []byte @@ -229,6 +233,11 @@ func TestRejectNonADR027(t *testing.T) { append(append(bodyBz, authInfoBz...), sigsBz...), false, }, + { + "longer varint than needed", + append(append(longVarintBodyBz, authInfoBz...), sigsBz...), + true, + }, } for _, tt := range tests { From 33939f46f34e7c59bc55601d5f73030a3e45abad Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 21 Jul 2021 19:27:18 +0200 Subject: [PATCH 3/7] fix fuzz test --- x/auth/tx/decoder.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index b9a314ab6641..84729d5c52fd 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -2,7 +2,6 @@ package tx import ( "encoding/binary" - "errors" "fmt" "google.golang.org/protobuf/encoding/protowire" @@ -105,7 +104,7 @@ func rejectNonADR027(txBytes []byte) error { for len(txBytes) > 0 { tagNum, _, m := protowire.ConsumeTag(txBytes) if m < 0 { - return errors.New("invalid length") + return fmt.Errorf("invalid length %w", protowire.ParseError(m)) } if tagNum < prevTagNum { @@ -118,7 +117,7 @@ func rejectNonADR027(txBytes []byte) error { // We make sure that the varint is as short as possible. lengthPrefix, m := protowire.ConsumeVarint(txBytes[m:]) if m < 0 { - return errors.New("invalid length") + return fmt.Errorf("invalid length %w", protowire.ParseError(m)) } buf := make([]byte, m) n := binary.PutUvarint(buf, lengthPrefix) // `n` is the shortest length for varint-encoding `lengthPrefix`. @@ -129,7 +128,7 @@ func rejectNonADR027(txBytes []byte) error { // Skip over the bytes that store fieldNumber and wireType bytes. _, _, m = protowire.ConsumeField(txBytes) if m < 0 { - return errors.New("invalid length") + return fmt.Errorf("invalid length %w", protowire.ParseError(m)) } txBytes = txBytes[m:] } From 24220c48c922190c0ffdf070d3dcc2daf57038e7 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Wed, 21 Jul 2021 19:31:28 +0200 Subject: [PATCH 4/7] better errors --- x/auth/tx/decoder.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index 84729d5c52fd..feffadb60374 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -104,7 +104,7 @@ func rejectNonADR027(txBytes []byte) error { for len(txBytes) > 0 { tagNum, _, m := protowire.ConsumeTag(txBytes) if m < 0 { - return fmt.Errorf("invalid length %w", protowire.ParseError(m)) + return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) } if tagNum < prevTagNum { @@ -117,7 +117,7 @@ func rejectNonADR027(txBytes []byte) error { // We make sure that the varint is as short as possible. lengthPrefix, m := protowire.ConsumeVarint(txBytes[m:]) if m < 0 { - return fmt.Errorf("invalid length %w", protowire.ParseError(m)) + return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) } buf := make([]byte, m) n := binary.PutUvarint(buf, lengthPrefix) // `n` is the shortest length for varint-encoding `lengthPrefix`. @@ -128,7 +128,7 @@ func rejectNonADR027(txBytes []byte) error { // Skip over the bytes that store fieldNumber and wireType bytes. _, _, m = protowire.ConsumeField(txBytes) if m < 0 { - return fmt.Errorf("invalid length %w", protowire.ParseError(m)) + return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) } txBytes = txBytes[m:] } From c46b93f134e3c6796fd4312de84c295b49ae2d22 Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Thu, 22 Jul 2021 11:20:46 +0200 Subject: [PATCH 5/7] Use hardcoded table for varint length --- x/auth/tx/decoder.go | 32 +++++++++++++++++++++++++++++--- x/auth/tx/encode_decode_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index feffadb60374..30e396674f5d 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -1,7 +1,6 @@ package tx import ( - "encoding/binary" "fmt" "google.golang.org/protobuf/encoding/protowire" @@ -119,8 +118,7 @@ func rejectNonADR027(txBytes []byte) error { if m < 0 { return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) } - buf := make([]byte, m) - n := binary.PutUvarint(buf, lengthPrefix) // `n` is the shortest length for varint-encoding `lengthPrefix`. + n := varintMinLength(lengthPrefix) if n != m { return fmt.Errorf("length prefix varint for tagNum %d is not as short as possible, read %d, only need %d", tagNum, m, n) } @@ -135,3 +133,31 @@ func rejectNonADR027(txBytes []byte) error { return nil } + +// varintMinLength returns the minimum number of bytes necessary to encode an +// uint using varint encoding. +func varintMinLength(n uint64) int { + switch { + // Note: 1< Date: Thu, 22 Jul 2021 11:24:20 +0200 Subject: [PATCH 6/7] Comments --- x/auth/tx/decoder.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index 30e396674f5d..d1b753cf23df 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -15,7 +15,7 @@ import ( // DefaultTxDecoder returns a default protobuf TxDecoder using the provided Marshaler. func DefaultTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { return func(txBytes []byte) (sdk.Tx, error) { - // Make sure txBytes adheres to ADR-027. + // Make sure txBytes follow ADR-027. err := rejectNonADR027(txBytes) if err != nil { return nil, sdkerrors.Wrap(sdkerrors.ErrTxDecode, err.Error()) @@ -90,7 +90,7 @@ func DefaultJSONTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { } } -// rejectNonADR027 rejects txBytes that do not adhere to ADR-027. This function +// rejectNonADR027 rejects txBytes that do not follow ADR-027. This function // only checks that: // - field numbers are in ascending order (1, 2, and potentially multiple 3s), // - and varints as as short as possible. From 002f2c1a470c25aa3cc0163b13c026077453afce Mon Sep 17 00:00:00 2001 From: Amaury M <1293565+amaurym@users.noreply.github.com> Date: Thu, 22 Jul 2021 19:42:56 +0200 Subject: [PATCH 7/7] Check wireType too --- x/auth/tx/decoder.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index d1b753cf23df..d93c7446db60 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -101,11 +101,13 @@ func rejectNonADR027(txBytes []byte) error { prevTagNum := protowire.Number(0) for len(txBytes) > 0 { - tagNum, _, m := protowire.ConsumeTag(txBytes) + tagNum, wireType, m := protowire.ConsumeTag(txBytes) if m < 0 { return fmt.Errorf("invalid length; %w", protowire.ParseError(m)) } - + if wireType != protowire.BytesType { + return fmt.Errorf("expected %d wire type, got %d", protowire.VarintType, wireType) + } if tagNum < prevTagNum { return fmt.Errorf("txRaw must follow ADR-027, got tagNum %d after tagNum %d", tagNum, prevTagNum) }