Mike Swanson
ded99c5882
Independent reviews of server/agent/dashboard by Gemini 3.1 Pro and Grok 4.3, each reading source via its own read_file tool, plus a Claude synthesis and a phased remediation plan. Top convergent finding: secrets/tokens in WebSocket URL query strings across agent + dashboard. See SYNTHESIS-three-way.md and REMEDIATION-PLAN.md.
26 lines
4.1 KiB
Markdown
26 lines
4.1 KiB
Markdown
# GuruConnect agent review — gemini — 2026-06-05
|
|
|
|
This code review of the GuruConnect agent (v2/SPEC-016/SPEC-018) identifies several critical and high-severity findings related to session security, privilege escalation, and resource management.
|
|
|
|
### Prioritized Findings List
|
|
|
|
| SEVERITY | FILE:LINE | FINDING | CONCRETE FIX |
|
|
| :--- | :--- | :--- | :--- |
|
|
| **CRITICAL** | `websocket.rs:43` | **Insecure Auth via Query String.** API keys and JWT tokens are passed in the URL query string. These are often logged by reverse proxies (nginx/IIS) and visible in process monitors, leading to session takeover or credential theft. | Move credentials to the `Authorization` header or the Protobuf handshake payload. |
|
|
| **CRITICAL** | `sas_service.rs:136` | **Command Injection in SYSTEM SAS Service.** The named pipe server reads a string, trims it, and matches it against `"sas"`. However, if the command protocol evolves or is misused, a lack of strict length/format validation on a SYSTEM-owned pipe is a high-risk surface for local privilege escalation (LPE). | Use a fixed-size byte enum or a proper Protobuf-serialized command structure for IPC. |
|
|
| **HIGH** | `update.rs:197` | **Insecure Auto-Update (No Signature).** Updates are verified via SHA-256 checksum sent over the same channel as the binary. A MITM attacker can serve a malicious binary + matching checksum. | Implement RSA/Ed25519 signature verification using an embedded public key before `install_update`. |
|
|
| **HIGH** | `credential_store.rs:309` | **LPE via `icacls` Subprocess.** The agent calls `icacls.exe` to secure the `cak_` store. If an attacker places a malicious `icacls.exe` in the search path (before the agent's working directory or in a user-writable path), the SYSTEM service will execute it. | Use the absolute path `C:\Windows\System32\icacls.exe` or use Win32 `SetNamedSecurityInfoW` API directly. |
|
|
| **HIGH** | `session.rs:498` | **Session DoS via Blocking Consent.** `prompt_consent` runs on `spawn_blocking`, but the main loop `await`s it. While heartbeats might survive a 60s timeout, a user ignoring the box stalls all other control messages (like Disconnect or AdminRestart). | Run the consent prompt as a background task that sends the response to the server without stalling the main message-processing loop. |
|
|
| **MEDIUM** | `main.rs:260` | **Service Run Panic Boundary.** `run_managed_agent_service` uses `catch_unwind` but runs inside a `tokio` runtime. Panics in spawned tasks or certain async contexts may still escape or cause the runtime to hang without triggering the SCM recovery. | Ensure the top-level `block_on` is wrapped, but also add a custom `PanicHook` to ensure the process exits cleanly so the SCM can restart it. |
|
|
| **MEDIUM** | `sas_service.rs:46` | **Pipe Squatting/Race.** The SAS service pipe uses a hardcoded name and `PIPE_UNLIMITED_INSTANCES`. An attacker starting before the service could squat the name. | Use `FILE_FLAG_FIRST_PIPE_INSTANCE` when creating the first pipe instance to ensure the service owns the name. |
|
|
| **LOW** | `config.rs:335` | **Config Exposure.** `EmbeddedConfig` contains `api_key` and `enrollment_key`. While it's in the binary, it's easily extractable via strings/binwalk. | The spec mentions `cek_` is low-sensitivity, but `api_key` (legacy) should be scrubbed from the binary after the first successful enrollment. |
|
|
|
|
---
|
|
|
|
### Overall Assessment
|
|
The codebase is well-structured and follows the SPEC-016/018 requirements closely, particularly the DPAPI integration and the transition to a SYSTEM service. However, it relies heavily on "security by obscurity" in its transport (credentials in URLs) and lacks a robust trust anchor for auto-updates. The use of shell-outs (`icacls`, `powershell`) from a SYSTEM context is a significant LPE risk vector.
|
|
|
|
### Single Most Important Thing to Fix
|
|
**Implement Cryptographic Signature Verification for Auto-Updates.**
|
|
The current checksum-only approach allows any attacker capable of intercepting the WebSocket or API traffic (e.g., via a compromised relay or DNS) to gain SYSTEM-level code execution on the entire agent fleet by pushing a "mandatory" update.
|