fix: scope storage-URI resolve endpoints to the user's organization - #9930
Open
akashjainn wants to merge 1 commit into
Open
fix: scope storage-URI resolve endpoints to the user's organization#9930akashjainn wants to merge 1 commit into
akashjainn wants to merge 1 commit into
Conversation
TaskResolveStorageUri and ProjectResolveStorageUri looked the object up by
bare primary key, so the only tenant check left was has_permission:
return not (self.organization_id and self.organization.has_deleted(user))
has_deleted only matches an OrganizationMember row that has deleted_at set, so
it is true for a revoked membership and false for someone who was never a
member at all. A user from another organization has no such row, the check
returns True, and the request goes on to presign or proxy the object. Ids are
sequential, so any authenticated user could walk another tenant's tasks and
projects and read from their connected bucket.
Add the organization to both lookups, matching how the task and project list
endpoints already scope their querysets. A cross-organization id now returns
404 before storage resolution is reached.
Reported in HumanSignal#9924 (GHSA-8mp9-cpp7-x63q).
👷 Deploy request for label-studio-docs-new-theme pending review.Visit the deploys page to approve it
|
👷 Deploy request for heartex-docs pending review.Visit the deploys page to approve it
|
✅ Deploy Preview for label-studio-storybook canceled.
|
✅ Deploy Preview for label-studio-playground canceled.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #9924.
Problem
GET /tasks/<task_id>/resolve/andGET /projects/<project_id>/resolve/fetch the object by bare primary key:Neither is scoped to an organization, so the only tenant check is
ResolveStorageUriAPIMixin.resolvecallinginstance.has_permission(request.user), which reachesProjectMixin.has_permission:Organization.has_deletedmatches anOrganizationMemberrow withdeleted_atset. That is true for a membership someone had and lost, and false for a user who was never a member at all, so the check rejects ex-members and admits strangers.Organization.has_permission, defined a few lines below it, is the one that requires an active membership.The result is that any authenticated user can pass another organization's task or project id and have Label Studio presign or proxy the file from that tenant's connected storage. Ids are sequential, so the set is walkable. The code comment on that permission check explains the assumption it was written under: "LSO always runs a single organization, so cross-org access does not exist by design."
Change
Both lookups now carry the organization, matching how the task and project list endpoints already scope their querysets (
tasks/api.py,projects/api.py). A cross-organization id returns 404 and never reaches storage resolution.Tests
TestResolveStorageUriCrossOrganizationinlabel_studio/io_storages/tests/test_proxy_api.py, four cases: an outside organization is refused on both endpoints, and a member of the owning organization still reaches resolution on both. They patchResolveStorageUriAPIMixin.resolveand assert on whether it is reached, because a 404 alone does not separate "not your tenant" from "no storage configured for that URI".Both refusal tests fail against the current code and pass with this change. The two same-organization tests pass either way, so they pin that the scope does not over-block.
Two existing tests asserted the exact lookup call (
assert_called_once_with(pk=1)), which is the thing this changes, so they now assert the organization is part of it.Validated against the reporter's PoC
poc_cross_org_idor.pyfrom #9924, run against this branch and against the same tree with the fix reverted. Two lines changed, the repo path and the response read described below, nothing in the attack itself:The reverted column reproduces the reporter's published output exactly. This is a stronger check than the unit tests, which assert on whether resolution is reached: the PoC mocks the storage stream and confirms the bytes themselves come back to the attacker.
One note for anyone else validating with it. The script reads the response as
resp.streaming_content if hasattr(...) else resp.content, which works on the vulnerable path because that returns a streaming response, but raisesContentNotRenderedErroron a patched build, where a 404 DRFResponsehas not been rendered yet. Callingresp.render()first is enough. As published it cannot report a successful fix, it throws instead.The PoC also confirms the arbitrary-key behaviour described below: an unreferenced key in the same bucket returned 200 with data before this change. Organization scoping closes that across tenants, not within one.
Acceptance criteria
GET /tasks/<task_id_from_A>/resolve/?fileuri=<base64 of the file URI>. Expect 404. Before this change it presigned or proxied the file.GET /projects/<project_id_from_A>/resolve/?fileuri=<same>. Expect 404.Not covered here
The report also notes that
can_resolve_urlchecks only the scheme and bucket of the suppliedfileuri, not that the key is one the task actually references, so a user inside the owning organization can still read unrelated objects from that bucket. That is a different trust boundary and a larger change, so it is left for a follow-up rather than mixed into this fix.