mirror of
https://github.com/PR0M3TH3AN/bitlogin.git
synced 2026-11-08 14:31:26 +00:00
Ban Math.random package-wide and enforce it in CI
Neither remaining Math.random call was a secret. The backup-quiz picker chooses which three recovery words to re-type -- an attacker who cannot see the phrase gains nothing from predicting them -- and the relay subscription id is a public wire identifier scoped to one socket, where collision is the only failure mode. They are changed anyway, because the point is the rule rather than these two call sites. A Math.random ban that carries a standing exception is worth much less than one that does not: the exception is what a future misuse hides behind, and reviewers stop reading a warning they have already agreed to ignore twice. Both now use the CSPRNG (randomUniformInt, and bytesToHex(randomBytes(8)) for the subscription id, same 16-hex-character shape as before), which costs nothing here. With no exception left, the rule can be absolute. .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 -- the COLDCARD defect class - bitlogin-no-time-or-id-seeded-secrets: no hashing Date.now() or performance.now() into a key Verified the rules fire rather than merely passing, which is the failure mode of a lint rule nobody tests: reintroducing the old Math.floor(Math.random() * pool.length) produces 1 blocking finding and exit 1, and the tree is clean at 0 findings with it restored. Path patterns are anchored (/packages/...) so semgrep's upcoming Semgrepignore v2 change does not silently alter their scope. Typecheck and the full suite pass (185 tests). This closes the entropy audit for this repo. The only gate remaining is an independent review of the RNG path by someone other than the author. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01HtNMsKzzFTPURemX7tT19u
This commit is contained in:
1 parent
e131f0313b
commit
98b2967116
6 files changed
+112
-20
No files matched your search
+9
-3
@@ -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
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user