From a3b97f9e98ac9097947422fcf8d574ff4e64fbf3 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Mon, 11 May 2026 19:04:16 -0500 Subject: [PATCH] fix(security): mass-assignment on owner edit + 3 minor hardening fixes - Radio.normalize_proposed/1 now Map.take/2 the allowed-edit keys before normalization. Owner / admin / pending-edit submit paths all funnel through this boundary, so a crafted form can no longer mass-assign user_id, flagged_invalid, inserted_at, etc. on a contact via String.to_existing_atom -> Ecto.Changeset.change. - Accounts.revoke_api_token/2 rescues Ecto.Query.CastError and returns {:error, :not_found} so a malformed UUID in DELETE /api/v1/me/api-tokens/:id renders the API's clean 404 problem+json instead of a 500. - ProfilesFile.read_etf decodes with :erlang.binary_to_term(bin, [:safe]). Defense-in-depth against tampered on-disk profiles (atom-table exhaustion via untrusted ETF). - Regression tests: contact_edit_test.exs covers the rejected mass-assignment fields; accounts_api_token_test.exs covers the malformed-UUID path. --- bugs.md | 61 +++++++++++++++++++ lib/microwaveprop/accounts.ex | 4 ++ .../propagation/profiles_file.ex | 6 +- lib/microwaveprop/radio.ex | 11 ++++ .../microwaveprop/accounts_api_token_test.exs | 4 ++ .../microwaveprop/radio/contact_edit_test.exs | 31 ++++++++++ 6 files changed, 116 insertions(+), 1 deletion(-) diff --git a/bugs.md b/bugs.md index 02030925..f858df07 100644 --- a/bugs.md +++ b/bugs.md @@ -26,3 +26,64 @@ inside the LiveView process and crashed the channel. existing `parse_band_param/1` plus a small `normalize_band_event/1` helper (MapLive). Unknown bands now fall back to a safe default or a no-op instead of crashing the process. + +## B. Mass assignment on `Radio.apply_owner_edit/3` + +**Status:** Fixed. +**Severity:** High +**Category:** OWASP A04 Insecure Design / mass assignment +**Files:** +- `lib/microwaveprop/radio.ex` +- `lib/microwaveprop_web/live/contact_live/show.ex` + +`apply_owner_edit` and `apply_admin_edit` accept the LiveView form's +`params` map, run it through `normalize_proposed/1`, then `diff_against_contact/2`, +then `apply_edit_to_contact/2`. The final stage calls +`String.to_existing_atom(key)` for every key in the diff and pipes the +resulting `{atom, value}` pairs into `Ecto.Changeset.change(contact, …)`. + +`normalize_proposed/1` did not whitelist keys, so an owner could craft +a form POST with arbitrary fields — `user_id`, `flagged_invalid`, +`inserted_at`, etc. — and have them persisted onto their own contact. + +**Fix:** Tighten `Radio.normalize_proposed/1` to `Map.take/2` only the +known editable keys (`station1/2`, `grid1/2`, `band`, `mode`, +`qso_timestamp`, `height1/2_ft`, `private`) before any normalization / +diffing. All three call sites (owner edit, admin edit, pending edit) +share this boundary, so no other change was needed. Regression test +in `contact_edit_test.exs`. + +## C. `Accounts.revoke_api_token/2` raised `Ecto.Query.CastError` on a malformed UUID + +**Status:** Fixed. +**Severity:** Low +**Category:** Availability / API contract + +`Repo.get_by(UserApiToken, id: token_id, …)` raises `Ecto.Query.CastError` +when `token_id` does not parse as a UUID. Phoenix.Ecto's +`Plug.Exception` impl converts this to a 400 in the browser pipeline, +but the API path (`DELETE /api/v1/me/api-tokens/:id`) bypasses the +Plug.Exception fallback for actions that already return tagged tuples, +so the malformed-UUID path leaked a 500 with a generic error body +instead of the API's structured `:not_found` problem+json. + +**Fix:** Rescue `Ecto.Query.CastError` in `revoke_api_token/2` and +return `{:error, :not_found}` — same shape that propagates as a clean +404 through the existing API fallback. Regression test in +`accounts_api_token_test.exs`. + +## D. `:erlang.binary_to_term/1` on disk-loaded propagation profiles + +**Status:** Fixed (defense-in-depth — pre-existing low risk). +**Severity:** Low +**Category:** OWASP A08 Software Integrity Failures +**File:** `lib/microwaveprop/propagation/profiles_file.ex` + +`read_etf/1` decoded `.etf` files from `/data/profiles/...` with +`:erlang.binary_to_term(binary)` (no `:safe` option). Files are written +by the propagation pipeline, so the realistic threat is filesystem +tampering — by which point the box is already compromised. Still, the +unsafe variant allows atom-table exhaustion DoS. + +**Fix:** Pass `[:safe]` so unknown atoms and external function +references are rejected on decode. diff --git a/lib/microwaveprop/accounts.ex b/lib/microwaveprop/accounts.ex index 6a535288..fc8cc9b2 100644 --- a/lib/microwaveprop/accounts.ex +++ b/lib/microwaveprop/accounts.ex @@ -517,6 +517,10 @@ defmodule Microwaveprop.Accounts do %UserApiToken{} = token -> {:ok, token} end + rescue + # Malformed UUID in the URL — treat as a clean 404 instead of + # bubbling a 500 out of `Repo.get_by`'s UUID cast. + Ecto.Query.CastError -> {:error, :not_found} end ## Token helper diff --git a/lib/microwaveprop/propagation/profiles_file.ex b/lib/microwaveprop/propagation/profiles_file.ex index 17774ad1..241dcd59 100644 --- a/lib/microwaveprop/propagation/profiles_file.ex +++ b/lib/microwaveprop/propagation/profiles_file.ex @@ -159,7 +159,11 @@ defmodule Microwaveprop.Propagation.ProfilesFile do defp read_etf(valid_time) do case File.read(path_for(valid_time)) do - {:ok, binary} -> {:ok, :erlang.binary_to_term(binary)} + # `:safe` rejects unknown atoms and external function refs in the + # decoded term — defense-in-depth in case the on-disk file is ever + # tampered with (it is owned by the prop pipeline, but the cost is + # zero and the failure mode without :safe is atom-table exhaustion). + {:ok, binary} -> {:ok, :erlang.binary_to_term(binary, [:safe])} {:error, _} -> {:error, :enoent} end end diff --git a/lib/microwaveprop/radio.ex b/lib/microwaveprop/radio.ex index 3440104f..a53aa03a 100644 --- a/lib/microwaveprop/radio.ex +++ b/lib/microwaveprop/radio.ex @@ -856,10 +856,21 @@ defmodule Microwaveprop.Radio do |> Repo.insert() end + # Whitelist of keys an owner / pending-edit submitter is allowed + # to propose. Anything not in this set is dropped before the + # changes hit `String.to_existing_atom` + `Ecto.Changeset.change` + # downstream — without this filter a crafted form could mass-assign + # `user_id`, `flagged_invalid`, `inserted_at`, etc. on a contact the + # caller owns. `band` is in the list because admins edit it through + # this same path; `ContactEdit.changeset` rejects `band` for + # non-admin pending edits via its own validation. + @editable_proposed_keys ~w(station1 station2 grid1 grid2 band mode qso_timestamp height1_ft height2_ft private) + @doc false @spec normalize_proposed(map()) :: map() def normalize_proposed(changes) do changes + |> Map.take(@editable_proposed_keys) |> normalize_string_field("station1") |> normalize_string_field("station2") |> normalize_string_field("grid1") diff --git a/test/microwaveprop/accounts_api_token_test.exs b/test/microwaveprop/accounts_api_token_test.exs index f92b35b6..396e09db 100644 --- a/test/microwaveprop/accounts_api_token_test.exs +++ b/test/microwaveprop/accounts_api_token_test.exs @@ -110,5 +110,9 @@ defmodule Microwaveprop.AccountsApiTokenTest do assert {:error, :not_found} = Accounts.revoke_api_token(other, record.id) end + + test "returns :not_found for a malformed UUID", %{user: user} do + assert {:error, :not_found} = Accounts.revoke_api_token(user, "not-a-uuid") + end end end diff --git a/test/microwaveprop/radio/contact_edit_test.exs b/test/microwaveprop/radio/contact_edit_test.exs index 33415675..629e56ba 100644 --- a/test/microwaveprop/radio/contact_edit_test.exs +++ b/test/microwaveprop/radio/contact_edit_test.exs @@ -451,6 +451,37 @@ defmodule Microwaveprop.Radio.ContactEditTest do Radio.apply_owner_edit(contact, user, %{"qso_timestamp" => "not a real date"}) end + test "ignores non-editable keys instead of mass-assigning them", %{user: user} do + # Crafted form posting `user_id` / `flagged_invalid` / `inserted_at` + # alongside an allowed field. Only the allowed field should land + # on the contact; the rest must be silently dropped. + contact = owned_contact(user) + original_user_id = contact.user_id + original_inserted_at = contact.inserted_at + + {:ok, other} = + Accounts.register_user(%{ + email: "evil-edit-#{System.unique_integer([:positive])}@example.com", + password: "passwordpassword", + callsign: "K0EVL", + name: "Evil" + }) + + params = %{ + "grid1" => "EM15ab", + "user_id" => other.id, + "flagged_invalid" => true, + "inserted_at" => "2000-01-01T00:00:00Z" + } + + assert {:ok, updated} = Radio.apply_owner_edit(contact, user, params) + + assert updated.grid1 == "EM15AB" + assert updated.user_id == original_user_id + assert updated.flagged_invalid == false + assert updated.inserted_at == original_inserted_at + end + test "unchanged private checkbox is treated as no-op", %{user: user} do contact = owned_contact(user) # Schema default is `private: false`; the form always submits the