From e861aeec2e937750321006b0844e90bd32771823 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Fri, 26 Jul 2024 11:22:03 +0300 Subject: [PATCH 1/3] manifest: disallow null groups, fix #3522 IsValid() is used by both compiler and ContractManagement then. Signed-off-by: Roman Khimov --- pkg/core/interop/runtime/ext_test.go | 3 ++- pkg/core/native/management_neotest_test.go | 6 ++++-- pkg/smartcontract/manifest/group.go | 3 +++ pkg/smartcontract/manifest/group_test.go | 7 ++++++- 4 files changed, 15 insertions(+), 4 deletions(-) diff --git a/pkg/core/interop/runtime/ext_test.go b/pkg/core/interop/runtime/ext_test.go index f8d2bcf89..7d11f025e 100644 --- a/pkg/core/interop/runtime/ext_test.go +++ b/pkg/core/interop/runtime/ext_test.go @@ -712,7 +712,8 @@ func TestSystemRuntimeNotify_HFBasilisk(t *testing.T) { require.NoError(t, err) m := &manifest.Manifest{ - Name: "ctr", + Name: "ctr", + Groups: []manifest.Group{}, ABI: manifest.ABI{ Methods: []manifest.Method{ { diff --git a/pkg/core/native/management_neotest_test.go b/pkg/core/native/management_neotest_test.go index 8c2f63d7f..7434feb3f 100644 --- a/pkg/core/native/management_neotest_test.go +++ b/pkg/core/native/management_neotest_test.go @@ -52,7 +52,8 @@ func TestManagement_DeployUpdate_HFBasilisk(t *testing.T) { require.NoError(t, err) m := &manifest.Manifest{ - Name: "ctr", + Name: "ctr", + Groups: []manifest.Group{}, ABI: manifest.ABI{ Methods: []manifest.Method{ { @@ -87,7 +88,8 @@ func TestManagement_CallInTheSameBlock(t *testing.T) { require.NoError(t, err) m := &manifest.Manifest{ - Name: "ctr", + Name: "ctr", + Groups: []manifest.Group{}, ABI: manifest.ABI{ Methods: []manifest.Method{ { diff --git a/pkg/smartcontract/manifest/group.go b/pkg/smartcontract/manifest/group.go index 96834ddb0..15485aa3c 100644 --- a/pkg/smartcontract/manifest/group.go +++ b/pkg/smartcontract/manifest/group.go @@ -40,6 +40,9 @@ func (g *Group) IsValid(h util.Uint160) error { // AreValid checks for groups correctness and uniqueness. // If the contract hash is empty, then hash-related checks are omitted. func (g Groups) AreValid(h util.Uint160) error { + if g == nil { + return errors.New("null groups") + } if !h.Equals(util.Uint160{}) { for i := range g { err := g[i].IsValid(h) diff --git a/pkg/smartcontract/manifest/group_test.go b/pkg/smartcontract/manifest/group_test.go index 2390da7f5..d38d8eeb8 100644 --- a/pkg/smartcontract/manifest/group_test.go +++ b/pkg/smartcontract/manifest/group_test.go @@ -19,6 +19,10 @@ func TestGroupJSONInOut(t *testing.T) { } func TestGroupsAreValid(t *testing.T) { + var gps Groups + + require.Error(t, gps.AreValid(util.Uint160{})) // null + h := util.Uint160{42, 42, 42} priv, err := keys.NewPrivateKey() require.NoError(t, err) @@ -29,7 +33,8 @@ func TestGroupsAreValid(t *testing.T) { gcorrect := Group{pub, priv.Sign(h.BytesBE())} gcorrect2 := Group{pub2, priv2.Sign(h.BytesBE())} gincorrect := Group{pub, priv.Sign(h.BytesLE())} - gps := Groups{gcorrect} + + gps = Groups{gcorrect} require.NoError(t, gps.AreValid(h)) gps = Groups{gincorrect} From 58ab24efdbc0aeca978b3c98714d65de493f3983 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Fri, 26 Jul 2024 11:43:10 +0300 Subject: [PATCH 2/3] manifest: don't accept manifests with invalid features Refs. #3522. Signed-off-by: Roman Khimov --- .../testdata/verify.manifest.json | 2 +- .../testdata/verifyrpc/verify.manifest.json | 1 + pkg/core/interop/runtime/ext_test.go | 5 +++-- pkg/core/native/management_neotest_test.go | 10 ++++++---- pkg/smartcontract/manifest/manifest.go | 17 +++++++++++++++-- pkg/smartcontract/manifest/manifest_test.go | 18 ++++++++++++++++++ 6 files changed, 44 insertions(+), 9 deletions(-) diff --git a/cli/smartcontract/testdata/verify.manifest.json b/cli/smartcontract/testdata/verify.manifest.json index 47bf7b91a..d6363f7f1 100755 --- a/cli/smartcontract/testdata/verify.manifest.json +++ b/cli/smartcontract/testdata/verify.manifest.json @@ -1 +1 @@ -{"name":"verify","abi":{"methods":[{"name":"verify","offset":0,"parameters":[],"returntype":"Boolean","safe":false},{"name":"onNEP17Payment","offset":5,"parameters":[{"name":"from","type":"ByteArray"},{"name":"amount","type":"Integer"},{"name":"data","type":"Any"}],"returntype":"Void","safe":false}],"events":[{"name":"Hello world!","parameters":[{"name":"args","type":"Array"}]}]},"groups":[],"permissions":[{"contract":"*","methods":"*"}],"supportedstandards":[],"trusts":[],"extra":null} +{"name":"verify","abi":{"methods":[{"name":"verify","offset":0,"parameters":[],"returntype":"Boolean","safe":false},{"name":"onNEP17Payment","offset":5,"parameters":[{"name":"from","type":"ByteArray"},{"name":"amount","type":"Integer"},{"name":"data","type":"Any"}],"returntype":"Void","safe":false}],"events":[{"name":"Hello world!","parameters":[{"name":"args","type":"Array"}]}]},"groups":[],"permissions":[{"contract":"*","methods":"*"}],"supportedstandards":[],"trusts":[],"extra":null,"features":{}} diff --git a/cli/smartcontract/testdata/verifyrpc/verify.manifest.json b/cli/smartcontract/testdata/verifyrpc/verify.manifest.json index 8b70f752b..eb243980c 100755 --- a/cli/smartcontract/testdata/verifyrpc/verify.manifest.json +++ b/cli/smartcontract/testdata/verifyrpc/verify.manifest.json @@ -4,6 +4,7 @@ "supportedstandards" : [], "name" : "verify", "trusts" : [], + "features": {}, "permissions" : [ { "methods" : "*", diff --git a/pkg/core/interop/runtime/ext_test.go b/pkg/core/interop/runtime/ext_test.go index 7d11f025e..641703948 100644 --- a/pkg/core/interop/runtime/ext_test.go +++ b/pkg/core/interop/runtime/ext_test.go @@ -712,8 +712,9 @@ func TestSystemRuntimeNotify_HFBasilisk(t *testing.T) { require.NoError(t, err) m := &manifest.Manifest{ - Name: "ctr", - Groups: []manifest.Group{}, + Name: "ctr", + Features: json.RawMessage("{}"), + Groups: []manifest.Group{}, ABI: manifest.ABI{ Methods: []manifest.Method{ { diff --git a/pkg/core/native/management_neotest_test.go b/pkg/core/native/management_neotest_test.go index 7434feb3f..13f91ad29 100644 --- a/pkg/core/native/management_neotest_test.go +++ b/pkg/core/native/management_neotest_test.go @@ -52,8 +52,9 @@ func TestManagement_DeployUpdate_HFBasilisk(t *testing.T) { require.NoError(t, err) m := &manifest.Manifest{ - Name: "ctr", - Groups: []manifest.Group{}, + Name: "ctr", + Features: json.RawMessage("{}"), + Groups: []manifest.Group{}, ABI: manifest.ABI{ Methods: []manifest.Method{ { @@ -88,8 +89,9 @@ func TestManagement_CallInTheSameBlock(t *testing.T) { require.NoError(t, err) m := &manifest.Manifest{ - Name: "ctr", - Groups: []manifest.Group{}, + Name: "ctr", + Features: json.RawMessage("{}"), + Groups: []manifest.Group{}, ABI: manifest.ABI{ Methods: []manifest.Method{ { diff --git a/pkg/smartcontract/manifest/manifest.go b/pkg/smartcontract/manifest/manifest.go index 3f5ac8417..9414cb5db 100644 --- a/pkg/smartcontract/manifest/manifest.go +++ b/pkg/smartcontract/manifest/manifest.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "math" + "strings" ojson "github.com/nspcc-dev/go-ordered-json" "github.com/nspcc-dev/neo-go/pkg/util" @@ -24,6 +25,8 @@ const ( NEP11Payable = "NEP-11-Payable" // NEP17Payable represents the name of contract interface which can receive NEP-17 tokens. NEP17Payable = "NEP-17-Payable" + + emptyFeatures = "{}" ) // Manifest represens contract metadata. @@ -53,7 +56,7 @@ func NewManifest(name string) *Manifest { Methods: []Method{}, Events: []Event{}, }, - Features: json.RawMessage("{}"), + Features: json.RawMessage(emptyFeatures), Groups: []Group{}, Permissions: []Permission{}, SupportedStandards: []string{}, @@ -107,6 +110,16 @@ func (m *Manifest) IsValid(hash util.Uint160, checkSize bool) error { if err != nil { return fmt.Errorf("ABI: %w", err) } + + if strings.Map(func(c rune) rune { + switch c { + case ' ', '\n', '\t', '\r': // Strip all JSON whitespace. + return -1 + } + return c + }, string(m.Features)) != emptyFeatures { // empty struct should be left. + return errors.New("invalid features") + } err = Groups(m.Groups).AreValid(hash) if err != nil { return err @@ -235,7 +248,7 @@ func (m *Manifest) FromStackItem(item stackitem.Item) error { if str[2].Type() != stackitem.MapT || str[2].(*stackitem.Map).Len() != 0 { return errors.New("invalid Features stackitem") } - m.Features = json.RawMessage("{}") + m.Features = json.RawMessage(emptyFeatures) if str[3].Type() != stackitem.ArrayT { return errors.New("invalid SupportedStandards stackitem type") } diff --git a/pkg/smartcontract/manifest/manifest_test.go b/pkg/smartcontract/manifest/manifest_test.go index e96c5062a..cd7a26055 100644 --- a/pkg/smartcontract/manifest/manifest_test.go +++ b/pkg/smartcontract/manifest/manifest_test.go @@ -144,6 +144,24 @@ func TestIsValid(t *testing.T) { require.NoError(t, m.IsValid(contractHash, true)) }) + t.Run("invalid, no features", func(t *testing.T) { + m.Features = nil + require.Error(t, m.IsValid(contractHash, true)) + }) + m.Features = json.RawMessage(emptyFeatures) + + t.Run("invalid, bad features", func(t *testing.T) { + m.Features = json.RawMessage(`{ "v" : true}`) + require.Error(t, m.IsValid(contractHash, true)) + }) + m.Features = json.RawMessage(emptyFeatures) + + t.Run("valid, features with spaces", func(t *testing.T) { + m.Features = json.RawMessage("{ \t\n\r }") + require.NoError(t, m.IsValid(contractHash, true)) + }) + m.Features = json.RawMessage(emptyFeatures) + m.ABI.Events = append(m.ABI.Events, Event{ Name: "itHappened", Parameters: []Parameter{}, From b10af1ed313283e081199a24cbda5ae5c99427b0 Mon Sep 17 00:00:00 2001 From: Roman Khimov Date: Fri, 26 Jul 2024 12:32:49 +0300 Subject: [PATCH 3/3] manifest: make default trusts invalid Refs. #3522. The core problem is the same as for groups/features: we can't allow empty trusts when they're unmarshalled from JSON. But unlike others we can't easily differentiate missing any value with other cases because the default value for WildPermissionDescs is a valid thing. Adding an additional field makes it invalid and we can build around it. Other options are implementing custom UnmarshalJSON for Manifest (too much for this) or making Trusts a pointer (an option, but can fail in too many ways). Signed-off-by: Roman Khimov --- pkg/core/interop/runtime/ext_test.go | 1 + pkg/core/native/management_neotest_test.go | 2 ++ pkg/core/state/contract_test.go | 1 + pkg/smartcontract/manifest/container.go | 13 +++++++++---- pkg/smartcontract/manifest/container_test.go | 3 +++ pkg/smartcontract/manifest/manifest.go | 5 ++++- pkg/smartcontract/manifest/manifest_test.go | 7 +++++++ 7 files changed, 27 insertions(+), 5 deletions(-) diff --git a/pkg/core/interop/runtime/ext_test.go b/pkg/core/interop/runtime/ext_test.go index 641703948..77b1c4bd2 100644 --- a/pkg/core/interop/runtime/ext_test.go +++ b/pkg/core/interop/runtime/ext_test.go @@ -715,6 +715,7 @@ func TestSystemRuntimeNotify_HFBasilisk(t *testing.T) { Name: "ctr", Features: json.RawMessage("{}"), Groups: []manifest.Group{}, + Trusts: manifest.WildPermissionDescs{Wildcard: true}, ABI: manifest.ABI{ Methods: []manifest.Method{ { diff --git a/pkg/core/native/management_neotest_test.go b/pkg/core/native/management_neotest_test.go index 13f91ad29..d8d8e0eac 100644 --- a/pkg/core/native/management_neotest_test.go +++ b/pkg/core/native/management_neotest_test.go @@ -55,6 +55,7 @@ func TestManagement_DeployUpdate_HFBasilisk(t *testing.T) { Name: "ctr", Features: json.RawMessage("{}"), Groups: []manifest.Group{}, + Trusts: manifest.WildPermissionDescs{Wildcard: true}, ABI: manifest.ABI{ Methods: []manifest.Method{ { @@ -92,6 +93,7 @@ func TestManagement_CallInTheSameBlock(t *testing.T) { Name: "ctr", Features: json.RawMessage("{}"), Groups: []manifest.Group{}, + Trusts: manifest.WildPermissionDescs{Wildcard: true}, ABI: manifest.ABI{ Methods: []manifest.Method{ { diff --git a/pkg/core/state/contract_test.go b/pkg/core/state/contract_test.go index 5bc62e0f1..9e5b257af 100644 --- a/pkg/core/state/contract_test.go +++ b/pkg/core/state/contract_test.go @@ -58,6 +58,7 @@ func TestContractStateToFromSI(t *testing.T) { t.Run("preserve wildcard trusts", func(t *testing.T) { contract.Manifest.Trusts.Value = nil + contract.Manifest.Trusts.Wildcard = true require.True(t, contract.Manifest.Trusts.IsWildcard()) actual := new(Contract) item, err := contract.ToStackItem() diff --git a/pkg/smartcontract/manifest/container.go b/pkg/smartcontract/manifest/container.go index f99132a48..ca2a84779 100644 --- a/pkg/smartcontract/manifest/container.go +++ b/pkg/smartcontract/manifest/container.go @@ -16,7 +16,8 @@ type WildStrings struct { // WildPermissionDescs represents a PermissionDescriptor set which can be a wildcard. type WildPermissionDescs struct { - Value []PermissionDesc + Value []PermissionDesc + Wildcard bool } // Contains checks if v is in the container. @@ -49,13 +50,16 @@ func (c *WildPermissionDescs) Contains(v PermissionDesc) bool { func (c *WildStrings) IsWildcard() bool { return c.Value == nil } // IsWildcard returns true iff the container is a wildcard. -func (c *WildPermissionDescs) IsWildcard() bool { return c.Value == nil } +func (c *WildPermissionDescs) IsWildcard() bool { return c.Wildcard } // Restrict transforms the container into an empty one. func (c *WildStrings) Restrict() { c.Value = []string{} } // Restrict transforms the container into an empty one. -func (c *WildPermissionDescs) Restrict() { c.Value = []PermissionDesc{} } +func (c *WildPermissionDescs) Restrict() { + c.Value = []PermissionDesc{} + c.Wildcard = false +} // Add adds v to the container. func (c *WildStrings) Add(v string) { c.Value = append(c.Value, v) } @@ -93,7 +97,8 @@ func (c *WildStrings) UnmarshalJSON(data []byte) error { // UnmarshalJSON implements the json.Unmarshaler interface. func (c *WildPermissionDescs) UnmarshalJSON(data []byte) error { - if !bytes.Equal(data, []byte(`"*"`)) { + c.Wildcard = bytes.Equal(data, []byte(`"*"`)) + if !c.Wildcard { us := []PermissionDesc{} if err := json.Unmarshal(data, &us); err != nil { return err diff --git a/pkg/smartcontract/manifest/container_test.go b/pkg/smartcontract/manifest/container_test.go index 8f8b238b9..1e3b5e420 100644 --- a/pkg/smartcontract/manifest/container_test.go +++ b/pkg/smartcontract/manifest/container_test.go @@ -24,6 +24,9 @@ func TestContainer_Restrict(t *testing.T) { t.Run("PermissionDesc", func(t *testing.T) { check := func(t *testing.T, u PermissionDesc) { c := new(WildPermissionDescs) + require.False(t, c.IsWildcard()) + require.False(t, c.Contains(u)) + c.Wildcard = true require.True(t, c.IsWildcard()) require.True(t, c.Contains(u)) c.Restrict() diff --git a/pkg/smartcontract/manifest/manifest.go b/pkg/smartcontract/manifest/manifest.go index 9414cb5db..95a2f41d3 100644 --- a/pkg/smartcontract/manifest/manifest.go +++ b/pkg/smartcontract/manifest/manifest.go @@ -124,6 +124,9 @@ func (m *Manifest) IsValid(hash util.Uint160, checkSize bool) error { if err != nil { return err } + if m.Trusts.Value == nil && !m.Trusts.Wildcard { + return errors.New("invalid (null?) trusts") + } if len(m.Trusts.Value) > 1 { hashes := make([]PermissionDesc, len(m.Trusts.Value)) copy(hashes, m.Trusts.Value) @@ -278,7 +281,7 @@ func (m *Manifest) FromStackItem(item stackitem.Item) error { m.Permissions[i] = *p } if _, ok := str[6].(stackitem.Null); ok { - m.Trusts = WildPermissionDescs{Value: nil} // wildcard by default + m.Trusts = WildPermissionDescs{Value: nil, Wildcard: true} // wildcard by default } else { if str[6].Type() != stackitem.ArrayT { return errors.New("invalid Trusts stackitem type") diff --git a/pkg/smartcontract/manifest/manifest_test.go b/pkg/smartcontract/manifest/manifest_test.go index cd7a26055..308829e0e 100644 --- a/pkg/smartcontract/manifest/manifest_test.go +++ b/pkg/smartcontract/manifest/manifest_test.go @@ -212,6 +212,13 @@ func TestIsValid(t *testing.T) { }) m.SupportedStandards = m.SupportedStandards[:1] + t.Run("invalid, no trusts", func(t *testing.T) { + m.Trusts.Value = nil + m.Trusts.Wildcard = false + require.Error(t, m.IsValid(contractHash, true)) + }) + m.Trusts.Restrict() + d := PermissionDesc{Type: PermissionHash, Value: util.Uint160{1, 2, 3}} m.Trusts.Add(d) t.Run("valid, with trust", func(t *testing.T) {