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.

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

TxDecoder panics on bad input #7739

Closed
4 tasks
ebuchman opened this issue Oct 30, 2020 · 0 comments · Fixed by #7770
Closed
4 tasks

TxDecoder panics on bad input #7739

ebuchman opened this issue Oct 30, 2020 · 0 comments · Fixed by #7770
Assignees
Labels

Comments

@ebuchman
Copy link
Member

Surfaced from Informal Systems IBC Audit of cosmos-sdk hash 82f15f3.

Summary of Bug

Malformed txs can cause the TxDecoder to panic. This doesn't seem to crash the node, since the panics are handled, but the TxDecoder should probably be more robust anyways.

Note that when you do run it against a node (eg. simd), you can't see the stack trace, the Response.Log is just "panic". Not sure if the stack trace is being purposefully or accidentally cut off here, but might be valuable to preserve it somewhere? Eg. even if its just in the process logs.

Version

82f15f3 (recent)

Steps to Reproduce

The bug was found by starting to play with go-fuzz. The following self-contained go test demonstrates the panic:

func TestDecoder(t *testing.T) {
	data, _ := hex.DecodeString("0A9F010A9C200A2D2F6962632E636F72652E636F6E6E656374696F6E2E76312E4D7367436F6E6E656374696F584F75656E496E6974126B0A0D6962637A65726F636C69656E74120B6962637A65726F636F6E6E1A1C0A0C6962636F6E65636C69656E74120A6962636F6E65636F6E6E00002205312E302E302A283235454635364341373935313335453430393336384536444238313130463232413442453035433212080A0612040A0208011A40143342993E25DA936CDDC7BE3D8F603CA6E9661518D536D0C482E18A0154AA096E438A6B9BCADFCFC2F0D689DCCAF55B96399D67A8361B70F5DA13091E2F929")
	cfg := simapp.MakeTestEncodingConfig()
	decoder := cfg.TxConfig.TxDecoder()
	decoder(data)
}

This results in

$ go test -run Decoder
--- FAIL: TestDecoder (0.00s)
panic: runtime error: slice bounds out of range [:-1] [recovered]
        panic: runtime error: slice bounds out of range [:-1]

goroutine 51 [running]:
testing.tRunner.func1.1(0x4f7d8e0, 0xc000fe9840)
        /Users/ethanbuchman/goRoot/1.14.4/src/testing/testing.go:940 +0x2f5
testing.tRunner.func1(0xc000fd1d40)
        /Users/ethanbuchman/goRoot/1.14.4/src/testing/testing.go:943 +0x3f9
panic(0x4f7d8e0, 0xc000fe9840)
        /Users/ethanbuchman/goRoot/1.14.4/src/runtime/panic.go:969 +0x166
github.com/cosmos/cosmos-sdk/codec/unknownproto.RejectUnknownFields(0xc0000e028c, 0x40, 0x134, 0x53efa80, 0xc0000f7130, 0xc000073e00, 0x6746088, 0xc000fe0ba0, 0x540abe0, 0xc000fe0ba0, ...)
        /Users/ethanbuchman/programming/goApps/pkg/mod/github.com/cosmos/[email protected]/codec/unknownproto/unknown_fields.go:95 +0xe37
github.com/cosmos/cosmos-sdk/codec/unknownproto.RejectUnknownFieldsStrict(...)
        /Users/ethanbuchman/programming/goApps/pkg/mod/github.com/cosmos/[email protected]/codec/unknownproto/unknown_fields.go:31
github.com/cosmos/cosmos-sdk/x/auth/tx.DefaultTxDecoder.func1(0xc0000e01e0, 0xed, 0x1e0, 0xc000fb7a60, 0x540df40, 0xc000f812c0, 0xc000010f38)
        /Users/ethanbuchman/programming/goApps/pkg/mod/github.com/cosmos/[email protected]/x/auth/tx/decoder.go:17 +0xe5
...

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@ebuchman ebuchman added this to the IBC 1.0 Implementation Audit milestone Oct 30, 2020
odeke-em added a commit that referenced this issue Nov 1, 2020
…n error

Given that protowire.ConsumeFieldValue returns -1 when it encounters an
error, perform a check for n < 0 and return the respectively obtained
error with context about the details.

Fixes an issue identified from a go-fuzz session, thanks to Ethan
Buchman and the IBC auditors from Informal Systems et al.

Fixes #7739.
@mergify mergify bot closed this as completed in #7770 Nov 9, 2020
@aaronc aaronc removed the backlog label Nov 9, 2020
mergify bot added a commit that referenced this issue Nov 9, 2020
…n an error (#7770)

* unknownproto: check result from protowire.ConsumeFieldValue and return error

Given that protowire.ConsumeFieldValue returns -1 when it encounters an
error, perform a check for n < 0 and return the respectively obtained
error with context about the details.

Fixes an issue identified from a go-fuzz session, thanks to Ethan
Buchman and the IBC auditors from Informal Systems et al.

Fixes #7739.

* Address AlexanderBez's suggestions

* Use require in tests

* Add issue #7846 to TODO

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants