AVM: Cleanly handle broken switch/match programs

[jannotti]

Previously, AVM panic'd while checking a malformed switch/match that was missing the byte encoding the number of cases. This meant the check failed, which is correct, but we endeavor to avoid panics.

[jasonpaulos]

Looks correct to me

[codecov]

Codecov Report

Merging #5782 (907caee) into master (c3bded8) will decrease coverage by 0.44%.
Report is 2 commits behind head on master.
The diff coverage is 70.58%.

@@            Coverage Diff             @@
##           master    #5782      +/-   ##
==========================================
- Coverage   55.91%   55.47%   -0.44%     
==========================================
  Files         477      473       -4     
  Lines       66739    66720      -19     
==========================================
- Hits        37317    37014     -303     
- Misses      26905    27192     +287     
+ Partials     2517     2514       -3     

Files	Coverage Δ
cmd/tealdbg/local.go	73.88% <100.00%> (-0.32%)	⬇️
data/transactions/logic/eval.go	92.30% <100.00%> (+0.22%)	⬆️
data/transactions/logic/sourcemap.go	100.00% <100.00%> (ø)
daemon/algod/api/server/v2/handlers.go	0.79% <0.00%> (ø)
cmd/tealdbg/debugger.go	74.41% <81.81%> (-1.10%)	⬇️
data/transactions/logic/assembler.go	93.98% <75.00%> (+<0.01%)	⬆️
cmd/goal/clerk.go	10.25% <37.50%> (+0.83%)	⬆️

... and 30 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[maldiohead]

interesting， I submit this security bug， Cost a lot of my time to write the report. you just don't think this is a problem. why you fix this? Your approach is so disappointing to me. I spent a lot of time studying avm and several hours found the bug. I feel that my work have not been respected.

[jannotti]

There is no security issue in the existing code. The CheckContract and Eval functions properly return errors. They do so by catching the panic. I prefer not to panic, so I've fixed it, and I appreciate you bringing it to my attention.

[maldiohead]

you know this can panic, .the panic will lead the algorand node crash, so this is a security vulnerability

[jannotti]

No, it will not crash.
This protects the CheckContract function.

func check(program []byte, params *EvalParams, mode RunMode) (err error) {
	defer func() {
		if x := recover(); x != nil {
			buf := make([]byte, 16*1024)
			stlen := runtime.Stack(buf, false)
			errstr := string(buf[:stlen])
			if params.Trace != nil {
				errstr += params.Trace.String()
			}
			err = panicError{x, errstr}
			params.log().Errorf("recovered panic in Check: %s", err)
		}
	}()

and the same thing is done to protect eval(). So the panic is caught, and the check returns an error, as expected.

Your PoC Test ended with:

err := CheckContract(data, ep)
if err != nil {
	return
}

That is, you didn't check to see whether CheckContract properly returns an error, you just returned no matter what. If you check that, you will see it does. There is no crash.

[maldiohead]

OK， switchTarget has no boundary check. If a hacker bypasses all previous checks,exploit this bug， the pc can also be controlled through the out-of-bounds read of this switchTarget.

[jannotti]

No. When switchTarget reads past the program, Go panics. The panic is caught above. The function returns an error. The PC is never updated in a bad way.

If I am wrong, just show me a crash, or a jump to an arbitrary program location.