From fc1075bf75882ea3ce5783eb9a18cd04937c536b Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 11 Sep 2019 11:52:39 +0300 Subject: [PATCH 1/3] vm: protect PUSHDATA from short reads Same thing done in a2a8981979f83e9a28650d12e0eaa123a83c181e for PUSHBYTES, failing to read the amount of bytes specified should lead to FAULT. Also makes readUint16() and readUint32() panic as this is the behavior we want in these cases. Add some tests along the way. --- pkg/vm/context.go | 4 +-- pkg/vm/vm.go | 9 ++++++ pkg/vm/vm_test.go | 81 ++++++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 87 insertions(+), 7 deletions(-) diff --git a/pkg/vm/context.go b/pkg/vm/context.go index e6fb159f6..bf4f061ee 100644 --- a/pkg/vm/context.go +++ b/pkg/vm/context.go @@ -89,7 +89,7 @@ func (c *Context) String() string { func (c *Context) readUint32() uint32 { start, end := c.IP(), c.IP()+4 if end > len(c.prog) { - return 0 + panic("failed to read uint32 parameter") } val := binary.LittleEndian.Uint32(c.prog[start:end]) c.ip += 4 @@ -99,7 +99,7 @@ func (c *Context) readUint32() uint32 { func (c *Context) readUint16() uint16 { start, end := c.IP(), c.IP()+2 if end > len(c.prog) { - return 0 + panic("failed to read uint16 parameter") } val := binary.LittleEndian.Uint16(c.prog[start:end]) c.ip += 2 diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index f5a24ad3b..e5c909508 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -262,16 +262,25 @@ func (v *VM) execute(ctx *Context, op Instruction) { case PUSHDATA1: n := ctx.readByte() b := ctx.readBytes(int(n)) + if b == nil { + panic("failed to read instruction parameter") + } v.estack.PushVal(b) case PUSHDATA2: n := ctx.readUint16() b := ctx.readBytes(int(n)) + if b == nil { + panic("failed to read instruction parameter") + } v.estack.PushVal(b) case PUSHDATA4: n := ctx.readUint32() b := ctx.readBytes(int(n)) + if b == nil { + panic("failed to read instruction parameter") + } v.estack.PushVal(b) // Stack operations. diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index 9506c7404..dfb4a5d30 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -99,16 +99,87 @@ func TestPushm1to16(t *testing.T) { } } -func TestPushData1(t *testing.T) { - +func TestPushData1BadNoN(t *testing.T) { + prog := []byte{byte(PUSHDATA1)} + vm := load(prog) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) } -func TestPushData2(t *testing.T) { - +func TestPushData1BadN(t *testing.T) { + prog := []byte{byte(PUSHDATA1), 1} + vm := load(prog) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) } -func TestPushData4(t *testing.T) { +func TestPushData1Good(t *testing.T) { + prog := makeProgram(PUSHDATA1, 3, 1, 2, 3) + vm := load(prog) + vm.Run() + assert.Equal(t, false, vm.state.HasFlag(faultState)) + assert.Equal(t, 1, vm.estack.Len()) + assert.Equal(t, []byte{1, 2, 3}, vm.estack.Pop().Bytes()) +} +func TestPushData2BadNoN(t *testing.T) { + prog := []byte{byte(PUSHDATA2)} + vm := load(prog) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + +func TestPushData2ShortN(t *testing.T) { + prog := []byte{byte(PUSHDATA2), 0} + vm := load(prog) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + +func TestPushData2BadN(t *testing.T) { + prog := []byte{byte(PUSHDATA2), 1, 0} + vm := load(prog) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + +func TestPushData2Good(t *testing.T) { + prog := makeProgram(PUSHDATA2, 3, 0, 1, 2, 3) + vm := load(prog) + vm.Run() + assert.Equal(t, false, vm.state.HasFlag(faultState)) + assert.Equal(t, 1, vm.estack.Len()) + assert.Equal(t, []byte{1, 2, 3}, vm.estack.Pop().Bytes()) +} + +func TestPushData4BadNoN(t *testing.T) { + prog := []byte{byte(PUSHDATA4)} + vm := load(prog) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + +func TestPushData4BadN(t *testing.T) { + prog := []byte{byte(PUSHDATA4), 1, 0, 0, 0} + vm := load(prog) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + +func TestPushData4ShortN(t *testing.T) { + prog := []byte{byte(PUSHDATA4), 0, 0, 0} + vm := load(prog) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + +func TestPushData4Good(t *testing.T) { + prog := makeProgram(PUSHDATA4, 3, 0, 0, 0, 1, 2, 3) + vm := load(prog) + vm.Run() + assert.Equal(t, false, vm.state.HasFlag(faultState)) + assert.Equal(t, 1, vm.estack.Len()) + assert.Equal(t, []byte{1, 2, 3}, vm.estack.Pop().Bytes()) } func TestNOTNoArgument(t *testing.T) { From 17f3a21e271b52933ea8de0eb915a04e56a08a11 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 11 Sep 2019 12:00:11 +0300 Subject: [PATCH 2/3] vm: harden SUBSTR implementation against bad index/offset values The upper index bound for slices is capacity, not length. Check for negative values also. Fixes #387. --- pkg/vm/vm.go | 9 +++++++++ pkg/vm/vm_test.go | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index e5c909508..e20f0cedd 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -320,6 +320,15 @@ func (v *VM) execute(ctx *Context, op Instruction) { l := int(v.estack.Pop().BigInt().Int64()) o := int(v.estack.Pop().BigInt().Int64()) s := v.estack.Pop().Bytes() + if l < 0 { + panic("negative length") + } + if o < 0 { + panic("negative index") + } + if l+o > len(s) { + panic("out of bounds access") + } v.estack.PushVal(s[o : o+l]) case LEFT: l := int(v.estack.Pop().BigInt().Int64()) diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index dfb4a5d30..ed36f27fd 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -878,6 +878,38 @@ func TestSUBSTRBadLen(t *testing.T) { assert.Equal(t, true, vm.state.HasFlag(faultState)) } +func TestSUBSTRBad387(t *testing.T) { + prog := makeProgram(SUBSTR) + vm := load(prog) + b := make([]byte, 6, 20) + copy(b, "abcdef") + vm.estack.PushVal(b) + vm.estack.PushVal(1) + vm.estack.PushVal(6) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + +func TestSUBSTRBadNegativeOffset(t *testing.T) { + prog := makeProgram(SUBSTR) + vm := load(prog) + vm.estack.PushVal([]byte("abcdef")) + vm.estack.PushVal(-1) + vm.estack.PushVal(3) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + +func TestSUBSTRBadNegativeLen(t *testing.T) { + prog := makeProgram(SUBSTR) + vm := load(prog) + vm.estack.PushVal([]byte("abcdef")) + vm.estack.PushVal(3) + vm.estack.PushVal(-1) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + func TestLEFTBadNoArgs(t *testing.T) { prog := makeProgram(LEFT) vm := load(prog) From 8311bda355ae300e0dd5c3d0386520e515888feb Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Wed, 11 Sep 2019 12:03:43 +0300 Subject: [PATCH 3/3] vm: harden LEFT and RIGHT implementations against negative indexes --- pkg/vm/vm.go | 6 ++++++ pkg/vm/vm_test.go | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/pkg/vm/vm.go b/pkg/vm/vm.go index e20f0cedd..d2638f740 100644 --- a/pkg/vm/vm.go +++ b/pkg/vm/vm.go @@ -332,6 +332,9 @@ func (v *VM) execute(ctx *Context, op Instruction) { v.estack.PushVal(s[o : o+l]) case LEFT: l := int(v.estack.Pop().BigInt().Int64()) + if l < 0 { + panic("negative length") + } s := v.estack.Pop().Bytes() if t := len(s); l > t { l = t @@ -339,6 +342,9 @@ func (v *VM) execute(ctx *Context, op Instruction) { v.estack.PushVal(s[:l]) case RIGHT: l := int(v.estack.Pop().BigInt().Int64()) + if l < 0 { + panic("negative length") + } s := v.estack.Pop().Bytes() v.estack.PushVal(s[len(s)-l:]) case XDROP: diff --git a/pkg/vm/vm_test.go b/pkg/vm/vm_test.go index ed36f27fd..a75d78b50 100644 --- a/pkg/vm/vm_test.go +++ b/pkg/vm/vm_test.go @@ -925,6 +925,15 @@ func TestLEFTBadNoString(t *testing.T) { assert.Equal(t, true, vm.state.HasFlag(faultState)) } +func TestLEFTBadNegativeLen(t *testing.T) { + prog := makeProgram(LEFT) + vm := load(prog) + vm.estack.PushVal([]byte("abcdef")) + vm.estack.PushVal(-1) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + func TestLEFTGood(t *testing.T) { prog := makeProgram(LEFT) vm := load(prog) @@ -962,6 +971,15 @@ func TestRIGHTBadNoString(t *testing.T) { assert.Equal(t, true, vm.state.HasFlag(faultState)) } +func TestRIGHTBadNegativeLen(t *testing.T) { + prog := makeProgram(RIGHT) + vm := load(prog) + vm.estack.PushVal([]byte("abcdef")) + vm.estack.PushVal(-1) + vm.Run() + assert.Equal(t, true, vm.state.HasFlag(faultState)) +} + func TestRIGHTGood(t *testing.T) { prog := makeProgram(RIGHT) vm := load(prog)