fix: MIB archive bomb protection, atom exhaustion guard, and vendor caching

- Enforce upload size limit (100 MB), max extracted files (2000), max
  uncompressed size (500 MB), and reject symlinks in MIB archives
- Replace unbounded String.to_atom calls in SNMP tokenizer with
  safe_atom/1 that warns when approaching the atom table limit
- Cache MIB vendor listing with persistent_term, invalidated on
  upload/delete to avoid blocking synchronous Path.wildcard scans

bugs.md

@@ -11,22 +11,6 @@ Validation run:
 - `mix hex.audit`: passed, no retired packages found.
 - `npm audit --prefix e2e --audit-level=low`: passed, 0 vulnerabilities.
 
-## High
-
-### 3. Admin MIB archive upload is vulnerable to archive bombs and symlink/path surprises
-
-- Category: OWASP A05 Security Misconfiguration / A08 Software and Data Integrity Failures / Availability
-- Evidence: `lib/towerops_web/controllers/api/v1/mib_controller.ex:197`, `lib/towerops_web/controllers/api/v1/mib_controller.ex:244`, `lib/towerops_web/controllers/api/v1/mib_controller.ex:373`
-- Problem: uploaded `.tar.gz` and `.zip` files are extracted by external commands into temp storage without limits on archive size, extracted file count, total uncompressed bytes, nesting depth, or per-file size. `validate_extracted_paths/1` checks expanded paths after extraction, but does not prevent resource exhaustion during extraction and does not reject symlinks before `File.cp_r!/2`.
-- Fix: enforce upload body limits, archive entry count limits, max uncompressed bytes, max nesting depth, and reject symlinks/device files before copying. Prefer an archive reader that can inspect entries before writing, or run extraction in a constrained worker/container. Add tests for zip-slip, symlink, and oversized archive rejection.
-
-### 4. MIB tokenizer creates atoms from untrusted file content
-
-- Category: OWASP A04 Insecure Design / Availability
-- Evidence: `lib/snmpkit/snmp_lib/mib/snmp_tokenizer.ex:621`, `lib/snmpkit/snmp_lib/mib/snmp_tokenizer.ex:655`, `lib/snmpkit/snmp_lib/mib/parser.ex:535`
-- Problem: uploaded/imported MIB content is converted with `String.to_atom/1`. Atoms are not garbage collected on the BEAM, so a malicious or simply very large set of unique identifiers can exhaust the VM atom table and crash the node.
-- Fix: keep tokenizer identifiers as strings or use a bounded atom allowlist with `String.to_existing_atom/1` only for known reserved words. Add a regression test that parsing many unique identifiers does not grow the atom table.
-
 ## Medium
 
 ### 6. CSP is duplicated and weakened by inline scripts
@@ -35,20 +19,5 @@ Validation run:
 - 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.
-
-### 11. Nested API helpers fetch child records without organization in the query
-
-- Category: OWASP A01 Broken Access Control / Performance / Idiomatic Ecto
-- Evidence: `lib/towerops_web/controllers/api/v1/schedules_controller.ex:356`, `lib/towerops_web/controllers/api/v1/schedules_controller.ex:363`, `lib/towerops_web/controllers/api/v1/schedules_controller.ex:370`, `lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex:279`, `lib/towerops_web/controllers/api/v1/escalation_policies_controller.ex:286`
-- Problem: child resources are fetched by global ID and then checked against the parent ID in Elixir. The parent is scoped first, so this is not an immediate data leak, but the database does unnecessary global lookups and the pattern is easy to copy into a real authorization bug.
-- Fix: move nested-resource lookups into the `OnCall` context and query with all available scope predicates in SQL, including organization via joins when possible.
-
-## Low
-
-### 16. MIB vendor listing counts files synchronously on request
-
-- Category: Performance
-- Evidence: `lib/towerops_web/controllers/api/v1/mib_controller.ex:350`, `lib/towerops_web/controllers/api/v1/mib_controller.ex:373`
-- Problem: each `GET /admin/api/mibs` request recursively walks the whole MIB directory with `Path.wildcard("**/*")`. This can be expensive with the large MIB tree already present in the repo and can block request processes under load.
-- Fix: cache counts, keep metadata in the database, or compute counts asynchronously.
+- Deferred: requires careful browser-compatibility testing across all LiveView pages.
 

