J3vb/OwnCord
1
0
Fork 0
mirror of https://github.com/J3vb/OwnCord.git synced 2026-09-03 03:50:00 +03:00
Code Issues Packages Projects Releases Wiki Activity

docs(security): add 2026-08-04 whole-codebase security review (#1326)

Browse Source 
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
2026-08-04 19:18:07 +02:00
committed by GitHub
co-authored by Claude
parent cdcfc03f80
commit 5630aa1248
1 changed files with 335 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/audit-2026-08-04.md
+335
View File
@@ -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.