mirror of
https://github.com/J3vb/OwnCord.git
synced 2026-09-03 03:50:00 +03:00
docs(security): add 2026-08-04 whole-codebase security review (#1326)
Read-only security review of the full tree (Go server, admin panel, WASM plugin host, LiveKit voice, Tauri client). No code changes. Three findings, all the same defect class — a security predicate enforced at some members of a handler family but not all: - A-2026-08-01 (HIGH) handleDeleteChannelPermission omits the hierarchy and grantability guards its PUT twin carries, so a MANAGE_CHANNELS holder can clear their own role's channel deny and read private channels. - A-2026-08-02 (HIGH) the admin channel list/patch/delete handlers omit the type == "dm" guard their sibling getPermChannel carries, so the same role can enumerate and irreversibly cascade-delete arbitrary DMs and group DMs. - A-2026-08-03 (MEDIUM) DMService.RingTargets omits the block check the five other DM interaction sinks perform, so a blocked user can ring the person who blocked them. Also records one non-vulnerability observation (backup restore writes to a hardcoded database path, silently no-opping when database.path is customised), the candidates rejected during verification, the areas verified clean, and the areas not examined. Claude-Session: https://claude.ai/code/session_01Q7GUJtdsHHHGs4pSiLn6LJ Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
J3vb
@@ -0,0 +1,335 @@
|
||||
# OwnCord — Security Review
|
||||
|
||||
**Date:** 2026-08-04
|
||||
**Branch:** `claude/security-review-workflows-oa0lm5` (audited tree: `cbc4f9e` = `dev`)
|
||||
**Scope:** whole-codebase security review — Go server, admin panel, WASM plugin host,
|
||||
LiveKit voice, Tauri desktop client. Read-only; no code changes ship with this audit.
|
||||
**Relationship to prior audits:** successor to
|
||||
[audit-2026-07-19.md](audit-2026-07-19.md), which remains the closure tracker for its
|
||||
own findings. This review is security-only and does not re-open architectural items.
|
||||
|
||||
> **Note on scope selection:** this branch carries no diff against `dev`, so a
|
||||
> pending-changes review had nothing to examine. The review was run against the full
|
||||
> codebase instead.
|
||||
|
||||
---
|
||||
|
||||
## Finding closure status (maintained; update statuses in place)
|
||||
|
||||
| ID | Sev | Finding | Status |
|
||||
|----|-----|---------|--------|
|
||||
| A-2026-08-01 | HIGH | `handleDeleteChannelPermission` omits the role-hierarchy and grantability guards its `PUT` twin carries — a MANAGE_CHANNELS holder can clear their own role's channel `deny` and read private channels | OPEN |
|
||||
| A-2026-08-02 | HIGH | Admin channel `LIST`/`PATCH`/`DELETE` handlers omit the `type == "dm"` guard their sibling `getPermChannel` carries — a MANAGE_CHANNELS holder can enumerate and irreversibly destroy arbitrary DMs and group DMs | OPEN |
|
||||
| A-2026-08-03 | MEDIUM | `DMService.RingTargets` omits the block check every other DM interaction sink performs — a blocked user can ring the person who blocked them | OPEN |
|
||||
|
||||
All three share one root cause: **a security predicate applied at some members of a
|
||||
handler family but not all of them.** The codebase states this rule in its own comments
|
||||
(`handlers_channel_perms.go:348`: *"clearing a higher-ranked member's override is the
|
||||
same authority as writing one, so gate it identically"*; `message_perms.go:95`:
|
||||
*"the single block-check implementation, called from every DM interaction sink"*) and
|
||||
then violates it in three places. See [§4](#4-systemic-observation) for the structural fix.
|
||||
|
||||
---
|
||||
|
||||
## 1. A-2026-08-01 — Missing hierarchy guard on channel role-override delete
|
||||
|
||||
* **Severity:** HIGH
|
||||
* **Category:** `authz_bypass` / privilege escalation
|
||||
* **Location:** `Server/admin/handlers_channel_perms.go:180` (`handleDeleteChannelPermission`)
|
||||
* **Route:** `DELETE /admin/api/channels/{id}/permissions/{roleId}`
|
||||
* **Attacker:** any authenticated user holding a role with `MANAGE_CHANNELS` and *not*
|
||||
`ADMINISTRATOR` — the seeded **Moderator** role (`0x000FFFFF`, position 60) qualifies.
|
||||
|
||||
### Description
|
||||
|
||||
The role layer of channel permissions is exposed as a `PUT`/`DELETE` pair, both gated
|
||||
only by `r.Use(requirePerm(permissions.ManageChannels))` (`Server/admin/api.go:70`).
|
||||
|
||||
`handlePutChannelPermission` carries two deliberate escalation guards
|
||||
(`handlers_channel_perms.go:141-150`):
|
||||
|
||||
```go
|
||||
// Escalation guard: a MANAGE_CHANNELS holder without ADMINISTRATOR
|
||||
// cannot grant bits their own role lacks via a channel override.
|
||||
if err := requireGrantableOverride(actorRole, allow, deny); err != nil { ... }
|
||||
// Hierarchy guard: a role override can only target a role strictly
|
||||
// below the actor's own position, mirroring service.requireBelowActor.
|
||||
if role.Position >= actorRole.Position { ... }
|
||||
```
|
||||
|
||||
`handleDeleteChannelPermission` has **neither**. Its whole body resolves the channel,
|
||||
parses `roleId`, and calls `database.DeleteChannelOverride`. It never reads
|
||||
`actorRoleFromContext(r)` at all, so no position comparison is possible.
|
||||
|
||||
Deleting an override *is* a permission mutation. `EffectiveChannelPerms`
|
||||
(`Server/permissions/permissions.go:161`) resolves to `(base &^ deny) | allow`, so
|
||||
removing the row reverts the role to its bare mask. A private channel in OwnCord is
|
||||
built precisely by writing a `deny` of `READ_MESSAGES` for the roles that must not see
|
||||
it — so deleting your own role's row restores exactly the access the `PUT` path refuses
|
||||
to grant.
|
||||
|
||||
The per-user sibling `handleDeleteChannelUserPermission`
|
||||
(`handlers_channel_perms.go:333-352`) *does* guard, with a comment stating the rule the
|
||||
role-layer delete breaks. There is also a dedicated regression test for the `PUT` case
|
||||
(`TestPutChannelPermission_RefusesEqualOrHigherRole`) and none for `DELETE`.
|
||||
|
||||
### Exploit scenario
|
||||
|
||||
1. Owner creates private `#staff-only` and locks moderators out:
|
||||
`PUT /admin/api/channels/42/permissions/3` with `{"allow":0,"deny":2}`
|
||||
(`2` = `READ_MESSAGES`). The channel correctly disappears from the moderator's
|
||||
`ready` payload, `ListVisibleChannels`, REST reads, and reconnect replay.
|
||||
2. Moderator confirms the override exists: `GET /admin/api/channels/42/permissions`.
|
||||
3. Moderator tries the sanctioned path and is refused:
|
||||
`PUT .../permissions/3` `{"allow":2,"deny":0}` → `403 FORBIDDEN`,
|
||||
*"cannot manage a role at or above your own rank"* (position 60 ≥ 60).
|
||||
4. Moderator sends **`DELETE /admin/api/channels/42/permissions/3`** with the same
|
||||
token. No guard runs. The row is deleted, `permInvalidator.InvalidateAll()` drops
|
||||
every cached verdict, and `hub.RefreshChannelVisibility(ch)` pushes a live
|
||||
`channel_create` to the attacker's socket.
|
||||
5. Effective mask is now the bare `0x000FFFFF`, which includes `READ_MESSAGES`. Full
|
||||
history, pins, attachments and search on the private channel are readable — plus
|
||||
`SEND_MESSAGES`, `MANAGE_MESSAGES` and bulk purge, all of which the deny withheld.
|
||||
|
||||
The same request with `roleId=1` or `2` strips an override protecting Owner or Admin —
|
||||
the exact cross-rank mutation the hierarchy rule exists to forbid.
|
||||
|
||||
### Recommendation
|
||||
|
||||
Give the handler the guards its twin has: load `actorRole := actorRoleFromContext(r)`
|
||||
(fail closed on `nil`), fetch the target role, and refuse `403` when
|
||||
`role.Position >= actorRole.Position` unless `permissions.HasAdmin(actorRole.Permissions)`.
|
||||
Optionally also run `requireGrantableOverride` against the bits the row being removed
|
||||
carries. Add the `DELETE` twin of `TestPutChannelPermission_RefusesEqualOrHigherRole`.
|
||||
|
||||
---
|
||||
|
||||
## 2. A-2026-08-02 — Admin channel handlers operate on DM channels
|
||||
|
||||
* **Severity:** HIGH
|
||||
* **Category:** `missing_authorization_guard` — metadata disclosure + irreversible destruction
|
||||
* **Location:** `Server/admin/handlers_channels.go:237` (`handleDeleteChannel`);
|
||||
same defect at `:159` (`handlePatchChannel`) and `:38` (`handleListChannels`)
|
||||
* **Attacker:** same as A-2026-08-01 — a `MANAGE_CHANNELS` holder who is not an administrator.
|
||||
|
||||
### Description
|
||||
|
||||
DMs and group DMs are ordinary rows in the `channels` table with `type = 'dm'`, sharing
|
||||
the autoincrement id space with guild channels (`migrations/009_dm_tables.sql`,
|
||||
`migrations/013_channel_type_constraint.sql`).
|
||||
|
||||
The override handlers in the same package resolve channels through `getPermChannel`,
|
||||
which explicitly refuses DMs (`handlers_channel_perms.go:40`):
|
||||
|
||||
```go
|
||||
if ch.Type == "dm" {
|
||||
writeErr(w, http.StatusBadRequest, "INVALID_INPUT", "DM channels do not support permission overrides")
|
||||
return nil
|
||||
}
|
||||
```
|
||||
|
||||
That guard is proof the authors knew DM ids reach this route family. Its siblings do not
|
||||
have it: `handlePatchChannel` and `handleDeleteChannel` both call a bare
|
||||
`database.GetChannel(r.Context(), id)` and inspect `ch.Type` nowhere.
|
||||
`handleListChannels` returns `db.ListChannels` verbatim, whose SQL is
|
||||
`SELECT ... FROM channels ORDER BY position ASC, id ASC` — no `type` predicate — so it
|
||||
enumerates every private conversation on the server.
|
||||
|
||||
The shipped admin UI already renders these rows: `Server/admin/static/index.html:905`
|
||||
suppresses only the *lock* button for `type==='dm'`, leaving **Edit** and **Delete**
|
||||
live. `AdminDeleteChannel` is `DELETE FROM channels WHERE id = ?` with `foreign_keys`
|
||||
enabled (`db/db.go:59`), and `messages`, `dm_participants` and `dm_open_state` all
|
||||
declare `ON DELETE CASCADE` — destruction is total and irreversible.
|
||||
|
||||
### Exploit scenario
|
||||
|
||||
1. Moderator authenticates to the admin panel; `adminAuthMiddleware` admits them because
|
||||
`AdminPerimeter` (`permissions.go:42`) includes `ManageChannels` on its own.
|
||||
2. `GET /admin/api/channels` returns every DM and group-DM row — ids, plus user-chosen
|
||||
group names, which act as a membership-graph oracle for conversations they are not
|
||||
party to, including the owner's.
|
||||
3. `DELETE /admin/api/channels/{dm_id}` → no `ch.Type` check → cascade wipes the entire
|
||||
conversation: every message, every participant row, every open-state row.
|
||||
4. Iterating step 2's ids destroys every private conversation on the server, including
|
||||
those of principals who strictly outrank the attacker. No hierarchy check, no
|
||||
participant check, no recovery short of a database restore.
|
||||
5. `PATCH /admin/api/channels/{dm_id}` rewrites a group DM's `name` and `archived` flag,
|
||||
silently relabelling the conversation for its real participants.
|
||||
|
||||
### Scope correction
|
||||
|
||||
`PATCH` cannot expose DM message **content**: `AdminUpdateChannel` never writes `type`,
|
||||
and every DM read path is independently participant-gated
|
||||
(`permissions/checker.go:113`, `service/message_query.go:25`, `service/channel.go:128`;
|
||||
`ws/hub_broadcast.go:340` skips `type=="dm"`). The real impact is **metadata
|
||||
enumeration, irreversible destruction, and silent renaming** — integrity and
|
||||
availability plus metadata confidentiality, not message-content disclosure.
|
||||
|
||||
### Recommendation
|
||||
|
||||
Route `handleListChannels`, `handlePatchChannel` and `handleDeleteChannel` through the
|
||||
same DM-refusing resolver `getPermChannel` already implements, returning `404 NOT_FOUND`
|
||||
so a DM's existence is not confirmed. Filter `type != 'dm'` out of the admin channel
|
||||
listing — DM lifecycle already has its own participant-gated surface in
|
||||
`service.DMService`. Add DM-rejection coverage to `admin/handlers_channels_test.go`,
|
||||
which currently has none.
|
||||
|
||||
---
|
||||
|
||||
## 3. A-2026-08-03 — `call_ring` / `call_decline` bypass DM block enforcement
|
||||
|
||||
* **Severity:** MEDIUM
|
||||
* **Category:** `access-control` — user-safety control bypass
|
||||
* **Location:** `Server/service/dm.go:336` (`DMService.RingTargets`), reached from
|
||||
`Server/ws/handlers_call.go:45` (`call_ring`) and `:73` (`call_decline`)
|
||||
* **Attacker:** any ordinary authenticated user who shares an existing 1:1 DM with the victim.
|
||||
|
||||
### Description
|
||||
|
||||
`requireDMNotBlocked` (`Server/service/message_perms.go:117`) describes itself as
|
||||
*"the single block-check implementation, called from every DM interaction sink — send,
|
||||
edit, react, pin and typing"*, and its doc comment explains precisely why partial
|
||||
coverage fails:
|
||||
|
||||
> Enforcing it on the send path alone left a blocked user an open channel to the
|
||||
> blocker: editing an already-sent message fans `MessageEditedDMEvent` out to every
|
||||
> participant, so arbitrary new text still reached the person who blocked them, and
|
||||
> reactions and typing indicators did the same.
|
||||
|
||||
Five sinks call it (`message_perms.go:79`, `message_crud.go:223`,
|
||||
`message_reactions.go:113`, `channel.go:136`, `message_query.go:212`). `RingTargets`
|
||||
does not — it checks `IsDMParticipant` and returns the other participants. Blocking does
|
||||
not remove `dm_participants` rows (`service/block.go` only inserts a block row), so a
|
||||
blocked user remains a participant, and `CreateDM` only gates *new* DMs — the normal
|
||||
case is a pre-existing conversation.
|
||||
|
||||
The resulting `CallSignalEvent` is delivered straight to the target's live socket via
|
||||
`SendToUserHigh` (`ws/emit.go:34`), with no block filtering at the hub. The client
|
||||
surfaces it as a banner naming the sender plus a repeating chime for 30 s
|
||||
(`Client/tauri-client/src/lib/call-ring.ts`). `call_ring` is limited to one per 3 s;
|
||||
`call_decline` has no limiter at all.
|
||||
|
||||
### Caveat — this sits on a documented design boundary
|
||||
|
||||
`Server/ws/deps.go:190` states that blocking is *deliberately* not consulted on the
|
||||
voice-**access** path (*"it is the message paths' rule … a blocked user is still a
|
||||
participant"*). That comment governs `hasChannelAccess`, not the ring fan-out, and
|
||||
`requireDMNotBlocked`'s own sink list does not name ringing. So the maintainers should
|
||||
decide whether a ring is a message-path sink or a voice-path one. The argument for
|
||||
treating it as a message-path sink is that it is functionally identical to the typing
|
||||
indicator the project already hardened: an unsolicited, identity-bearing event pushed to
|
||||
the blocker's client. Group DMs are correctly out of scope — blocks there are enforced
|
||||
at `CreateGroupDM` by design.
|
||||
|
||||
### Recommendation
|
||||
|
||||
Call `requireDMNotBlocked` inside `RingTargets` alongside the existing `IsDMParticipant`
|
||||
check. One call site covers both handlers and matches the five sibling sinks.
|
||||
|
||||
---
|
||||
|
||||
## 4. Systemic observation
|
||||
|
||||
All three findings are the same defect class: **one member of a handler family enforces
|
||||
a security predicate and a sibling does not.** In every case the guard already exists,
|
||||
correct, a few dozen lines away, and in two of the three the codebase's own comments
|
||||
state the rule being broken.
|
||||
|
||||
This is the same shape as audit-2026-07-19's A-2026-07-07 and A-2026-07-16 (a
|
||||
channel-visibility rule copy-pasted across five sites, and a server-permission rule
|
||||
hand-rolled at two), both closed by collapsing the duplicates onto one shared predicate.
|
||||
That remedy was applied to the *read* paths; these three are *write* and *notify* paths
|
||||
that were not part of that sweep.
|
||||
|
||||
Suggested follow-up, in preference order:
|
||||
|
||||
1. **Make the resolver own the guard.** Handlers should not receive a `*db.Channel` they
|
||||
are trusted to validate. One `resolveManageableChannel(r)` helper that refuses DMs and
|
||||
enforces hierarchy, used by every handler in the admin channel family, makes the
|
||||
asymmetry impossible rather than merely fixed.
|
||||
2. **Pair-test the mutation surface.** Every guard test that asserts a refusal on one
|
||||
verb should have a twin asserting the same refusal on the inverse verb. Both HIGH
|
||||
findings would have been caught by that rule alone.
|
||||
3. **Audit the remaining families** for the same shape — the `PUT`/`DELETE`,
|
||||
`add`/`remove` and `REST`/`WS` pairs elsewhere in `admin/` and `ws/`.
|
||||
|
||||
---
|
||||
|
||||
## 5. Additional observation — not a vulnerability
|
||||
|
||||
**`Server/admin/handlers_backup.go:177` — restore writes to a hardcoded database path.**
|
||||
|
||||
```go
|
||||
dbPath := filepath.Join("data", "chatserver.db")
|
||||
```
|
||||
|
||||
`main.go:129` opens the live database at `cfg.Database.Path`, which is operator-settable
|
||||
via `database.path` and `OWNCORD_DATABASE_PATH`. The restore handler ignores both. The
|
||||
same file deliberately resolves `backupBaseDir` with `filepath.Abs` at init *"so handlers
|
||||
don't depend on the process CWD (L14)"* — the database path never got the same treatment.
|
||||
|
||||
Because the shipped default *is* `data/chatserver.db`, this is latent: it only bites an
|
||||
operator who changed the path or runs the server from a different working directory.
|
||||
When it does bite, `POST /admin/api/backups/{name}/restore` copies the backup over a
|
||||
decoy file, returns `200 "database restored — server restarting"`, and respawns against
|
||||
the untouched original — a **silent no-op in the disaster-recovery path**. In an
|
||||
incident-response context (rolling back to a known-good snapshot after a compromise) the
|
||||
operator is told they have rolled back and has not.
|
||||
|
||||
A secondary failure sits on the same path: if `copyFile` fails, the handler returns `500`
|
||||
*without* calling `requestRestart`, but the database was already `Close()`d — so the
|
||||
process keeps serving every request against a closed database.
|
||||
|
||||
This is listed as an observation rather than a finding because no attacker controls it
|
||||
and it requires a privileged operator action; it is a correctness bug with security
|
||||
consequences, not an exploitable vulnerability. Root cause is structural:
|
||||
`admin.NewAdminAPI` is never passed a `*config.Config` and `db.DB` exposes no path
|
||||
accessor, so the admin package cannot learn the real path.
|
||||
`admin/handlers_backup_test.go:336` `chdir`s to a temp dir and hand-creates
|
||||
`data/chatserver.db`, encoding the hardcoded assumption instead of contrasting it with a
|
||||
configured path.
|
||||
|
||||
---
|
||||
|
||||
## 6. Coverage and method
|
||||
|
||||
Two review rounds ran, deliberately along different axes so the second could catch what
|
||||
the first's shape would miss.
|
||||
|
||||
**Round 1 — by subsystem (12 hunts):** authentication/session/TOTP, authorization,
|
||||
injection, path/file handling, plugin sandbox, cryptography and the update channel,
|
||||
admin panel and setup, WebSocket protocol, SSRF, uploads and media, client rendering and
|
||||
IPC, data exposure. One finding (A-2026-08-01).
|
||||
|
||||
**Round 2 — by cross-cutting modality (6 hunts):** guard asymmetry between sibling code
|
||||
paths, fail-open error branches in security decisions, sink-driven grep of the whole Go
|
||||
tree, the unauthenticated surface, cross-user data boundaries traced up from the query
|
||||
layer, and recently changed code. Two findings (A-2026-08-02, A-2026-08-03) — both of
|
||||
which the subsystem-shaped round missed, which is the argument for running the second axis.
|
||||
|
||||
Every candidate was put through two independent adversarial reviewers (one instructed to
|
||||
refute, one applying an exclusion policy) and, on surviving both, a final adjudicator
|
||||
that re-traced the path from source. Findings below confidence 8/10 were dropped.
|
||||
|
||||
**Rejected during verification** (recorded so they are not re-raised):
|
||||
|
||||
| Candidate | Why rejected |
|
||||
|-----------|--------------|
|
||||
| `ws_proxy.rs` TOFU pins self-approvable via `accept_cert_fingerprint` | Only attacker is one who already has arbitrary JS in the webview, who can already call `load_identity_key` — strictly stronger. No marginal gain. |
|
||||
| `ptt.rs` `ptt_set_key` omits the `is_allowed_ptt_capture_vk` allowlist | Same precondition. Impact is a lossy one-key-at-a-time oracle, not keystroke recovery; the actual keylogging primitive (`ptt_listen_for_key`) *is* still allowlisted. Worth a small fix, not a MEDIUM. |
|
||||
|
||||
**Verified clean** (read and found sound, recorded to save future effort): reconnect
|
||||
replay authorization across both the hot ring buffer and cold `EventStore` tier,
|
||||
including the empty-`channelIDs` degenerate case; group-DM membership churn
|
||||
(`LeaveGroupDM` drops `dm_participants` and `dm_open_state` in one serializable tx);
|
||||
LiveKit grant scoping (`RoomJoin` bound to `channel-<id>`, no `RoomCreate`, no wildcard);
|
||||
the migration runner and its per-file transactions; `token_cli.go`; plugin
|
||||
`host_storage.go`; the admin log-stream ticket (32-byte `crypto/rand`, single-use, TTL'd,
|
||||
re-checks `HasAdmin` per frame); `client_update.go`; `proc_spawner_nix.go`; and the
|
||||
metrics and diagnostics endpoints.
|
||||
|
||||
**Not examined.** Coverage was not total. No hunt targeted `Server/telemetry/`,
|
||||
`Server/syncutil/`, `Server/stackutil/`, `Server/logctx/`, `db/dbgen/`,
|
||||
`ws/topic_rate_limiter.go`, `ws/event_pruner.go`, `Server/scripts/`, or
|
||||
`tools/mcp-introspect/`. Vulnerability classes not hunted include backup/restore
|
||||
integrity beyond §5, SQLite-specific dynamic-`IN` construction, and protocol codegen.
|
||||
Reference in New Issue
Block a user