cosmos / ics23

Building generic merkle proof format for IBC
Apache License 2.0
116 stars 68 forks source link

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

Closed odeke-em closed 9 months ago

odeke-em commented 10 months ago

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

codecov[bot] commented 10 months ago

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%.

:exclamation: 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 :warning:
go/ops.go 66.66% 4 Missing and 2 partials :warning:
Additional details and impacted files ```diff @@ 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](https://app.codecov.io/gh/cosmos/ics23/pull/244/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cosmos) | Coverage Δ | | |---|---|---| | [go](https://app.codecov.io/gh/cosmos/ics23/pull/244/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cosmos) | `38.66% <74.50%> (?)` | | | [rust](https://app.codecov.io/gh/cosmos/ics23/pull/244/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cosmos) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=cosmos#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

odeke-em commented 10 months ago

Kindly cc-ing @crodriguezvega

crodriguezvega commented 10 months ago

Kindly cc-ing @crodriguezvega

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

odeke-em commented 9 months ago

/cc @elias-orijtech @julienrbrt

odeke-em commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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

odeke-em commented 9 months ago

Oh okay, gotcha and thank you @tac0turtle!