From 55b2746be1ce2b8ff18262a25078e2ee3fd1ffb9 Mon Sep 17 00:00:00 2001 From: Hyunsoo Shin Date: Fri, 30 Jun 2023 17:37:11 +0900 Subject: [PATCH 1/4] [ABI] Added a gaurd that checks if offset is greater than the buffer --- accounts/abi/abi_test.go | 37 +++++++++++++++++++++++++++++++++++++ accounts/abi/unpack.go | 5 ++++- 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/accounts/abi/abi_test.go b/accounts/abi/abi_test.go index 3ffcf9e48e..d7805b8a48 100644 --- a/accounts/abi/abi_test.go +++ b/accounts/abi/abi_test.go @@ -33,6 +33,7 @@ import ( "github.com/klaytn/klaytn/common" "github.com/klaytn/klaytn/common/math" "github.com/klaytn/klaytn/crypto" + "github.com/stretchr/testify/assert" ) const jsondata = ` @@ -763,6 +764,42 @@ func TestUnpackEvent(t *testing.T) { } } +// Testset (ABI and hexdata) was created based on this contract format. +/* +contract T { + event eventInDynamicType(uint elem1, string[3] elem2); + constructor() {} + function test123() public { + string[3] memory vals = ["A","B","C"]; + emit eventInDynamicType(123, vals); + } + } +*/ +func TestUnpackEventOffsetBound(t *testing.T) { + const abiJSON = `[{"inputs":[],"stateMutability":"nonpayable","type":"constructor"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"elem1","type":"uint256"},{"indexed":false,"internalType":"string[3]","name":"elem2","type":"string[3]"}],"name":"ev123","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"elem1","type":"uint256"},{"indexed":false,"internalType":"string[3]","name":"elem2","type":"string[3]"}],"name":"eventInDynamicType","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"address","name":"sender","type":"address"},{"indexed":false,"internalType":"uint256","name":"amount","type":"uint256"},{"indexed":false,"internalType":"bytes","name":"memo","type":"bytes"}],"name":"received","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"address","name":"sender","type":"address"}],"name":"receivedAddr","type":"event"},{"inputs":[],"name":"test123","outputs":[],"stateMutability":"nonpayable","type":"function"}]` + + abi, err := JSON(strings.NewReader(abiJSON)) + if err != nil { + t.Fatal(err) + } + + // Commented bytes are pure data. Offset value was modified to make a crash + // const hexdata = `000000000000000000000000000000000000000000000000000000000000007b0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000140000000000000000000000000000000000000000000000000000000000000004141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141410000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000354242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242000000000000000000000000000000000000000000000000000000000000000000000000000000000000684343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343000000000000000000000000000000000000000000000000` + const hexdata = `000000000000000000000000000000000000000000000000000000000000007b0000000000000000000000000000000000000000000000000000000000000fff000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000140000000000000000000000000000000000000000000000000000000000000004141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141410000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000354242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242000000000000000000000000000000000000000000000000000000000000000000000000000000000000684343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343000000000000000000000000000000000000000000000000` + data, err := hex.DecodeString(hexdata) + if err != nil { + t.Fatal(err) + } + + type evObj struct { + Myval *big.Int + Myvals [3]string + } + + var params evObj + assert.NotNil(t, abi.Unpack(¶ms, "eventInDynamicType", data)) +} + func TestUnpackEventIntoMap(t *testing.T) { const abiJSON = `[{"constant":false,"inputs":[{"name":"memo","type":"bytes"}],"name":"receive","outputs":[],"payable":true,"stateMutability":"payable","type":"function"},{"anonymous":false,"inputs":[{"indexed":false,"name":"sender","type":"address"},{"indexed":false,"name":"amount","type":"uint256"},{"indexed":false,"name":"memo","type":"bytes"}],"name":"received","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"name":"sender","type":"address"}],"name":"receivedAddr","type":"event"}]` abi, err := JSON(strings.NewReader(abiJSON)) diff --git a/accounts/abi/unpack.go b/accounts/abi/unpack.go index 46c4c3ad1d..3e81075159 100644 --- a/accounts/abi/unpack.go +++ b/accounts/abi/unpack.go @@ -227,7 +227,10 @@ func toGoType(index int, t Type, output []byte) (interface{}, error) { return forEachUnpack(t, output[begin:], 0, length) case ArrayTy: if isDynamicType(*t.Elem) { - offset := int64(binary.BigEndian.Uint64(returnOutput[len(returnOutput)-8:])) + offset := binary.BigEndian.Uint64(returnOutput[len(returnOutput)-8:]) + if offset > uint64(len(output)) { + return nil, fmt.Errorf("abi: toGoType offset greater than output length: offset: %d, len(output): %d", offset, len(output)) + } return forEachUnpack(t, output[offset:], 0, t.Size) } return forEachUnpack(t, output[index:], 0, t.Size) From 857c10a466677df01c914b4491ee64f5a32404fb Mon Sep 17 00:00:00 2001 From: Hyunsoo Shin Date: Tue, 4 Jul 2023 14:45:14 +0900 Subject: [PATCH 2/4] [ABI] Refactored --- accounts/abi/abi_test.go | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/accounts/abi/abi_test.go b/accounts/abi/abi_test.go index d7805b8a48..8ca9158099 100644 --- a/accounts/abi/abi_test.go +++ b/accounts/abi/abi_test.go @@ -770,7 +770,7 @@ contract T { event eventInDynamicType(uint elem1, string[3] elem2); constructor() {} function test123() public { - string[3] memory vals = ["A","B","C"]; + string[3] memory vals = ["A...","B...","C..."]; emit eventInDynamicType(123, vals); } } @@ -779,25 +779,24 @@ func TestUnpackEventOffsetBound(t *testing.T) { const abiJSON = `[{"inputs":[],"stateMutability":"nonpayable","type":"constructor"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"elem1","type":"uint256"},{"indexed":false,"internalType":"string[3]","name":"elem2","type":"string[3]"}],"name":"ev123","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"uint256","name":"elem1","type":"uint256"},{"indexed":false,"internalType":"string[3]","name":"elem2","type":"string[3]"}],"name":"eventInDynamicType","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"address","name":"sender","type":"address"},{"indexed":false,"internalType":"uint256","name":"amount","type":"uint256"},{"indexed":false,"internalType":"bytes","name":"memo","type":"bytes"}],"name":"received","type":"event"},{"anonymous":false,"inputs":[{"indexed":false,"internalType":"address","name":"sender","type":"address"}],"name":"receivedAddr","type":"event"},{"inputs":[],"name":"test123","outputs":[],"stateMutability":"nonpayable","type":"function"}]` abi, err := JSON(strings.NewReader(abiJSON)) - if err != nil { - t.Fatal(err) - } + assert.Nil(t, err, err) - // Commented bytes are pure data. Offset value was modified to make a crash - // const hexdata = `000000000000000000000000000000000000000000000000000000000000007b0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000140000000000000000000000000000000000000000000000000000000000000004141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141410000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000354242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242000000000000000000000000000000000000000000000000000000000000000000000000000000000000684343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343000000000000000000000000000000000000000000000000` - const hexdata = `000000000000000000000000000000000000000000000000000000000000007b0000000000000000000000000000000000000000000000000000000000000fff000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000140000000000000000000000000000000000000000000000000000000000000004141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141410000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000354242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242000000000000000000000000000000000000000000000000000000000000000000000000000000000000684343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343000000000000000000000000000000000000000000000000` - data, err := hex.DecodeString(hexdata) - if err != nil { - t.Fatal(err) - } + const rawData = `000000000000000000000000000000000000000000000000000000000000007b0000000000000000000000000000000000000000000000000000000000000040000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000e00000000000000000000000000000000000000000000000000000000000000140000000000000000000000000000000000000000000000000000000000000004141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141414141410000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000354242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242424242000000000000000000000000000000000000000000000000000000000000000000000000000000000000684343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343434343000000000000000000000000000000000000000000000000` + // Modify offset value (0x40 -> 0xffff) to attemp out-of-bounds access + modifiedRawData := rawData[:124] + "ffff" + rawData[128:] + + data, err := hex.DecodeString(modifiedRawData) + assert.Nil(t, err, err) type evObj struct { - Myval *big.Int - Myvals [3]string + Elem1 *big.Int + Elem2 [3]string } var params evObj - assert.NotNil(t, abi.Unpack(¶ms, "eventInDynamicType", data)) + err = abi.Unpack(¶ms, "eventInDynamicType", data) + // Must return error + assert.NotNil(t, err, err) } func TestUnpackEventIntoMap(t *testing.T) { From f42fbad60451417f6a7d4ecc612c1dbf54b743cf Mon Sep 17 00:00:00 2001 From: Hyunsoo Shin Date: Tue, 4 Jul 2023 16:32:13 +0900 Subject: [PATCH 3/4] [] Format fixed --- accounts/abi/abi_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/accounts/abi/abi_test.go b/accounts/abi/abi_test.go index 8ca9158099..29e6efe982 100644 --- a/accounts/abi/abi_test.go +++ b/accounts/abi/abi_test.go @@ -789,13 +789,13 @@ func TestUnpackEventOffsetBound(t *testing.T) { assert.Nil(t, err, err) type evObj struct { - Elem1 *big.Int + Elem1 *big.Int Elem2 [3]string } var params evObj err = abi.Unpack(¶ms, "eventInDynamicType", data) - // Must return error + // Must return error assert.NotNil(t, err, err) } From 00e0e5948cccf54ddea5651e7b2238d08a7cd92d Mon Sep 17 00:00:00 2001 From: Hyunsoo Shin Date: Tue, 18 Jul 2023 12:12:33 +0900 Subject: [PATCH 4/4] [Test] Removed ineffective --- accounts/abi/abi_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/accounts/abi/abi_test.go b/accounts/abi/abi_test.go index 29e6efe982..2848fc65e5 100644 --- a/accounts/abi/abi_test.go +++ b/accounts/abi/abi_test.go @@ -786,7 +786,7 @@ func TestUnpackEventOffsetBound(t *testing.T) { modifiedRawData := rawData[:124] + "ffff" + rawData[128:] data, err := hex.DecodeString(modifiedRawData) - assert.Nil(t, err, err) + assert.Nil(t, err) type evObj struct { Elem1 *big.Int