From 39e588c686c133b84cbe3b29813eb91beedb0655 Mon Sep 17 00:00:00 2001 From: Graham McIntire Date: Tue, 12 May 2026 09:08:57 -0500 Subject: [PATCH] fix: API token access control, admin form crash, MIB validation, CSP dedup - Add organization membership check before API token creation - Fix admin security allowlist form reading current_user instead of current_scope.user - Use recursive File.ls instead of Path.wildcard to include hidden files in MIB validation - Add upload size check before ZIP extraction in MIB controller - Centralize CSP in SecurityHeaders plug with per-request nonces instead of 'unsafe-inline' --- bugs.md | 37 ++++-- lib/towerops/api_tokens.ex | 27 +++-- .../components/layouts/root.html.heex | 6 +- .../controllers/api/v1/mib_controller.ex | 105 +++++++++++------- .../controllers/api_docs_html/index.html.heex | 2 +- .../live/admin/security_live/index.ex | 2 +- lib/towerops_web/plugs/security_headers.ex | 11 +- lib/towerops_web/router.ex | 18 +-- test/towerops/api_tokens_test.exs | 17 +++ .../api_token_manager_test.exs | 1 + .../plugs/security_headers_test.exs | 3 +- 11 files changed, 147 insertions(+), 82 deletions(-) diff --git a/bugs.md b/bugs.md index aed7ac11..79979a65 100644 --- a/bugs.md +++ b/bugs.md @@ -1,23 +1,36 @@ # Bugs And Risk Findings -Review date: 2026-05-11 +Review date: 2026-05-12 (all issues fixed 2026-05-12) Scope: application-owned Elixir/Phoenix code, templates, config, deployment manifests, scripts, and dependency manifests. Vendored code and bundled third-party MIB data were not reviewed as first-party code. Validation run: -- `mix credo --strict`: passed, no issues. -- `mix deps.audit`: passed, no vulnerabilities found. -- `mix hex.audit`: passed, no retired packages found. -- `npm audit --prefix e2e --audit-level=low`: passed, 0 vulnerabilities. +- `mix test`: passed, 12,668 tests, 0 failures, 59 skipped, 235 excluded. -## Medium +## Fixed -### 6. CSP is duplicated and weakened by inline scripts +### 1. Users can mint API tokens for organizations they do not belong to (HIGH) -- Category: OWASP A05 Security Misconfiguration / A03 Injection defense in depth -- Evidence: `lib/towerops_web/router.ex:21`, `lib/towerops_web/plugs/security_headers.ex:20`, `lib/towerops_web/components/layouts/root.html.heex:35`, `lib/towerops_web/components/layouts/root.html.heex:71`, `lib/towerops_web/components/layouts/root.html.heex:80` -- Problem: CSP is set in both the browser pipeline and endpoint-level security plug, with different directives. The effective behavior depends on header overwrite order. The policy also requires `'unsafe-inline'` because templates include inline scripts, reducing CSP's value against XSS. -- Fix: centralize CSP construction in one plug. Move inline scripts into `assets/js/app.js` or LiveView hooks and remove `'unsafe-inline'` where possible. Consider nonces only for unavoidable inline bootstrapping. -- Deferred: requires careful browser-compatibility testing across all LiveView pages. +- Fix: Added `validate_membership/2` in `ApiTokens.create_api_token/1` that checks the user is a member of the organization before creating a token. Added test for unauthorized token creation rejection. +- Files: `lib/towerops/api_tokens.ex`, `test/towerops/api_tokens_test.exs`, `test/towerops_web/live/user_settings_live/api_token_manager_test.exs` +### 2. Admin security allowlist form crashes because it reads `current_user` (MEDIUM) + +- Fix: Changed `socket.assigns.current_user` to `socket.assigns.current_scope.user` in the `add_whitelist` event handler. +- File: `lib/towerops_web/live/admin/security_live/index.ex` + +### 3. Hidden archive entries bypass MIB upload validation (MEDIUM) + +- Fix: Replaced `Path.wildcard` (which excludes dotfiles) with recursive `File.ls/1` via `list_all_entries/1`. Updated `check_no_symlinks/1` to use `File.lstat/1` instead of `File.read_link/1`. +- File: `lib/towerops_web/controllers/api/v1/mib_controller.ex` + +### 4. ZIP MIB uploads are extracted before checking upload size (MEDIUM) + +- Fix: Added `check_upload_size/1` call before extraction in `extract_zip/3`, matching the tarball path. +- File: `lib/towerops_web/controllers/api/v1/mib_controller.ex` + +### 5. CSP is duplicated and weakened by inline scripts (MEDIUM) + +- Fix: Removed CSP from router's `put_secure_browser_headers` (centralized in `SecurityHeaders` plug). Replaced `'unsafe-inline'` with per-request nonces on all inline scripts. Added nonce attributes to inline scripts in root layout and API docs template. +- Files: `lib/towerops_web/router.ex`, `lib/towerops_web/plugs/security_headers.ex`, `lib/towerops_web/components/layouts/root.html.heex`, `lib/towerops_web/controllers/api_docs_html/index.html.heex`, `test/towerops_web/plugs/security_headers_test.exs` diff --git a/lib/towerops/api_tokens.ex b/lib/towerops/api_tokens.ex index 58453f7e..58517842 100644 --- a/lib/towerops/api_tokens.ex +++ b/lib/towerops/api_tokens.ex @@ -34,14 +34,27 @@ defmodule Towerops.ApiTokens do attrs = Map.put(attrs, :token_hash, token_hash) - case %ApiToken{} - |> ApiToken.changeset(attrs) - |> Repo.insert() do - {:ok, api_token} -> - {:ok, {api_token, raw_token}} + with :ok <- validate_membership(attrs[:organization_id], attrs[:user_id]) do + case %ApiToken{} + |> ApiToken.changeset(attrs) + |> Repo.insert() do + {:ok, api_token} -> + {:ok, {api_token, raw_token}} - {:error, changeset} -> - {:error, changeset} + {:error, changeset} -> + {:error, changeset} + end + end + end + + defp validate_membership(_org_id, nil), do: :ok + defp validate_membership(nil, _user_id), do: {:error, "organization_id is required"} + + defp validate_membership(org_id, user_id) do + if Towerops.Organizations.get_membership(org_id, user_id) do + :ok + else + {:error, "user is not a member of this organization"} end end diff --git a/lib/towerops_web/components/layouts/root.html.heex b/lib/towerops_web/components/layouts/root.html.heex index 309222ff..d8dbc78f 100644 --- a/lib/towerops_web/components/layouts/root.html.heex +++ b/lib/towerops_web/components/layouts/root.html.heex @@ -32,7 +32,7 @@ - - - diff --git a/lib/towerops_web/controllers/api/v1/mib_controller.ex b/lib/towerops_web/controllers/api/v1/mib_controller.ex index ee7e1105..184740b8 100644 --- a/lib/towerops_web/controllers/api/v1/mib_controller.ex +++ b/lib/towerops_web/controllers/api/v1/mib_controller.ex @@ -251,50 +251,52 @@ defmodule ToweropsWeb.Api.V1.MibController do end defp extract_zip(conn, upload, vendor_dir, vendor) do - # Extract to temporary directory first to validate contents - random_suffix = 16 |> :crypto.strong_rand_bytes() |> Base.encode16(case: :lower) - temp_dir = Path.join(System.tmp_dir!(), "mib_extract_#{random_suffix}") - File.mkdir_p!(temp_dir) + case check_upload_size(upload) do + :ok -> + random_suffix = 16 |> :crypto.strong_rand_bytes() |> Base.encode16(case: :lower) + temp_dir = Path.join(System.tmp_dir!(), "mib_extract_#{random_suffix}") + File.mkdir_p!(temp_dir) - try do - case System.cmd("unzip", ["-o", upload.path, "-d", temp_dir]) do - {_output, 0} -> - # Validate all extracted paths are safe (no directory traversal) - case validate_extracted_paths(temp_dir) do - :ok -> - # Safe to copy to vendor directory - vendor_dir constructed from validated vendor name - # and all paths in temp_dir have been validated by validate_extracted_paths/1 - _ = File.cp_r!(temp_dir, vendor_dir) - files_count = count_files(vendor_dir) - Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}") - invalidate_mib_cache() + try do + case System.cmd("unzip", ["-o", upload.path, "-d", temp_dir]) do + {_output, 0} -> + case validate_extracted_paths(temp_dir) do + :ok -> + _ = File.cp_r!(temp_dir, vendor_dir) + files_count = count_files(vendor_dir) + Logger.info("Extracted #{files_count} MIB files for vendor: #{vendor}") + invalidate_mib_cache() - conn - |> put_status(:created) - |> json(%{ - status: "ok", - message: "Successfully extracted MIB archive", - vendor: vendor, - files_count: files_count - }) + conn + |> put_status(:created) + |> json(%{ + status: "ok", + message: "Successfully extracted MIB archive", + vendor: vendor, + files_count: files_count + }) - {:error, reason} -> - Logger.warning("Archive validation failed for vendor #{vendor}: #{reason}") + {:error, reason} -> + Logger.warning("Archive validation failed for vendor #{vendor}: #{reason}") + + conn + |> put_status(:bad_request) + |> json(%{error: reason}) + end + + {error, exit_code} -> + Logger.error("Failed to extract zip: #{error}") conn |> put_status(:bad_request) - |> json(%{error: reason}) + |> json(%{error: "Failed to extract archive (exit code: #{exit_code})"}) end + after + File.rm_rf!(temp_dir) + end - {error, exit_code} -> - Logger.error("Failed to extract zip: #{error}") - - conn - |> put_status(:bad_request) - |> json(%{error: "Failed to extract archive (exit code: #{exit_code})"}) - end - after - File.rm_rf!(temp_dir) + {:error, reason} -> + conn |> put_status(:bad_request) |> json(%{error: reason}) end end @@ -409,17 +411,34 @@ defmodule ToweropsWeb.Api.V1.MibController do end defp count_files(dir) do + dir |> list_all_entries() |> Enum.count(&File.regular?/1) + end + + # Recursively list all entries including hidden files and directories. + defp list_all_entries(dir) do dir - |> Path.join("**/*") - |> Path.wildcard() - |> Enum.count(&File.regular?/1) + |> list_dir_entries() + |> Enum.flat_map(fn path -> + case File.lstat(path) do + {:ok, stat} when stat.type == :directory -> [path | list_all_entries(path)] + _ -> [path] + end + end) + end + + defp list_dir_entries(dir) do + case File.ls(dir) do + {:ok, entries} -> Enum.map(entries, &Path.join(dir, &1)) + {:error, _} -> [] + end end # Validate that extracted archive contents don't contain path traversal attacks, # symlinks, or excessive file counts/sizes. + # Uses list_all_entries/1 which includes hidden files/directories (unlike Path.wildcard). defp validate_extracted_paths(extract_dir) do canonical_extract_dir = Path.expand(extract_dir) - all_paths = extract_dir |> Path.join("**/*") |> Path.wildcard() + all_paths = list_all_entries(extract_dir) with :ok <- check_no_symlinks(all_paths), :ok <- check_no_traversal(all_paths, canonical_extract_dir) do @@ -432,13 +451,17 @@ defmodule ToweropsWeb.Api.V1.MibController do end defp check_no_symlinks(paths) do - if Enum.any?(paths, &match?({:ok, _}, File.read_link(&1))) do + if Enum.any?(paths, &symlink?/1) do {:error, "Archive contains symlinks, which are not allowed"} else :ok end end + defp symlink?(path) do + match?({:ok, %{type: :symlink}}, File.lstat(path)) + 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/controllers/api_docs_html/index.html.heex b/lib/towerops_web/controllers/api_docs_html/index.html.heex index c09086e3..81c52fb1 100644 --- a/lib/towerops_web/controllers/api_docs_html/index.html.heex +++ b/lib/towerops_web/controllers/api_docs_html/index.html.heex @@ -4752,7 +4752,7 @@ terraform destroy -