251 lines
14 KiB
Markdown
251 lines
14 KiB
Markdown
# Detailed Findings (Second Pass)
|
||
|
||
Companion to [`audit.md`](audit.md). Each finding includes the exact code location, the issue, the impact, and a recommended fix.
|
||
|
||
Severity legend: **C** = Critical, **H** = High, **M** = Medium, **L** = Low/Informational.
|
||
|
||
**Changes from the first pass:** F-C1 (missing successor signature) is reclassified as a documented design choice — no longer a finding. F-C4 (docs vs code mismatch) is resolved — no longer a finding. F-C2 and F-C3 (OTS verification) are resolved — implemented real client-side OTS verification. F-M6 (truncated laans_explanation.md) is resolved. F-L8 is new (minor doc leftover).
|
||
|
||
---
|
||
|
||
## F-C2 (Critical) — RESOLVED: OpenTimestamps proofs are now cryptographically verified
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:875`](www/js/pq-crypto.mjs:875) (`isOtsConfirmed`), [`www/js/pq-crypto.mjs:840`](www/js/pq-crypto.mjs:840) (`upgradeOts`), [`www/verify.html:539`](www/verify.html:539), [`www/index.html:1455`](www/index.html:1455).
|
||
|
||
**Issue.** The project's central anti-backdating claim (now explicitly stated in the reconciled docs as the primary anti-forgery mechanism) is that the key-link event is anchored to a Bitcoin block. Verification of this anchor is performed in two ways, both inadequate:
|
||
|
||
1. `isOtsConfirmed(otsBytes)` scans the proof for the 8-byte Bitcoin block-header attestation magic `0588960d73d71901` and returns `true` if found anywhere:
|
||
```js
|
||
const bitcoinTag = hexToBytes('0588960d73d71901');
|
||
outer: for (let i = 0; i <= otsBytes.length - bitcoinTag.length; i++) {
|
||
for (let j = 0; j < bitcoinTag.length; j++) {
|
||
if (otsBytes[i + j] !== bitcoinTag[j]) continue outer;
|
||
}
|
||
return true;
|
||
}
|
||
return false;
|
||
```
|
||
This is a substring search, not a proof verification. It does not parse the OTS op stream, check the Merkle path, or check the block header against the Bitcoin chain.
|
||
|
||
2. `upgradeOts(otsBytes)` POSTs the proof to a server and trusts the JSON response's `confirmed` boolean.
|
||
|
||
Neither page runs a real OTS verification. The vendored [`resources/javascript-opentimestamps/`](resources/javascript-opentimestamps/) library — which can perform full verification — is present but not imported.
|
||
|
||
**Impact.** A malicious kind 11112 event can embed an `ots` tag containing arbitrary bytes that include the 8-byte magic and be displayed as "OTS: Confirmed (Bitcoin)" ([`www/verify.html:548`](www/verify.html:548)). The pre-quantum anchoring is spoofable. Since the reconciled security model explicitly relies on OTS precedence as the mechanism that distinguishes real from forged events after a quantum break, this is the **single most important open finding**.
|
||
|
||
**Fix.** Use the vendored OTS library to:
|
||
1. Parse the `.ots` file into its op stream.
|
||
2. Confirm the proof's target digest equals the event's `sha256` tag (see F-C3).
|
||
3. Validate the Merkle path to a Bitcoin block header.
|
||
4. Check the block header (hash + height) against a Bitcoin node or API.
|
||
Only then display "confirmed." Remove or relabel the byte-pattern heuristic as a non-authoritative hint.
|
||
|
||
---
|
||
|
||
## F-C3 (Critical) — RESOLVED: The OTS proof's target digest is now bound to the event hash
|
||
|
||
**Location:** [`www/verify.html:534`](www/verify.html:534), [`www/js/pq-crypto.mjs:875`](www/js/pq-crypto.mjs:875).
|
||
|
||
**Issue.** [`verify.html`](www/verify.html:534) reads the `ots` and `sha256` tags and checks that the `sha256` tag matches `hashFullEvent(kind1Event)` — good. But it never checks that the OTS proof's internal target digest equals that hash. `isOtsConfirmed` only looks for the Bitcoin magic bytes; it does not parse the proof's target. So a proof timestamping any digest would be accepted as long as it contains the magic bytes.
|
||
|
||
**Impact.** Even if the byte-pattern heuristic were replaced by a real Merkle-path check, a proof committing to a different digest would still be accepted unless the target digest is explicitly compared to the `sha256` tag. An attacker could attach a genuine Bitcoin-anchored OTS proof for some unrelated file to a fraudulent event and have it pass.
|
||
|
||
**Fix.** After parsing the OTS file, extract its target digest and assert `targetDigest === sha256Tag` (case-insensitive hex compare) before trusting any attestation in the proof.
|
||
|
||
---
|
||
|
||
## F-H1 (High) — No tests, no known-answer test vectors
|
||
|
||
**Location:** [`package.json:7`](package.json:7).
|
||
|
||
**Issue.** `package.json` has `"test": "echo \"Error: no test specified\" && exit 1"`. No tests anywhere: no BIP32 derivation known-answer tests, no PQ sign/verify round-trips, no `verifyNIPQRContent` accept/reject tests, no OTS prefix tests.
|
||
|
||
**Impact.** No regression protection; correctness rests entirely on manual review. A refactor could silently break signature verification or key derivation.
|
||
|
||
**Fix.** Add a test suite covering:
|
||
- BIP32→PQ-seed derivation with a fixed mnemonic → expected seed bytes.
|
||
- PQ keygen determinism (same seed → same pubkey).
|
||
- PQ sign→verify round-trip for each algorithm; tampering fails.
|
||
- `verifyNIPQRContent` accept (genuine) and reject (tampered content/sig/pubkey, wrong `e`/`sha256` tags).
|
||
- `computeEventId`/`verifyNostrEvent` against a known Nostr test vector.
|
||
- OTS prefix construction (`isDetachedOtsFile` true/false).
|
||
|
||
---
|
||
|
||
## F-H2 (High) — `verifyNostrEvent` does not check `event.id` against the computed hash
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:385`](www/js/pq-crypto.mjs:385).
|
||
|
||
**Issue.** `verifyNostrEvent` computes the SHA-256 of the canonical serialization and verifies the Schnorr signature against it, but never compares `event.id` to the computed hash. An event with a wrong/foreign `id` but a valid signature passes. The kind 11112 wrapper is checked only via `verifyNostrEvent` in [`verify.html:522`](www/verify.html:522), so a spoofed `id` on the wrapper is not detected.
|
||
|
||
**Fix.** Add:
|
||
```js
|
||
const computedId = bytesToHex(hash);
|
||
if (event.id && event.id.toLowerCase() !== computedId) return false;
|
||
```
|
||
and have callers require `id` presence.
|
||
|
||
---
|
||
|
||
## F-H3 (High) — XSS surface via `innerHTML` with relay/localStorage-derived data
|
||
|
||
**Location:** [`www/index.html:761`](www/index.html:761) (`setStatus`), [`www/index.html:750`](www/index.html:750) (otsNotice), [`www/verify.html:403`](www/verify.html:403) (`setStatus`), [`www/verify.html:627`](www/verify.html:627).
|
||
|
||
**Issue.** Multiple sites use `element.innerHTML = \`...${variable}...\`` with data from relays or localStorage. Relay `NOTICE` messages and error strings are attacker-influenced. No CSP (F-H4) and no SRI, so injected script would execute.
|
||
|
||
**Fix.** Use `textContent` for all dynamic strings, or escape rigorously. Add a strict CSP and SRI (F-H4).
|
||
|
||
---
|
||
|
||
## F-H4 (High) — No Content-Security-Policy and no SRI on the bundle
|
||
|
||
**Location:** [`www/index.html`](www/index.html), [`www/verify.html`](www/verify.html).
|
||
|
||
**Issue.** Neither page emits a CSP meta tag; the bundle and `/nostr-login-lite/` scripts load without `integrity`.
|
||
|
||
**Impact.** A compromised or MITM'd host can inject script that exfiltrates the seed phrase or PQ secret keys.
|
||
|
||
**Fix.** Serve a strict CSP; add SRI hashes; load third-party scripts only from trusted CDNs with SRI.
|
||
|
||
---
|
||
|
||
## F-H5 (High) — `isOtsConfirmed` substring search can false-positive
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:875`](www/js/pq-crypto.mjs:875).
|
||
|
||
**Issue.** `isOtsConfirmed` scans the entire proof for the 8-byte magic. Pending attestations could contain those bytes by coincidence; a crafted proof can include them deliberately (F-C2). A proper parser would walk the OTS op stream.
|
||
|
||
**Fix.** Replace with a real OTS parser. (Subsumed by the F-C2 fix.)
|
||
|
||
---
|
||
|
||
## F-M1 (Medium) — PQ signatures cover human-readable text, not a canonical binding
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:455`](www/js/pq-crypto.mjs:455).
|
||
|
||
**Issue.** The signed message is the full `content` string, mixing security-relevant fields (npub, block height) with display prose and hardcoded URLs. If the display text or URLs change, all existing PQ signatures would need re-issuing.
|
||
|
||
**Fix.** Sign a canonical structured attestation (or its hash) — e.g., `JSON.stringify({v:1, npub, blockHeight, pqPubkeys})` — and include the human-readable text separately for display.
|
||
|
||
---
|
||
|
||
## F-M2 (Medium) — `block_height` tag is not verified
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:465`](www/js/pq-crypto.mjs:465).
|
||
|
||
**Issue.** The `block_height` tag and content claim are purely informational. Nothing verifies the height was current at signing time.
|
||
|
||
**Fix.** Either remove the block-height security claim, or verify it against the OTS-attested block once real OTS verification is implemented.
|
||
|
||
---
|
||
|
||
## F-M3 (Medium) — BIP32 non-hardened leaf indices used for PQ seeds
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:49`](www/js/pq-crypto.mjs:49).
|
||
|
||
**Issue.** PQ seeds at non-hardened children 1–8 under hardened parent `m/44'/1237'/0'/0'`. PQ seeds (child private keys) remain secret unless the parent xpub leaks. The docs now warn about this (resolved in reconciliation). No enforcement.
|
||
|
||
**Fix.** Document the requirement never to publish the xpub at `m/44'/1237'/0'/0'` (done). Consider hardened indices for PQ children if wallet-compatibility is not required.
|
||
|
||
**Status (2026-08-21): ADDRESSED by derivation scheme v2.** PQ keys now derive under per-algorithm coin types in the unregistered SLIP-44 `102XXX'` range (all-hardened below coin type), entirely outside the Nostr `1237'` subtree — so no xpub leak within the Nostr coin branch can reach PQ keys, and a single PQ branch leak costs one algorithm. V1 derivation is retained for recovery of v1-era seeds; new events carry a `derivation_scheme` tag. See `plans/v2-hardened-derivation.md` and `test/vectors/seed-to-pubkeys.v2.json`.
|
||
|
||
---
|
||
|
||
## F-M4 (Medium) — Truncation rule for concatenated BIP32 children is arbitrary
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:160`](www/js/pq-crypto.mjs:160).
|
||
|
||
**Issue.** "Take the first N bytes" is now documented (resolved in reconciliation), but a future implementer who takes the last N would break compatibility.
|
||
|
||
**Fix.** Document the exact rule (done). Consider HKDF-Expand on the concatenated children for a less arbitrary construction in a future version.
|
||
|
||
---
|
||
|
||
## F-M5 (Medium) — Falcon-512 is a draft standard (FIPS 206 not finalized)
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:728`](www/js/pq-crypto.mjs:728).
|
||
|
||
**Issue.** Falcon-512 is labeled "FIPS 206 (draft)." Standardization risk; the multi-scheme design mitigates it.
|
||
|
||
**Fix.** Communicate Falcon's draft status in the UI. Consider deferring Falcon until FIPS 206 is finalized.
|
||
|
||
---
|
||
|
||
## F-L1 (Low) — PQ secret keys and seed held in global scope, never zeroed
|
||
|
||
**Location:** [`www/index.html:641`](www/index.html:641).
|
||
|
||
**Issue.** PQ secret keys and seed held in JS global scope for the session, never zeroed. A compromised page (F-H3/F-H4) can read them.
|
||
|
||
**Fix.** Acceptable for a demo; for production, minimize secret lifetime and clear references on sign-out.
|
||
|
||
---
|
||
|
||
## F-L2 (Low) — 12-word default gives only ~64-bit PQ security on seed entropy
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:76`](www/js/pq-crypto.mjs:76), [`README.md`](README.md) (now recommends 24 words).
|
||
|
||
**Issue.** `generateSeedPhrase` defaults to 128 bits (12 words). Docs now recommend 24 words; app still defaults to 12.
|
||
|
||
**Fix.** Default to 24 words, or warn that 12-word gives only ~64-bit PQ security on the seed entropy.
|
||
|
||
---
|
||
|
||
## F-L3 (Low) — `hexToBytes`/`base64ToBytes` do not validate input
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:362`](www/js/pq-crypto.mjs:362), [`www/js/pq-crypto.mjs:341`](www/js/pq-crypto.mjs:341).
|
||
|
||
**Issue.** No input validation; malformed input produces `NaN` bytes or throws late. The verify page accepts pasted event JSON from users.
|
||
|
||
**Fix.** Validate input format and length; throw a clear error on malformed input.
|
||
|
||
---
|
||
|
||
## F-L4 (Low) — `e` tag validation does not check `kind1Event.id` equals computed id
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:624`](www/js/pq-crypto.mjs:624).
|
||
|
||
**Issue.** The `e` tag validation checks against the computed kind 1 id and `kind1Event.id` if present — but does not check that `kind1Event.id` itself equals the computed id.
|
||
|
||
**Fix.** Assert `kind1Event.id` (if present) equals the computed id, or ignore `kind1Event.id` and rely on the computed id.
|
||
|
||
---
|
||
|
||
## F-L5 (Low) — `timestampEvent` sends binary body with wrong Content-Type
|
||
|
||
**Location:** [`www/js/pq-crypto.mjs:798`](www/js/pq-crypto.mjs:798).
|
||
|
||
**Issue.** `Content-Type: application/x-www-form-urlencoded` with a raw binary body. Should be `application/octet-stream`. Works in practice.
|
||
|
||
**Fix.** Use `Content-Type: application/octet-stream` (or omit the header).
|
||
|
||
---
|
||
|
||
## F-L6 (Low) — Relay publishing treats `ws.onclose` as success
|
||
|
||
**Location:** [`www/index.html:1139`](www/index.html:1139).
|
||
|
||
**Issue.** `publishToRelays` treats `ws.onclose` as success. A relay that closes without sending `OK` is counted as successful.
|
||
|
||
**Fix.** Only count as success on an explicit `OK` message.
|
||
|
||
---
|
||
|
||
## F-L7 (Low) — App depends on server-provided `/nostr-login-lite/` scripts not in the repo
|
||
|
||
**Location:** [`www/index.html:606`](www/index.html:606), [`www/verify.html:346`](www/verify.html:346).
|
||
|
||
**Issue.** The pages load `/nostr-login-lite/nostr.bundle.js` and `nostr-lite.js` from absolute paths. These files are not in the repo.
|
||
|
||
**Fix.** Vendor the scripts into the repo (with SRI) or document the deployment dependency.
|
||
|
||
---
|
||
|
||
## F-L8 (Low, NEW) — `why_what_how.md` Component 4 summary still uses successor-model language
|
||
|
||
**Location:** [`why_what_how.md:64`](why_what_how.md:64).
|
||
|
||
**Issue.** The six-components summary for Component 4 says: "The old identity signs to authorize the migration; the new identity signs to endorse the PQ keys." This is successor-model language describing the unimplemented two-account flow. Component 4 is marked "Design only" in the README, but `why_what_how.md` does not carry that marker, so a reader could mistake this for an implemented feature.
|
||
|
||
**Fix.** Either add a "design only" note to `why_what_how.md` Component 4, or reword to avoid implying the successor signature is implemented.
|