From 9f4480723072f05c1d0a565a355325cf19ace6b6 Mon Sep 17 00:00:00 2001 From: Mike Swanson Date: Sat, 30 May 2026 18:48:48 -0700 Subject: [PATCH] =?UTF-8?q?audit:=20security=20pass=20re-audit=20(2026-05-?= =?UTF-8?q?30)=20=E2=80=94=203=20CRITICALs=20verified=20CLOSED?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Independent /gc-audit --pass=security re-derivation of the v2 secure-session-core rebuild: all three 2026-05-29 relay CRITICALs confirmed closed with no bypass (any-JWT-joins-session, viewer-WS blacklist, JWT-as-agent-key). Relay plane clean; consent/code paths fail closed; abuse surface bounded; rate limiting proxy-aware. Net-new: 1 HIGH (agent auto-update disables TLS cert verification -> MITM-RCE, agent/src/update.rs:45,111 — outside the relay plane), 1 LOW (chat content logged), 2 INFO. Report: reports/2026-05-30-gc-audit.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- reports/2026-05-30-gc-audit.md | 120 +++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 reports/2026-05-30-gc-audit.md diff --git a/reports/2026-05-30-gc-audit.md b/reports/2026-05-30-gc-audit.md new file mode 100644 index 0000000..68598c4 --- /dev/null +++ b/reports/2026-05-30-gc-audit.md @@ -0,0 +1,120 @@ +# GuruConnect Audit Report — 2026-05-30 + +**Auditor:** Claude (claude-opus-4-8[1m]) +**Passes:** Security & Remote-Session Integrity (`--pass=security` only) +**Previous audit:** 2026-05-29 (the audit that defined the v2 secure-session-core work) +**Scope note:** Single-pass security re-audit to formally verify the v2 secure-session-core +rebuild closed the three relay CRITICALs. Independent re-derivation against the code — not an +echo of the 2026-05-30 Tasks 3–5 code review. + +--- + +## Executive Summary + +| Pass | Total | Critical | High | Medium | Low | Info | +|------|-------|----------|------|--------|-----|------| +| Security & Session | 5 | 0 | 1 | 0 | 1 | 3 | + +**The three 2026-05-29 audit CRITICALs are CLOSED with no bypass.** The relay/server plane is +clean. The one serious finding is **net-new and outside the relay plane** — the Windows agent +auto-update path disables TLS verification (MITM → RCE). + +**Requires action:** [HIGH] Agent auto-update disables TLS cert verification → MITM-to-RCE. + +--- + +## The three audit CRITICALs — verification + +### CRITICAL #1 — "any JWT joins any session" → **CLOSED (no bypass)** +- `viewer_ws_handler` (`server/src/relay/mod.rs:463`) calls `validate_viewer_token`, which enforces + signature + expiry + `purpose=="viewer"` (`server/src/auth/jwt.rs:268,284`). A login JWT lacks the + `ViewerClaims` shape and is rejected (test `test_login_token_rejected_as_viewer_token`). +- Session binding: `claim_session_id != requested_session_id → 403` (`relay/mod.rs:494-501`). +- Mint authz: `mint_viewer_token` (`server/src/api/sessions.rs:114-131`) — `is_admin() || + has_permission("control")` → Control; else `has_permission("view")` → ViewOnly; else 403. +- View-only enforcement: access mode is in the **signed** claim; relay drops + `MouseEvent`/`KeyEvent`/`SpecialKey` when `!access.can_control()` (`relay/mod.rs:1313-1335`). + +### CRITICAL #2 — "viewer-WS blacklist bypass" → **CLOSED (no bypass)**, documented residual +- `token_blacklist.is_revoked(&token)` consulted on the viewer WS path before upgrade + (`relay/mod.rs:476`). +- **Residual [TRACKED — v2-secure-session-core plan.md, MEDIUM]:** logout + (`server/src/api/auth_logout.rs:64`) revokes only the login JWT; minted viewer tokens are not + blacklisted, so a leaked viewer token stays valid until natural expiry (5-min TTL, + `jwt.rs:56`). Known follow-up; the enforcement plumbing is correct for any future force-revoke. + +### CRITICAL #3 — "JWT accepted as agent key" → **CLOSED (no bypass), fails closed** +- `validate_agent_api_key` (`relay/mod.rs:384`) has no JWT branch — only a per-agent `cak_` key + (`verify_agent_key`, `server/src/auth/agent_keys.rs:71`; SHA-256 vs `connect_agent_keys`, + `revoked_at IS NULL`) or the deprecated shared `AGENT_API_KEY` (WARNING-logged). +- Identity binding fails closed: unresolved machine → 503 (`relay/mod.rs:276-286`); a client-supplied + `agent_id` is overridden by the key's machine identity (`relay/mod.rs:263-270`). + +--- + +## Pass 5: Security & Remote-Session Integrity + +### [HIGH] Agent auto-update disables TLS certificate verification → MITM-to-RCE +**File:** `agent/src/update.rs:45` and `:111` (live path: `agent/src/session/mod.rs:461-468,695-702`) +**Detail:** Both the `check_for_update` and `download_update` reqwest clients are built with +`.danger_accept_invalid_certs(true)` ("for self-signed certs in dev"). This is the **live** auto-update +path — the agent session loop calls `check_for_update → perform_update → download_update → +install_update → restart_with_new_version` at runtime. A network MITM (the relay endpoint is a public +hostname) can serve an arbitrary `guruconnect-update.exe`; the agent writes it over its own binary and +executes it — full code execution on every managed endpoint. `verify_checksum` (`update.rs:294`) does +NOT mitigate: the expected SHA-256 (`version_info.checksum_sha256`) arrives in the **same** `/api/version` +JSON over the unverified TLS connection, so the attacker controls both binary and checksum. +**Recommendation:** Remove `danger_accept_invalid_certs(true)` from both clients (gate behind a dev-only +`cfg`/env flag never set in release). Defense-in-depth: sign the update binary/manifest with an embedded +public key and verify the signature before `install_update`, so update trust does not rest solely on TLS. + +### [LOW] Chat message content logged at INFO +**File:** `server/src/relay/mod.rs:776` and `:1373` +**Detail:** Full chat text is logged (`Chat from client/technician: {content}`). Support-session chat can +carry sensitive data (spoken passwords, account numbers) — PII/secret-bearing content in server logs. +**Recommendation:** Drop content from the log line (log length or hash, or move to `trace!`). + +### [INFO] Bootstrap admin password logged on credentials-file write failure +**File:** `server/src/main.rs:200-201` +**Detail:** Self-documented one-time fallback at first boot if `.admin-credentials` write fails; password +is meant to be changed immediately. Awareness only, not a defect. + +### [INFO] Viewer-token mint not scoped to a specific machine/session ACL — **[TRACKED — SPEC-002 Phase 4]** +**File:** `server/src/api/sessions.rs:27,114` +**Detail:** Any `control`-permission user can mint a control token for any live session (no per-machine / +per-client ACL). Intentional for Phase 1; per-tenant narrowing deferred to Phase 4. + +### [INFO] Verified-sound areas +Single-use support codes (atomic `consume_for_bind`, `support_codes.rs:240` / `db/support_codes.rs:110`); +consent gate (viewer refused until `allows_viewer()`, `session/mod.rs:393`; deny/timeout tears down before +the relay loop, no pre-consent window); frame caps (agent 4 MiB / viewer 64 KiB, `relay/mod.rs:330,524`); +input throttle (200 ev/s token bucket + non-blocking `try_send`); rate limiting wired + proxy-aware +(`main.rs:317-370`, `utils/ip_extract.rs:147`); `JWT_SECRET` length-checked ≥32 at startup; Argon2id +passwords; no secret/token/code logging; parameterized runtime sqlx (no `format!`-built SQL); `wss://` default. + +--- + +## Verdict + +| CRITICAL | Verdict | Enforced at | +|----------|---------|-------------| +| #1 any-JWT-joins-any-session | **CLOSED** | `api/sessions.rs:114` (authz) + `relay/mod.rs:463,494` (token type + session bind) | +| #2 viewer-WS blacklist bypass | **CLOSED** (TTL-bounded residual tracked) | `relay/mod.rs:476` | +| #3 JWT-accepted-as-agent-key | **CLOSED** | `relay/mod.rs:384` (no JWT branch; fail-closed binding `:276`) | + +**Net-new findings:** CRITICAL 0 · HIGH 1 · MEDIUM 0 · LOW 1 · INFO 2. +**Overall posture:** The relay/server plane is clean — all three CRITICALs genuinely closed, fail-closed +session/consent/code paths, bounded abuse surface, correct proxy-aware rate limiting, no SQL injection or +credential logging. The one serious net-new issue is on the Windows **agent** (auto-update TLS bypass → +MITM-RCE), independent of the v2 secure-session-core work and in no existing spec — fix before the next +agent release. + +--- + +## Recommended Action Order +1. **[HIGH]** Remove `danger_accept_invalid_certs(true)` from `agent/src/update.rs` (+ sign update binary). +2. **[LOW]** Stop logging chat content (`relay/mod.rs:776,1373`). +3. **[TRACKED/MEDIUM]** Viewer-token logout revocation (existing plan.md follow-up). + +*Note: only `--pass=security` was run. The API-surface, Rust-quality, TypeScript, protocol-integrity, +docs-reconciliation, and CI/CD passes were not executed this run.*