Skip to content

Fix/wallet encryption hardening#3

Open
0x3639 wants to merge 8 commits into
digitalSloth:mainfrom
0x3639:fix/wallet-encryption-hardening
Open

Fix/wallet encryption hardening#3
0x3639 wants to merge 8 commits into
digitalSloth:mainfrom
0x3639:fix/wallet-encryption-hardening

Conversation

@0x3639

@0x3639 0x3639 commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hardens the offline brute-force boundary for encrypted wallets (a High-severity review finding). Encrypted keyfiles are persisted to browser storage, but the app only required 8-character passwords and the SDK's Argon2id KDF runs at timeCost: 1 — so an attacker who exfiltrates a keyfile can brute-force it offline, where the in-memory unlock throttling offers no protection. This PR raises both axes of that boundary: passphrase entropy (search space) and KDF cost (time per guess).

What changed

Passphrase strength

  • New pure estimator src/core/password-strength.ts — entropy ≈ length × log2(charset pools present), scored 0–4. No dictionary/blocklist (deliberate, to keep the bundle lean).
  • New PasswordStrengthMeter.vue — live strength bar + label + suggestion, shown in the Create and Import flows.
  • Floor raised: MIN_PASSWORD_LENGTH 8 → 12, plus MIN_PASSWORD_SCORE = 2 ("Fair"). Submit is blocked until the password clears the floor. Note: a 12-char single-character-class password is intentionally rejected — clearing the floor requires ≥2 character classes.

KDF cost + migration

  • Argon2id timeCost 1 → 3 for new/imported wallets.
  • The SDK reads a static KeyFile.DEFAULT_CONFIG at hash time and the keyfile JSON doesn't persist its KDF params, so a naive bump would make existing wallets undecryptable. Instead, src/core/kdf.ts withKdfParams() sets/restores that config around each encrypt/decrypt (cast confined to one file), and each wallet records its kdfVersion in our own storage.
  • Transparent migration: existing wallets (no kdfVersion) decrypt at the legacy cost, then are silently re-encrypted to the stronger params on next unlock — best-effort, wrapped so a failure can never block the unlock or corrupt stored ciphertext.

Migration & compatibility

  • New wallets: strong KDF from creation.
  • Existing wallets: decrypt with legacy params (which match the SDK default exactly, so they always decrypt), then upgrade on first unlock. No user action, no data loss.
  • The unlock throttling/lockout and the unlock dialog are unchanged.

Testing

  • npm run typecheck, npm run lint, npm run build all pass (no new errors).
  • Manual: create/import (meter + floor enforced), lock/unlock a new wallet, and confirmed a pre-existing wallet still unlocks and upgrades to the new params.

0x3639 and others added 5 commits June 4, 2026 11:07
Passphrase entropy floor + strength meter, and an Argon2id timeCost
increase with no-patch KDF orchestration and upgrade-on-unlock migration.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@digitalSloth

digitalSloth commented Jun 5, 2026

Copy link
Copy Markdown
Owner

Looks good, but I think this points at an underlying issue in the SDK which we should address first.

The KeyFile.DEFAULT_CONFIG is hardcoded in keyFile.ts and only the argon2Params.salt is written to the KeyFileEncryptedData I think we need to do 3 things:

  • Extend KeyFileEncryptedData.crypto.argon2Params to include timeCost, memoryCost, and parallelism (not just salt). These should be written on encrypt and read back on decrypt — the keyfile should be self-describing, like PBKDF2 stores its iteration count in PKCS structures.
  • Make encrypt() accept an optional KDF config param, which defaults to DEFAULT_CONFIG but can be overridden per-call.
  • Add a new static helper method to the keyFile: static needsUpgrade(json: KeyFileEncryptedData, target: Partial<KdfConfig>): boolean

I realise this is more work than you signed up for but I think its best to fix at source and then this PR becomes a bit simpler. Let me know if you want to have a go at the SDK PR, if not I can do it.

I had a quick go at the SDK changes, see what you think - digitalSloth/znn-typescript-sdk#20

One other note can gitignore your docs/superpowers directory or omit the files from the PRs please? They are useful to see the PR creating process but we dont want loads of these docs in the repo really

These design docs are useful locally for the PR-authoring process but
shouldn't live in the repo. Ignore the directory and remove the tracked
spec from version control (the file stays on disk).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@0x3639

0x3639 commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator Author

Went ahead and gitignored + untracked docs/superpowers/. Good catch.

Approach on changing the SDK makes sense. PR#20 looks good. No comments there. I can simplify this PR once the SDK is updated. I'll post my review over on that PR.

@digitalSloth

Copy link
Copy Markdown
Owner

@0x3639 New SDK version is available - https://github.com/digitalSloth/znn-typescript-sdk/releases/tag/v1.0.4

0x3639 and others added 2 commits June 14, 2026 12:14
Replaces the DEFAULT_CONFIG mutation workaround and our own kdfVersion
tracking with the SDK's self-describing keyfile, per PR digitalSloth#3 review.

