The removeReferencesTo function doesn't check authorization correctly

[jportner]

Kibana version: 7.11+

Describe the bug:

This bug was introduced when the Saved Objects Tagging feature was added in #79096.
The Saved Objects Client (SOC) exposes a removeReferencesTo function that allows you to remove all references to a given object. Under the hood, it uses the Elasticsearch client's updateByQuery functionality to achieve the desired result.

There are two problems:

1. The authorization check only ensures that the user has privileges to "delete" the object in question. It should instead ensure that the user has privileges to "update" all of the objects that have a reference to this object.
2. The only audit event is "REMOVE_REFERENCES" for the referenced object. There should also be "UPDATE" audit events for the referencing objects.

This isn't a practical problem right now as the only consumer is the TagsClient, and we consider tags to just be metadata. However, other consumers could adopt removeReferencesTo in the future and this could pose a security risk.

Steps to reproduce:

See the integration test for deleting tags:

kibana/x-pack/test/saved_object_tagging/api_integration/security_and_spaces/apis/delete.ts

Lines 37 to 46 in f466ebf

	unauthorized: {
	httpCode: 403,
	expectResponse: ({ body }) => {
	expect(body).to.eql({
	statusCode: 403,
	error: 'Forbidden',
	message: 'Unable to delete tag',
	});
	},
	},

In that test assertion, the call to TagsClient.delete fails during removeReferencesTo:

kibana/x-pack/plugins/saved_objects_tagging/server/services/tags/tags_client.ts

Lines 65 to 72 in 3730dd0

	public async delete(id: string) {
	// `removeReferencesTo` security check is the same as a `delete` operation's, so we can use the scoped client here.
	// If that was to change, we would need to use the internal client instead. A FTR test is ensuring
	// that this behave properly even with only 'tag' SO type write permission.
	await this.soClient.removeReferencesTo(this.type, id);
	// deleting the tag after reference removal in case of failure during the first call.
	await this.soClient.delete(this.type, id);
	}

Expected behavior:

This function should be refactored to (1) search for all "inbound references", (2) check privileges for each of those referencing objects, and (3) bulk update any of those that the user is authorized to change. This effectively means that removeReferencesTo would behave more like the find function and never experience a 403 error when calling it. If/when that change is made, the integration test above will continue to work, because the authorization check will still fail during the subsequent delete function call for the tag itself with the same error message.

We made a similar change for the "Delete space" API recently (#124145).

Such a change for removeReferencesTo is impractical now with the current paradigm of "SOC wrappers", but we are undertaking a major refactor that will remove the wrappers and move this functionality into the Saved Objects Repository (#134395). Once that is complete, removeReferencesTo authorization/auditing can be changed with ease.

Any additional context:

When an authorization check for removeReferencesTo fails, the error message that is returned is currently "Unable to delete ". We are currently undertaking a major refactor of the SOC and its wrappers in #134395, and in that proof-of-concept PR I have change that authorization check. Now the failure message is "Unable to update ". The authorization check is a little less wrong -- even if, in practice, a user will never be able to update an object without deleting it -- but the authorization check still needs to be fixed.

[elasticmachine]

Pinging @elastic/kibana-core (Team:Core)

[elasticmachine]

Pinging @elastic/kibana-security (Team:Security)

[afharo]

Pinging Security folks because it feels like related to the SOC wrapper as well?

[jeramysoucy]

@legrego @azasypkin Just want to clarify something. Should this be an all or nothing operation? If the user is not authorized to update ALL of the referencing objects but is authorized to update SOME, should we abort or should we update the objects for which the user us authorized?

Use case given is deletion of a tag. If we cannot remove all of the tag references, but proceed with the deletion of the tag, what ultimately happens to the orphaned references to the deleted tag? I assume this will cause some problems. And if we proceed with removing SOME of the references, and then bypass the tag delete (not sure we can do this currently with existing returns) we're still left with an undesired result.

In other use cases, partial authorization may not be a problem.

@elastic/kibana-core As this is a data integrity question, could we get some input on this?

UPDATE: additional question, should removeReferencesTo operate across multiple spaces (like find)?

[rudolf]

To summarise the slack conversation. There's two ways that a plugin might use removeReferencesTo

1. To "unlink" the reference between two object.
   E.g. if there's a dashboard -> lens reference and a user removes the lens from the dashboard but doesn't want to delete the lens itself. Ito authz this kind of operation feels like it's best modeled as an update on the dashboard. If a user can't update dashboards they also shouldn't remove references from that dashboard.
2. To ensure referential integrity when a child is deleted.
   This is the only existing use case of this API. When a tag is deleted we might have hundreds of saved objects still having a reference to this tag. So if we allow a user to delete tags but not allow them to update all the references to this tag we'll likely end up with more problems. Now we could ask ourselves should a user be allowed to delete a tag when it's used by a dashboard that they don't have permission to edit? The privilege to delete tags is a "dangerous" privilege in the sense that you're potentially affecting saved objects which you otherwise wouldn't be allowed to affect. But I'm not sure we need to change this, I think it's very intuitive that if you allow users to delete tags those tags are no longer going to be visible at the places it was previously used.

So for the one use case we have today it feels like the behaviour is correct/sufficient. To ensure it's not abused we could consider making removeReferencesTo a private API and always calling it when a saved object was deleted. This introduces challenges to bulkDelete because then we'd need something like a bulkRemoveReferences. But ultimately it feels like the right thing to do by removing the burden to keep referential integrity from plugins and into core.

[jeramysoucy]

Thanks @rudolf! After discussing with @elastic/kibana-security, we're ok leaving the existing behavior. We can re-evaluate in the future, given additional use cases and referential integrity considerations. For now, I will update the method documentation with intended usage details and a link to this issue/discussion.

[rayafratkina]

With no plans to address, closing this issue