fix: fix vuln demonstrated by #583 #584

grepsuzette · 2023-03-10T04:50:57Z

This is a fix for the 2022-10-07 Binance vuln demonstrated in #583.

Original fix was simply (cosmos/iavl#582):

if len(pin.Left) > 0 && len(pin.Right) > 0 {
	return nil, errors.New("both left and right child hashes are set")
}

Our iavl functions however don't return errors. Proposing to use panic() instead, as it does in other parts of this file.

More about the vuln, for comments and archival:

…ngle PR

tbruyelle · 2023-03-31T13:21:09Z

@grepsuzette Thanks for all the links, that's very interesting to dive again into that. Have you noticed that they literally removed the range proof from cosmos/iavl, in favor of the ics23 proof ?

I came across this while trying to find range_proof.go in the cosmos/iavl repo, it's no longer there ! Then I find this commit cosmos/iavl@401725a which removes it.
Maybe we should follow that too, wdyt?

grepsuzette · 2023-03-31T16:53:44Z

@grepsuzette Thanks for all the links, that's very interesting to dive again into that. Have you noticed that they literally removed the range proof from cosmos/iavl, in favor of the ics23 proof ?

Yes I did.

I came across this while trying to find range_proof.go in the cosmos/iavl repo, it's no longer there ! Then I find this commit cosmos/iavl@401725a which removes it. Maybe we should follow that too, wdyt?

Range proofs I think were written by Jae; my guess, also I think I read that somewhere, is that they found it scary to modify (because it's a little bit scary; also whoever modifies it must be a kamikaze). Ics23 is more of a community effort, more reassuring for a community. But, if you've seen Dragonberry, it was a vuln in ics23 that could have resulted in all funds in ibc channels being hacked. So there's no silver bullet.

Still I think the ics23 iavl is an interesting question. It would take more studying to have an opinion. First of all, this vuln should be studied https://blog.verichains.io/p/vsa-2022-100-tendermint-forging-membership-proof ; does it affect GNO's iavl? But in any case even if we decided to adopt it (ics23), I think it should be in a different PR.

tbruyelle · 2023-03-31T17:58:34Z

Thanks for the additional details @grepsuzette I agree with your arguments, range proof isn't necessarily something we need to replace for now.

About the tendermint vuln, you're right we should definitively study it since it concerns the tendermint/crypto/merkle package and GNO has the same package pkg/crypto/merkle, which is used in pkg/iavl.

anyway, LGTM for this PR 👍

grepsuzette · 2023-04-03T08:38:16Z

Thanks, appreciate you taking the time to review and talk.

zivkovicmilos

Thank you for applying the fix 🙏

This fix is just part of a bigger issue of maintaining a stable iavl implementation.
I don't agree with the way we handled this fix with a panic, but it's a good starting point to discuss how we can make this safer and more reliable

cc @moul

pkgs/iavl/proof_forgery_test.go

zivkovicmilos · 2023-04-04T10:13:02Z

@grepsuzette

Please resolve the conflicts in this PR 🙏

Original fix at cosmos/iavl#582, is simply: ``` if len(pin.Left) > 0 && len(pin.Right) > 0 { return nil, errors.New("both left and right child hashes are set") } ``` Our iavl functions however don't return errors. Proposing to use `panic()` instead, as it does in other parts of this file. More about this 2022-10-07 vuln: https://medium.com/@Beosin_com/how-did-the-bnb-chain-exploiter-pass-iavl-proof-verification-an-in-depth-analysis-by-beosin-c925b77bc13e https://twitter.com/buchmanster/status/1578879225574350848

Won't compile yet. Is a test of the iavl proof forgery as exploited during BSC 2022-10-07 hack. Notes: 1. proof_forgery_test.go comes from cosmos/iavl#582 2. gist showing the same vuln at https://gist.github.com/samczsun/8635f49fac0ec66a5a61080835cae3db The test is not going to compile as is, it needs some work.

…lnerable) output follows --- FAIL: TestProofForgery (0.00s) proof_forgery_test.go:69: Error Trace: /home/bob/opt/src/COINS/Cosmos/GNO/gno/pkgs/iavl/proof_forgery_test.go:69 Error: Should be empty, but was [73 209 82 89 222 179 131 99 170 27 180 58 80 20 211 (...) 94 7 254 45 183 20 244] Test: TestProofForgery Messages: roothash must be empty if both left and right are set FAIL FAIL command-line-arguments 0.003s FAIL

and fails otherwise (fails when vuln not fixed) It should now be ok for review.

applying @zikovicmilos suggestion

grepsuzette · 2023-04-10T07:35:47Z

@zivkovicmilos done

grepsuzette · 2023-04-10T08:08:33Z

I don't understand why there's one cancelled check.

ajnavarro · 2023-06-01T13:04:02Z

@moul shall we review/merge this? thanks!

codecov · 2023-09-22T18:47:49Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.05% 🎉

Comparison is base (e10c0c7) 46.98% compared to head (f4766e5) 47.04%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #584      +/-   ##
==========================================
+ Coverage   46.98%   47.04%   +0.05%     
==========================================
  Files         365      365              
  Lines       61159    61161       +2     
==========================================
+ Hits        28737    28771      +34     
+ Misses      30062    30036      -26     
+ Partials     2360     2354       -6

Flag	Coverage Δ
gnovm-_test.cmd	`45.89% <ø> (ø)`
gnovm-_test.gnolang.native	`63.09% <ø> (ø)`
gnovm-_test.gnolang.other	`16.63% <ø> (ø)`
gnovm-_test.gnolang.pkg0	`17.98% <ø> (ø)`
gnovm-_test.gnolang.pkg1	`8.21% <ø> (ø)`
gnovm-_test.gnolang.pkg2	`9.87% <ø> (ø)`
gnovm-_test.gnolang.realm	`41.68% <ø> (ø)`
gnovm-_test.gnolang.stdlibs	`53.53% <ø> (ø)`
gnovm-_test.pkg	`25.95% <ø> (ø)`
tm2-_test.flappy	`∅ <ø> (∅)`
tm2-_test.pkg.amino	`58.32% <ø> (ø)`
tm2-_test.pkg.bft	`63.67% <ø> (+0.16%)`	⬆️
tm2-_test.pkg.others	`59.27% <100.00%> (+0.06%)`	⬆️

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

Files Changed	Coverage Δ
tm2/pkg/iavl/proof.go	`92.98% <100.00%> (+0.12%)`	⬆️

... and 5 files with indirect coverage changes

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

This is a fix for the 2022-10-07 Binance vuln demonstrated in gnolang#583. **Original fix** was simply (cosmos/iavl#582): ``` if len(pin.Left) > 0 && len(pin.Right) > 0 { return nil, errors.New("both left and right child hashes are set") } ``` Our iavl functions however don't return errors. Proposing to use `panic()` instead, as it does in other parts of this file. --- More about the vuln, for comments and archival: * https://twitter.com/buchmanster/status/1578879225574350848 * https://medium.com/@Beosin_com/how-did-the-bnb-chain-exploiter-pass-iavl-proof-verification-an-in-depth-analysis-by-beosin-c925b77bc13e --------- Co-authored-by: grepsuzette <grepsuzette@users.noreply.github.com> Co-authored-by: Antonio Navarro Perez <antnavper@gmail.com> Co-authored-by: Manfred Touron <94029+moul@users.noreply.github.com>

grepsuzette requested a review from a team as a code owner March 10, 2023 04:50

grepsuzette mentioned this pull request Mar 10, 2023

Add iavl test proving forgery #583

Closed

grepsuzette changed the title ~~fix vuln demonstrated by https://github.com/gnolang/gno/pull/583~~ fix vuln demonstrated by #583 Mar 10, 2023

grepsuzette mentioned this pull request Mar 10, 2023

Cosmos-sdk vulnerability retrospective #587

Open

moul added the 🐞 bug Something isn't working label Mar 27, 2023

moul assigned grepsuzette Mar 27, 2023

grepsuzette added a commit to grepsuzette/gno that referenced this pull request Mar 30, 2023

Merge the fix (gnolang#584) with the tests (gnolang#583) to have a si…

817171d

…ngle PR

grepsuzette changed the title ~~fix vuln demonstrated by #583~~ fix: fix vuln demonstrated by #583 Mar 30, 2023

tbruyelle approved these changes Apr 3, 2023

View reviewed changes

zivkovicmilos approved these changes Apr 4, 2023

View reviewed changes

pkgs/iavl/proof_forgery_test.go Outdated Show resolved Hide resolved

pkgs/iavl/proof_forgery_test.go Outdated Show resolved Hide resolved

grepsuzette added 5 commits April 10, 2023 14:34

Test now passes whenever gnolang#583 is applied

81cac3b

and fails otherwise (fails when vuln not fixed) It should now be ok for review.

make test pass

f65e6ce

grepsuzette force-pushed the fix-failing-test-pr584 branch from 817171d to f65e6ce Compare April 10, 2023 06:37

remove a useless panic

7e835d6

applying @zikovicmilos suggestion

Merge branch 'master' into fix-failing-test-pr584

e4a0544

ajnavarro requested review from jaekwon and moul as code owners June 1, 2023 12:53

github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jun 1, 2023

moul added this to the 🚀 main.gno.land milestone Sep 6, 2023

Merge branch 'master' into fix-failing-test-pr584

f4766e5

moul approved these changes Sep 22, 2023

View reviewed changes

moul merged commit b449e89 into gnolang:master Sep 22, 2023
173 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix vuln demonstrated by #583 #584

fix: fix vuln demonstrated by #583 #584

grepsuzette commented Mar 10, 2023 •

edited

Loading

tbruyelle commented Mar 31, 2023

grepsuzette commented Mar 31, 2023 •

edited

Loading

tbruyelle commented Mar 31, 2023

grepsuzette commented Apr 3, 2023

zivkovicmilos left a comment

zivkovicmilos commented Apr 4, 2023

grepsuzette commented Apr 10, 2023

grepsuzette commented Apr 10, 2023

ajnavarro commented Jun 1, 2023

codecov bot commented Sep 22, 2023 •

edited

Loading

fix: fix vuln demonstrated by #583 #584

fix: fix vuln demonstrated by #583 #584

Conversation

grepsuzette commented Mar 10, 2023 • edited Loading

tbruyelle commented Mar 31, 2023

grepsuzette commented Mar 31, 2023 • edited Loading

tbruyelle commented Mar 31, 2023

grepsuzette commented Apr 3, 2023

zivkovicmilos left a comment

Choose a reason for hiding this comment

zivkovicmilos commented Apr 4, 2023

grepsuzette commented Apr 10, 2023

grepsuzette commented Apr 10, 2023

ajnavarro commented Jun 1, 2023

codecov bot commented Sep 22, 2023 • edited Loading

Codecov Report

grepsuzette commented Mar 10, 2023 •

edited

Loading

grepsuzette commented Mar 31, 2023 •

edited

Loading

codecov bot commented Sep 22, 2023 •

edited

Loading