Skip to content

docs(security): state the at-rest and provider-visibility threat model - #127

Open
dmnyc wants to merge 2 commits into
mainfrom
fix/115-threat-model-docs
Open

docs(security): state the at-rest and provider-visibility threat model#127
dmnyc wants to merge 2 commits into
mainfrom
fix/115-threat-model-docs

Conversation

@dmnyc

@dmnyc dmnyc commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Fixes #115 (M-6, M-7, L-10, L-15).

SECURITY.md now says plainly what the codebase actually guarantees: a profile reader gets the descriptor (all addresses + SLIP-77, secret-equivalent, not 'watch-only'), the scan history, and metadata; chain-data providers see and can cluster the address set (with the self-hosted Esplora escape hatch); SideSwap sees the swapped asset's UTXOs with blinding factors; registry asset labels are issuer-steerable display hints. Sits next to the existing 'no telemetry' statement so the honest caveat travels with the claim. Links the 2026-08 scan.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

@dmnyc

dmnyc commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

@claude review

@claude

claude Bot commented Aug 15, 2026

Copy link
Copy Markdown

Claude finished @dmnyc's task in 3m 24s —— View job


Review: docs(security) — at-rest & provider-visibility threat model

This is a genuinely good addition — the framing ("this is the standard tradeoff of light wallets; we do not claim otherwise") is the right tone for a security policy, and it lands the four scan findings it targets. But because this is SECURITY.md, every sentence is a promise, and I found one claim the shipped build cannot deliver plus several that are stronger or less precise than the code supports.


1. Blocking: the self-hosted Esplora escape hatch does not exist in the shipped UI

You can point Apogee at your own Esplora (Settings → Chain server) to restrict this to infrastructure you control.

This isn't true on HEAD. The Chain server control is a presets-only <select>src/sidepanel/screens/Wallet.tsx:2252-2270, options from chainPresetsFor() at Wallet.tsx:2731-2745, which returns exactly Automatic / Liquid.network / Blockstream.info. Both presets are the same public operators the paragraph warns about.

Three independent confirmations:

  • Wallet.tsx:1896-1898"Free-form custom URLs were removed — planned to return inside a future debug panel; the SW/engine plumbing still accepts any validated URL."
  • Wallet.tsx:1950-1957 — a custom URL persisted by an older build is actively cleared on load ("the picker is presets-only now ... rather than let an unrepresentable override keep steering scans").
  • manifest.shared.ts:20-47 — production host_permissions enumerate waterfalls, *.blockstream.info, liquid.network, price APIs and SideSwap. Loopback is mode === "development" only. Even if the picker accepted a URL, a store build cannot reach a self-hosted node.

The scan doc's M-7 mitigation line ("a user-configurable genesis-pinned chain-server override enables self-hosted Esplora") describes the engine plumbing, which does still validate arbitrary URLs — but the user-facing capability was removed. A security policy has to describe what ships.

Suggested replacement:

  balances and activity. Settings → Advanced → Chain server lets you choose
  which public Esplora provider is used; pointing Apogee at a self-hosted
  node is not currently exposed in the UI.

Note the path is also Settings → Advanced → Chain server (the field lives inside the Advanced drawer, Wallet.tsx:2241-2252), not Settings → Chain server.

Fix this →


2. "an Electrum waterfall server" — wrong protocol

Waterfalls is an Esplora-API server, consumed through new lwk.EsploraClient(...) (src/engine/engine-core.ts:642; endpoints at engine-core.ts:71-75). There is no Electrum protocol anywhere in the codebase. Suggest just "a Waterfalls scan server".

3. Waterfalls receives the descriptor, not the address set

The bullet says sync "sends the wallet's address set to an Electrum waterfall server (encrypted to that server's key) with fallback to public Esplora". Per engine-core.ts:63-70, lwk fetches the server's age recipient key and encrypts the descriptor to it — that's how one request replaces ~40 gap-limit queries. That's strictly more disclosure than an address set: the server can derive every future address too, not just the current gap window. The plain-Esplora fallback (engine-core.ts:656-660) is the path that issues per-address queries.

Worth splitting, because the difference is the interesting part:

- **Chain-data providers see your whole wallet.** The default sync path sends
  the full descriptor — encrypted to the Waterfalls server's own key, so it is
  that server, not a passive observer, that reads it — which lets the operator
  derive and cluster every address you will ever use. On fallback, public
  Esplora instances (liquid.network, blockstream.info) instead receive the
  gap-limited address set over TLS. Either operator — or whoever compels
  them — can observe your balances and activity.

4. SideSwap sees your whole balance of the send asset, not just what you spend

tells the dealer (SideSwap) the UTXOs you spend

