Skip to content

Commit

Permalink
[nspcc-dev#1420] object/acl: Fix correlation of object session to req…
Browse files Browse the repository at this point in the history 
…uest

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

Signed-off-by: Leonard Lyubich <[email protected]>
  • Loading branch information
Leonard Lyubich committed Sep 16, 2022
1 parent d6fef68 commit 9df94c3
Show file tree
Hide file tree
Showing 3 changed files with 147 additions and 84 deletions.
123 changes: 83 additions & 40 deletions pkg/services/object/acl/v2/service.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,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
Expand All @@ -135,12 +147,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)
Expand Down Expand Up @@ -172,11 +179,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
Expand All @@ -194,12 +213,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)
Expand Down Expand Up @@ -228,6 +242,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
Expand All @@ -245,11 +266,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 {
Expand All @@ -271,11 +287,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
Expand All @@ -293,12 +321,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)
Expand All @@ -315,11 +338,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
Expand All @@ -337,11 +372,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)
Expand All @@ -364,11 +395,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
Expand All @@ -386,12 +429,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)
Expand Down Expand Up @@ -427,6 +465,11 @@ func (p putStreamBasicChecker) Send(request *objectV2.PutRequest) error {
return fmt.Errorf("invalid object owner: %w", err)
}

obj, err := decodeRequestedObject(part.GetObjectID(), false)
if err != nil {
return err
}

var sTok *sessionSDK.Object

if tokV2 := request.GetMetaHeader().GetSessionToken(); tokV2 != nil {
Expand All @@ -436,6 +479,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())
Expand All @@ -455,12 +503,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)
Expand Down
78 changes: 34 additions & 44 deletions pkg/services/object/acl/v2/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,64 +93,33 @@ func originalSessionToken(header *sessionV2.RequestMetaHeader) (*sessionSDK.Obje
return &tok, nil
}

func getObjectIDFromRequestBody(body interface{}) (*oid.ID, error) {
var idV2 *refsV2.ObjectID
// 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) {
return decodeRequestedObject(body.GetAddress().GetObjectID(), true)
}

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()
}
// decodeRequestedObject decodes oid.ID from refsV2.ObjectID message if latter
// is presented, otherwise returns an error iff message is required.
func decodeRequestedObject(m *refsV2.ObjectID, required bool) (*oid.ID, error) {
if m == nil {
if required {
return nil, errors.New("missing object ID")
}

if idV2 == nil {
return nil, nil
}

var id oid.ID

err := id.ReadFromV2(*idV2)
err := id.ReadFromV2(*m)
if err != nil {
return nil, err
}

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() {
Expand Down Expand Up @@ -231,3 +200,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
}
30 changes: 30 additions & 0 deletions pkg/services/object/acl/v2/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.LimitByObject(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))
}

0 comments on commit 9df94c3

Please sign in to comment.