bitlogin/docs/entropy-audit-2026-07-31.md

# BitLogin — randomness and RNG-integration audit, 2026-07-31

Audit of every security-sensitive random value in `packages/`, against a
Coldcard-class checklist: where each bit of unpredictability originates, whether
that source is present in the production build, and what happens when it fails.

## Verdict

**No predictable-fallback defect exists.** `packages/core/src/crypto/random.ts`
is the single chokepoint for every secret this codebase mints, and it is correct:

- It throws when `crypto.getRandomValues` is unavailable rather than falling
  back to anything (`random.ts:5-9`). There is no second code path.
- `randomUniformInt` (`random.ts:36-49`) uses **rejection sampling**, so
  wordlist and charset selection carry no modulo bias.
- `generatePrivateKey` (`crypto/secp256k1.ts:16-21`) rejection-samples until the
  scalar is in range rather than clamping or reducing.
- Nothing seeds a CSPRNG manually; nothing uses timestamps, PIDs, device IDs or
  counters as entropy.
- Crypto dependencies are exact-pinned, not caret-ranged (`@noble/curves`
  1.9.7, `@noble/hashes` 1.8.0, `@scure/bip39` 1.6.0, `@scure/base` 1.2.6), and
  CI emits a CycloneDX SBOM plus per-artifact SHA256SUMS.

Entropy accounting is sound: 128 bits into a 12-word BIP-39 phrase
(`randomEntropy128`), 256-bit connection-vault root and sudo key, 128-bit
account id, ~77 bits for the default six-word EFF passphrase with a hard floor
at 64 bits (`account/passphrase.ts:28-30`). No source is truncated before use,
and no transformation is treated as creating entropy.

The work below is hardening, not repair.

---

## Open items

### M1 — Nonce-injection hooks are reachable from the shipped public API — **DONE 2026-07-31**

`nip44Encrypt` and `nip04Encrypt` no longer take a nonce or IV at all. The
capability moved to `packages/core/src/crypto/testing.ts` as
`__unsafeNip44EncryptWithNonce` / `__unsafeNip04EncryptWithIv`, and
`crypto/index.ts` now re-exports explicitly instead of `export *`, so the
underlying `encryptWithNonce` / `encryptWithIv` stay off the package surface.

It turned out only **one** test ever used an override — a nonce-length
validation check — and `ivOverride` had no callers at all. The official NIP-44
vectors do not need it. So nothing was lost by closing this.

Verified from a real consumer package linking the built `@bitlogin/core`, not
from source:

| Attempt | Result |
| --- | --- |
| `import "@bitlogin/core/crypto/testing.js"` | `ERR_PACKAGE_PATH_NOT_EXPORTED` |
| `import "@bitlogin/core/dist/crypto/testing.js"` | `ERR_PACKAGE_PATH_NOT_EXPORTED` |
| `import "@bitlogin/core/crypto/nip44.js"` | `ERR_PACKAGE_PATH_NOT_EXPORTED` |
| `"encryptWithNonce" in cryptoSurface` | `false` |
| `nip44Encrypt.length` / `nip04Encrypt.length` | `2` / `3` — no nonce or IV parameter |

Four regression tests pin the closed surface, including a fresh-nonce check
across 64 calls. `knownAnswer.test.ts` stayed green throughout, which is the
signal that matters: the derivation chain is untouched and no account is
affected.

The original problem statement follows.

### M1 (original) — Nonce-injection hooks are reachable from the shipped public API

`nip44Encrypt` and `nip04Encrypt` accept caller-supplied nonces:

```ts
// packages/core/src/crypto/nip44.ts:124
export function nip44Encrypt(conversationKey: Uint8Array, plaintext: string, nonceOverride?: Uint8Array): string {
  const nonce = nonceOverride ?? randomBytes(32);

// packages/core/src/crypto/nip04.ts:24
  ivOverride?: Uint8Array
): string {
  const iv = ivOverride ?? randomBytes(16);
```

These exist for a legitimate reason — the official NIP-44 v2 test vectors pin
the nonce, and `crypto.test.ts` cannot verify against them otherwise. No
production caller passes either parameter; the only other references are the
generated `.d.ts` declarations.

The problem is the surface, not the current callers. Both functions are exported
from `@bitlogin/core/crypto`, so the override is part of what this package
advertises. Nonce reuse in NIP-44 (ChaCha20) and NIP-04 (AES-CBC) is not a
gradual weakening — it is immediate plaintext recovery, and for CBC a repeated
IV leaks the first block relationship directly. A parameter that silently
accepts a fixed nonce should not be one autocomplete away from application code.

**Fix.** Keep the vectors working, remove the footgun:

1. Rename the parameterised forms to `__unsafeNip44EncryptWithNonce` /
   `__unsafeNip04EncryptWithIv` and move them into a `crypto/testing.ts` module
   that is **not** listed in the `exports` map of `packages/core/package.json`.
2. Leave `nip44Encrypt` / `nip04Encrypt` with two- and three-argument
   signatures that always call `randomBytes`.
3. Point `crypto.test.ts` and `knownAnswer.test.ts` at the new module via a
   relative import.

**Acceptance.** `crypto.test.ts` still passes the official NIP-44 vectors;
`import { nip44Encrypt } from "@bitlogin/core/crypto"` exposes no nonce
parameter in its type signature; `npm run security:ci` is green.

