From 0e8b51cf752611d2b1a10d66bc09d6d0da13ac8a Mon Sep 17 00:00:00 2001 From: Leonard Lyubich Date: Tue, 9 Jul 2024 11:29:33 +0400 Subject: [PATCH 1/4] object/id: Test decoding failures for invalid ID length Signed-off-by: Leonard Lyubich --- object/id/id_test.go | 49 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/object/id/id_test.go b/object/id/id_test.go index baa1b221..3daea26e 100644 --- a/object/id/id_test.go +++ b/object/id/id_test.go @@ -180,3 +180,52 @@ func TestID_Encode(t *testing.T) { require.Equal(t, emptyID, id.EncodeToString()) }) } + +func TestID_Unmarshal(t *testing.T) { + t.Run("invalid value length", func(t *testing.T) { + var id ID + var m refs.ObjectID + for _, tc := range []struct { + err string + val []byte + }{ + {"invalid length 0", nil}, + {"invalid length 0", []byte{}}, + {"invalid length 31", make([]byte, 31)}, + {"invalid length 33", make([]byte, 33)}, + } { + m.SetValue(tc.val) + b := m.StableMarshal(nil) + require.EqualError(t, id.Unmarshal(b), tc.err) + } + }) +} + +func TestIDDecodingFailures(t *testing.T) { + var id ID + var m refs.ObjectID + for _, tc := range []struct { + err string + corrupt func(*refs.ObjectID) + }{ + {"invalid length 0", func(m *refs.ObjectID) { m.SetValue(nil) }}, + {"invalid length 0", func(m *refs.ObjectID) { m.SetValue([]byte{}) }}, + {"invalid length 31", func(m *refs.ObjectID) { m.SetValue(make([]byte, 31)) }}, + {"invalid length 33", func(m *refs.ObjectID) { m.SetValue(make([]byte, 33)) }}, + } { + id.WriteToV2(&m) + tc.corrupt(&m) + t.Run(tc.err+"/api", func(t *testing.T) { + require.EqualError(t, id.ReadFromV2(m), tc.err) + }) + t.Run(tc.err+"/binary", func(t *testing.T) { + b := m.StableMarshal(nil) + require.EqualError(t, id.Unmarshal(b), tc.err) + }) + t.Run(tc.err+"/json", func(t *testing.T) { + b, err := m.MarshalJSON() + require.NoError(t, err, tc.err) + require.EqualError(t, id.UnmarshalJSON(b), tc.err) + }) + } +} From bb7b0064136140ed0ccba1e8f9ab805f5fefecbf Mon Sep 17 00:00:00 2001 From: Leonard Lyubich Date: Tue, 9 Jul 2024 11:34:48 +0400 Subject: [PATCH 2/4] object/id: Fix silent swallowing of incorrect length on unmarshalling Previously, `Unmarshal`/`UnmarshalJSON` methods accepted any ID length: - those that were shorter were padded with zeros; - those that were longer were cut off. This behavior is not declared in the protocol, thus it was incorrect and could cover potential client/transport bugs. Now the length is strictly checked. In particular, corresponding unit test now passes. Signed-off-by: Leonard Lyubich --- object/id/id.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/object/id/id.go b/object/id/id.go index c3436b56..e203f581 100644 --- a/object/id/id.go +++ b/object/id/id.go @@ -141,9 +141,7 @@ func (id *ID) Unmarshal(data []byte) error { return err } - copy(id[:], v2.GetValue()) - - return nil + return id.ReadFromV2(v2) } // MarshalJSON encodes ID to protobuf JSON format. @@ -161,7 +159,5 @@ func (id *ID) UnmarshalJSON(data []byte) error { return err } - copy(id[:], v2.GetValue()) - - return nil + return id.ReadFromV2(v2) } From 2cca6c7a0c5929186f3d9496fbfe6cde95a2897b Mon Sep 17 00:00:00 2001 From: Leonard Lyubich Date: Tue, 9 Jul 2024 11:59:48 +0400 Subject: [PATCH 3/4] eacl: Provide API converter checking container ID length `NewTableFromV2` function does not check the length accepts any container ID length: - those that were shorter were padded with zeros; - those that were longer were cut off. This behavior is incorrect because it goes against the NeoFS API protocol. However, it is not possible to fix the function backwards compatible since it does not return an error. Thus, a new `ReadFromV2` method is added (it is already widely used in the library). The method checks container ID format and returns an error. Consequently, now the eACL format is also correctly checked when decoding a bearer token and `ContainerService.GetExtendedACLResponse` protocol message by the API client. `NewTableFromV2` is marked buggy and deprecated, it's going to be purged for release later. Signed-off-by: Leonard Lyubich --- bearer/bearer.go | 4 +- client/container.go | 9 +++-- eacl/table.go | 89 +++++++++++++++++++++++++-------------------- eacl/table_test.go | 3 ++ 4 files changed, 61 insertions(+), 44 deletions(-) diff --git a/bearer/bearer.go b/bearer/bearer.go index 8ae93fb6..7e6d0684 100644 --- a/bearer/bearer.go +++ b/bearer/bearer.go @@ -48,7 +48,9 @@ func (b *Token) readFromV2(m acl.BearerToken, checkFieldPresence bool) error { eaclTable := body.GetEACL() if b.eaclTableSet = eaclTable != nil; b.eaclTableSet { - b.eaclTable = *eacl.NewTableFromV2(eaclTable) + if err = b.eaclTable.ReadFromV2(*eaclTable); err != nil { + return fmt.Errorf("invalid eACL") + } } else if checkFieldPresence { return errors.New("missing eACL table") } diff --git a/client/container.go b/client/container.go index ba3a8f3a..49c89de5 100644 --- a/client/container.go +++ b/client/container.go @@ -437,14 +437,15 @@ func (c *Client) ContainerEACL(ctx context.Context, id cid.ID, prm PrmContainerE } cc.result = func(r responseV2) { resp := r.(*v2container.GetExtendedACLResponse) - + const fieldEACL = "eACL" eACL := resp.GetBody().GetEACL() if eACL == nil { - cc.err = newErrMissingResponseField("eACL") + cc.err = newErrMissingResponseField(fieldEACL) return } - - res = *eacl.NewTableFromV2(eACL) + if cc.err = res.ReadFromV2(*eACL); cc.err != nil { + cc.err = newErrInvalidResponseField(fieldEACL, cc.err) + } } // process call diff --git a/eacl/table.go b/eacl/table.go index 613db68d..c103d5d6 100644 --- a/eacl/table.go +++ b/eacl/table.go @@ -77,9 +77,50 @@ func (t *Table) AddRecord(r *Record) { } } +// ReadFromV2 reads Table from the [v2acl.Table] message. Returns an error if +// the message is malformed according to the NeoFS API V2 protocol. The message +// must not be nil. +// +// ReadFromV2 is intended to be used by the NeoFS API V2 client/server +// implementation only and is not expected to be directly used by applications. +// +// See also [Table.ToV2]. +func (t *Table) ReadFromV2(m v2acl.Table) error { + // set container id + if id := m.GetContainerID(); id != nil { + if t.cid == nil { + t.cid = new(cid.ID) + } + if err := t.cid.ReadFromV2(*id); err != nil { + return fmt.Errorf("invalid container ID: %w", err) + } + } + + // set version + if v := m.GetVersion(); v != nil { + ver := version.Version{} + ver.SetMajor(v.GetMajor()) + ver.SetMinor(v.GetMinor()) + + t.SetVersion(ver) + } + + // set eacl records + v2records := m.GetRecords() + t.records = make([]Record, len(v2records)) + + for i := range v2records { + t.records[i] = *NewRecordFromV2(&v2records[i]) + } + + return nil +} + // ToV2 converts Table to v2 acl.EACLTable message. // // Nil Table converts to nil. +// +// See also [Table.ReadFromV2]. func (t *Table) ToV2() *v2acl.Table { if t == nil { return nil @@ -133,6 +174,9 @@ func CreateTable(cid cid.ID) *Table { } // NewTableFromV2 converts v2 acl.EACLTable message to Table. +// +// Deprecated: BUG: container ID length is not checked. Use [Table.ReadFromV2] +// instead. func NewTableFromV2(table *v2acl.Table) *Table { t := new(Table) @@ -187,20 +231,11 @@ func (t Table) SignedData() []byte { // Unmarshal unmarshals protobuf binary representation of Table. func (t *Table) Unmarshal(data []byte) error { - fV2 := new(v2acl.Table) - if err := fV2.Unmarshal(data); err != nil { - return err - } - - // format checks - err := checkFormat(fV2) - if err != nil { + var m v2acl.Table + if err := m.Unmarshal(data); err != nil { return err } - - *t = *NewTableFromV2(fV2) - - return nil + return t.ReadFromV2(m) } // MarshalJSON encodes Table to protobuf JSON format. @@ -210,19 +245,11 @@ func (t *Table) MarshalJSON() ([]byte, error) { // UnmarshalJSON decodes Table from protobuf JSON format. func (t *Table) UnmarshalJSON(data []byte) error { - tV2 := new(v2acl.Table) - if err := tV2.UnmarshalJSON(data); err != nil { - return err - } - - err := checkFormat(tV2) - if err != nil { + var m v2acl.Table + if err := m.UnmarshalJSON(data); err != nil { return err } - - *t = *NewTableFromV2(tV2) - - return nil + return t.ReadFromV2(m) } // EqualTables compares Table with each other. @@ -249,19 +276,3 @@ func EqualTables(t1, t2 Table) bool { return true } - -func checkFormat(v2 *v2acl.Table) error { - var cID cid.ID - - cidV2 := v2.GetContainerID() - if cidV2 == nil { - return nil - } - - err := cID.ReadFromV2(*cidV2) - if err != nil { - return fmt.Errorf("could not convert V2 container ID: %w", err) - } - - return nil -} diff --git a/eacl/table_test.go b/eacl/table_test.go index e4ef39cb..7aec9346 100644 --- a/eacl/table_test.go +++ b/eacl/table_test.go @@ -31,6 +31,9 @@ func TestTable(t *testing.T) { newTable := eacl.NewTableFromV2(v2) require.Equal(t, table, newTable) + var res eacl.Table + require.NoError(t, res.ReadFromV2(*v2)) + require.Equal(t, table, newTable) t.Run("new from nil v2 table", func(t *testing.T) { require.Equal(t, new(eacl.Table), eacl.NewTableFromV2(nil)) From d5a5fbc5330ff498cdbc87eaa6787d2372bad334 Mon Sep 17 00:00:00 2001 From: Leonard Lyubich Date: Tue, 9 Jul 2024 12:01:40 +0400 Subject: [PATCH 4/4] *: Convert slices to arrays instead of copy where possible Became possible with Go 1.20. Signed-off-by: Leonard Lyubich --- container/id/id.go | 4 ++-- object/id/id.go | 4 ++-- object/search_test.go | 10 ++-------- object/slicer/slicer.go | 10 ++-------- session/common.go | 2 +- 5 files changed, 9 insertions(+), 21 deletions(-) diff --git a/container/id/id.go b/container/id/id.go index fdedaf4b..5eeb59f8 100644 --- a/container/id/id.go +++ b/container/id/id.go @@ -65,14 +65,14 @@ func (id *ID) Decode(src []byte) error { return fmt.Errorf("invalid length %d", len(src)) } - copy(id[:], src) + *id = ID(src) return nil } // SetSHA256 sets container identifier value to SHA256 checksum of container structure. func (id *ID) SetSHA256(v [sha256.Size]byte) { - copy(id[:], v[:]) + *id = v } // Equals defines a comparison relation between two ID instances. diff --git a/object/id/id.go b/object/id/id.go index e203f581..1b944554 100644 --- a/object/id/id.go +++ b/object/id/id.go @@ -65,14 +65,14 @@ func (id *ID) Decode(src []byte) error { return fmt.Errorf("invalid length %d", len(src)) } - copy(id[:], src) + *id = ID(src) return nil } // SetSHA256 sets object identifier value to SHA256 checksum. func (id *ID) SetSHA256(v [sha256.Size]byte) { - copy(id[:], v[:]) + *id = v } // Equals defines a comparison relation between two ID instances. diff --git a/object/search_test.go b/object/search_test.go index 585aca4d..39ea2183 100644 --- a/object/search_test.go +++ b/object/search_test.go @@ -341,11 +341,8 @@ func TestSearchFilters_AddPayloadHashFilter(t *testing.T) { func ExampleSearchFilters_AddPayloadHashFilter() { hash, _ := hex.DecodeString("66842cfea090b1d906b52400fae49d86df078c0670f2bdd059ba289ebe24a498") - var v [sha256.Size]byte - copy(v[:], hash[:sha256.Size]) - var cs checksum.Checksum - cs.SetSHA256(v) + cs.SetSHA256([sha256.Size]byte(hash)) fmt.Println(hex.EncodeToString(cs.Value())) // Output: 66842cfea090b1d906b52400fae49d86df078c0670f2bdd059ba289ebe24a498 @@ -377,11 +374,8 @@ func TestSearchFilters_AddHomomorphicHashFilter(t *testing.T) { func ExampleSearchFilters_AddHomomorphicHashFilter() { hash, _ := hex.DecodeString("7e302ebb3937e810feb501965580c746048db99cebd095c3ce27022407408bf904dde8d9aa8085d2cf7202345341cc947fa9d722c6b6699760d307f653815d0c") - var v [tz.Size]byte - copy(v[:], hash[:tz.Size]) - var cs checksum.Checksum - cs.SetTillichZemor(v) + cs.SetTillichZemor([tz.Size]byte(hash)) fmt.Println(hex.EncodeToString(cs.Value())) // Output: 7e302ebb3937e810feb501965580c746048db99cebd095c3ce27022407408bf904dde8d9aa8085d2cf7202345341cc947fa9d722c6b6699760d307f653815d0c diff --git a/object/slicer/slicer.go b/object/slicer/slicer.go index d350a0ed..44d54742 100644 --- a/object/slicer/slicer.go +++ b/object/slicer/slicer.go @@ -600,17 +600,11 @@ func (x *PayloadWriter) _writeChild(ctx context.Context, meta dynamicObjectMetad func flushObjectMetadata(signer neofscrypto.Signer, meta dynamicObjectMetadata, header *object.Object) (oid.ID, error) { var cs checksum.Checksum - var csBytes [sha256.Size]byte - copy(csBytes[:], meta.checksum.Sum(nil)) - - cs.SetSHA256(csBytes) + cs.SetSHA256([sha256.Size]byte(meta.checksum.Sum(nil))) header.SetPayloadChecksum(cs) if meta.homomorphicChecksum != nil { - var csHomoBytes [tz.Size]byte - copy(csHomoBytes[:], meta.homomorphicChecksum.Sum(nil)) - - cs.SetTillichZemor(csHomoBytes) + cs.SetTillichZemor([tz.Size]byte(meta.homomorphicChecksum.Sum(nil))) header.SetPayloadHomomorphicHash(cs) } diff --git a/session/common.go b/session/common.go index e72a3ff2..89bf9322 100644 --- a/session/common.go +++ b/session/common.go @@ -32,7 +32,7 @@ type contextReader func(session.TokenContext, bool) error func (x commonData) copyTo(dst *commonData) { dst.idSet = x.idSet - copy(dst.id[:], x.id[:]) + dst.id = x.id dst.issuerSet = x.issuerSet iss := x.issuer