src/sideswap/orchestrator.ts:216-224 fetches all wallet UTXOs and filters only by asset (filterSendAssetUtxos, orchestrator.ts:120-131), then hands the entire filtered set to startQuotes (orchestrator.ts:249-256) — before any coin selection. The dealer therefore learns your complete UTXO set (and thus balance) for the send asset, including outputs the swap never spends. "Other holdings are not revealed" remains correct — the filter is by asset ID. Suggest "every UTXO you hold of the asset you're selling — not just the ones the swap spends".

5. "the asset ID and exact base-unit amounts are always shown alongside" — only on the send approval screen

True there: src/sidepanel/screens/Approval.tsx:308-327 renders an Asset row and a Base units row. Three caveats against "always":

  • The Base units row is conditional on sendReview.assetPrecision != null (Approval.tsx:321) — which is itself registry data, so a registry that omits precision removes the safety row.
  • The asset ID is middle-truncated to shortenHex(assetId, 18, 12); the full value is hover-only.
  • Elsewhere, hex is a fallback for the label, not a companion to it: Swap.tsx:174-186, 864, 957 and Wallet.tsx:972, 1165, 1565 all read KNOWN_ASSETS[id]?.label ?? info?.ticker ?? info?.name ?? shortenHex(id, …). In the swap flow and the balance list, a registry-supplied ticker is displayed with no asset ID next to it.

Also, L-13's actual recommendation (badge registry-sourced labels as unverified in approval screens) isn't implemented, so the doc shouldn't imply the UI already neutralizes the issue. Suggest scoping: "on the send approval screen the asset ID and exact base-unit amount are shown alongside" + "elsewhere in the UI a registry label may appear on its own".


Smaller points

6. "every derived address ... is stored unencrypted". What apogee_keystore holds is ct(slip77(..),elwpkh(..)) — the descriptor (src/keystore/keystore.ts:55), i.e. the xpub every address derives from, not a stored address list. Immaterial to the threat, but this paragraph's value is its precision: "the extended public key every address derives from, plus the SLIP-77 master blinding key".

7. The device-level bullet undersells its own inventory. It claims a profile reader gets history, amounts, scan state and metadata — all true — but the scan's storage inventory also lists two at-rest items exposed to exactly this adversary and not covered by the descriptor: apogee:asseticon:<assetId> keys, which fingerprint which assets you hold and survive wallet/reset (L-8, src/lib/asset-icons.ts:35,53), and the 7-day cleartext manifest/lending terminal records carrying txid, action and account identifier (L-9, src/tx-manifest/idempotency.ts:8-40). Since #115 asks for an honest at-rest model, one clause covering them would close the gap. The genuinely positive counterpart is also worth a sentence: storage.sync is never used, so nothing goes to browser-vendor cloud sync.

8. A fourth network disclosure is missing. The section says "the network connections below are how a light wallet works, and they are the one place your wallet activity is observable" — then lists three. Asset metadata and icons are fetched per asset ID from https://liquid.network/api/v1/asset (src/lib/asset-icons.ts:19-20) and via lwk.Registry (engine-core.ts:1190-1195), disclosing which assets you hold to a third party. Fiat price sources (Coinbase/Kraken/CoinGecko/CoinPaprika/blockchain.info/mempool.space) receive only a currency code — cheap to state, and stating it strengthens the claim. Either add a bullet or soften "below" to "the main ones".

9. The "See also" description overstates the linked doc. It promises "severity ratings, and remediation status"; docs/security-vulnerability-scan-2026-08.md has severities and recommendations but no remediation/status tracking. Suggest "full findings, severity ratings, and recommendations".

10. Consider stating the scan's date/authorship inline. "2026-08 security vulnerability scan" reads as a first-party audit; the doc itself is explicit that it's an automated GLM 5.3 pass, not a human audit, and audits/ holds the human ones. One qualifier keeps the link from over-promising.


Nits

  • Pre-existing, in the line just above this diff: "issues requiring a already-compromised device" → "an already-compromised". Cheap to fix while you're here.
  • The SLIP-77 sentence "losing it does not risk funds, but it is what keeps your amounts private" is exactly right and matches keystore.ts:55 / L-10 — no change, just flagging it as the strongest line in the section.

Verdict

Direction and tone are right, and items 2–10 are wording fixes. Item 1 should block merge: as written, SECURITY.md would offer users a privacy mitigation the shipped extension does not have, which is worse than not mentioning one. Everything else I checked in the section — PBKDF2 → AES-256-GCM, cleartext descriptor at rest, descriptor-keyed scan-state encryption, SideSwap blinding factors, fresh receive/change addresses, asset-only UTXO filtering, "no telemetry" — verified accurate against the code.
fix/115-threat-model-docs

@dmnyc

dmnyc commented Aug 15, 2026

Copy link
Copy Markdown
Contributor Author

Review feedback addressed in 46dd65d — every finding, since a security policy only works if every sentence is true of the shipped build:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[M-6 + M-7 + L-10 + L-15] Document the at-rest and provider-visibility threat model

1 participant