audit: security pass re-audit (2026-05-30) — 3 CRITICALs verified CLOSED
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) <noreply@anthropic.com>
This commit is contained in:
120
reports/2026-05-30-gc-audit.md
Normal file
120
reports/2026-05-30-gc-audit.md
Normal file
@@ -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.*
|
||||
Reference in New Issue
Block a user