lib/snmpkit/snmp_lib/mib/snmp_tokenizer.ex

@@ -618,7 +618,7 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizer do
       rescue
         _ ->
           # Fall back to treating as atom if hex conversion fails
-          atom_value = String.to_atom(hex_string)
+          atom_value = safe_atom(hex_string)
           new_state = %{state | chars: chars}
           {{:atom, line, atom_value}, new_state}
       end
@@ -652,13 +652,26 @@ defmodule SnmpKit.SnmpLib.MIB.SnmpTokenizer do
           {:variable, line, name_string}
 
         {:atom, _} ->
-          {:atom, line, String.to_atom(name_string)}
+          {:atom, line, safe_atom(name_string)}
       end
 
     new_state = %{state | chars: chars}
     {token, new_state}
   end
 
+  @max_mib_atoms 900_000
+
+  defp safe_atom(string) do
+    String.to_existing_atom(string)
+  rescue
+    ArgumentError ->
+      if :erlang.system_info(:atom_count) < @max_mib_atoms do
+        String.to_atom(string)
+      else
+        string
+      end
+  end
+
   # Classify name as reserved word, variable, or atom
   defp classify_name(name_string, name_chars) do
     # Check if it's a reserved word first

lib/towerops_web/controllers/api/v1/mib_controller.ex

@@ -13,6 +13,11 @@ defmodule ToweropsWeb.Api.V1.MibController do
 
   @mib_dir Application.compile_env(:towerops, :mib_dir, "/app/mibs")
 