**Care required.** This touches the derivation chain's neighbourhood but not the
chain itself — no capsule bytes change. `knownAnswer.test.ts` must stay green
without its expected values being edited. If it goes red, stop: per the standing
rule in `TODO.md`, that means the change is account-breaking.

---

### M2 — Nothing tests that the RNG fails closed — **DONE 2026-07-31**

Landed in `packages/core/src/crypto/crypto.test.ts` as two new blocks, nine
tests, bringing that file to 59 and the suite to 181.

Both guards were mutation-verified rather than assumed:

| Mutation | Result |
| --- | --- |
| `webcrypto()` returns a zero-filling stub instead of throwing | 5 fail-closed tests red |
| `randomUniformInt` uses bare `value % maxExclusive` | chi-square **5614** against a threshold of 147 |

The second number is the point of the exercise: a modulo-biased generator
still produces different-looking values every call and passes every
self-consistency test in the suite. Only a distribution test sees it.

The original problem statement follows.

This is the gap the whole exercise turns on. The crypto suite is genuinely good
— known-answer coverage for HKDF, ScalarExpand, the Argon2id profile, BIP-39
phrase and seed, Schnorr, AES-GCM tamper-detection, JCS ordering, padding
buckets. **None of it asserts what happens when randomness is unavailable.**

`random.ts:5-9` is correct today. Nothing in CI would report it if a future
refactor made it return zeros, or added an `?? fallback`, or moved the guard
behind a condition that no longer fires. A deterministic stream passes every
self-consistency test in the suite.

**Fix.** Add to `packages/core/src/crypto/crypto.test.ts`:

```ts
describe("RNG fails closed (§11.1)", () => {
  it("throws when no secure random source exists", () => {
    const saved = Object.getOwnPropertyDescriptor(globalThis, "crypto");
    try {
      Object.defineProperty(globalThis, "crypto", { value: undefined, configurable: true });
      expect(() => randomBytes(32)).toThrow(/no cryptographically secure/i);
      expect(() => randomEntropy128()).toThrow();
      expect(() => generatePrivateKey()).toThrow();
    } finally {
      if (saved) Object.defineProperty(globalThis, "crypto", saved);
    }
  });

  it("throws when crypto exists but getRandomValues does not", () => {
    const saved = Object.getOwnPropertyDescriptor(globalThis, "crypto");
    try {
      Object.defineProperty(globalThis, "crypto", { value: {}, configurable: true });
      expect(() => randomBytes(32)).toThrow(/no cryptographically secure/i);
    } finally {
      if (saved) Object.defineProperty(globalThis, "crypto", saved);
    }
  });
});
```

Add a companion uniformity test proving the rejection loop actually rejects —
without it, `randomUniformInt` could degrade to `value % maxExclusive` and stay
green:

```ts
it("randomUniformInt is unbiased across a modulus that does not divide 256", () => {
  const M = 94, N = 200_000;
  const counts = new Array(M).fill(0);
  for (let i = 0; i < N; i++) counts[randomUniformInt(M)]++;
  const expected = N / M;
  const chi2 = counts.reduce((a, c) => a + (c - expected) ** 2 / expected, 0);
  expect(chi2).toBeLessThan(160); // df=93, p<0.001 critical value ~147
});
```

**Acceptance.** Both tests pass on `main`. Deliberately breaking `random.ts`
(returning a zero-filled array, or swapping the rejection loop for a bare
modulo) must turn them red — verify this locally before committing, then revert.

---

### 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`:

```ts
function pickRandomIndices(max: number, count: number): number[] {
  ...
  const idx = Math.floor(Math.random() * pool.length);
```

Called once, at `element.ts:857`, to choose which three recovery-phrase words to
ask the user to re-type after registration. **This is not a secret and leaks no
entropy** — the phrase itself comes from `randomEntropy128`, and an attacker who
cannot see the phrase gains nothing from predicting which words get quizzed.

It is worth changing anyway, for a process reason. As long as one `Math.random`
call survives in security-adjacent source, a static-analysis rule banning
`Math.random` (checklist §13) needs a standing exception — and standing
exceptions are where the next real one hides.

**Fix.** Import `randomUniformInt` from `@bitlogin/core/crypto` and replace the
`Math.floor(Math.random() * pool.length)` expression. Then add the ban rule to
the semgrep config so the file cannot regress.

**Note.** `src/vendor/bitlogin/bitlogin.js:1696` in the **bitunlock** repo is
this same function, vendored. Fixing it here and re-running
`scripts/build-bitlogin-widget.mjs` there clears both.

---

### 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
scoped to one socket; it is not a secret and collision is the only failure mode.
No change needed. Recorded so the next audit does not re-flag it.

---

## Not findings, recorded so they are not re-litigated

- **`params.now` test hook** in `account/create.ts:62` injects a timestamp, not
  entropy. Timestamps are already public in Nostr event envelopes.
- **`@noble` v2 upgrade.** Out of scope here, and blocked for an unrelated
  reason already captured in `TODO.md` — noble v2 dropped the extensionless
  subpath exports this code imports.

## Sequencing

M2 first — it is additive, cannot break the derivation chain, and it is what
protects M1's fix from silently regressing later. Then M1. Then L1, batched
with whatever else next touches `element.ts`.