fix(gnovm): closure/method privilege escalation

[ltzmaxwell]

continuous work follow #4569 -> #4584.

• fix closure privilege escalation

initial issue: https://github.com/gnolang/gno/security/advisories/GHSA-64hv-3ww9-mc3f

• fix another hidden issue cause by soft cross into the receiver’s owner realm

^^, This can happen if a method containing malicious logic is declared outside the target realm, and the receiver object is then passed into that realm. When the method is executed, privilege escalation occurs because it runs within the target realm via a soft cross — essentially the same issue as the “closure” case. check this test: 4d193f9

[ltzmaxwell]

Prevent mutation with executing method of receiver passed in from external realm.

[ltzmaxwell]

unreal one.

[thehowl]

My 2c: I think this PR, and the other attempts, are trying to fix a broken specification by adding more rules that add more cognitive complexity. If it were for me, we should have no implicit switching and the inter-realm specification should be able to be explained in 5 sentences or less, but my opinion wasn't considered at the due time and so now we have this... thing.

I think that the underlying AllowedDAO issue has been fixed separately; @moul finds that the code as it was before the PR should have not had the exploit, I argue that no, in the context of the spec it makes complete sense, and that really rather than adding more exceptions and rule on top of that spec we should look to remove and replace rules.

So I think we should keep the state of master because 1. we need to launch a chain and cannot afford to have more break-the-world changes 2. we should not add more rules to a spec that already causes too much pain and suffering and that I routinely discover nobody, even on our own core team (!), understands fully except for those who have dug deep and absorbed all the intricacies.

This is to say, I'm calling myself out of this discussion to fix the "exploit" and any other discussions that want to add more rules to the interrealm spec. The Gno1 ship has sailed, hopefully Gno2 will come and suck less. Also, if Jae starts considering this problem, he'll just do whatever and probably break the world anyway, so I don't think our energies are well-placed here.

[ltzmaxwell]

this is not right, can also mutate the fv's container realm... maybe need to revert to the last commit, that just check if fv's realm == receiver's realm? thinking...

[omarsy]

We will consider so we cannot pass callback to a realm in gnokey maketx run ?

[omarsy]

thinking about using package to escape privilege.
Something like this:

# simple test for the `gnoland restart` command;
# should restart the gno.land node and recover state.

loadpkg gno.land/r/demo/crossrealm $WORK/crossrealm
loadpkg gno.land/p/demo/middle $WORK/middle
loadpkg gno.land/r/demo/main $WORK/main
gnoland start

gnokey maketx call -pkgpath gno.land/r/demo/main -func Exec -gas-fee 1000000ugnot -gas-wanted 10000000 -broadcast -chainid=tendermint_test test1
stdout 'OK!'

-- crossrealm/crossrealm.gno --
package crossrealm


var AllowedList = []int{1, 2}
var  Closure func()

func SetClosure(cur realm, f func()) {
	Closure = f
}

func ExecuteClosure(cur realm) {
	Closure()
}



-- middle/middle.gno --
package middle

func F(v []int) func()   {
	return func()  {
	 v[0] = 5
	}
}

-- main/main.gno --
package main

import (
	"gno.land/r/demo/crossrealm"
	"gno.land/p/demo/middle"
)

func Exec(cur realm) []int {
	crossrealm.SetClosure(cross, middle.F(crossrealm.AllowedList))
	crossrealm.ExecuteClosure(cross)
	return crossrealm.AllowedList
}

[omarsy]

Based on the specification, I think this isn't an issue too.I think if we want to change this behavior, we should apply it to all non-crossing functions.

[ltzmaxwell]

close in favor of #4890