From c7e17dfb1d5697bf4e577b470c958c5454ea9313 Mon Sep 17 00:00:00 2001 From: Ryan Stewart Date: Thu, 1 Jul 2021 20:20:40 +0100 Subject: [PATCH 1/3] re-order signature check to support collaborators --- .../handler/object_operation_handler.go | 29 ++++++++++--------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/code/go/0chain.net/blobbercore/handler/object_operation_handler.go b/code/go/0chain.net/blobbercore/handler/object_operation_handler.go index 9936fb61d..0811249ca 100644 --- a/code/go/0chain.net/blobbercore/handler/object_operation_handler.go +++ b/code/go/0chain.net/blobbercore/handler/object_operation_handler.go @@ -281,9 +281,9 @@ func (fsh *StorageHandler) DownloadFile(ctx context.Context, r *http.Request) ( // authorize file access var ( - isOwner = clientID == alloc.OwnerID - isRepairer = clientID == alloc.RepairerID - isCollaborator = reference.IsACollaborator(ctx, fileref.ID, clientID) + isOwner = clientID == alloc.OwnerID + isRepairer = clientID == alloc.RepairerID + isCollaborator = reference.IsACollaborator(ctx, fileref.ID, clientID) ) if !isOwner && !isRepairer && !isCollaborator { @@ -923,11 +923,6 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl return nil, common.NewError("invalid_parameters", "Invalid allocation id passed."+err.Error()) } - valid, err := verifySignatureFromRequest(r, allocationObj.OwnerPublicKey) - if !valid || err != nil { - return nil, common.NewError("invalid_signature", "Invalid signature") - } - if allocationObj.IsImmutable { return nil, common.NewError("immutable_allocation", "Cannot write to an immutable allocation") } @@ -988,6 +983,8 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl exisitingFileRef := fsh.checkIfFileAlreadyExists(ctx, allocationID, formData.Path) existingFileRefSize := int64(0) exisitingFileOnCloud := false + + publicKey := allocationObj.OwnerPublicKey if mode == allocation.INSERT_OPERATION { if allocationObj.OwnerID != clientID && allocationObj.RepairerID != clientID { return nil, common.NewError("invalid_operation", "Operation needs to be performed by the owner or the payer of the allocation") @@ -1001,13 +998,18 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl return nil, common.NewError("invalid_file_update", "File at path does not exist for update") } - if allocationObj.OwnerID != clientID && - allocationObj.RepairerID != clientID && - !reference.IsACollaborator(ctx, exisitingFileRef.ID, clientID) { + if reference.IsACollaborator(ctx, exisitingFileRef.ID, clientID) { + publicKey = ctx.Value(constants.CLIENT_KEY_CONTEXT_KEY).(string) + } else if allocationObj.OwnerID != clientID && allocationObj.RepairerID != clientID { return nil, common.NewError("invalid_operation", "Operation needs to be performed by the owner, collaborator or the payer of the allocation") } } + valid, err := verifySignatureFromRequest(r, publicKey) + if !valid || err != nil { + return nil, common.NewError("invalid_signature", "Invalid signature") + } + if exisitingFileRef != nil { existingFileRefSize = exisitingFileRef.Size exisitingFileOnCloud = exisitingFileRef.OnCloud @@ -1020,9 +1022,8 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl defer origfile.Close() thumbfile, thumbHeader, _ := r.FormFile("uploadThumbnailFile") - thumbnailPresent := false - if thumbHeader != nil { - thumbnailPresent = true + thumbnailPresent := thumbHeader != nil + if thumbnailPresent { defer thumbfile.Close() } From 3f318e8ea6ea8725e4470ec5b357d41f45c3ac6d Mon Sep 17 00:00:00 2001 From: Ryan Stewart Date: Thu, 1 Jul 2021 23:20:24 +0100 Subject: [PATCH 2/3] refactor to allow signature check to remain one of the first checks (prevent info leakage) --- .../handler/object_operation_handler.go | 101 +++++++++++------- 1 file changed, 64 insertions(+), 37 deletions(-) diff --git a/code/go/0chain.net/blobbercore/handler/object_operation_handler.go b/code/go/0chain.net/blobbercore/handler/object_operation_handler.go index 0811249ca..a06d69516 100644 --- a/code/go/0chain.net/blobbercore/handler/object_operation_handler.go +++ b/code/go/0chain.net/blobbercore/handler/object_operation_handler.go @@ -923,12 +923,25 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl return nil, common.NewError("invalid_parameters", "Invalid allocation id passed."+err.Error()) } + allocationID := allocationObj.ID + fileOperation := getFileOperation(r) + existingFileRef := getExistingFileRef(fsh, ctx, r, allocationObj, fileOperation) + isCollaborator := existingFileRef != nil && reference.IsACollaborator(ctx, existingFileRef.ID, clientID) + publicKey := allocationObj.OwnerPublicKey + + if isCollaborator { + publicKey = ctx.Value(constants.CLIENT_KEY_CONTEXT_KEY).(string) + } + + valid, err := verifySignatureFromRequest(r, publicKey) + if !valid || err != nil { + return nil, common.NewError("invalid_signature", "Invalid signature") + } + if allocationObj.IsImmutable { return nil, common.NewError("immutable_allocation", "Cannot write to an immutable allocation") } - allocationID := allocationObj.ID - if len(clientID) == 0 { return nil, common.NewError("invalid_operation", "Operation needs to be performed by the owner or the payer of the allocation") } @@ -953,14 +966,8 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl defer mutex.Unlock() result := &UploadResult{} - mode := allocation.INSERT_OPERATION - if r.Method == "PUT" { - mode = allocation.UPDATE_OPERATION - } else if r.Method == "DELETE" { - mode = allocation.DELETE_OPERATION - } - if mode == allocation.DELETE_OPERATION { + if fileOperation == allocation.DELETE_OPERATION { if allocationObj.OwnerID != clientID && allocationObj.RepairerID != clientID { return nil, common.NewError("invalid_operation", "Operation needs to be performed by the owner or the payer of the allocation") } @@ -968,51 +975,38 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl if err != nil { return nil, err } - } else if mode == allocation.INSERT_OPERATION || mode == allocation.UPDATE_OPERATION { + } else if fileOperation == allocation.INSERT_OPERATION || fileOperation == allocation.UPDATE_OPERATION { + formField := getFormFieldName(fileOperation) var formData allocation.UpdateFileChange - formField := "uploadMeta" - if mode == allocation.UPDATE_OPERATION { - formField = "updateMeta" - } uploadMetaString := r.FormValue(formField) err = json.Unmarshal([]byte(uploadMetaString), &formData) if err != nil { return nil, common.NewError("invalid_parameters", "Invalid parameters. Error parsing the meta data for upload."+err.Error()) } - exisitingFileRef := fsh.checkIfFileAlreadyExists(ctx, allocationID, formData.Path) existingFileRefSize := int64(0) - exisitingFileOnCloud := false - - publicKey := allocationObj.OwnerPublicKey - if mode == allocation.INSERT_OPERATION { + existingFileOnCloud := false + if fileOperation == allocation.INSERT_OPERATION { if allocationObj.OwnerID != clientID && allocationObj.RepairerID != clientID { return nil, common.NewError("invalid_operation", "Operation needs to be performed by the owner or the payer of the allocation") } - if exisitingFileRef != nil { + if existingFileRef != nil { return nil, common.NewError("duplicate_file", "File at path already exists") } - } else if mode == allocation.UPDATE_OPERATION { - if exisitingFileRef == nil { + } else if fileOperation == allocation.UPDATE_OPERATION { + if existingFileRef == nil { return nil, common.NewError("invalid_file_update", "File at path does not exist for update") } - if reference.IsACollaborator(ctx, exisitingFileRef.ID, clientID) { - publicKey = ctx.Value(constants.CLIENT_KEY_CONTEXT_KEY).(string) - } else if allocationObj.OwnerID != clientID && allocationObj.RepairerID != clientID { + if allocationObj.OwnerID != clientID && allocationObj.RepairerID != clientID && !isCollaborator { return nil, common.NewError("invalid_operation", "Operation needs to be performed by the owner, collaborator or the payer of the allocation") } } - valid, err := verifySignatureFromRequest(r, publicKey) - if !valid || err != nil { - return nil, common.NewError("invalid_signature", "Invalid signature") - } - - if exisitingFileRef != nil { - existingFileRefSize = exisitingFileRef.Size - exisitingFileOnCloud = exisitingFileRef.OnCloud + if existingFileRef != nil { + existingFileRefSize = existingFileRef.Size + existingFileOnCloud = existingFileRef.OnCloud } origfile, _, err := r.FormFile("uploadFile") @@ -1027,7 +1021,7 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl defer thumbfile.Close() } - fileInputData := &filestore.FileInputData{Name: formData.Filename, Path: formData.Path, OnCloud: exisitingFileOnCloud} + fileInputData := &filestore.FileInputData{Name: formData.Filename, Path: formData.Path, OnCloud: existingFileOnCloud} fileOutputData, err := filestore.GetFileStore().WriteFile(allocationID, fileInputData, origfile, connectionObj.ConnectionID) if err != nil { return nil, common.NewError("upload_error", "Failed to upload the file. "+err.Error()) @@ -1075,12 +1069,12 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl allocationChange := &allocation.AllocationChange{} allocationChange.ConnectionID = connectionObj.ConnectionID allocationChange.Size = allocationSize - existingFileRefSize - allocationChange.Operation = mode + allocationChange.Operation = fileOperation connectionObj.Size += allocationChange.Size - if mode == allocation.INSERT_OPERATION { + if fileOperation == allocation.INSERT_OPERATION { connectionObj.AddChange(allocationChange, &formData.NewFileChange) - } else if mode == allocation.UPDATE_OPERATION { + } else if fileOperation == allocation.UPDATE_OPERATION { connectionObj.AddChange(allocationChange, &formData) } } @@ -1092,3 +1086,36 @@ func (fsh *StorageHandler) WriteFile(ctx context.Context, r *http.Request) (*Upl return result, nil } + +func getFormFieldName(mode string) string { + formField := "uploadMeta" + if mode == allocation.UPDATE_OPERATION { + formField = "updateMeta" + } + + return formField +} + +func getFileOperation(r *http.Request) string { + mode := allocation.INSERT_OPERATION + if r.Method == "PUT" { + mode = allocation.UPDATE_OPERATION + } else if r.Method == "DELETE" { + mode = allocation.DELETE_OPERATION + } + + return mode +} + +func getExistingFileRef(fsh *StorageHandler, ctx context.Context, r *http.Request, allocationObj *allocation.Allocation, fileOperation string) *reference.Ref { + if fileOperation == allocation.INSERT_OPERATION || fileOperation == allocation.UPDATE_OPERATION { + var formData allocation.UpdateFileChange + uploadMetaString := r.FormValue(getFormFieldName(fileOperation)) + err := json.Unmarshal([]byte(uploadMetaString), &formData) + + if err == nil { + return fsh.checkIfFileAlreadyExists(ctx, allocationObj.ID, formData.Path) + } + } + return nil +} From ca127e988343c19946892ab18708f7de5c71698d Mon Sep 17 00:00:00 2001 From: Ryan Stewart Date: Thu, 1 Jul 2021 23:34:31 +0100 Subject: [PATCH 3/3] unit test updates to support re-ordering of db calls --- code/go/0chain.net/blobbercore/handler/handler_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/code/go/0chain.net/blobbercore/handler/handler_test.go b/code/go/0chain.net/blobbercore/handler/handler_test.go index 0f613734d..6f3f1a127 100644 --- a/code/go/0chain.net/blobbercore/handler/handler_test.go +++ b/code/go/0chain.net/blobbercore/handler/handler_test.go @@ -984,6 +984,10 @@ func TestHandlers_Requiring_Signature(t *testing.T) { AddRow(alloc.Terms[0].ID, alloc.Terms[0].AllocationID), ) + mock.ExpectQuery(regexp.QuoteMeta(`SELECT * FROM "reference_objects"`)). + WithArgs(aa). + WillReturnError(gorm.ErrRecordNotFound) + mock.ExpectQuery(regexp.QuoteMeta(`SELECT * FROM "allocation_connections" WHERE`)). WithArgs(connectionID, alloc.ID, alloc.OwnerID, allocation.DeletedConnection). WillReturnRows( @@ -991,10 +995,6 @@ func TestHandlers_Requiring_Signature(t *testing.T) { AddRow(), ) - mock.ExpectQuery(regexp.QuoteMeta(`SELECT * FROM "reference_objects"`)). - WithArgs(aa). - WillReturnError(gorm.ErrRecordNotFound) - mock.ExpectExec(`INSERT INTO "allocation_connections"`). WithArgs(aa, aa, aa, aa, aa, aa, aa). WillReturnResult(sqlmock.NewResult(0, 0))