sbp opened a new issue, #1369:
URL: https://github.com/apache/tooling-trusted-releases/issues/1369
This is a compound issue.
-----
### FINDING-006: category_add/category_remove don't verify project belongs
to member's committee
| Attribute | Details |
|---|---|
| **Severity** | High |
| **Verdict** | CONFIRMED |
| **Category** | authz |
| **File** | `storage/writers/project.py:167-215` |
| **Source** | chunk-55 |
**Description**
Unlike edit_metadata/edit_security, which call
__validate_project_in_committee to confirm project.committee_key ==
self.__committee_key, category_add and category_remove load the project by key
alone and never check it belongs to self.__committee_key. A CommitteeMember
whose authorization was established for committee A can pass a project_key
belonging to committee B and modify its categories.
**Evidence**
`self.__data.project(key=str(project_key)).get() with no committee check, vs
ReleaseManager.__validate_project_in_committee comparing project.committee_key
!= self.__committee_key`
**Verification — CONFIRMED**
- **Trigger:** A CommitteeMember writer constructed with
committee_key='committee-A' (the caller's own membership) calls
category_add(project_key='project-in-committee-B', new_category='foo'); the
code loads project B by key, ensures it is active, appends the category, and
commits — no project.committee_key == self.__committee_key check.
- **Reasoning:** category_add/category_remove (and language_add/remove) load
the project by key alone and never call __validate_project_in_committee,
whereas the sibling edit_metadata/edit_security and
FoundationAdmin.__resolve_or_create_project_no_commit all explicitly compare
committee_key — that the parallel methods enforce the check confirms
committee_key is an independent input and the omission is a real
cross-committee authz gap.
---
### FINDING-007: create() trusts caller-supplied committee_key instead of
self.__committee_key
| Attribute | Details |
|---|---|
| **Severity** | High |
| **Verdict** | CONFIRMED |
| **Category** | authz |
| **File** | `storage/writers/project.py:217-233` |
| **Source** | chunk-55 |
**Description**
CommitteeMember.create takes a committee_key argument and passes it straight
to _build_and_add_project without comparing it to self.__committee_key (the
committee for which the caller was authorized). A member authorized for
committee A could create a project under committee B by passing committee_key=B.
**Evidence**
`await _build_and_add_project(self.__data, self.__asf_uid, committee_key,
display_name, project_key) using the parameter, never self.__committee_key`
**Verification — CONFIRMED**
- **Trigger:** A user who is a member of committee A obtains
WriteAsCommitteeMember bound to committee_key=A, then calls
create(committee_key="B", display_name, project_key) — the method passes the
caller-supplied "B" straight to _build_and_add_project (which sets
committee_key=str("B")) and never compares it to self.__committee_key, creating
a project under committee B.
- **Reasoning:** Every other authorized method in this class
(edit_metadata/edit_security via __validate_project_in_committee) enforces
project.committee_key == self.__committee_key, but create() omits any such
check; per this codebase's convention the writer transaction is the authority
(route checks are UX only), so the missing comparison is a genuine
cross-committee authz hole.
---
### FINDING-008: archive/delete don't restrict to the member's committee
| Attribute | Details |
|---|---|
| **Severity** | High |
| **Verdict** | CONFIRMED |
| **Category** | authz |
| **File** | `storage/writers/project.py:235-311` |
| **Source** | chunk-55 |
**Description**
archive() and delete() query the project by key/status without filtering or
asserting project.committee_key == self.__committee_key. A CommitteeMember
bound to committee A could archive or delete a project belonging to committee B
by supplying that project_key.
**Evidence**
`self.__data.project(key=str(project_key), status=sql.ProjectStatus.ACTIVE,
_releases=True).get() with no committee check before status mutation / delete`
**Verification — CONFIRMED**
- **Trigger:** A user who is a CommitteeMember of committee A obtains
write_as_committee_member(committee A) and calls archive(project_key) or
delete(project_key) with a project_key belonging to committee B; the project is
fetched solely by key+ACTIVE status, no project.committee_key ==
self.__committee_key check runs, so status is set to RETIRED (or the row
deleted).
- **Reasoning:** The sibling edit_metadata/edit_security path in the same
inheritance chain explicitly enforces `if project.committee_key !=
self.__committee_key: raise AccessError`, proving the writer's committee_key
does not by itself constrain which project_keys may be supplied; archive() and
delete() omit that guard entirely, allowing cross-committee mutation.
---
### FINDING-009: language_add/language_remove don't verify project belongs
to member's committee
| Attribute | Details |
|---|---|
| **Severity** | High |
| **Verdict** | CONFIRMED |
| **Category** | authz |
| **File** | `storage/writers/project.py:314-356` |
| **Source** | chunk-55 |
**Description**
language_add and language_remove load the project by key only and never
verify project.committee_key == self.__committee_key, allowing a member
authorized for one committee to mutate programming_languages of a project in
another committee. edit_metadata/edit_security gate this; these methods do not.
**Evidence**
`project = await self.__data.project(key=str(project_key)).get() with no
committee membership assertion`
**Verification — CONFIRMED**
- **Trigger:** A user who is a member of committee A obtains a
CommitteeMember writer bound to committee A, then calls
language_add('projectB-foo', 'Rust') where projectB-foo belongs to committee B;
the project is loaded by key only with no committee_key comparison, so
programming_languages is mutated and committed cross-committee.
- **Reasoning:** The sibling methods edit_metadata/edit_security in the same
class deliberately call __validate_project_in_committee (checking
project.committee_key == self.__committee_key), but
language_add/language_remove load by key and never assert committee ownership;
per this codebase's convention the writer transaction is the authorization
authority, so the missing check is a genuine authz gap.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
