mirror of
https://github.com/block/buzz.git
synced 2026-08-18 06:50:31 +02:00
00ecf2cac7
## Summary
NIP-29 `kind:9000` (PUT_USER) role changes were only authorized when the
**new** role was elevated. Demotions were unauthorized, so any
authenticated user could strip a channel owner to `member` with a single
event — and the demotion was unrecoverable, since the ex-owner then
lacked the privilege to restore themselves.
Reported by @Tyler in `#buzz-security`. Verified true, plus two adjacent
defects the report flagged and one it did not.
## The defects
1. **Demotion unauthorized.** The actor check only fired when the
*requested* role was elevated. Lowering someone's role skipped it
entirely.
2. **Open channels skipped the actor check.** It was nested under
`visibility == "private"`.
3. **`add_member` had no last-owner guard** while `remove_member` did —
so a channel could be left with zero owners.
4. **(Not in the report.)** An absent `role` tag defaulted to `Member`,
so a bare self-targeted PUT_USER silently demoted the sender. No
attacker required.
## The fix
**`crates/buzz-db/src/channel.rs`** — the authority, because it also
covers the desktop/admin callers that bypass the relay validator:
- Changing an **active** member's role requires an elevated actor **in
both directions**. Re-adding at the same role stays unguarded and
idempotent (the huddle bot-add and `kind:9021` join paths depend on
this).
- Last-owner guard in `add_member`, mirroring `remove_member`.
- Keyed on the **active** role (`removed_at IS NULL`). A soft-removed
row's role is history, not live authority — otherwise soft-deleted
ownership becomes a resurrection token: a kicked owner self-rejoins via
`9021` and silently regains ownership.
- New `pg_advisory_xact_lock` on a channel-membership namespace, taken
as the first statement in both `add_member` and `remove_member`. Both
read an owner `COUNT` and then write a *different* row, so READ
COMMITTED alone lets two concurrent demotions each observe 2 owners and
together leave 0.
- `remove_member`'s `is_agent_owner` lookup moved before the transaction
opens — it borrows a second pool connection, and issuing it while
holding the lock could self-deadlock on a small pool. Safe because
`agent_owner_pubkey` is immutable (first-mint-wins).
**`crates/buzz-relay/src/handlers/side_effects.rs`**:
- Role tag is now `Option` — absent means "no role change requested"
rather than defaulting to `Member`.
- Actor-role lookup hoisted out of the `visibility == "private"` block,
so open channels are covered.
- Role-change and last-owner guards on every visibility. Rejecting here
*as well as* in the DB means clients get a real error instead of an `OK`
whose side effect then fails silently.
## Verification
**Mutation tested — every guard stubbed individually to confirm a test
actually dies.** Three of eight guards were originally uncovered and
survived being disabled with the suite fully green:
| Guard | Dying test |
|---|---|
| DB actor-auth | *survived* → **new**
`unprivileged_member_cannot_demote_a_co_owner` |
| DB last-owner | `owner_can_still_manage_roles_after_demotion_guard` |
| DB active-role (soft-remove) |
`kicked_owner_rejoins_as_member_not_owner` + 3 |
| `add_member` advisory lock |
`membership_writes_serialize_on_the_shared_channel_lock` |
| `remove_member` advisory lock | +
`remove_member_rejects_an_actor_demoted_while_it_waited` |
| relay no-role-tag preservation |
`test_nip29_put_user_without_role_tag_preserves_role` |
| relay actor-auth | *survived* → **new**
`test_nip29_relay_rejects_role_change_by_unprivileged_actor` |
| relay last-owner | *survived* → **new**
`test_nip29_relay_rejects_last_owner_self_demotion` |
The three gaps shared one cause: every existing test asserts resulting
**state** ("the role did not change"), and the DB guards enforce that
state, masking every layer above them. With a relay guard stubbed the
relay answers `accepted:true` and logs `Side effect failed: access
denied: ...` while the state assertion still passes — the entire relay
validator could be deleted unnoticed. The new relay tests assert
`accepted == false` instead, the one observable only the validator
controls. Each new test is verified in both directions: green against
the real fix, failing with its intended message when its guard alone is
stubbed.
**Test runs** (at `9461eedb`):
- `buzz-db`, serial: **210 passed / 3 failed** — the same 3 failures as
clean `main` (202/3), which are pre-existing and unrelated
(`concurrent_same_owner_create…`,
`create_community_with_owner_is_atomic…`,
`test_usage_metrics_lock_has_single_owner…`). +8 = the new tests.
- `e2e_relay --ignored`: **40 passed / 3 failed**. Clean `main` on the
same relay is 35/6 — the same 3 infra failures
(`test_invite_mint_and_claim…`, `test_subscription_limit_enforced`,
`test_unarchive_emits_member_added_notification`) plus the 3 security
tests that fail unpatched and pass here.
- `cargo fmt`, `clippy`, `git diff --check` all clean.
**Live manual drive** against a locally running relay, using raw
`nak`-signed events (the `buzz` CLI refuses malformed `kind:9000`, so
the guards have to be exercised directly):
- *Rejected:* member demotes owner; member demotes admin; self-promote
to owner; self-promote to admin; admin demotes the last owner; sole
owner self-demote; demoted ex-owner demotes last owner; private-channel
member demotes owner; non-member demotes owner in private.
- *Allowed:* bare PUT_USER with no role tag (owner keeps role);
idempotent re-add at same role; owner promotes admin→owner, then owner2
legitimately demotes owner1.
- *Resurrection defeated:* owner promotes attacker to admin → kicks them
(`9001`) → attacker self-rejoins (`9021`) → returns as **member**, not
admin, and cannot demote the owner.
- Normal ops unaffected throughout: channel creation, messaging, member
listing, and legitimate governance all work.
## Behavior change to be aware of
Huddle bot-add sends `role="bot"`. If the target is **already an active
member at a different role**, that is now a role change and requires an
elevated actor. Previously it silently re-roled them — the same privesc
primitive through a different door, so narrowing it is intended.
This does not break the huddle flow in the path that matters: the
ephemeral channel add (the one that fails hard) is performed by the
host, who *created* that channel and is therefore its owner — verified
live. The parent-channel add is already explicitly best-effort,
capturing the error into `parent_error` with a comment anticipating "may
already be member"; adding a non-member agent there still works.
Flagging it rather than burying it.
## Notes
- Commit is **signoff-only, not cryptographically signed** — `-S` fails
in this environment (git tries to load the agent npub as an SSH key
file). DCO trailers are present and correct.
- Branch was merged with `origin/main` via `--no-ff` (not rebased).
Upstream had 17 commits, none touching these files, no migration
changes.
---------
Signed-off-by: tlongwell-block <109685178+tlongwell-block@users.noreply.github.com>
Co-authored-by: Dawn (sprout agent) <c6237ef84fa537c78dcee78efd2d4e59f728859c7f194da42ac51ededfa0be05@sprout-oss.stage.blox.sqprod.co>