Add security review documents
This commit is contained in:
Claudio Ortolina
@@ -0,0 +1,175 @@
|
||||
---
|
||||
id: doc-19
|
||||
title: GPT-5.5 Security Review
|
||||
type: other
|
||||
created_date: "2026-05-13 18:27"
|
||||
---
|
||||
|
||||
## Trust boundaries
|
||||
|
||||
| Boundary | Entrants | Trusted side | Notes |
|
||||
| ---------------------------------------- | -------------------------------------------------------------------- | ----------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------- |
|
||||
| Anonymous HTTP to Phoenix | `/public/assets/:transform_payload`, Last.fm callback, static assets | Controllers, contexts, ETS cache, SQLite, native image processing | The public asset route is reachable without session or API token. |
|
||||
| Authenticated browser session to Phoenix | LiveViews, controllers, form events, file uploads | Context modules, background workers, external API clients | The app is effectively an operator console; forged LiveView events are still treated as authenticated operator actions. |
|
||||
| API bearer token to Phoenix | `/api/v1/*` | Controllers and contexts | Token is compared server-side before API routes run. |
|
||||
| External providers to app | MusicBrainz, Last.fm, Discogs, Wikipedia, Brave Search, Mailgun | API clients, parsers, persistence, renderers | Providers are fixed by client configuration except selected image URLs returned to logged-in users. |
|
||||
| Database and local filesystem to app | SQLite DBs, uploads, backups, release assets, configured DB paths | Repo, Mix tasks, archive controller, native extensions | Most file paths are config- or framework-controlled, not direct request parameters. |
|
||||
| Markdown/HTML to browser | User notes, chat output, Wikipedia HTML, formatted debug data | Phoenix templates, MDEx sanitizer, `raw/1` call sites | Raw rendering is only safe where upstream HTML is trusted or content is sanitized first. |
|
||||
|
||||
## Inventory
|
||||
|
||||
| ID | Sink / behavior | Location | Boundary | Security relevance |
|
||||
| --- | ------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------- | ---------------------------------------------------------------------------------------------- |
|
||||
| S1 | Asset transform payload decode into `%Transform{hash, width}` | `lib/music_library/assets/transform.ex:43` | Anonymous/public asset request | User-controlled JSON controls image lookup and resize dimensions. |
|
||||
| S2 | Asset transform execution and response cache population | `lib/music_library_web/controllers/asset_controller.ex:12`, `lib/music_library_web/controllers/asset_controller.ex:37` | Anonymous/public asset request | Decoded payload drives image conversion or resize and then stores output in ETS. |
|
||||
| S3 | Native libvips resize from untrusted width | `lib/music_library/assets/image.ex:19` | Anonymous/public asset request into native image processing | Width is passed to `Vix.Vips.Operation.thumbnail_buffer/3`. |
|
||||
| S4 | ETS asset cache with caller-selected transform payload in key | `lib/music_library/assets/cache.ex:39`, `lib/music_library/assets/cache.ex:42` | Anonymous/public asset request into in-memory cache | Unique payloads can create unique cached image variants. |
|
||||
| S5 | Brave image download by selected URL | `lib/brave_search/api.ex:44`, `lib/music_library_web/live/artist_live/form.ex:234`, `lib/music_library_web/components/record_form.ex:515` | Authenticated browser action to server-side HTTP client | Potential SSRF sink if exposed to untrusted users. |
|
||||
| S6 | Discogs artist image download by provider URL | `lib/discogs/api.ex:25` | Discogs provider data to server-side HTTP client | Provider-supplied URL is fetched server-side. |
|
||||
| S7 | MusicBrainz cover art download by URL | `lib/music_brainz/api.ex:488`, `lib/music_brainz/api.ex:494`, `lib/music_library/records/enrichment.ex:45` | MusicBrainz/Cover Art Archive data to server-side HTTP client | Cover URL can come from persisted release metadata. |
|
||||
| S8 | Fixed-host upstream API clients | `lib/wikipedia/api.ex`, `lib/last_fm/api.ex`, `lib/music_brainz/api.ex`, `lib/brave_search/api.ex` | External network | HTTP clients cross the network boundary and parse provider responses. |
|
||||
| S9 | Last.fm XML parsing | `lib/last_fm/session.ex:44` | Last.fm callback token to fixed upstream XML response | XML parser processes provider-controlled response bodies. |
|
||||
| S10 | Markdown to HTML with unsafe render enabled and sanitizer | `lib/music_library_web/markdown.ex:11` | User/provider text to browser HTML | `unsafe: true` allows HTML input before sanitizer runs. |
|
||||
| S11 | Markdown custom link transformation into HTML inline nodes | `lib/music_library_web/markdown.ex:105`, `lib/music_library_web/markdown.ex:122` | User text to browser HTML | Custom HTML inline rendering can become XSS if escaping is incomplete. |
|
||||
| S12 | Raw HTML render call sites | `lib/music_library_web/components/chat.ex:228`, `lib/music_library_web/components/notes.ex:159`, `lib/music_library_web/live/artist_live/biography.ex:65`, `lib/music_library_web/live/artist_live/show.ex:753`, `lib/music_library_web/components/core_components.ex:137` | Sanitized/trusted HTML to browser | `raw/1` and `HTML.raw/1` bypass template escaping. |
|
||||
| S13 | External links rendered to browser | `lib/music_brainz/external_link.ex:24`, `lib/music_library_web/components/core_components.ex:188` | Provider URL to browser navigation | External URL rendering can enable script URLs, opener attacks, or phishing if not constrained. |
|
||||
| S14 | Online store URL templates | `lib/music_library/online_store_templates/online_store_template.ex:31`, `lib/music_library/online_store_templates/online_store_template.ex:82` | Authenticated configuration to outbound browser URL | Template output becomes navigable URLs. |
|
||||
| S15 | Full-text search SQL and FTS query syntax | `lib/music_library/records/search.ex:49`, `lib/music_library/records/search.ex:98` | User search input to SQLite FTS | FTS `MATCH` syntax can crash or alter query behavior if not escaped and parameterized. |
|
||||
| S16 | Listening stats SQL | `lib/music_library/listening_stats.ex` | Browser filters to SQL | Aggregation queries consume user-selected date/filter values. |
|
||||
| S17 | Scrobble rules raw SQL builder | `lib/music_library/scrobble_rules.ex:495` | Stored rule fields to raw SQL | SQL fragments are assembled for JSON paths and rule targets. |
|
||||
| S18 | Record set reorder raw SQL | `lib/music_library/record_sets.ex:137` | Authenticated reorder input to SQL | Reordering writes positions based on request values. |
|
||||
| S19 | Error search `LIKE` patterns | `lib/music_library/errors.ex:143` | User search input to SQL | Search strings enter `LIKE` expressions. |
|
||||
| S20 | Telemetry raw SQL | `lib/music_library_web/telemetry/storage.ex:203` | Runtime telemetry filters to telemetry DB SQL | Raw SQL is used against telemetry storage. |
|
||||
| S21 | sqlite-vec/vector query fragments | `lib/sqlite_vec/ecto/query.ex`, `lib/music_library/records/similarity.ex:261` | Embeddings and limits to SQL fragments/native extension | Vector search crosses into SQLite extension code. |
|
||||
| S22 | Query reporter file writes | `lib/music_library/query_reporter.ex:30` | Developer IEx/Tidewave call to filesystem | Developer-supplied report path writes query logs. |
|
||||
| S23 | Upload temp file reads | `lib/music_library_web/live/artist_live/form.ex:201`, `lib/music_library_web/components/record_form.ex:428` | Phoenix upload temp path to filesystem | Server reads temporary upload paths. |
|
||||
| S24 | Release Mix tasks network/file writes | `lib/mix/tasks/esbuild/release.ex`, `lib/mix/tasks/tailwind/release.ex`, `lib/mix/tasks/sqlean/release.ex` | Developer release command to network/filesystem | Tasks download or write release artifacts. |
|
||||
| S25 | Archive DB download | `lib/music_library_web/controllers/archive_controller.ex:7` | Authenticated browser to configured filesystem path | Sends configured SQLite database path as a download. |
|
||||
| S26 | Native SQLite extension loading | `lib/music_library/repo.ex:35`, `config/runtime.exs:43` | Runtime config to native extension loader | Loads extension shared libraries by constructed paths. |
|
||||
| S27 | Typst document generation | `lib/music_library/records/tracklist_pdf.ex:50` | Record metadata to Typst source/PDF engine | User/provider text is interpolated into Typst markup. |
|
||||
| S28 | Cryptographic operations | `config/runtime.exs:59`, `lib/music_library/secrets.ex`, `lib/music_library_web/auth.ex:10`, `lib/last_fm/api/signature.ex` | Secrets, auth credentials, external API protocol | Sensitive comparison, encrypted storage, and Last.fm signing. |
|
||||
| S29 | ETS rate limiter | `lib/req/rate_limiter.ex:36` | Internal HTTP clients to in-memory counters | ETS stores per-service request timing state. |
|
||||
|
||||
## Findings
|
||||
|
||||
### F1: Public asset transforms allow unbounded image resize and cache amplification
|
||||
|
||||
**Sinks:** S1, S2, S3, S4
|
||||
**Rating:** Medium
|
||||
**Confidence:** High
|
||||
**CWE:** CWE-400, Uncontrolled Resource Consumption
|
||||
|
||||
**Trace:** An unauthenticated request can reach `GET /public/assets/:transform_payload` from `lib/music_library_web/router.ex:61`. `AssetController.show/2` decodes the path segment through `MusicLibrary.Assets.Transform.decode/1` and passes the resulting struct to `cached_get/3` (`lib/music_library_web/controllers/asset_controller.ex:12`, `lib/music_library_web/controllers/asset_controller.ex:37`). `Transform.decode/1` base64-decodes arbitrary JSON and builds `%Transform{hash: params["hash"], width: params["width"]}` without enforcing a schema, integer type, positive range, maximum size, or canonical payload form (`lib/music_library/assets/transform.ex:43`). If `width` is truthy and the asset hash exists, `cached_get/3` calls `Image.resize(asset.content, transform.width, format)`, which passes the value directly into `Vix.Vips.Operation.thumbnail_buffer/3` (`lib/music_library/assets/image.ex:19`). Successful output is stored in ETS by `{payload, format}` (`lib/music_library/assets/cache.ex:39`, `lib/music_library/assets/cache.ex:42`).
|
||||
|
||||
**Boundary:** This crosses the anonymous HTTP boundary into native image processing and then into an in-memory cache. Public asset URLs are intentionally generated for email rendering by `RecordsOnThisDayEmail.cover_image_url/2` (`lib/music_library_web/records_on_this_day_email.ex:164`), so a recipient or observer of a valid email image URL can learn a real asset hash and mutate only the `width` field in the unsigned payload.
|
||||
|
||||
**Missing validation:** The asset hash is not required to match the application's expected hash shape before lookup, and `width` is not constrained to `nil` or a bounded positive integer. There is also no signature tying the payload to server-generated transform parameters, so attackers can generate unlimited distinct payloads for the same asset.
|
||||
|
||||
**Prior art:** `backlog/completed/ml-35 - Harden-the-public-asset-endpoint-against-invalid-payloads.md` and commit `92a36b91` hardened the public asset endpoint against invalid base64, invalid JSON, null hashes, and transform failures. The existing controller tests cover malformed payloads and corrupt image failures, but do not cover non-integer widths, negative widths, very large widths, or cache-key amplification by variant payloads. The prior hardening stopped request crashes, but left resource-control validation open.
|
||||
|
||||
**Reachability:** This is reachable without a session or API token. A valid public URL contains enough information to construct additional valid-looking transform payloads for the same asset. Each distinct payload misses the ETS cache and can force a new native resize attempt. When the resize succeeds, the result is cached for a week under the attacker-chosen payload key; when it fails, the process still spends CPU and memory in decode/image handling before returning an error response.
|
||||
|
||||
**Validation:** The following reproduction script is provided for local verification only and was not executed during this audit:
|
||||
|
||||
```elixir
|
||||
# scripts/music_library/public_asset_unbounded_width.exs
|
||||
# Run from the project root with:
|
||||
# MIX_ENV=test mix run scripts/music_library/public_asset_unbounded_width.exs
|
||||
#
|
||||
# Do not run against production. This demonstrates that widths are accepted
|
||||
# and processed without an application-level maximum.
|
||||
|
||||
Application.ensure_all_started(:music_library)
|
||||
|
||||
alias MusicLibrary.Assets
|
||||
alias MusicLibrary.Assets.{Asset, Cache, Image, Transform}
|
||||
|
||||
png =
|
||||
"iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/p9sAAAAASUVORK5CYII="
|
||||
|> Base.decode64!()
|
||||
|
||||
hash = Asset.hash(png)
|
||||
|
||||
asset =
|
||||
case Assets.store(%{content: png, format: "image/png"}) do
|
||||
{:ok, %{hash: nil}} -> Assets.get!(hash)
|
||||
{:ok, asset} -> asset
|
||||
end
|
||||
|
||||
for width <- [96, 4096, 12_000] do
|
||||
payload =
|
||||
%{hash: asset.hash, width: width}
|
||||
|> JSON.encode!()
|
||||
|> Base.url_encode64(padding: false)
|
||||
|
||||
{:ok, %Transform{width: ^width} = transform} = Transform.decode(payload)
|
||||
|
||||
{usec, result} =
|
||||
:timer.tc(fn ->
|
||||
Image.resize(asset.content, transform.width, "image/jpeg")
|
||||
end)
|
||||
|
||||
case result do
|
||||
{:ok, data} ->
|
||||
Cache.set(payload, "image/jpeg", data)
|
||||
IO.puts("width=#{width} bytes=#{byte_size(data)} ms=#{div(usec, 1000)} path=/public/assets/#{payload}")
|
||||
|
||||
{:error, reason} ->
|
||||
IO.puts("width=#{width} error=#{inspect(reason)}")
|
||||
end
|
||||
end
|
||||
|
||||
IO.puts("cache_bytes=#{Cache.total_content_size()}")
|
||||
```
|
||||
|
||||
**Rating rationale:** The impact is availability-focused rather than direct data disclosure or code execution. The endpoint is anonymous, the work crosses into native image processing, and the cache key is attacker-influenced, which makes repeated amplification practical. The attacker still needs a valid asset hash from a public image URL, and libvips may fail some extreme inputs before caching output, so Medium is more appropriate than High.
|
||||
|
||||
**Fix:** Validate transform payloads before asset lookup or image processing. Require `hash` to match the stored asset hash format, require `width` to be either absent/`nil` or a positive integer in a small allowlist or documented maximum, and reject all other payloads with `{:error, :invalid}`. Consider signing transform payloads, or deriving cache keys from a canonical validated transform struct rather than the raw path segment. Add tests for string widths, negative widths, zero, very large widths, unknown fields, non-canonical payloads, and repeated variant payloads.
|
||||
|
||||
## Ruled out
|
||||
|
||||
### S5: Brave selected image download
|
||||
|
||||
Ruled out at reachability step 5 for the current threat model. `select_image` / `select_cover` events can cause server-side `Req.get/1` of a URL value, and Req redirects are enabled by default. However, these events are behind an authenticated browser session and are part of an operator workflow that intentionally downloads selected remote images. I did not find an unauthenticated path or lower-privileged multi-user boundary that lets an attacker force arbitrary server-side URL fetches. If this app becomes multi-user or exposes these events to untrusted accounts, this should be reclassified as an SSRF candidate and constrained with host/IP allow/deny checks and redirect target validation.
|
||||
|
||||
### S6, S7, S8: Provider HTTP clients and fixed upstream requests
|
||||
|
||||
Ruled out at source and trust-boundary step 2. MusicBrainz, Last.fm, Wikipedia, Brave Search, and Cover Art Archive requests use fixed API hosts or URLs produced by provider metadata in trusted ingestion flows. I did not find `verify: :verify_none`, shelling out to curl, or request options that disable TLS verification. Discogs and MusicBrainz image downloads do fetch provider-supplied image URLs, but the reachable callers are authenticated/operator-driven ingestion or refresh paths, not anonymous arbitrary URL fetch endpoints.
|
||||
|
||||
### S9: Last.fm XML parsing
|
||||
|
||||
Ruled out at step 2. The public callback receives a `token`, but the XML body parsed by `LastFm.Session` comes from the fixed Last.fm API endpoint after the app exchanges that token. The parser is not fed raw request body content. The code extracts a small set of expected XML nodes and does not expand the callback token into XML. No direct XXE-style local file or attacker-controlled XML input path was identified.
|
||||
|
||||
### S10, S11, S12: Markdown and raw HTML rendering
|
||||
|
||||
Ruled out at validation step 3. The app enables MDEx unsafe rendering but immediately configures MDEx sanitization with `MDEx.Document.default_sanitize_options()`. Custom markdown link processing escapes generated URL, title, and text before constructing HTML inline nodes. Raw call sites for notes, chat, and artist biography render the output of `Markdown.to_html/2`, not unsanitized request text. `CoreComponents.pretty_json/1` renders JSON generated by the server for debug display.
|
||||
|
||||
Wikipedia biography HTML remains a deliberate trust decision rather than a sanitizer path. Commit `fbc548e9` and `backlog/completed/ml-15 - Sanitize-Wikipedia-bio_html-in-ArtistLive.Show.md` document the decision to trust Wikipedia HTML with a Sobelow skip. The upstream API use is fixed-host and title-encoded. MediaWiki documents the parse/extract API surface for HTML extracts in its API documentation: [API:Get the contents of a page](https://www.mediawiki.org/wiki/API:Get_the_contents_of_a_page) and [API:Query](https://www.mediawiki.org/wiki/API:Query). Because this is not raw user-supplied HTML, I did not rate it as an app-side XSS finding.
|
||||
|
||||
### S13, S14: External links and online store templates
|
||||
|
||||
Ruled out at step 5. MusicBrainz external links are filtered by known relationship URL patterns before rendering, and rendered links include `target="_blank"` with `rel="noopener noreferrer"`. Online store templates validate that configured URLs use `http` or `https`, then URL-encode artist/title/format placeholder values before substitution. The remaining risk is user/operator configuration quality, not an injection path from untrusted request data to script execution.
|
||||
|
||||
### S15, S16, S17, S18, S19, S20, S21: SQL and SQLite extension sinks
|
||||
|
||||
Ruled out at validation step 3. The searched SQL sinks use parameter binding for user/provider values. Full-text search terms are escaped before entering `MATCH`; commit `ba256682` fixed special-character FTS crashes by quoting terms, and current code parameterizes the resulting query strings. `ScrobbleRules` builds raw SQL for hardcoded column and JSON-path fragments while binding dynamic values, and `backlog/completed/ml-99 - Raw-SQL-with-string-concatenation-in-ScrobbleRules.md` documents that review as a maintainability/testability concern rather than exploitable SQL injection. Record set reorder, errors search, telemetry queries, and vector similarity queries likewise keep request values in parameters or constrained numeric fields.
|
||||
|
||||
### S22, S23, S24, S25: Filesystem reads and writes
|
||||
|
||||
Ruled out at step 2. `QueryReporter` writes to a developer-supplied path from IEx/Tidewave and is not reachable from HTTP request parameters. Upload reads use Phoenix-managed temporary upload paths. Release Mix tasks are developer-run commands that download or write predetermined tooling artifacts. `ArchiveController` sends a configured SQLite database path, not a request-controlled file path, and is behind authenticated browser routing.
|
||||
|
||||
### S26: Native SQLite extension loading
|
||||
|
||||
Ruled out at step 2. Extension paths are constructed from internal OS/architecture detection and literal extension names configured in `config/runtime.exs`. I did not find a request parameter, environment value containing arbitrary path content, or database value that can select an extension filename.
|
||||
|
||||
### S27: Typst PDF generation
|
||||
|
||||
Ruled out at validation step 3. Tracklist PDF generation interpolates record and track metadata into Typst markup only after passing text through `Typst.Format.escape`. I did not find unescaped metadata entering Typst source in the audited generation paths.
|
||||
|
||||
### S28: Cryptographic operations
|
||||
|
||||
Ruled out at step 4. Secret storage uses Cloak AES-GCM with a base64-decoded runtime key. Login password and API bearer token comparisons use `Plug.Crypto.secure_compare/2`, which checks byte size and delegates to constant-time comparison for equal-length binaries. Last.fm request signing uses MD5 because that is the Last.fm API signature protocol, not because the app is hashing passwords or authenticating local users with MD5.
|
||||
|
||||
### S29: ETS rate limiter
|
||||
|
||||
Ruled out at step 1. The ETS table stores per-service request timing for internal HTTP clients. Keys are service atoms supplied by application code, not attacker-controlled terms or unbounded request strings. No direct injection, information disclosure, or cross-request authorization decision depends on this table.
|
||||
@@ -0,0 +1,156 @@
|
||||
---
|
||||
id: doc-20
|
||||
title: Deepseek-v4-pro Security Review
|
||||
type: other
|
||||
created_date: "2026-05-13 18:27"
|
||||
---
|
||||
|
||||
## Trust boundaries
|
||||
|
||||
| Actor | Trusted | Controls | Source |
|
||||
| ---------------------- | ----------- | -------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------- |
|
||||
| Web user (browser) | no | URL params, form fields, file uploads, LiveView events, HTTP headers | All routes behind `require_logged_in` (except login, health, Last.fm callback, public assets) — `MusicLibraryWeb.Auth` |
|
||||
| Presto API consumer | conditional | HTTP Authorization header, query params (`?q=`) | Requires Bearer token validated via `Plug.Crypto.secure_compare/2` in `Auth.require_api_token/2` — `api/v1/*` routes |
|
||||
| Last.fm OAuth callback | no | `token` query parameter (validated server-side against Last.fm) | `LastFmController` `moduledoc` — single-user deployment, token validated via `LastFm.get_session/1` |
|
||||
| Operator / config | yes | Application environment variables and config files | `config/runtime.exs` and `config/prod.exs` — all secrets originate from env vars |
|
||||
| MusicBrainz API | conditional | JSON responses with record/artist metadata | `MusicBrainz.API` — data going to HTML is sanitised via MDEx/ammonia; SQL uses parameterised bindings |
|
||||
| Last.fm API | conditional | JSON responses with scrobble/artist data | `LastFm.API` — same sanitisation path |
|
||||
| OpenAI API | conditional | Chat responses (markdown), embeddings (float arrays) | `OpenAI.API` — responses rendered through MDEx HTML sanitisation |
|
||||
| Wikipedia API | conditional | Plain-text article extracts | `Wikipedia.API` — extracts go through MDEx markdown sanitisation |
|
||||
| Brave Search / Discogs | conditional | Image URLs, artist metadata | Rendered through MDEx, or used as `href` targets |
|
||||
| Mailgun (outbound) | yes | Outbound email only (no inbound parsing) | `MusicLibrary.Mailer` — Swoosh adapter |
|
||||
|
||||
## Inventory
|
||||
|
||||
| ID | Location | Class | Consumes |
|
||||
| --- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------ | ----------------------------------------------------------------------------------------------------------- |
|
||||
| S1 | `lib/music_library_web/components/notes.ex:164` | Template / `raw()` | Note content (user-authored markdown) via `Markdown.to_html()` |
|
||||
| S2 | `lib/music_library_web/components/core_components.ex:149` | Template / `raw()` | Debug JSON via `Lumis.highlight!()` — dev/monitoring routes only |
|
||||
| S3 | `lib/music_library_web/components/search_components.ex:401` | Template / `raw()` | Record set description via `Markdown.to_html()` |
|
||||
| S4 | `lib/music_library_web/components/chat.ex:228` | Template / `raw()` | AI-generated assistant message via `Markdown.to_html()` |
|
||||
| S5 | `lib/music_library_web/components/chat.ex:243` | Template / `raw()` | AI streaming response via `Markdown.streaming_to_html()` |
|
||||
| S6 | `lib/music_library_web/live/artist_live/biography.ex:70` | Template / `raw()` | Wikipedia/Last.fm extract via `Markdown.to_html()` |
|
||||
| S7 | `lib/music_library_web/live/artist_live/show.ex:756` | Template / `raw()` | Pre-compiled biography HTML (`biography.bio_html`) |
|
||||
| S8 | `lib/music_library_web/live/record_set_live/show.ex:275` | Template / `raw()` | Record set description via `Markdown.to_html()` |
|
||||
| S9 | `lib/music_library_web/live/record_set_live/index.ex:446` | Template / `raw()` | Record set description via `Markdown.to_html()` |
|
||||
| S10 | `lib/music_library_web/live/stats_live/top_by_period.ex:118` | Resource consumption | URL param `period` via `String.to_existing_atom/1` |
|
||||
| S11 | `lib/music_library/online_store_templates.ex:87-90` | Template (URL) | User-defined `url_template` interpolated with `String.replace` + `URI.encode_www_form` |
|
||||
| S12 | `lib/music_library_web/router.ex` (CSP header) | Validation / CSP | Content-Security-Policy with `'unsafe-inline'` scripts and `'wasm-unsafe-eval'` |
|
||||
| S13 | All HTTP client modules (`LastFm.API`, `MusicBrainz.API`, `OpenAI.API`, `Discogs.API`, `BraveSearch.API`, `Wikipedia.API`) | Network | Req default TLS verification (no `:verify_none`) |
|
||||
| S14 | `lib/last_fm/api.ex` (log callbacks) | Shared mutable state / log injection | `sanitize_url/2` redacts API key from logged URLs |
|
||||
| S15 | `lib/music_library_web/controllers/archive_controller.ex:21` | File operations | `send_download` with config-derived database path |
|
||||
| S16 | `lib/music_library_web/components/record_form.ex:434` | File operations | `File.read!/1` on LiveView upload temp path |
|
||||
| S17 | `lib/music_library_web/live/artist_live/form.ex:209` | File operations | `File.read!/1` on LiveView upload temp path |
|
||||
| S18 | `lib/music_library_web/auth.ex:25` | Cryptography | Login password comparison via `Plug.Crypto.secure_compare/2` |
|
||||
| S19 | `lib/music_library_web/auth.ex:42` | Cryptography | API token comparison via `Plug.Crypto.secure_compare/2` |
|
||||
| S20 | `lib/music_library_web/controllers/error_controller.ex:11-16` | Validation / resource consumption | API query params `status`, `muted`, `search`, `limit`, `offset` — parsed to atoms/integers, no raw SQL |
|
||||
| S21 | `lib/music_library_web/markdown.ex:103-107` | Template / `raw()` return value | `process_double_bracket_links/1` — `URI.encode_www_form` on bracket content, then MDEx/ammonia sanitisation |
|
||||
| S22 | `lib/music_library_web/records_on_this_day_email.ex:59` | Template / interpolation | Email HTML body: record titles and artist names via `html_escape/1` |
|
||||
| S23 | `lib/music_library/chats.ex:74-77` | Template / SQL | Chat `topic` from first-message content truncated to 80 chars — `String.slice`, no raw SQL context |
|
||||
| S24 | All Ecto `fragment()` call-sites in `records/search.ex`, `scrobble_rules.ex`, `maintenance.ex`, `listening_stats.ex`, `collection.ex`, `artists.ex`, `errors.ex`, `records/import.ex` | SQL injection | User-supplied or externally-supplied strings used in `fragment()` calls with `^` parameterised bindings |
|
||||
|
||||
## Findings
|
||||
|
||||
### F1 — Artist name parameter smuggling in online store URL templates
|
||||
|
||||
**Severity:** Low
|
||||
**CWE:** CWE-88 (Argument Injection)
|
||||
**Location:** `lib/music_library/online_store_templates.ex:87-90`
|
||||
**Sinks:** S11
|
||||
|
||||
**Trace:**
|
||||
|
||||
1. Operator creates an `OnlineStoreTemplate` with `url_template` = `"https://store.example.com/search?q={artist}+{title}"`.
|
||||
2. Artist names originate from MusicBrainz JSON responses → `MusicBrainz.Release.from_api_response/1` → stored in `records.artists` JSON column → read via `Record.artist_names/1`.
|
||||
3. `OnlineStoreTemplates.generate_url/2` calls `String.replace(template.url_template, "{artist}", URI.encode_www_form(artists_string))`.
|
||||
4. `artists_string = Enum.map_join(record.artists, " ", & &1.name)` — this produces a string with literal spaces, not `%20`.
|
||||
5. `URI.encode_www_form/1` encodes spaces as `+` but does not strip or encode structural characters (`?`, `#`, `&`, `=`) that may already be present in artist names from upstream data.
|
||||
|
||||
A MusicBrainz artist whose name contains `&sort=price` (or any URL structural character) causes the generated store URL to carry an unintended query parameter or fragment. For example, an artist named `"Porcupine Tree &sort=price"` would produce a URL where `&sort=price` is interpreted as a separate query parameter by the target store.
|
||||
|
||||
**Boundary:** Artist name originates at the MusicBrainz API (trusted but not controlled by the operator). The template is operator-defined and validated via `URI.parse/1` to require `http` or `https` scheme.
|
||||
|
||||
**Validation:**
|
||||
|
||||
```elixir
|
||||
# scripts/music_library/online_store_url_smuggling.exs
|
||||
Mix.install([{:jason, "~> 1.4"}])
|
||||
|
||||
# Simulate the generate_url logic
|
||||
defmodule Smuggle do
|
||||
def generate_url(template, artists_string, title) do
|
||||
encoded_artists = URI.encode_www_form(artists_string)
|
||||
encoded_title = URI.encode_www_form(title)
|
||||
|
||||
template
|
||||
|> String.replace("{artist}", encoded_artists)
|
||||
|> String.replace("{title}", encoded_title)
|
||||
|> String.replace("{format}", URI.encode_www_form("vinyl"))
|
||||
end
|
||||
end
|
||||
|
||||
# Benign case
|
||||
IO.puts("Benign:")
|
||||
url1 = Smuggle.generate_url(
|
||||
"https://store.example.com/search?q={artist}+{title}",
|
||||
"Porcupine Tree",
|
||||
"In Absentia"
|
||||
)
|
||||
IO.puts(" #{url1}")
|
||||
|
||||
# Artist name with structural characters
|
||||
IO.puts("\nArtist name containing '&sort=price':")
|
||||
url2 = Smuggle.generate_url(
|
||||
"https://store.example.com/search?q={artist}+{title}",
|
||||
"Porcupine Tree &sort=price",
|
||||
"In Absentia"
|
||||
)
|
||||
IO.puts(" #{url2}")
|
||||
|
||||
# Artist name with fragment
|
||||
IO.puts("\nArtist name containing '#fragment':")
|
||||
url3 = Smuggle.generate_url(
|
||||
"https://store.example.com/search?q={artist}+{title}",
|
||||
"Artist Name #bonus",
|
||||
"Album Title"
|
||||
)
|
||||
IO.puts(" #{url3}")
|
||||
```
|
||||
|
||||
**Prior art:** No related git commits, issues, or PRs found. The `validate_url_template/1` changeset function was added to prevent non-HTTP schemes, but structural-character encoding in substitution values was not addressed.
|
||||
|
||||
**Reach:** Consumer is a single-user personal music-collection application. The `generate_url/2` output is rendered as an `<a href>` on the wishlist detail page. Exploitation requires a malicious upstream data source (MusicBrainz) to serve an artist name with URL structural characters, which is then stored and later interpolated into a store template URL. The impact is link manipulation on an internal-facing wishlist page — an operator clicking a store link could be sent to a URL with additional query parameters or a truncated fragment.
|
||||
|
||||
**Rating:** Low — requires an attacker who can control MusicBrainz artist data (unrealistic for a curated database like MusicBrainz) or the operator to manually create a record with a crafted artist name, which is self-inflicted. Impact is limited to query-parameter injection in outbound store links, not to the application itself.
|
||||
|
||||
**Suggested fix:** Apply `URI.encode_www_form/1` to the complete query-value portion (i.e. encode the per-artist names individually and join with `+`), or use `URI.encode/2` with `&URI.char_unreserved?/1` to encode all reserved characters including `&`, `?`, `#`, and `=`.
|
||||
|
||||
---
|
||||
|
||||
## Ruled out
|
||||
|
||||
- **S1–S9** (step 3 — sanitisation) — Every `raw()` call site is preceded by `Markdown.to_html/1` (backed by `MDEx.Document.default_sanitize_options()` which wraps ammonia), by `Lumis.highlight!/2` outputting static syntax-highlighted HTML from controlled debug JSON (S2, dev routes only), or by a pre-compiled `biography.bio_html` string that was itself produced through the MDEx pipeline (S7). The `sobelow_skip` annotations at each site correctly identify that the upstream sanitisation has already run. No path from untrusted input reaches `raw()` without traversing ammonia-based HTML filtering.
|
||||
|
||||
- **S10** (step 1 — internal) — `String.to_existing_atom/1` only succeeds for atoms already present in the atom table (the `:period` enum values: `:7d`, `:30d`, `:90d`, `:1y`, `:all_time`). If the URL param doesn't match, it raises `ArgumentError`. This does not create new atoms and cannot exhaust the atom table.
|
||||
|
||||
- **S12** (step 5 — reach) — `'unsafe-inline'` for scripts/styles is a known tradeoff in Phoenix LiveView applications (morphdom inline patching, colocated hooks). `'wasm-unsafe-eval'` is required by `zxing-wasm` (barcode-detector). The app additionally uses `protect_from_forgery`, CSRF tokens on LiveSocket, and `Content-Type: application/x-www-form-urlencoded` form submissions. The risk surface is comparable to any modern LiveView deployment; no additional CSP bypass vector beyond the standard LiveView surface exists.
|
||||
|
||||
- **S13** (step 1 — no boundary crossing) — All HTTP clients use Req's default TLS verification. No `verify: :verify_none`, no `transport_opts` override, no custom `:ssl` configuration found in any API module or config file. The `force_ssl` option in `runtime.exs` is commented out but references a Coolify reverse-proxy TLS terminator in production (documented in `docs/production-infrastructure.md`).
|
||||
|
||||
- **S14** (step 3 — validated) — `sanitize_url/2` in `LastFm.API` redacts the API key from log output with `String.replace(url, api_key, "<redacted_api_key>")`. MusicBrainz/OpenAI/BraveSearch/Discogs/Wikipedia API modules log full URLs but do not include API keys in URL paths (they use headers or auth structs, not query params). No API key leakage to logs.
|
||||
|
||||
- **S15** (step 1 — config-derived) — `database_path/0` reads `Application.get_env(:music_library, MusicLibrary.Repo)[:database]`, a value set from `DATABASE_PATH` env var in `runtime.exs`. No user input reaches the file path; the `sobelow_skip ["Traversal.SendDownload"]` annotation is correct.
|
||||
|
||||
- **S16–S17** (step 2 — Phoenix-managed) — LiveView `allow_upload` provides temporary paths via `entry.client_type`. Phoenix generates these on disk; the caller reads them via `File.read!/1` before storing the content as an Asset (content-addressed by SHA-256 hash). No path traversal possible because the path is assigned by the framework, not derived from user input.
|
||||
|
||||
- **S18–S19** (step 3 — validated) — Both login password and API token comparisons use `Plug.Crypto.secure_compare/2`, which is constant-time. No timing side-channel.
|
||||
|
||||
- **S20** (step 3 — validated) — `ErrorController` parses all query params into explicit atoms/integers with fallback defaults. The `search` parameter reaches `Errors.list_errors/1` which uses `like/2` with Ecto parameterised bindings (`^"%#{escaped}%"` with LIKE escape). No SQL injection.
|
||||
|
||||
- **S21** (step 3 — sanitised) — `process_double_bracket_links/1` extracts content from `[[...]]` brackets, `URI.encode_www_form/1` encodes it for URL query strings, then the full markdown text is processed through MDEx/ammonia. No HTML/URL injection path.
|
||||
|
||||
- **S22** (step 3 — sanitised) — `build_html/3` in `RecordsOnThisDayEmail` uses `html_escape/1` (delegating to `Phoenix.HTML.html_escape/1`) on all user-derived values (record titles, artist names). The email body cannot inject HTML from record data.
|
||||
|
||||
- **S23** (step 1 — truncated string) — Chat topic is `String.slice(content, 0, 80)` of user input, stored via Ecto changeset. It is displayed in templates where Phoenix auto-escapes HEEx expressions. No raw or unescaped path.
|
||||
|
||||
- **S24** (step 3 — validated) — Every `fragment()` call across the codebase uses `^value` parameterised bindings for all user-supplied or externally-supplied strings. The `fts_escape/1` function in `records/search.ex` wraps search terms in double-quoted FTS5 phrase syntax with `""` escaping for literal double quotes, preventing FTS5 query injection. String concatenation (`||`) in `fragment()` calls (e.g., `"records_search_index MATCH 'genres : ' || ?"`) only combines static SQL fragments with `^`-bound values, never dynamic string interpolation. No SQL injection surface identified.
|
||||
Reference in New Issue
Block a user