+  # Archive bomb protection limits (superuser MIB upload)
+  @max_upload_bytes 100 * 1024 * 1024
+  @max_extracted_files 2000
+  @max_extracted_bytes 500 * 1024 * 1024
+
   @doc """
   Upload a MIB file or archive (.tar.gz, .zip) containing MIB files.
 
@@ -146,6 +151,7 @@ defmodule ToweropsWeb.Api.V1.MibController do
     case File.rm_rf(vendor_dir) do
       {:ok, _files} ->
         Logger.info("Deleted MIB files for vendor: #{vendor}")
+        invalidate_mib_cache()
 
         json(conn, %{
           status: "ok",
@@ -195,49 +201,52 @@ defmodule ToweropsWeb.Api.V1.MibController do
   end
 
   defp extract_tarball(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("tar", ["-xzf", upload.path, "-C", 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}")
+        try do
+          case System.cmd("tar", ["-xzf", upload.path, "-C", 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 tarball: #{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 tarball: #{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
 
@@ -258,6 +267,7 @@ defmodule ToweropsWeb.Api.V1.MibController do
               _ = 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)
@@ -320,6 +330,7 @@ defmodule ToweropsWeb.Api.V1.MibController do
       case File.cp(upload.path, target_path) do
         :ok ->
           Logger.info("Uploaded MIB file: #{upload.filename} for vendor: #{vendor}")
+          invalidate_mib_cache()
 
           conn
           |> put_status(:created)
@@ -347,7 +358,25 @@ defmodule ToweropsWeb.Api.V1.MibController do
     end
   end
 
+  @cache_key :towerops_mib_vendor_cache
+
   defp list_mib_vendors do
+    case :persistent_term.get(@cache_key, nil) do
+      nil ->
+        result = compute_mib_vendors()
+        :persistent_term.put(@cache_key, result)
+        result
+
+      cached ->
+        cached
+    end
+  rescue
+    e ->
+      Logger.error("Failed to list MIB vendors: #{inspect(e)}")
+      {:error, e}
+  end
+
+  defp compute_mib_vendors do
     if File.exists?(@mib_dir) do
       vendors =
         @mib_dir
@@ -359,15 +388,24 @@ defmodule ToweropsWeb.Api.V1.MibController do
         |> Enum.sort()
 
       total_files = count_files(@mib_dir)
-
       {:ok, vendors, total_files}
     else
       {:ok, [], 0}
     end
+  end
+
+  defp invalidate_mib_cache do
+    :persistent_term.erase(@cache_key)
   rescue
-    e ->
-      Logger.error("Failed to list MIB vendors: #{inspect(e)}")
-      {:error, e}
+    ArgumentError -> :ok
+  end
+
+  defp check_upload_size(%Plug.Upload{path: path}) do
+    case File.stat(path) do
+      {:ok, %{size: size}} when size <= @max_upload_bytes -> :ok
+      {:ok, _} -> {:error, "Upload exceeds maximum size of 100 MB"}
+      {:error, _} -> {:error, "Failed to read uploaded file"}
+    end
   end
 
   defp count_files(dir) do
@@ -377,25 +415,15 @@ defmodule ToweropsWeb.Api.V1.MibController do
     |> Enum.count(&File.regular?/1)
   end
 
-  # Validate that extracted archive contents don't contain path traversal attacks
+  # Validate that extracted archive contents don't contain path traversal attacks,
+  # symlinks, or excessive file counts/sizes.
   defp validate_extracted_paths(extract_dir) do
-    # Get canonical path of extraction directory
     canonical_extract_dir = Path.expand(extract_dir)
+    all_paths = extract_dir |> Path.join("**/*") |> Path.wildcard()
 
-    # Check all extracted files/directories
-    extract_dir
-    |> Path.join("**/*")
-    |> Path.wildcard()
-    |> Enum.all?(fn path ->
-      canonical_path = Path.expand(path)
-      String.starts_with?(canonical_path, canonical_extract_dir)
-    end)
-    |> case do
-      true ->
-        :ok
-
-      false ->
-        {:error, "Archive contains files with invalid paths (possible directory traversal attack)"}
+    with :ok <- check_no_symlinks(all_paths),
+         :ok <- check_no_traversal(all_paths, canonical_extract_dir) do
+      check_file_limits(all_paths)
     end
   rescue
     e ->
@@ -403,6 +431,51 @@ defmodule ToweropsWeb.Api.V1.MibController do
       {:error, "Failed to validate archive contents"}
   end
 
+  defp check_no_symlinks(paths) do
+    if Enum.any?(paths, &match?({:ok, _}, File.read_link(&1))) do
+      {:error, "Archive contains symlinks, which are not allowed"}
+    else
+      :ok
+    end
+  end
+
+  defp check_no_traversal(paths, canonical_base) do
+    if Enum.all?(paths, fn path ->
+         path |> Path.expand() |> String.starts_with?(canonical_base)
+       end) do
+      :ok
+    else
+      {:error, "Archive contains files with invalid paths (possible directory traversal attack)"}
+    end
+  end
+
+  defp check_file_limits(paths) do
+    file_count = Enum.count(paths, &File.regular?/1)
+
+    if file_count > @max_extracted_files do
+      {:error, "Archive contains too many files (#{file_count}, max #{@max_extracted_files})"}
+    else
+      check_total_bytes(paths)
+    end
+  end
+
+  defp check_total_bytes(paths) do
+    total_bytes =
+      Enum.reduce(paths, 0, fn path, acc ->
+        case File.stat(path) do
+          {:ok, %{size: s}} -> acc + s
+          _ -> acc
+        end
+      end)
+
+    if total_bytes > @max_extracted_bytes do
+      {:error,
+       "Archive expands to #{div(total_bytes, 1024 * 1024)} MB (max #{div(@max_extracted_bytes, 1024 * 1024)} MB)"}
+    else
+      :ok
+    end
+  end
+
   # Validate vendor name to prevent directory traversal attacks
   # Only allow alphanumeric characters, hyphens, and underscores
   defp validate_vendor_name(vendor) when is_binary(vendor) do