Reject unknown fields in TxDecoder and sign mode handlers #6883

aaronc · 2020-07-29T17:09:39Z

This should wrap up or almost wrap up the critical path of #6213.

integrates unknown field rejection into the protobuf TxDecoder, rejecting:
- any unknown fields in TxRaw
- non-critical unknown fields in TxBody
- any unknown fields in AuthInfo
fixes Address SIGN_MODE_LEGACY_AMINO_JSON security issues #6863 by:
- separating the StdTx and proto Tx amino JSON sign mode handling
- rejecting non-critical unknown fields in the proto TxBody
- rejecting proto extensions and non_critical_extensions fields
- rejecting timeout_height which isn't supported in StdTx
adds a boolean return parameter to RejectUnknownFields to track if non-critical fields are present in a message
fixes critical bugs related to field decoding in RejectUnknownFields (cc @odeke-em)

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-07-29T17:14:00Z

Codecov Report

Merging #6883 into master will increase coverage by 3.46%.
The diff coverage is 82.66%.

@@            Coverage Diff             @@
##           master    #6883      +/-   ##
==========================================
+ Coverage   61.57%   65.04%   +3.46%     
==========================================
  Files         518      389     -129     
  Lines       31996    24253    -7743     
==========================================
- Hits        19703    15775    -3928     
+ Misses      10716     7271    -3445     
+ Partials     1577     1207     -370

…ronc/6192-unknown-field-rejection

odeke-em

Thank you for this change @aaronc and for wiring things up! I have left some questions and suggestions, but also I still don't get why the boolean, when the error could be checked.

codec/unknownproto/doc.go

codec/unknownproto/unknown_fields.go

x/auth/tx/direct.go

x/auth/tx/legacy_amino_json.go

aaronc · 2020-07-30T23:46:46Z

Thank you for this change @aaronc and for wiring things up! I have left some questions and suggestions, but also I still don't get why the boolean, when the error could be checked.

There is a case where we want to know if there are non-critical fields without returning an error.

odeke-em · 2020-07-30T23:49:16Z

There is a case where we want to know if there are non-critical fields without returning an error.

Oh okay, so an explicit customization, gotcha! Thanks for answering my question.

odeke-em

To ensure that you aren't blocked @aaronc when you get back this, from the protoc-side LGTM with just the change to avoid the allocation and revert to (*types.Any)(nil). Thank you and great to see this all come alive!

anilcse

lgtm

codec/unknownproto/unknown_fields.go

aaronc · 2020-08-03T19:53:00Z

x/auth/tx/legacy_amino_json.go

+	if body.TimeoutHeight != 0 {
+		return nil, sdkerrors.Wrap(sdkerrors.ErrInvalidRequest,
+			"SIGN_MODE_LEGACY_AMINO_JSON does not support timeout height.")
+	}


@alexanderbez note that TimeoutHeight is being now rejected by SIGN_MODE_LEGACY_AMINO_JSON (because otherwise it's a malleability issue) so if you are adding it to StdTx in #6089, you'll need to remove these lines.

Thanks for pointing that out!

* WIP on unknown field rejection in TxDecoder * WIP on unknown field rejection in TxDecoder * WIP * WIP * WIP * WIP * Fix bugs with RejectUnknownFields * Fix tests * Fix bug and update docs * Lint * Add tests * Add unknown field tests * Lint * Address review comments

WIP on unknown field rejection in TxDecoder

dc8530a

aaronc changed the title ~~WIP on unknown field rejection in TxDecoder~~ Reject unknown fields in TxDecoder and sign mode handlers Jul 29, 2020

aaronc mentioned this pull request Jul 30, 2020

Proto Any Tx migration #6213

Closed

27 tasks

aaronc added 13 commits July 30, 2020 11:55

WIP on unknown field rejection in TxDecoder

2d6f141

WIP

9a90553

WIP

c1e710a

Merge branch 'master' of https://github.com/cosmos/cosmos-sdk into aa…

c3db575

…ronc/6192-unknown-field-rejection

WIP

8b96a05

WIP

19ceea6

Fix bugs with RejectUnknownFields

d7fabf9

Fix tests

ed5a77e

Fix bug and update docs

ecf9dc7

Lint

4c91215

Add tests

021b8d3

Add unknown field tests

f78ee0c

Lint

e09f365

aaronc added C:Encoding T: Security C:x/auth labels Jul 30, 2020

Merge branch 'master' into aaronc/6192-unknown-field-rejection

55f64fc

aaronc requested a review from odeke-em July 30, 2020 22:39

aaronc marked this pull request as ready for review July 30, 2020 22:40

aaronc requested review from alessio, alexanderbez, clevinson, fedekunze and jgimeno as code owners July 30, 2020 22:40

aaronc requested review from amaury1093 and anilcse July 30, 2020 22:40

aaronc requested review from atheeshp and sahith-narahari July 30, 2020 22:40

odeke-em reviewed Jul 30, 2020

View reviewed changes

odeke-em approved these changes Jul 31, 2020

View reviewed changes

amaury1093 self-assigned this Aug 3, 2020

anilcse approved these changes Aug 3, 2020

View reviewed changes

codec/unknownproto/unknown_fields.go Show resolved Hide resolved

aaronc and others added 2 commits August 3, 2020 11:44

Merge branch 'master' into aaronc/6192-unknown-field-rejection

3284dd3

Address review comments

8180e38

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Aug 3, 2020

mergify bot merged commit 6d93744 into master Aug 3, 2020

mergify bot deleted the aaronc/6192-unknown-field-rejection branch August 3, 2020 19:47

aaronc commented Aug 3, 2020

View reviewed changes

BananaLF mentioned this pull request Jul 26, 2022

only support single sig mode okx/exchain#2329

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reject unknown fields in TxDecoder and sign mode handlers #6883

Reject unknown fields in TxDecoder and sign mode handlers #6883

aaronc commented Jul 29, 2020 •

edited

Loading

codecov bot commented Jul 29, 2020 •

edited

Loading

odeke-em left a comment

aaronc commented Jul 30, 2020

odeke-em commented Jul 30, 2020

odeke-em left a comment

anilcse left a comment

aaronc Aug 3, 2020

alexanderbez Aug 3, 2020 •

edited

Loading

Reject unknown fields in TxDecoder and sign mode handlers #6883

Reject unknown fields in TxDecoder and sign mode handlers #6883

Conversation

aaronc commented Jul 29, 2020 • edited Loading

codecov bot commented Jul 29, 2020 • edited Loading

Codecov Report

odeke-em left a comment

Choose a reason for hiding this comment

aaronc commented Jul 30, 2020

odeke-em commented Jul 30, 2020

odeke-em left a comment

Choose a reason for hiding this comment

anilcse left a comment

Choose a reason for hiding this comment

aaronc Aug 3, 2020

Choose a reason for hiding this comment

alexanderbez Aug 3, 2020 • edited Loading

Choose a reason for hiding this comment

aaronc commented Jul 29, 2020 •

edited

Loading

codecov bot commented Jul 29, 2020 •

edited

Loading

alexanderbez Aug 3, 2020 •

edited

Loading