From e54b52ec037cbcf4119dc46654abcf4a17285d78 Mon Sep 17 00:00:00 2001 From: Leonard Lyubich Date: Fri, 16 Sep 2022 14:22:09 +0400 Subject: [PATCH] [#1420] object/acl: Fix correlation of object session to request In previous implementation of `neofs-node` app object session was not checked for substitution of the object related to it. Also, for access checks, the session object was substituted instead of the one from the request. This, on the one hand, made it possible to inherit the session from the parent object for authorization for certain actions. On the other hand, it covered the mentioned object substitution, which is a critical vulnerability. Next changes are applied to processing of all Object service requests: - check if object session relates to the requested object - use requested object in access checks. Disclosed problem of object context inheritance will be solved within Signed-off-by: Leonard Lyubich --- CHANGELOG.md | 5 + cmd/neofs-cli/modules/session/util.go | 2 +- go.mod | 4 +- go.sum | Bin 116706 -> 116706 bytes pkg/services/object/acl/v2/service.go | 131 ++++++++++++++++-------- pkg/services/object/acl/v2/util.go | 71 +++++-------- pkg/services/object/acl/v2/util_test.go | 30 ++++++ 7 files changed, 155 insertions(+), 88 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 13fe707c3..aeb3107b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ Changelog for NeoFS Node - Fail startup if metabase has an old version (#1809) - Storage nodes could enter the network with any state (#1796) - Missing check of new state value in `ControlService.SetNetmapStatus` (#1797) +- Correlation of object session to request (#1420) ### Removed - Remove WIF and NEP2 support in `neofs-cli`'s --wallet flag (#1128) @@ -46,6 +47,10 @@ Storage nodes under maintenance are not excluded from the network map, but don't serve object operations. (*) can be fetched from network configuration via `neofs-cli netmap netinfo` command. +When issuing an object session token for root (virtual, "big") objects, +additionally include all members of the split-chain. If session context +includes root object only, it is not spread to physical ("small") objects. + ## [0.32.0] - 2022-09-14 - Pungdo (풍도, 楓島) ### Added diff --git a/cmd/neofs-cli/modules/session/util.go b/cmd/neofs-cli/modules/session/util.go index 6a710da80..674b90613 100644 --- a/cmd/neofs-cli/modules/session/util.go +++ b/cmd/neofs-cli/modules/session/util.go @@ -63,7 +63,7 @@ func Prepare(cmd *cobra.Command, cnr cid.ID, obj *oid.ID, key *ecdsa.PrivateKey, tok.BindContainer(cnr) if obj != nil { - tok.LimitByObject(*obj) + tok.LimitByObjects(*obj) } err := tok.Sign(*key) diff --git a/go.mod b/go.mod index 99d9f161c..b7a78da71 100644 --- a/go.mod +++ b/go.mod @@ -17,9 +17,9 @@ require ( github.com/nspcc-dev/hrw v1.0.9 github.com/nspcc-dev/neo-go v0.99.2 github.com/nspcc-dev/neo-go/pkg/interop v0.0.0-20220809123759-3094d3e0c14b // indirect - github.com/nspcc-dev/neofs-api-go/v2 v2.13.2-0.20220919124434-cf868188ef9c + github.com/nspcc-dev/neofs-api-go/v2 v2.13.2-0.20221004142957-5fc2644c680d github.com/nspcc-dev/neofs-contract v0.15.5-0.20220930133158-d95bc535894c - github.com/nspcc-dev/neofs-sdk-go v1.0.0-rc.6.0.20220926102839-c6576c8112ee + github.com/nspcc-dev/neofs-sdk-go v1.0.0-rc.6.0.20221005093951-1325b4f27218 github.com/nspcc-dev/tzhash v1.6.1 github.com/panjf2000/ants/v2 v2.4.0 github.com/paulmach/orb v0.2.2 diff --git a/go.sum b/go.sum index d3b13b1aaf27ee56479022d342cda467c1afe6ac..57de2360961c14ccf5e00bfc99ad10f96af8d902 100644 GIT binary patch delta 293 zcmaFV&;F>NeS^UoH$wvh6GIatOH*@Q)3jtGGZT|!GYf+hg$zS0|71f`M~ifq`~dwB zXVdHyuS~O$a7)KL{{qvL$jsE_D1#C&m!Qxv^Ro2Gf~&O^jrELl4fKo*jEry?y7|zW z*(@L%O${uKElmw|4ULUVlT6Z#%#92!KsH8{I6HfVgn0&4h8vrk1XPqerTCXwNeS^UoHv>yUOG6_Q6JrzI^>Xu5K&Iw+8G0L;M1`CB7Kau& vW*V8B7$z56=%+{NXJwfgX6AUL8u?YG`Ibid7Ef;6q}1%OX}iZJ##y2OfPY$! diff --git a/pkg/services/object/acl/v2/service.go b/pkg/services/object/acl/v2/service.go index 33d04f120..756422c90 100644 --- a/pkg/services/object/acl/v2/service.go +++ b/pkg/services/object/acl/v2/service.go @@ -11,6 +11,7 @@ import ( "github.com/nspcc-dev/neofs-node/pkg/services/object" "github.com/nspcc-dev/neofs-sdk-go/container/acl" cid "github.com/nspcc-dev/neofs-sdk-go/container/id" + oid "github.com/nspcc-dev/neofs-sdk-go/object/id" sessionSDK "github.com/nspcc-dev/neofs-sdk-go/session" "github.com/nspcc-dev/neofs-sdk-go/user" "go.uber.org/zap" @@ -113,11 +114,23 @@ func (b Service) Get(request *objectV2.GetRequest, stream object.GetObjectStream return err } + obj, err := getObjectIDFromRequestBody(request.GetBody()) + if err != nil { + return err + } + sTok, err := originalSessionToken(request.GetMetaHeader()) if err != nil { return err } + if sTok != nil { + err = assertSessionRelation(*sTok, cnr, obj) + if err != nil { + return err + } + } + bTok, err := originalBearerToken(request.GetMetaHeader()) if err != nil { return err @@ -135,12 +148,7 @@ func (b Service) Get(request *objectV2.GetRequest, stream object.GetObjectStream return err } - reqInfo.obj, err = getObjectIDFromRequestBody(request.GetBody()) - if err != nil { - return err - } - - useObjectIDFromSession(&reqInfo, sTok) + reqInfo.obj = obj if !b.checker.CheckBasicACL(reqInfo) { return basicACLErr(reqInfo) @@ -172,11 +180,23 @@ func (b Service) Head( return nil, err } + obj, err := getObjectIDFromRequestBody(request.GetBody()) + if err != nil { + return nil, err + } + sTok, err := originalSessionToken(request.GetMetaHeader()) if err != nil { return nil, err } + if sTok != nil { + err = assertSessionRelation(*sTok, cnr, obj) + if err != nil { + return nil, err + } + } + bTok, err := originalBearerToken(request.GetMetaHeader()) if err != nil { return nil, err @@ -194,12 +214,7 @@ func (b Service) Head( return nil, err } - reqInfo.obj, err = getObjectIDFromRequestBody(request.GetBody()) - if err != nil { - return nil, err - } - - useObjectIDFromSession(&reqInfo, sTok) + reqInfo.obj = obj if !b.checker.CheckBasicACL(reqInfo) { return nil, basicACLErr(reqInfo) @@ -228,6 +243,13 @@ func (b Service) Search(request *objectV2.SearchRequest, stream object.SearchStr return err } + if sTok != nil { + err = assertSessionRelation(*sTok, id, nil) + if err != nil { + return err + } + } + bTok, err := originalBearerToken(request.GetMetaHeader()) if err != nil { return err @@ -245,11 +267,6 @@ func (b Service) Search(request *objectV2.SearchRequest, stream object.SearchStr return err } - reqInfo.obj, err = getObjectIDFromRequestBody(request.GetBody()) - if err != nil { - return err - } - if !b.checker.CheckBasicACL(reqInfo) { return basicACLErr(reqInfo) } else if err := b.checker.CheckEACL(request, reqInfo); err != nil { @@ -271,11 +288,23 @@ func (b Service) Delete( return nil, err } + obj, err := getObjectIDFromRequestBody(request.GetBody()) + if err != nil { + return nil, err + } + sTok, err := originalSessionToken(request.GetMetaHeader()) if err != nil { return nil, err } + if sTok != nil { + err = assertSessionRelation(*sTok, cnr, obj) + if err != nil { + return nil, err + } + } + bTok, err := originalBearerToken(request.GetMetaHeader()) if err != nil { return nil, err @@ -293,12 +322,7 @@ func (b Service) Delete( return nil, err } - reqInfo.obj, err = getObjectIDFromRequestBody(request.GetBody()) - if err != nil { - return nil, err - } - - useObjectIDFromSession(&reqInfo, sTok) + reqInfo.obj = obj if !b.checker.CheckBasicACL(reqInfo) { return nil, basicACLErr(reqInfo) @@ -315,11 +339,23 @@ func (b Service) GetRange(request *objectV2.GetRangeRequest, stream object.GetOb return err } + obj, err := getObjectIDFromRequestBody(request.GetBody()) + if err != nil { + return err + } + sTok, err := originalSessionToken(request.GetMetaHeader()) if err != nil { return err } + if sTok != nil { + err = assertSessionRelation(*sTok, cnr, obj) + if err != nil { + return err + } + } + bTok, err := originalBearerToken(request.GetMetaHeader()) if err != nil { return err @@ -337,11 +373,7 @@ func (b Service) GetRange(request *objectV2.GetRangeRequest, stream object.GetOb return err } - reqInfo.obj, err = getObjectIDFromRequestBody(request.GetBody()) - if err != nil { - return err - } - useObjectIDFromSession(&reqInfo, sTok) + reqInfo.obj = obj if !b.checker.CheckBasicACL(reqInfo) { return basicACLErr(reqInfo) @@ -364,11 +396,23 @@ func (b Service) GetRangeHash( return nil, err } + obj, err := getObjectIDFromRequestBody(request.GetBody()) + if err != nil { + return nil, err + } + sTok, err := originalSessionToken(request.GetMetaHeader()) if err != nil { return nil, err } + if sTok != nil { + err = assertSessionRelation(*sTok, cnr, obj) + if err != nil { + return nil, err + } + } + bTok, err := originalBearerToken(request.GetMetaHeader()) if err != nil { return nil, err @@ -386,12 +430,7 @@ func (b Service) GetRangeHash( return nil, err } - reqInfo.obj, err = getObjectIDFromRequestBody(request.GetBody()) - if err != nil { - return nil, err - } - - useObjectIDFromSession(&reqInfo, sTok) + reqInfo.obj = obj if !b.checker.CheckBasicACL(reqInfo) { return nil, basicACLErr(reqInfo) @@ -427,6 +466,18 @@ func (p putStreamBasicChecker) Send(request *objectV2.PutRequest) error { return fmt.Errorf("invalid object owner: %w", err) } + objV2 := part.GetObjectID() + var obj *oid.ID + + if objV2 != nil { + obj = new(oid.ID) + + err = obj.ReadFromV2(*objV2) + if err != nil { + return err + } + } + var sTok *sessionSDK.Object if tokV2 := request.GetMetaHeader().GetSessionToken(); tokV2 != nil { @@ -436,6 +487,11 @@ func (p putStreamBasicChecker) Send(request *objectV2.PutRequest) error { if err != nil { return fmt.Errorf("invalid session token: %w", err) } + + err = assertSessionRelation(*sTok, cnr, obj) + if err != nil { + return err + } } bTok, err := originalBearerToken(request.GetMetaHeader()) @@ -455,12 +511,7 @@ func (p putStreamBasicChecker) Send(request *objectV2.PutRequest) error { return err } - reqInfo.obj, err = getObjectIDFromRequestBody(part) - if err != nil { - return err - } - - useObjectIDFromSession(&reqInfo, sTok) + reqInfo.obj = obj if !p.source.checker.CheckBasicACL(reqInfo) || !p.source.checker.StickyBitCheck(reqInfo, idOwner) { return basicACLErr(reqInfo) diff --git a/pkg/services/object/acl/v2/util.go b/pkg/services/object/acl/v2/util.go index 3d2797c22..ec94caf77 100644 --- a/pkg/services/object/acl/v2/util.go +++ b/pkg/services/object/acl/v2/util.go @@ -93,24 +93,12 @@ func originalSessionToken(header *sessionV2.RequestMetaHeader) (*sessionSDK.Obje return &tok, nil } -func getObjectIDFromRequestBody(body interface{}) (*oid.ID, error) { - var idV2 *refsV2.ObjectID - - switch v := body.(type) { - default: - return nil, nil - case interface { - GetObjectID() *refsV2.ObjectID - }: - idV2 = v.GetObjectID() - case interface { - GetAddress() *refsV2.Address - }: - idV2 = v.GetAddress().GetObjectID() - } - +// getObjectIDFromRequestBody decodes oid.ID from the common interface of the +// object reference's holders. Returns an error if object ID is missing in the request. +func getObjectIDFromRequestBody(body interface{ GetAddress() *refsV2.Address }) (*oid.ID, error) { + idV2 := body.GetAddress().GetObjectID() if idV2 == nil { - return nil, nil + return nil, errors.New("missing object ID") } var id oid.ID @@ -123,34 +111,6 @@ func getObjectIDFromRequestBody(body interface{}) (*oid.ID, error) { return &id, nil } -func useObjectIDFromSession(req *RequestInfo, token *sessionSDK.Object) { - if token == nil { - return - } - - // TODO(@cthulhu-rider): It'd be nice to not pull object identifiers from - // the token, but assert them. Track #1420 - var tokV2 sessionV2.Token - token.WriteToV2(&tokV2) - - ctx, ok := tokV2.GetBody().GetContext().(*sessionV2.ObjectSessionContext) - if !ok { - panic(fmt.Sprintf("wrong object session context %T, is it verified?", tokV2.GetBody().GetContext())) - } - - idV2 := ctx.GetAddress().GetObjectID() - if idV2 == nil { - return - } - - req.obj = new(oid.ID) - - err := req.obj.ReadFromV2(*idV2) - if err != nil { - panic(fmt.Sprintf("unexpected protocol violation error after correct session token decoding: %v", err)) - } -} - func ownerFromToken(token *sessionSDK.Object) (*user.ID, *keys.PublicKey, error) { // 1. First check signature of session token. if !token.VerifySignature() { @@ -231,3 +191,24 @@ func assertVerb(tok sessionSDK.Object, op acl.Op) bool { return false } + +// assertSessionRelation checks if given token describing the NeoFS session +// relates to the given container and optional object. Missing object +// means that the context isn't bound to any NeoFS object in the container. +// Returns no error iff relation is correct. Criteria: +// +// session is bound to the given container +// object is not specified or session is bound to this object +// +// Session MUST be bound to the particular container, otherwise behavior is undefined. +func assertSessionRelation(tok sessionSDK.Object, cnr cid.ID, obj *oid.ID) error { + if !tok.AssertContainer(cnr) { + return errors.New("requested container is not related to the session") + } + + if obj != nil && !tok.AssertObject(*obj) { + return errors.New("requested object is not related to the session") + } + + return nil +} diff --git a/pkg/services/object/acl/v2/util_test.go b/pkg/services/object/acl/v2/util_test.go index 792114f0d..bac91e168 100644 --- a/pkg/services/object/acl/v2/util_test.go +++ b/pkg/services/object/acl/v2/util_test.go @@ -10,6 +10,8 @@ import ( "github.com/nspcc-dev/neofs-api-go/v2/session" bearertest "github.com/nspcc-dev/neofs-sdk-go/bearer/test" aclsdk "github.com/nspcc-dev/neofs-sdk-go/container/acl" + cidtest "github.com/nspcc-dev/neofs-sdk-go/container/id/test" + oidtest "github.com/nspcc-dev/neofs-sdk-go/object/id/test" sessionSDK "github.com/nspcc-dev/neofs-sdk-go/session" sessiontest "github.com/nspcc-dev/neofs-sdk-go/session/test" "github.com/stretchr/testify/require" @@ -104,3 +106,31 @@ func TestIsVerbCompatible(t *testing.T) { } } } + +func TestAssertSessionRelation(t *testing.T) { + var tok sessionSDK.Object + cnr := cidtest.ID() + cnrOther := cidtest.ID() + obj := oidtest.ID() + objOther := oidtest.ID() + + // make sure ids differ, otherwise test won't work correctly + require.False(t, cnrOther.Equals(cnr)) + require.False(t, objOther.Equals(obj)) + + // bind session to the container (required) + tok.BindContainer(cnr) + + // test container-global session + require.NoError(t, assertSessionRelation(tok, cnr, nil)) + require.NoError(t, assertSessionRelation(tok, cnr, &obj)) + require.Error(t, assertSessionRelation(tok, cnrOther, nil)) + require.Error(t, assertSessionRelation(tok, cnrOther, &obj)) + + // limit the session to the particular object + tok.LimitByObjects(obj) + + // test fixed object session (here obj arg must be non-nil everywhere) + require.NoError(t, assertSessionRelation(tok, cnr, &obj)) + require.Error(t, assertSessionRelation(tok, cnr, &objOther)) +}