fix(security): harden server against verified code-review findings

Applies fixes for 20 adversarially-verified findings from a whole-codebase
security review (server side). All Go build-tag variants build, `go vet` is
clean, and the suite passes (the sole failing test, ws TestEmitEvents, is a
pre-existing nil-harness failure unrelated to these changes).

High severity:
- auth: close TOCTOU in TOTP verify rate-limit by recording each attempt
  atomically up-front (was Check-then-Allow), restoring the per-user
  brute-force cap.
- plugin: enforce the CPU/time budget on every WASM guest call via a
  WithTimeout context (WithCloseOnContextDone interrupts runaways); the
  configured budget was previously parsed but never applied.
- api/waf: inspect request bodies for chunked (ContentLength==-1) requests
  so the SQLi/XSS/RCE body rules can no longer be bypassed.
- ws: rate-limit voice_join/voice_leave and voice_e2ee announce/offer, which
  fan out to every participant and could force mass disconnects.

Medium severity:
- api: run bcrypt on the unknown-user login path (no || short-circuit) to
  remove the timing-based username-enumeration oracle.
- ws: verify LiveKit webhooks via the SDK receiver so the signature is bound
  to the body hash (kills forgery/replay).
- authz: require READ_MESSAGES for reactions and for plugin-command
  broadcasts; route the latter through RequireChannelAccess.
- api: cache the client-update signature fetch and rate-limit the endpoint.
- service: propagate DeleteOtherSessions failure from ChangePassword instead
  of silently reporting success.
- api: trust the rightmost non-proxy X-Forwarded-For entry, not the
  client-controllable leftmost one.
- plugin: route auto-registered commands through the conflict-checked
  RegisterCommand; pin the DNS-validated IP for host_http dials
  (DNS-rebinding TOCTOU).
- api: mark access-controlled downloads private/no-cache + Vary: Origin.

Low severity:
- auth: fail closed when a fully-shaped TOTP ciphertext fails GCM auth
  (was returning the ciphertext as plaintext).
- api: apply the livekit-proxy path allowlist to WebSocket upgrades too.
- service: verify attachment ownership before linking (IDOR).
- admin: bound the bootstrap setup invite (5 uses / 24h); re-verify the
  update binary hash immediately before rename+spawn (TOCTOU).
- service: require BanMembers + role hierarchy for moderation ban/unban.

chore: stop tracking the stray Server/owncord-server.exe build artifact.

