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

fix(go): bullet-proof against nil dereferences + more fuzzers #244

Merged

Conversation

odeke-em
Copy link
Contributor

@odeke-em odeke-em commented Dec 5, 2023

This change fixes a bunch of issues identified by Orijtech Inc's audit
of ics23 which is a critical cosmos-sdk dependency and as per reports
about the Dragonberry & Elderberry vulnerability reports, this package
was put back on our radar to further audit and voila that uncovered
some issues, some of which have beenfixed in this change. While here
also added more fuzzers. To ensure that the fuzzers can run alright,
added -short to any invocations of "go test".

Fixes #241
Fixes #242
Fixes #243

@odeke-em odeke-em force-pushed the fix-various-panics+more-fuzzers branch 3 times, most recently from fda1509 to 6579e2c Compare December 5, 2023 06:12
Copy link

codecov bot commented Dec 5, 2023

Codecov Report

Attention: 13 lines in your changes are missing coverage. Please review.

Comparison is base (c7c7288) 65.61% compared to head (1f01815) 38.66%.

❗ Current head 1f01815 differs from pull request most recent head ab5ea00. Consider uploading reports for the commit ab5ea00 to get more accurate results

Files Patch % Lines
go/proof.go 76.66% 6 Missing and 1 partial ⚠️
go/ops.go 66.66% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #244       +/-   ##
===========================================
- Coverage   65.61%   38.66%   -26.95%     
===========================================
  Files           7        5        -2     
  Lines        3621     4298      +677     
===========================================
- Hits         2376     1662      -714     
- Misses       1245     2314     +1069     
- Partials        0      322      +322     
Flag Coverage Δ
go 38.66% <74.50%> (?)
rust ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odeke-em odeke-em changed the title go: bullet-proof against nil dereferences + more fuzzers fix(go): bullet-proof against nil dereferences + more fuzzers Dec 5, 2023
@odeke-em odeke-em force-pushed the fix-various-panics+more-fuzzers branch from 6579e2c to 5d2250a Compare December 5, 2023 06:16
@odeke-em
Copy link
Contributor Author

odeke-em commented Dec 5, 2023

Kindly cc-ing @crodriguezvega

@crodriguezvega
Copy link
Contributor

Kindly cc-ing @crodriguezvega

Thank you for opening the issues and this PR, @odeke-em. I will review this week.

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thanks a lot for bulletproofing the code, @odeke-em! I have left a bunch of comments for now; let me know what you think about them.

go/proof.go Show resolved Hide resolved
go/proof.go Show resolved Hide resolved
go/proof_data_test.go Show resolved Hide resolved
go/ops.go Outdated Show resolved Hide resolved
go/ops.go Outdated Show resolved Hide resolved
go/ops.go Show resolved Hide resolved
go/fuzz_test.go Show resolved Hide resolved
go/fuzz_test.go Show resolved Hide resolved
go/Makefile Outdated Show resolved Hide resolved
@odeke-em odeke-em force-pushed the fix-various-panics+more-fuzzers branch 3 times, most recently from e43e4a9 to 160fd1f Compare December 12, 2023 05:40
@odeke-em
Copy link
Contributor Author

/cc @elias-orijtech @julienrbrt

Copy link
Contributor

@crodriguezvega crodriguezvega left a comment

Choose a reason for hiding this comment

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

Thank you, @odeke-em. I still find weird that we need to explicitly check if the pointer receiver is nil. When I remove the check and run make test no tests fail... Maybe I am not running the tests correctly?

Besides, if that case was reachable, then why not add a similar check to all other functions with a pointer receiver?

.github/workflows/go.yml Outdated Show resolved Hide resolved
@odeke-em odeke-em force-pushed the fix-various-panics+more-fuzzers branch from 160fd1f to 1f01815 Compare December 19, 2023 07:30
This change fixes a bunch of issues identified by Orijtech Inc's audit
of ics23 which is a critical cosmos-sdk dependency and as per reports
about the Dragonberry & Elderberry vulnerability reports, this package
was put back on our radar to further audit and voila that uncovered
some issues, some of which have beenfixed in this change. While here
also added more fuzzers. To ensure that the fuzzers can run alright,
added -short to any invocations of "go test".

Fixes cosmos#241
Fixes cosmos#242
Fixes cosmos#243
@odeke-em odeke-em force-pushed the fix-various-panics+more-fuzzers branch from 1f01815 to ab5ea00 Compare December 19, 2023 09:26
@odeke-em
Copy link
Contributor Author

Besides, if that case was reachable, then why not add a similar check to all other functions with a pointer receiver?

I mean the fuzzer flagged it but sure I did undo it. Kindly help me take another look @crodriguezvega and then merge if possible. Thank you!

@odeke-em
Copy link
Contributor Author

Just cc-ing the cosmos-sdk team to remind us to pull in the updated release once this code is merged in /cc @tac0turtle @julienrbrt @elias-orijtech

@tac0turtle
Copy link
Member

@odeke-em sdk team doesnt maintain this repo. Carlos and the ibc team are best to do merges and approvals here

@odeke-em
Copy link
Contributor Author

Oh okay, gotcha and thank you @tac0turtle!

Copy link
Member

@AdityaSripal AdityaSripal left a comment

Choose a reason for hiding this comment

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

Thank you @odeke-em !

@crodriguezvega crodriguezvega merged commit bf89d95 into cosmos:master Dec 21, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants