From adff7d804d3fbbf332c8a5967889ff9837c92f4e Mon Sep 17 00:00:00 2001 From: Amaury <1293565+amaurym@users.noreply.github.com> Date: Thu, 22 Jul 2021 19:54:14 +0200 Subject: [PATCH] feat: TxRaw must follow ADR 027 (#9743) ## Description Closes: #XXXX --- ### Author Checklist *All items are required. Please add a note to the item if the item is not applicable and please add links to any relevant follow up issues.* I have... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### Reviewers Checklist *All items are required. Please add a note if the item is not applicable and please add your handle next to the items reviewed if you only reviewed selected items.* I have... - [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] confirmed `!` in the type prefix if API or client breaking change - [ ] confirmed all author checklist items have been addressed - [ ] reviewed state machine logic - [ ] reviewed API design and naming - [ ] reviewed documentation is accurate - [ ] reviewed tests and test coverage - [ ] manually tested (if applicable) --- x/auth/tx/decoder.go | 86 +++++++++++++++++++- x/auth/tx/encode_decode_test.go | 134 ++++++++++++++++++++++++++++++-- 2 files changed, 213 insertions(+), 7 deletions(-) diff --git a/x/auth/tx/decoder.go b/x/auth/tx/decoder.go index a32a118604f9..d93c7446db60 100644 --- a/x/auth/tx/decoder.go +++ b/x/auth/tx/decoder.go @@ -1,6 +1,10 @@ package tx import ( + "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 +15,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 follow 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 +89,77 @@ func DefaultJSONTxDecoder(cdc codec.ProtoCodecMarshaler) sdk.TxDecoder { }, nil } } + +// 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. +// 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 with this variable. + prevTagNum := protowire.Number(0) + + for len(txBytes) > 0 { + 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) + } + 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 fmt.Errorf("invalid length; %w", protowire.ParseError(m)) + } + 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) + } + + // 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)) + } + txBytes = txBytes[m:] + } + + 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<