Test infra: add uploader_id to the hand-rolled ws test attachment schemas and
make MemStore.GetAttachmentByID a no-op lookup, matching production/DB behavior.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
J3vb
2026-07-17 21:08:54 +02:00
co-authored by Claude Opus 4.8
parent 34a84bcd5d
commit 7b178ff30b
30 changed files with 449 additions and 104 deletions
+28 -11
View File
@@ -76,26 +76,43 @@ func (r *Registry) HTTPDo(ctx context.Context, inst *Instance, req HTTPRequest)
for k, v := range req.Header {
httpReq.Header.Set(k, v)
}
// Custom transport with a guarded DialContext: every actual TCP dial
// re-checks the resolved IP, closing the DNS-rebinding TOCTOU window
// between rejectPrivateAddrs above and the underlying dial.
// Custom transport with a guarded DialContext: the host is resolved once,
// every candidate IP is validated against the blocklist, and the actual
// connection is made to that specific vetted IP — never re-resolved by
// hostname. This closes the DNS-rebinding TOCTOU window where a second
// lookup (the one net.Dialer would perform on a hostname) could return an
// internal IP after rejectPrivateAddrs above had already approved the name.
dialer := &net.Dialer{Timeout: httpTimeout}
transport := &http.Transport{
DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
h, _, splitErr := net.SplitHostPort(addr)
h, port, splitErr := net.SplitHostPort(addr)
if splitErr != nil {
return nil, splitErr
}
ip := net.ParseIP(h)
if ip == nil {
// Hostname — resolve and validate every address before dial.
if err := rejectPrivateAddrs(ctx, h); err != nil {
// IP literal: validate and dial as-is (no resolution happens).
if ip := net.ParseIP(h); ip != nil {
if err := ipAllowed(ip); err != nil {
return nil, fmt.Errorf("%w: %v", ErrHTTPHostDenied, err)
}
} else if err := ipAllowed(ip); err != nil {
return nil, fmt.Errorf("%w: %v", ErrHTTPHostDenied, err)
return dialer.DialContext(ctx, network, addr)
}
return dialer.DialContext(ctx, network, addr)
// Hostname: resolve once, validate every returned address, then
// dial the concrete vetted IP so the connection target is exactly
// the address that was checked.
resolver := &net.Resolver{}
ips, lookupErr := resolver.LookupIPAddr(ctx, h)
if lookupErr != nil {
return nil, fmt.Errorf("%w: dns lookup failed: %v", ErrHTTPHostDenied, lookupErr)
}
if len(ips) == 0 {
return nil, fmt.Errorf("%w: no addresses for %s", ErrHTTPHostDenied, h)
}
for _, resolved := range ips {
if err := ipAllowed(resolved.IP); err != nil {
return nil, fmt.Errorf("%w: %v", ErrHTTPHostDenied, err)
}
}
return dialer.DialContext(ctx, network, net.JoinHostPort(ips[0].IP.String(), port))
},
}
client := &http.Client{
+43 -9
View File
@@ -5,7 +5,7 @@
// elsewhere in the repo so the default sqlite-only build does not pull
// wazero into go.mod at runtime.
//
// Architecture
// # Architecture
//
// The wazero-tagged build provides:
//
@@ -36,8 +36,10 @@ import (
"encoding/json"
"fmt"
"io"
"log/slog"
"os"
"strings"
"time"
"github.com/tetratelabs/wazero"
"github.com/tetratelabs/wazero/api"
@@ -121,17 +123,26 @@ func (r *Registry) activateWithRuntime(ctx context.Context, platform any, inst *
return fmt.Errorf("plugin %q: instantiate: %w", inst.Manifest.Name, err)
}
// Register the module and auto-bind any commands the plugin exports
// via list_commands. The plugin must also have declared the `commands`
// capability in its manifest, otherwise no binding happens.
// Store the module under the lock, then auto-bind any commands the plugin
// exports via list_commands. Binding is routed through RegisterCommand so
// each name goes through the same normalization (trim "/" + lowercase) and
// conflict check the direct registration path uses: a command already owned
// by a DIFFERENT plugin is refused rather than silently clobbered, closing
// the cross-plugin command-hijack hole. RegisterCommand acquires r.mu
// itself, so it is called outside the lock below to avoid re-entrant
// locking. The plugin must also have declared the `commands` capability in
// its manifest, otherwise no binding happens.
r.mu.Lock()
inst.module = module
r.mu.Unlock()
if inst.Manifest.HasCapability(CapCommands) {
for _, cmd := range listExportedCommands(ctx, module) {
r.commands[cmd] = inst
if err := r.RegisterCommand(cmd, inst); err != nil {
slog.Warn("plugin: skipping command binding",
"plugin", inst.Manifest.Name, "command", cmd, "err", err)
}
}
}
r.mu.Unlock()
return nil
}
@@ -195,9 +206,27 @@ func (r *Registry) invokeCommand(ctx context.Context, inst *Instance, userID, ch
return &CommandResult{Reply: fmt.Sprintf("plugin %s: marshal payload: %v", inst.Manifest.Name, err)}, true
}
// Enforce the plugin's CPU budget. The effective budget is the manifest's
// Resources.CPUBudgetMs, falling back to the configured default, then a
// hard 100ms floor so a zero/negative value can never mean "no limit".
// Every guest call (allocate / command_dispatch / deallocate) runs under
// this deadline instead of the long-lived WebSocket context. The runtime
// was created WithCloseOnContextDone(true), so an expired deadline closes
// the module and interrupts a runaway guest (e.g. `for {}`) — the Call
// returns an error rather than panicking, which the paths below surface.
budgetMs := inst.Manifest.Resources.CPUBudgetMs
if budgetMs <= 0 {
budgetMs = r.cfg.CPUBudgetMs
}
if budgetMs <= 0 {
budgetMs = 100
}
callCtx, cancel := context.WithTimeout(ctx, time.Duration(budgetMs)*time.Millisecond)
defer cancel()
// Allocate guest memory for the input payload.
size := uint64(len(payload))
ptrs, callErr := allocFn.Call(ctx, size)
ptrs, callErr := allocFn.Call(callCtx, size)
if callErr != nil || len(ptrs) == 0 {
return &CommandResult{Reply: fmt.Sprintf("plugin %s: allocate(%d): %v", inst.Manifest.Name, size, callErr)}, true
}
@@ -208,14 +237,19 @@ func (r *Registry) invokeCommand(ctx context.Context, inst *Instance, userID, ch
return &CommandResult{Reply: fmt.Sprintf("plugin %s: memory write at %d failed", inst.Manifest.Name, ptr)}, true
}
results, callErr := dispatchFn.Call(ctx, ptr, size)
results, callErr := dispatchFn.Call(callCtx, ptr, size)
// Free the input buffer regardless of dispatch outcome.
if deallocFn != nil {
_, _ = deallocFn.Call(ctx, ptr, size)
_, _ = deallocFn.Call(callCtx, ptr, size)
}
if callErr != nil {
// Surface a CPU-budget overrun as a clean, specific error rather than
// leaking the raw "module closed with context deadline exceeded".
if callCtx.Err() == context.DeadlineExceeded {
return &CommandResult{Reply: fmt.Sprintf("plugin %s: command exceeded CPU budget of %dms", inst.Manifest.Name, budgetMs)}, true
}
return &CommandResult{Reply: fmt.Sprintf("plugin %s: dispatch: %v", inst.Manifest.Name, callErr)}, true
}
if len(results) < 2 {