diff --git a/.github/workflows/security.yml b/.github/workflows/security.yml index 9c7a707..dbd8388 100644 --- a/.github/workflows/security.yml +++ b/.github/workflows/security.yml @@ -48,6 +48,8 @@ jobs: run: pipx install semgrep==1.171.0 - name: Scan repository rules without per-rule timeouts run: semgrep scan --config auto --error --timeout 0 packages + - name: Enforce local randomness rules (entropy audit §13) + run: semgrep scan --config .semgrep.yml --error --timeout 0 packages - name: Scan repository history for secrets uses: gitleaks/gitleaks-action@v2 env: diff --git a/.semgrep.yml b/.semgrep.yml new file mode 100644 index 0000000..2c66c73 --- /dev/null +++ b/.semgrep.yml @@ -0,0 +1,56 @@ +# Local semgrep rules, run alongside `--config auto` in +# .github/workflows/security.yml. +# +# Checklist §13 of docs/entropy-audit-2026-07-31.md: make the randomness rules +# machine-enforced rather than review-enforced. Every rule here is currently +# clean across packages/ — they exist to keep it that way, not to flag a +# backlog. +rules: + - id: bitlogin-no-math-random + languages: [typescript, javascript] + severity: ERROR + message: >- + Math.random() is not a CSPRNG and must not be used anywhere in this + package, including for values that are not secrets. Use randomBytes or + randomUniformInt from @bitlogin/core/crypto. A ban with a standing + exception is where the next real misuse hides -- if you believe this + specific call is safe, the correct move is still to use the CSPRNG, which + costs nothing here. + paths: + include: + - /packages/*/src/** + pattern: Math.random(...) + + - id: bitlogin-no-crypto-fallback + languages: [typescript, javascript] + severity: ERROR + message: >- + Do not fall back to another source when getRandomValues is unavailable. + Key generation must fail closed -- "continue with reduced security" is the + COLDCARD defect class. crypto/random.ts throws by design; keep it that way. + paths: + include: + - /packages/*/src/** + patterns: + - pattern-either: + - pattern: $C.getRandomValues(...) ?? $FALLBACK + - pattern: $C.getRandomValues(...) || $FALLBACK + - pattern: | + try { ... $C.getRandomValues(...) ... } catch(...) { ... return $FALLBACK; ... } + + - id: bitlogin-no-time-or-id-seeded-secrets + languages: [typescript, javascript] + severity: ERROR + message: >- + Timestamps, counters and identifiers are not entropy. A secret derived + from Date.now() or performance.now() is predictable to anyone who knows + roughly when it was created. + paths: + include: + - /packages/*/src/** + patterns: + - pattern-either: + - pattern: | + $KEY = $HASH(Date.now(), ...) + - pattern: | + $KEY = $HASH(performance.now(), ...) diff --git a/docs/TODO.md b/docs/TODO.md index a1ef5d9..5ef7d3d 100644 --- a/docs/TODO.md +++ b/docs/TODO.md @@ -21,9 +21,15 @@ avoid modulo bias. Three hardening items, in order: Verified from a real consumer against the built package: all three deep-import routes fail with `ERR_PACKAGE_PATH_NOT_EXPORTED`. `knownAnswer.test.ts` green throughout — the derivation chain is untouched. -- **L1** — `Math.random` in the backup-quiz word picker (`element.ts:1471`). - Not a secret, but it forces a standing exception in any `Math.random` ban rule. - Last remaining item from the entropy audit in this repo. +- **L1 — done.** Both `Math.random` calls (backup-quiz picker, relay subscription + id) now use the CSPRNG, so the ban has no standing exception. `.semgrep.yml` + enforces it in CI alongside two more rules: no fallback around + `getRandomValues`, and no timestamp-seeded secrets. Verified the rules fire — + reintroducing the old call gives 1 blocking finding and exit 1. + +**The entropy audit is closed for this repo.** The only gate left is an +independent review of the RNG path by someone other than the author, which is +not something a coding agent can sign off. ## Owner action items diff --git a/docs/entropy-audit-2026-07-31.md b/docs/entropy-audit-2026-07-31.md index 097491e..e35ef06 100644 --- a/docs/entropy-audit-2026-07-31.md +++ b/docs/entropy-audit-2026-07-31.md @@ -188,7 +188,29 @@ modulo) must turn them red — verify this locally before committing, then rever --- -### L1 — `Math.random` in the backup-verification quiz +### L1 — `Math.random` in the backup-verification quiz — **DONE 2026-07-31** + +`pickRandomIndices` now uses `randomUniformInt`, and `relay.ts`'s subscription +id is `bytesToHex(randomBytes(8))` rather than a hash over `Math.random()`. +Neither was a secret; both were changed so the ban below has **no standing +exception**. + +That ban is now machine-enforced. `.semgrep.yml` adds three rules over +`packages/*/src/**`, run in CI as a separate step after `--config auto`: + +- `bitlogin-no-math-random` — no `Math.random` anywhere, secret or not +- `bitlogin-no-crypto-fallback` — no `?? fallback`, `|| fallback`, or + catch-and-substitute around `getRandomValues` +- `bitlogin-no-time-or-id-seeded-secrets` — no hashing `Date.now()` / + `performance.now()` into a key + +Verified the rules actually fire rather than merely passing: reintroducing the +old `Math.floor(Math.random() * pool.length)` produces 1 blocking finding and +exit 1; the tree is clean at 0 findings with it restored. + +The original note follows. + +### L1 (original) — `Math.random` in the backup-verification quiz `packages/widget/src/element.ts:1471`: @@ -218,7 +240,7 @@ this same function, vendored. Fixing it here and re-running --- -### L2 — `Math.random` in the relay subscription id +### L2 — `Math.random` in the relay subscription id — **DONE 2026-07-31** (folded into L1) `packages/core/src/nostr/relay.ts:223` mixes `Math.random()` into the material hashed for a subscription id. A subscription id is a public wire identifier diff --git a/packages/core/src/nostr/relay.ts b/packages/core/src/nostr/relay.ts index 3cf8fb2..f605864 100644 --- a/packages/core/src/nostr/relay.ts +++ b/packages/core/src/nostr/relay.ts @@ -2,14 +2,11 @@ * A single relay WebSocket connection: NIP-01 EVENT/REQ/CLOSE, NIP-42 AUTH, * and NIP-11 relay information document fetches (§11.8, §16.1, §17.3, §19.2). */ -import { - computeEventId, - signNostrEvent, - verifyNostrEvent, - type NostrEvent, -} from "./event.js"; +import { signNostrEvent, verifyNostrEvent, type NostrEvent } from "./event.js"; import { KIND_AUTH } from "./kinds.js"; import { getPublicKeyHex } from "../crypto/secp256k1.js"; +import { randomBytes } from "../crypto/random.js"; +import { bytesToHex } from "../crypto/encoding.js"; import { isAllowedRelayUrl } from "./relayUrl.js"; export interface NostrFilter { @@ -215,13 +212,11 @@ export class RelayConnection { timeoutMs = 8000, ): Promise { await this.connect(); - const subId = computeEventId({ - pubkey: "0".repeat(64), - created_at: Date.now(), - kind: 0, - tags: [], - content: JSON.stringify(filter) + Math.random(), - }).slice(0, 16); + // Subscription ids are public wire identifiers scoped to one socket, not + // secrets — collision is the only failure mode. Drawn from the CSPRNG + // regardless, so `Math.random` stays banned package-wide (.semgrep.yml) + // with no standing exception to reason about at review time. + const subId = bytesToHex(randomBytes(8)); return new Promise((resolve, reject) => { const events: NostrEvent[] = []; diff --git a/packages/widget/src/element.ts b/packages/widget/src/element.ts index 309e26a..26840de 100644 --- a/packages/widget/src/element.ts +++ b/packages/widget/src/element.ts @@ -7,6 +7,7 @@ import { type RecoveryExportFile } from "@bitlogin/core/account"; import { encodeNpub } from "@bitlogin/core/nostr"; +import { randomUniformInt } from "@bitlogin/core/crypto"; import { WorkerClient } from "./worker/workerClient.js"; import { createNip07Provider, type Nip07Provider } from "./provider.js"; import { readConfigFromElement } from "./config.js"; @@ -1468,12 +1469,22 @@ export class BitLoginAuthElement extends HTMLElement { } } +/** + * Picks `count` distinct indices in [0, max) for the write-it-down confirmation + * quiz. + * + * This is not a secret and leaks no entropy — the recovery phrase itself comes + * from randomEntropy128, and an attacker who cannot see the phrase gains + * nothing from predicting which words get quizzed. It uses the CSPRNG anyway so + * that `Math.random` can be banned outright in this package (see + * .semgrep.yml). A ban with a standing exception is where the next real misuse + * hides. + */ function pickRandomIndices(max: number, count: number): number[] { const pool = Array.from({ length: max }, (_, i) => i); const chosen: number[] = []; for (let i = 0; i < count && pool.length > 0; i++) { - const idx = Math.floor(Math.random() * pool.length); - chosen.push(pool.splice(idx, 1)[0]!); + chosen.push(pool.splice(randomUniformInt(pool.length), 1)[0]!); } return chosen.sort((a, b) => a - b); }