From cec8eabcd83f019ffa32810b4c9999136f922db6 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 12 May 2026 12:54:48 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20M13,=20M14,=20M15,=20M18=20=E2=80=94=20d?= =?UTF-8?q?ownload=20safety,=20queue=20blocking,=20PubSub=20chunking,=20pr?= =?UTF-8?q?otobuf=20validation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - M18: Add validate_snmp_device/1 with 255-byte limits on community, v3_auth_password, and v3_priv_password to prevent memory exhaustion - M13: Add client-side mime_type allowlist to phx:download handler - M14: Replace Process.sleep loop in WeatherSyncWorker with Task.async_stream to avoid blocking the Oban queue slot - M15: Chunk device list in CloudLatencyProbeWorker PubSub broadcasts (100 devices per message) to prevent megabyte-sized messages --- assets/js/app.ts | 15 ++++ bugs.md | 72 ------------------- lib/towerops/proto/decode.ex | 11 ++- .../workers/cloud_latency_probe_worker.ex | 9 ++- lib/towerops/workers/weather_sync_worker.ex | 22 ++++-- 5 files changed, 50 insertions(+), 79 deletions(-) diff --git a/assets/js/app.ts b/assets/js/app.ts index 7b75cc0e..a1f79299 100644 --- a/assets/js/app.ts +++ b/assets/js/app.ts @@ -380,10 +380,25 @@ window.addEventListener("phx:copy", (event: any) => { if (text) navigator.clipboard.writeText(text).catch(err => console.error('clipboard:', err)) }) +const ALLOWED_MIME_TYPES = new Set([ + "text/csv", + "text/plain", + "application/json", + "application/pdf", + "application/octet-stream", + "application/vnd.ms-excel", + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", +]) + // Handle file download events window.addEventListener("phx:download", (event: any) => { const { content, filename, mime_type } = event.detail + if (!ALLOWED_MIME_TYPES.has(mime_type)) { + console.error(`phx:download blocked unsafe mime_type: "${mime_type}"`) + return + } + // Create a blob from the content const blob = new Blob([content], { type: mime_type }) diff --git a/bugs.md b/bugs.md index 36d26aec..79227f32 100644 --- a/bugs.md +++ b/bugs.md @@ -99,53 +99,9 @@ -### M13. CSV/Download Content-Type Validation Missing -**File:** `assets/js/app.ts:374-393` -**Severity:** MEDIUM — Downloaded HTML file could execute scripts -**Description:** The `phx:download` handler accepts any `mime_type` from server. If server ever allows user content to influence mime_type, an HTML download with scripts could be created. - -**Fix:** Add client-side `mime_type` allowlist. - ---- - -### M14. WeatherSyncWorker Blocks Queue with `Process.sleep` - -**File:** `lib/towerops/workers/weather_sync_worker.ex:46-53` - -**Severity:** MEDIUM — Worker holds queue slot for 110s+ for 100 sites - -**Description:** `Process.sleep(1_100)` between each site fetch blocks the Oban queue slot. - -**Fix:** Use `Task.async_stream` with `max_concurrency: 1` and `timeout`, or dispatch per-site jobs. - ---- - -### M15. CloudLatencyProbeWorker Unbounded PubSub Message - -**File:** `lib/towerops/workers/cloud_latency_probe_worker.ex:39-43` - -**Severity:** MEDIUM — Megabyte-sized PubSub messages with many devices - -**Description:** Full device list broadcast as single PubSub message. With thousands of devices, this can overwhelm PubSub. - -**Fix:** Chunk the device list or send IDs that agents resolve independently. - ---- - -### M18. No String Length Validation for SNMP Credentials in Protobuf - -**File:** `lib/towerops/proto/decode.ex:1509-1593` - -**Severity:** MEDIUM — Memory exhaustion via oversized credential strings - -**Description:** `decode_snmp_device` does NOT call `validate_string` on `community`, `v3_auth_password`, `v3_priv_password`. An attacker could send multi-megabyte credential strings. - -**Fix:** Add `validate_string` with reasonable max lengths (255 bytes). - ---- ### M19. Missing Validation on Multiple Protobuf Decode Paths @@ -296,31 +252,3 @@ **Severity:** LOW — If two nodes share a node name, ETS table creation could conflict **Fix:** Use unique ETS names incorporating node identifier. - ---- - -## SUMMARY - -### By Severity - -| Severity | Count | Key Areas | -|----------|-------|-----------| -| CRITICAL | 13 | XSS (maps, status page), IDOR (consent, members, reports), auth bypass (GDPR halt, webhook), credential exposure (k8s/secrets.yaml, Stripe in git, MikroTik over PubSub), runtime crash (Repo.all_by), dead code (brute force), SSL (no force_ssl), race condition (device quota) | -| HIGH | 30 | N+1 queries (mobile, Oban, alerts, capacity, latency), race conditions (create_check, apply_agent), injection (Search wildcard, OID, user input → atom/integer), IDOR (reports, checks), auth gaps (webhooks, sessions), CSP/HSTS, hardcoded salts, cookie flags, missing validation (protobuf, check creation), worker reliability (sync errors swallowed, weather never starts, unbounded processing) | -| MEDIUM | 23 | IDOR (resource scoping), email case-sensitivity, open redirect, Stripe webhook not enforced, rate limiting key, overfetching, missing unique constraint, error disclosure, pattern match crashes, admin defense-in-depth, download content-type, queue blocking, PubSub unbounded, memory leaks (MutationObserver), MD5 default, incomplete archive validation, public ETS, impersonation expiry, silent permission failures | -| LOW | 16 | Log leaks, TTL extension, IP normalization, config documentation, vault dev/prod bleed, version disclosure, device capacity scoping, insights IDOR, code duplication, listener cleanup, path validation, liveSocket global, broad changesets, pagination, API key in URL, ETS naming | - -**Total: 82 issues** (13 Critical, 30 High, 23 Medium, 16 Low) - -### Top 10 Priority Fixes - -1. **C1** — Stored XSS in Sites Map popup -2. **C2** — IDOR in policy consent handler (client-controlled user_id) -3. **C3** — Broken `halt()` in GDPR data export -4. **C6** — `Repo.all_by/2` runtime crash on password/email changes -5. **C9** — Remove `k8s/secrets.yaml` with live production credentials -6. **C10** — Remove hardcoded Stripe test key from source -7. **C8** — Sanitize `custom_css` in status page to prevent HTML injection -8. **C11** — Enable `force_ssl` in production -9. **H7** — Add auth plug to webhook pipeline -10. **H17** — Fix missing org scope on individual report operations diff --git a/lib/towerops/proto/decode.ex b/lib/towerops/proto/decode.ex index d4e71cba..c5928f85 100644 --- a/lib/towerops/proto/decode.ex +++ b/lib/towerops/proto/decode.ex @@ -1476,7 +1476,8 @@ defmodule Towerops.Proto.Decode do 4 -> with {:ok, {msg_data, rest2}} <- Wire.decode_bytes(rest), - {:ok, snmp_device} <- decode_snmp_device(msg_data) do + {:ok, snmp_device} <- decode_snmp_device(msg_data), + {:ok, snmp_device} <- validate_snmp_device(snmp_device) do decode_agent_job_fields(rest2, %{acc | snmp_device: snmp_device}) end @@ -1832,6 +1833,14 @@ defmodule Towerops.Proto.Decode do end end + defp validate_snmp_device(d) do + with :ok <- validate_string("community", d.community, @max_short_string_length), + :ok <- validate_string("v3_auth_password", d.v3_auth_password, @max_short_string_length), + :ok <- validate_string("v3_priv_password", d.v3_priv_password, @max_short_string_length) do + {:ok, d} + end + end + defp validate_snmp_result(sr) do with :ok <- validate_uuid("device_id", sr.device_id), :ok <- validate_map_size("oid_values", sr.oid_values, @max_oid_values), diff --git a/lib/towerops/workers/cloud_latency_probe_worker.ex b/lib/towerops/workers/cloud_latency_probe_worker.ex index 383337cc..bbc37532 100644 --- a/lib/towerops/workers/cloud_latency_probe_worker.ex +++ b/lib/towerops/workers/cloud_latency_probe_worker.ex @@ -28,19 +28,24 @@ defmodule Towerops.Workers.CloudLatencyProbeWorker do end end + @chunk_size 100 + defp dispatch_probes(cloud_pollers, devices) do Logger.info( "Cloud latency probe: dispatching to #{length(cloud_pollers)} poller(s) " <> "for #{length(devices)} device(s)" ) + device_chunks = Enum.chunk_every(devices, @chunk_size) + for poller <- cloud_pollers do - _ = + for chunk <- device_chunks do Phoenix.PubSub.broadcast( Towerops.PubSub, "agent:#{poller.id}:latency_probe", - {:latency_probe_jobs, devices} + {:latency_probe_jobs, chunk} ) + end end :ok diff --git a/lib/towerops/workers/weather_sync_worker.ex b/lib/towerops/workers/weather_sync_worker.ex index 85c26349..aaf1f89b 100644 --- a/lib/towerops/workers/weather_sync_worker.ex +++ b/lib/towerops/workers/weather_sync_worker.ex @@ -45,10 +45,24 @@ defmodule Towerops.Workers.WeatherSyncWorker do defp fetch_all_sites(sites) do sites - |> Enum.with_index(1) - |> Enum.map(fn {site, idx} -> - if idx > 1, do: Process.sleep(1_100) - fetch_site_weather(site) + |> Task.async_stream( + &fetch_site_weather(&1), + max_concurrency: 1, + timeout: 30_000, + ordered: false, + on_timeout: :kill_task + ) + |> Enum.map(fn + {:ok, result} -> + result + + {:exit, reason} -> + Logger.warning("Weather fetch task exited: #{inspect(reason)}") + :error + + {:error, reason} -> + Logger.warning("Weather fetch task failed: #{inspect(reason)}") + :error end) end