- Bump znn-typescript-sdk to ^1.0.4
- encrypt(keyStore, KDF_CONFIG): SDK persists the Argon2id params in the
  keyfile; decrypt reads them back (falls back to legacy DEFAULT_CONFIG
  for paramless keyfiles)
- Gate upgrade-on-unlock on KeyFile.needsUpgrade() instead of kdfVersion
- Delete src/core/kdf.ts (withKdfParams private-static hack)
- Collapse config KDF version machinery to a single KDF_CONFIG target
- argon2Params gains optional timeCost/memoryCost/hashLength/parallelism;
  drop redundant Wallet.kdfVersion

Verified: legacy paramless wallets decrypt and re-encrypt to timeCost:3
(new salt) on unlock; new wallets persist strong params from creation.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces the dependency-free entropy heuristic (which still passed
keyboard walks, monotonic runs, and dictionary words) with zxcvbn-ts.

- zxcvbn core + language packs are dynamically imported into their own
  chunk, loaded on first password keystroke - not the initial bundle.
- estimatePasswordStrength is async; usePasswordStrength wraps it as a
  reactive ref that fails closed on every change (resets to EMPTY until
  the new score resolves) with a sequence token to drop stale results.
- Create/Import submit handlers snapshot the submitted values up front,
  re-score that snapshot as the authoritative gate, set the in-flight
  flag before the await (no double-submit), and create/import only from
  the snapshot - so editing the still-enabled fields mid-await can't
  swap a weaker password past the gate.

Verified: weak patterns (keyboard walks, monotonic runs, common words)
score below the floor; strong passwords pass; zxcvbn chunks load on
demand; strong->weak->submit creates no wallet; rapid double-click
creates exactly one wallet; and an imported wallet unlocks only with the
submitted password, not a value swapped into the fields after submit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@0x3639

0x3639 commented Jun 14, 2026

Copy link
Copy Markdown
Collaborator Author

Pushed two commits that act on your SDK feedback and tighten up the password gate.

1. Self-describing keyfile (SDK v1.0.4)

Adopted znn-typescript-sdk@^1.0.4 and removed the app-side workaround entirely:

  • Deleted the KeyFile.DEFAULT_CONFIG shim (src/core/kdf.ts) and our own kdfVersion tracking.
  • New wallets: KeyFile.encrypt(keyStore, KDF_CONFIG) — the SDK now persists the Argon2id params in the keyfile.
  • Unlock: KeyFile.decrypt(...) reads those params back (and falls back to the legacy default for old keyfiles), and the upgrade-on-unlock check is now KeyFile.needsUpgrade(keyfile, KDF_CONFIG).
  • Collapsed the config to a single KDF_CONFIG target; argon2Params gained the optional
    timeCost/memoryCost/hashLength/parallelism fields.

Net: −86 lines in that commit. Migration verified end-to-end — a legacy paramless timeCost:1 wallet decrypts and re-encrypts to timeCost:3 (with a fresh salt) on first unlock; new wallets persist the strong params from creation.

⚠️ Migration note: wallets created by an earlier revision of this branch (encrypted at timeCost:3 but paramless, due to the old shim) won't decrypt under the new self-describing decrypt. No shipped users are affected — main wallets are paramless timeCost:1 and migrate cleanly.

2. Password strength → lazy-loaded zxcvbn

Replaced the dependency-free entropy heuristic with zxcvbn-ts (dictionary / keyboard-pattern / sequence aware), since the heuristic still let keyboard walks and monotonic runs through.

  • Lazy-loaded: zxcvbn core + language packs are dynamically imported into their own chunk, loaded on the first password keystroke — not in the initial bundle (verified: 0 chunks before typing, 3 after). This keeps the bundle lean, which was the original reason for avoiding a dictionary.
  • Fail-closed: usePasswordStrength resets to EMPTY (gate closed) on every change until the new (async) score resolves, with a sequence token to drop stale results.
  • Authoritative submit gate: the Create/Import handlers snapshot the submitted values up front, re-score that snapshot, set the in-flight flag before the await (no double-submit), and create/import only from the snapshot — so editing the still-enabled fields mid-submit can't swap a weaker password past the gate.

Verified: qwertyuiopasdf, abc…xyz, and common words now score below the floor; strong passwords pass; strong→weak→submit creates no wallet; a rapid double-click creates exactly one wallet; and an imported wallet unlocks only with the submitted password.

Out of scope (pre-existing, not from this PR)

  • npm run typecheck fails on broader SDK BigNumber/Vue model type drift (PillarTab, PlasmaTab, stake-service, etc.) — present on main, unrelated to wallet hardening.
  • npm run build:extension fails on missing icons/icon-{16,48,128}.png — also fails on main; a packaging gap, not touched here.

Happy to address either in a separate PR.

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.

2 participants