From f87657fbfbe3072d145fbdf1a3709554cc25d1f7 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 12 May 2026 14:22:20 -0500 Subject: [PATCH] =?UTF-8?q?fix:=20M20,=20M21,=20M22,=20L14=20=E2=80=94=20a?= =?UTF-8?q?rchive=20safety,=20ETS=20protection,=20impersonation=20expiry,?= =?UTF-8?q?=20check=20pagination?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - M20: Add check_no_hardlinks and check_no_special_files to MIB upload archive extraction to prevent hard link, device node, and FIFO attacks - M21: Change RateLimit ETS table from :public to :protected; route hit/get/reset through GenServer.call instead of direct ETS access - M22: Add 8-hour impersonation timeout — store impersonated_at in session and auto-revoke impersonation when expired - L14: Add default limit (500) and optional offset to list_checks for pagination --- bugs.md | 42 ------------ lib/towerops/monitoring.ex | 15 +++++ lib/towerops/rate_limit.ex | 66 ++++++++++++------- .../controllers/api/v1/mib_controller.ex | 32 +++++++++ lib/towerops_web/user_auth.ex | 11 +++- test/towerops/rate_limit_test.exs | 6 +- test/towerops_web/user_auth_test.exs | 3 +- 7 files changed, 106 insertions(+), 69 deletions(-) diff --git a/bugs.md b/bugs.md index 5cb61a46..8062c253 100644 --- a/bugs.md +++ b/bugs.md @@ -92,41 +92,8 @@ --- -### M20. MIB Upload — Hard Links Not Checked in Archive Extraction -**File:** `lib/towerops_web/controllers/api/v1/mib_controller.ex:453-458` -**Severity:** MEDIUM — Hard-linked files in archive could overwrite system files - -**Description:** `check_no_symlinks` only checks symlinks. Hard links, device nodes, and fifos are not inspected. - -**Fix:** Add `check_no_hardlinks(paths)` and `check_no_special_files(paths)` using `File.stat`. - ---- - -### M21. ETS Rate Limit Table Is Public - -**File:** `lib/towerops/rate_limit.ex:156-164` - -**Severity:** MEDIUM — Any Erlang/Elixir process can manipulate rate limit counters - -**Description:** `:ets.new(table, [:named_table, :set, :public, ...])` — `:public` allows any process to read/write counters. - -**Fix:** Use `:protected` and have GenServer expose `hit/3` via `call`. - ---- - -### M22. Impersonation Session Has No Expiry - -**File:** `lib/towerops_web/user_auth.ex:879-910` - -**Severity:** MEDIUM — Superuser impersonation persists indefinitely - -**Description:** If a superuser starts impersonating and walks away, the session continues impersonating until manually stopped. - -**Fix:** Tie impersonation to sudo mode or add dedicated `expires_at`. - ---- ## LOW @@ -158,15 +125,6 @@ -### L14. `list_checks` No Pagination - -**File:** `lib/towerops/monitoring.ex:24-32` - -**Severity:** LOW — No limit on check listing for orgs with thousands of checks - -**Fix:** Add default limit and pagination support. - ---- ### L15. OpenWeatherMap API Key in URL Query Param diff --git a/lib/towerops/monitoring.ex b/lib/towerops/monitoring.ex index 2831fff5..c9282f34 100644 --- a/lib/towerops/monitoring.ex +++ b/lib/towerops/monitoring.ex @@ -20,6 +20,13 @@ defmodule Towerops.Monitoring do @doc """ Returns the list of checks for an organization. + + Options: + - `:device_id` — filter by device + - `:check_type` — filter by check type + - `:enabled` — filter by enabled status + - `:limit` — max results to return (default: 500) + - `:offset` — offset for pagination """ def list_checks(organization_id, opts \\ []) do organization_id @@ -28,9 +35,17 @@ defmodule Towerops.Monitoring do |> maybe_filter_by_device(opts[:device_id]) |> maybe_filter_by_check_type(opts[:check_type]) |> maybe_filter_by_enabled(opts[:enabled]) + |> maybe_limit(opts[:limit]) + |> maybe_offset(opts[:offset]) |> Repo.all() end + defp maybe_limit(query, nil), do: limit(query, ^500) + defp maybe_limit(query, n) when is_integer(n) and n > 0, do: limit(query, ^n) + + defp maybe_offset(query, nil), do: query + defp maybe_offset(query, n) when is_integer(n) and n >= 0, do: offset(query, ^n) + defp maybe_filter_by_device(query, nil), do: query defp maybe_filter_by_device(query, device_id), do: CheckQuery.for_device(query, device_id) diff --git a/lib/towerops/rate_limit.ex b/lib/towerops/rate_limit.ex index 5f90c9c6..43b5b286 100644 --- a/lib/towerops/rate_limit.ex +++ b/lib/towerops/rate_limit.ex @@ -106,18 +106,7 @@ defmodule Towerops.RateLimit do def hit(table, key, scale, limit, increment) when is_atom(table) and is_integer(scale) and scale > 0 and is_integer(limit) and limit > 0 and is_integer(increment) and increment >= 0 do - now = now_ms() - window = div(now, scale) - expires_at = (window + 1) * scale - full_key = {key, window} - - count = :ets.update_counter(table, full_key, increment, {full_key, 0, expires_at}) - - if count <= limit do - {:allow, count} - else - {:deny, expires_at - now} - end + GenServer.call(table, {:hit, key, scale, limit, increment}) end @doc """ @@ -126,12 +115,7 @@ defmodule Towerops.RateLimit do """ @spec get(atom(), key(), scale_ms()) :: count() def get(table \\ @default_table, key, scale) when is_integer(scale) and scale > 0 do - window = div(now_ms(), scale) - - case :ets.lookup(table, {key, window}) do - [{_full_key, count, _expires_at}] -> count - [] -> 0 - end + GenServer.call(table, {:get, key, scale}) end @doc """ @@ -139,9 +123,7 @@ defmodule Towerops.RateLimit do """ @spec reset(atom(), key(), scale_ms()) :: :ok def reset(table \\ @default_table, key, scale) when is_integer(scale) and scale > 0 do - window = div(now_ms(), scale) - :ets.delete(table, {key, window}) - :ok + GenServer.call(table, {:reset, key, scale}) end # --------------------------------------------------------------------------- @@ -157,7 +139,7 @@ defmodule Towerops.RateLimit do :ets.new(table, [ :named_table, :set, - :public, + :protected, {:read_concurrency, true}, {:write_concurrency, true}, {:decentralized_counters, true} @@ -167,6 +149,46 @@ defmodule Towerops.RateLimit do {:ok, %{table: table, clean_period: clean_period}} end + @impl GenServer + def handle_call({:hit, key, scale, limit, increment}, _from, state) do + now = now_ms() + window = div(now, scale) + expires_at = (window + 1) * scale + full_key = {key, window} + + count = + :ets.update_counter(state.table, full_key, increment, {full_key, 0, expires_at}) + + result = + if count <= limit do + {:allow, count} + else + {:deny, expires_at - now} + end + + {:reply, result, state} + end + + @impl GenServer + def handle_call({:get, key, scale}, _from, state) do + window = div(now_ms(), scale) + + count = + case :ets.lookup(state.table, {key, window}) do + [{_full_key, c, _expires_at}] -> c + [] -> 0 + end + + {:reply, count, state} + end + + @impl GenServer + def handle_call({:reset, key, scale}, _from, state) do + window = div(now_ms(), scale) + :ets.delete(state.table, {key, window}) + {:reply, :ok, state} + end + @impl GenServer def handle_info(:clean, state) do sweep(state.table) diff --git a/lib/towerops_web/controllers/api/v1/mib_controller.ex b/lib/towerops_web/controllers/api/v1/mib_controller.ex index 184740b8..b5ddf0cc 100644 --- a/lib/towerops_web/controllers/api/v1/mib_controller.ex +++ b/lib/towerops_web/controllers/api/v1/mib_controller.ex @@ -441,6 +441,8 @@ defmodule ToweropsWeb.Api.V1.MibController do all_paths = list_all_entries(extract_dir) with :ok <- check_no_symlinks(all_paths), + :ok <- check_no_hardlinks(all_paths), + :ok <- check_no_special_files(all_paths), :ok <- check_no_traversal(all_paths, canonical_extract_dir) do check_file_limits(all_paths) end @@ -462,6 +464,36 @@ defmodule ToweropsWeb.Api.V1.MibController do match?({:ok, %{type: :symlink}}, File.lstat(path)) end + defp check_no_hardlinks(paths) do + if Enum.any?(paths, &hardlink?/1) do + {:error, "Archive contains hard links, which are not allowed"} + else + :ok + end + end + + defp hardlink?(path) do + case File.lstat(path) do + {:ok, %{nlink: nlink}} -> nlink > 1 + _ -> false + end + end + + defp check_no_special_files(paths) do + if Enum.any?(paths, &special_file?/1) do + {:error, "Archive contains device nodes, FIFOs, or other special files"} + else + :ok + end + end + + defp special_file?(path) do + case File.lstat(path) do + {:ok, stat} -> stat.type not in [:regular, :directory, :symlink] + _ -> false + end + end + defp check_no_traversal(paths, canonical_base) do if Enum.all?(paths, fn path -> path |> Path.expand() |> String.starts_with?(canonical_base) diff --git a/lib/towerops_web/user_auth.ex b/lib/towerops_web/user_auth.ex index ddadb8da..afa833f8 100644 --- a/lib/towerops_web/user_auth.ex +++ b/lib/towerops_web/user_auth.ex @@ -42,6 +42,7 @@ defmodule ToweropsWeb.UserAuth do # token. This can be set to a value greater than `@max_cookie_age_in_days` to disable # the reissuing of tokens completely. @session_reissue_age_in_days 7 + @impersonation_timeout_hours 8 @doc """ Logs the user in. @@ -881,14 +882,21 @@ defmodule ToweropsWeb.UserAuth do defp build_impersonation_scope(session) do superuser_id = session["superuser_id"] target_user_id = session["target_user_id"] + impersonated_at = session["impersonated_at"] - if superuser_id && target_user_id do + if superuser_id && target_user_id && impersonation_active?(impersonated_at) do fetch_impersonation_users(superuser_id, target_user_id) else Scope.for_user(nil) end end + defp impersonation_active?(nil), do: false + + defp impersonation_active?(impersonated_at) do + DateTime.diff(DateTime.utc_now(), impersonated_at, :hour) < @impersonation_timeout_hours + end + defp fetch_impersonation_users(superuser_id, target_user_id) do with superuser when not is_nil(superuser) <- Accounts.get_user(superuser_id), target_user when not is_nil(target_user) <- Accounts.get_user(target_user_id) do @@ -945,6 +953,7 @@ defmodule ToweropsWeb.UserAuth do |> put_session(:superuser_id, superuser.id) |> put_session(:target_user_id, target_user.id) |> put_session(:impersonating, true) + |> put_session(:impersonated_at, DateTime.utc_now()) |> assign(:current_scope, Scope.for_impersonation(superuser, target_user)) |> maybe_set_default_organization(target_user) |> put_flash(:info, t_admin("Now impersonating %{email}", email: target_user.email)) diff --git a/test/towerops/rate_limit_test.exs b/test/towerops/rate_limit_test.exs index 620aa55a..f68fb09b 100644 --- a/test/towerops/rate_limit_test.exs +++ b/test/towerops/rate_limit_test.exs @@ -13,7 +13,7 @@ defmodule Towerops.RateLimitTest do start_supervised!( {RateLimit, [ - name: :"rl_proc_#{System.unique_integer([:positive])}", + name: table, table: table, clean_period: 60_000 ]} @@ -164,7 +164,7 @@ defmodule Towerops.RateLimitTest do start_supervised!( {RateLimit, [ - name: :"rl_auto_proc_#{System.unique_integer([:positive])}", + name: table, table: table, clean_period: 30 ]}, @@ -213,7 +213,7 @@ defmodule Towerops.RateLimitTest do # Defensive: the public API does not validate, but callers pass positive # ints. We document this contract by exercising a positive-only call. table = :"rl_validate_#{System.unique_integer([:positive])}" - _pid = start_supervised!({RateLimit, [name: :rl_validate_proc, table: table]}, id: :validate) + _pid = start_supervised!({RateLimit, [name: table, table: table]}, id: :validate) assert {:allow, 1} = RateLimit.hit(table, "ok", 1, 1) end diff --git a/test/towerops_web/user_auth_test.exs b/test/towerops_web/user_auth_test.exs index fc60ed7d..472d7d60 100644 --- a/test/towerops_web/user_auth_test.exs +++ b/test/towerops_web/user_auth_test.exs @@ -1258,7 +1258,8 @@ defmodule ToweropsWeb.UserAuthTest do %{ "impersonating" => true, "superuser_id" => superuser.id, - "target_user_id" => target_user.id + "target_user_id" => target_user.id, + "impersonated_at" => DateTime.utc_now() }, socket )