From 3192c9f3060bc4354d3af06aa396ff401705c068 Mon Sep 17 00:00:00 2001 From: thePR0M3TH3AN <53631862+PR0M3TH3AN@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:43:24 -0400 Subject: [PATCH] Offer a 24-word master seed, and put the entropy draw beyond reach of any handler L2: generate_bip85_seed and create_profile_from_generated_seed now take words_num, accepting 12 or 24 and rejecting anything else with a clear message. The default stays 12 -- every existing profile uses it, 128 bits is entirely adequate, and moving the default would change nothing about existing vaults while surprising anyone creating a new one. 24 is offered because the master seed protects the whole vault and was the one secret shorter than the seeds derived beneath it, which already default to 24. L3: fixed structurally rather than by narrowing exception types. os.urandom(32) now sits OUTSIDE the try block, so no handler can stand between the entropy draw and the caller -- an entropy failure is unreachable by anything that could substitute a value. Also removes a redundant `except Exception` whose body was identical to the `Bip85Error` branch above it. The structural form is stronger than an exception list, and the test says so precisely: it asserts the OSError reaching the caller is the SAME OBJECT os.urandom threw, which holds only if nothing caught and repackaged it. Listing exception types would still leave a handler in the path for someone to widen later. Adds five tests: the identity assertion above, 12- and 24-word generation, the default staying 12, and rejection of 0/11/18/25/-12. Determinism gate green (89 tests, up from 84). The seed/profile regression subset is 143 passed with one failure -- test_profile_export_import_round_trip -- confirmed against the pre-change baseline as pre-existing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01HtNMsKzzFTPURemX7tT19u --- TODO.md | 14 +++++--- docs/entropy_audit_2026-07-31.md | 37 +++++++++++++++++++-- src/constants.py | 11 +++++++ src/seedpass/core/manager.py | 43 ++++++++++++++++++------- src/tests/test_entropy_integrity.py | 50 +++++++++++++++++++++++++++++ 5 files changed, 138 insertions(+), 17 deletions(-) diff --git a/TODO.md b/TODO.md index d5196baf..e3f1d3b1 100644 --- a/TODO.md +++ b/TODO.md @@ -41,8 +41,13 @@ wild.** v1 is frozen; a fix that changes derivation goes into a new version, nev - [ ] **Opt-in upgrade UX for existing entries** — a per-entry action that re-derives at v2, with a confirmation making the consequence unmissable ("this changes the password; update it at the site first"). Never bulk-migrate. Until this ships, existing entries stay on v1 by design. -- [ ] **L2** — master seed is `os.urandom(32)` reduced to a hardcoded 12-word (128-bit) mnemonic - while derived seeds default to 24 words. Not a defect; offer 24 words at profile creation. +- [x] **L2 (done)** — `generate_bip85_seed(words_num=...)` accepts 12 or 24 and rejects anything + else; threaded through `create_profile_from_generated_seed`. Default stays 12 so existing + profiles and callers are unaffected. +- [x] **L3 (done for the RNG path)** — `os.urandom(32)` moved outside the try block in + `generate_bip85_seed`, so no handler can stand between the entropy draw and the caller, and + the redundant duplicate `except Exception` is gone. A test asserts the `OSError` arrives as + the same object that was thrown, which is only true if nothing repackaged it. - [x] **L4 (code fixes done)** — `torch/` temp filename now uses `randomBytes(8)`; the `Math.random` value misnamed `nonce` in `relay-health.mjs` is now `probeSuffix` from `randomBytes(6)`. @@ -56,8 +61,9 @@ wild.** v1 is frozen; a fix that changes derivation goes into a new version, nev `requirements.lock`. Verified with the real tool: CycloneDX 1.6, 96 components, crypto chain recorded at exact versions. -L3 (broad `except Exception` around crypto — these re-raise, so no silent fallback) overlaps the -Robustness item further down. +The remaining broad `except Exception` handlers in `password_generation.py` wrap deterministic +derivation with no RNG in it and re-raise rather than substituting; they belong to the Robustness +item further down, not to this audit. ## Security diff --git a/docs/entropy_audit_2026-07-31.md b/docs/entropy_audit_2026-07-31.md index 1a9a86e0..046cb082 100644 --- a/docs/entropy_audit_2026-07-31.md +++ b/docs/entropy_audit_2026-07-31.md @@ -320,7 +320,19 @@ anyway, to prove that. --- -### L2 — Master seed entropy accounting +### L2 — Master seed entropy accounting — **DONE 2026-07-31** + +`generate_bip85_seed` and `create_profile_from_generated_seed` now take +`words_num`, accepting 12 or 24 and rejecting anything else. **The default stays +12**: every existing profile uses it, it is entirely adequate, and moving the +default would surprise without improving any existing vault. 24 is now available +for a 256-bit master seed, which removes the oddity that the secret protecting +the whole vault was shorter than the seeds derived beneath it (those default to +24). + +The original note follows. + +### L2 (original) — Master seed entropy accounting `generate_bip85_seed` (`core/manager.py:1810-1812`) draws `os.urandom(32)` — 256 bits — then derives a **12-word** mnemonic, which encodes 128. The word @@ -338,7 +350,28 @@ unaffected — this only changes what new seeds look like. --- -### L3 — Broad `except Exception` around crypto +### L3 — Broad `except Exception` around crypto — **DONE 2026-07-31** (for the RNG path) + +Fixed structurally rather than by narrowing exception types. `os.urandom(32)` +now sits **outside** the try block in `generate_bip85_seed`, so no handler can +stand between the entropy draw and the caller. The redundant duplicate +`except Exception` — whose body was identical to the `Bip85Error` branch above +it — is gone. + +This is stronger than listing which exceptions to catch: an entropy failure is +now unreachable by any handler that could substitute a value, and +`test_entropy_draw_sits_outside_the_try_block` asserts the raised `OSError` +arrives as the *same object* that `os.urandom` threw, which is only true if +nothing caught and repackaged it. + +The remaining broad handlers in `password_generation.py` (lines 280, 485) wrap +deterministic derivation with no RNG in it, and they log and re-raise rather +than substituting. They are covered by the general "replace broad +`except Exception`" item in `TODO.md` rather than being an entropy concern. + +The original note follows. + +### L3 (original) — Broad `except Exception` around crypto `password_generation.py:280`, `:485`, `core/manager.py:1817`. All of these log-and-re-raise, so **there is no silent fallback** — the operation aborts, as diff --git a/src/constants.py b/src/constants.py index fffc8a34..6c5467b1 100644 --- a/src/constants.py +++ b/src/constants.py @@ -49,6 +49,17 @@ MAX_PASSWORD_LENGTH = 128 # Maximum allowed password length # Characters considered safe for passwords when limiting punctuation SAFE_SPECIAL_CHARS = "!@#$%^*-_+=?" +# Master seed word count. +# ----------------------------------- +# 12 words = 128 bits, 24 words = 256 bits. The default stays 12: it is +# entirely adequate and every existing profile uses it, so changing the default +# would alter nothing about existing vaults but would surprise. 24 is offered +# because the master seed protects the whole vault and was previously the one +# secret shorter than the seeds derived beneath it, which default to 24 +# (entropy audit L2, docs/entropy_audit_2026-07-31.md). +DEFAULT_SEED_WORD_COUNT = 12 +SUPPORTED_SEED_WORD_COUNTS = frozenset({12, 24}) + # Timeout in seconds before the vault locks due to inactivity INACTIVITY_TIMEOUT = 15 * 60 # 15 minutes diff --git a/src/seedpass/core/manager.py b/src/seedpass/core/manager.py index f6fd6903..62f9fdbf 100644 --- a/src/seedpass/core/manager.py +++ b/src/seedpass/core/manager.py @@ -89,6 +89,8 @@ from constants import ( DEFAULT_PASSWORD_LENGTH, INACTIVITY_TIMEOUT, DEFAULT_SEED_BACKUP_FILENAME, + DEFAULT_SEED_WORD_COUNT, + SUPPORTED_SEED_WORD_COUNTS, NOTIFICATION_DURATION, initialize_app, ) @@ -1802,35 +1804,54 @@ class PasswordManager: logging.error(f"Error validating BIP-85 seed: {e}") return False - def generate_bip85_seed(self) -> str: + def generate_bip85_seed(self, words_num: int = DEFAULT_SEED_WORD_COUNT) -> str: """ Generates a new BIP-85 seed phrase. + Parameters: + words_num (int): 12 (128-bit, default) or 24 (256-bit). The default + stays 12 so existing callers are unaffected; 24 is offered + because the master seed protects the whole vault and was + previously shorter than the seeds derived beneath it, which + default to 24 (entropy audit L2). + Returns: - str: The generated 12-word mnemonic seed phrase. + str: The generated mnemonic seed phrase. """ + if words_num not in SUPPORTED_SEED_WORD_COUNTS: + raise SeedPassError( + f"Seed word count must be one of {sorted(SUPPORTED_SEED_WORD_COUNTS)}, " + f"got {words_num!r}." + ) + + # Deliberately OUTSIDE the try block (entropy audit L3). Nothing below + # may catch an entropy failure and substitute a value -- that is the + # COLDCARD defect class. If os.urandom raises, it propagates unchanged + # and the operation aborts, which is the only acceptable outcome. + master_seed = os.urandom(32) + try: - master_seed = os.urandom(32) # Generate a random 32-byte seed bip85 = BIP85(master_seed) - mnemonic = bip85.derive_mnemonic(index=0, words_num=12) - return mnemonic + return bip85.derive_mnemonic(index=0, words_num=words_num) except Bip85Error as e: logging.error(f"Failed to generate BIP-85 seed: {e}", exc_info=True) print(colored(f"Error: Failed to generate BIP-85 seed: {e}", "red")) raise SeedPassError(f"Failed to generate BIP-85 seed: {e}") from e - except Exception as e: - logging.error(f"Failed to generate BIP-85 seed: {e}", exc_info=True) - print(colored(f"Error: Failed to generate BIP-85 seed: {e}", "red")) - raise SeedPassError(f"Failed to generate BIP-85 seed: {e}") from e def create_profile_from_generated_seed( self, *, password: str, seed: Optional[str] = None, + words_num: int = DEFAULT_SEED_WORD_COUNT, ) -> tuple[str, str]: - """Create a new profile from a generated seed and return ``(fingerprint, seed)``.""" - new_seed = seed or self.generate_bip85_seed() + """Create a new profile from a generated seed and return ``(fingerprint, seed)``. + + ``words_num`` selects the master seed length (12 or 24) and is ignored + when ``seed`` is supplied, since that phrase already fixes its own + length. + """ + new_seed = seed or self.generate_bip85_seed(words_num=words_num) fingerprint = self._finalize_existing_seed(new_seed, password=password) if not fingerprint: raise SeedPassError("Failed to create profile from generated seed.") diff --git a/src/tests/test_entropy_integrity.py b/src/tests/test_entropy_integrity.py index 9c0b6d53..4f5c3b1d 100644 --- a/src/tests/test_entropy_integrity.py +++ b/src/tests/test_entropy_integrity.py @@ -191,6 +191,56 @@ def test_seed_generation_aborts_on_short_read(monkeypatch): manager_module.PasswordManager.generate_bip85_seed(object()) +def test_entropy_draw_sits_outside_the_try_block(monkeypatch): + """L3: no except clause may stand between os.urandom and the caller. + + The generator's exception handling wraps BIP-85 failures in SeedPassError, + which is fine -- but an entropy failure must never be reachable by any + handler that could substitute a value. Keeping the draw outside the try is + the structural version of that guarantee, stronger than narrowing which + exception types get caught. + """ + from seedpass.core import manager as manager_module + + sentinel = OSError("entropy pool unavailable") + + def _explode(_n): + raise sentinel + + monkeypatch.setattr(manager_module.os, "urandom", _explode) + + with pytest.raises(OSError) as excinfo: + manager_module.PasswordManager.generate_bip85_seed(object()) + + # Propagates as itself, not repackaged -- proof nothing caught it. + assert excinfo.value is sentinel + + +@pytest.mark.parametrize("words_num,expected", [(12, 12), (24, 24)]) +def test_master_seed_word_count_is_selectable(words_num, expected): + """L2: the master seed can now be 256-bit, not only 128-bit.""" + from seedpass.core.manager import PasswordManager + + mnemonic = PasswordManager.generate_bip85_seed(object(), words_num=words_num) + assert len(mnemonic.split()) == expected + + +def test_master_seed_default_is_unchanged(): + """Existing profiles are 12-word; the default must not move under them.""" + from seedpass.core.manager import PasswordManager + + assert len(PasswordManager.generate_bip85_seed(object()).split()) == 12 + + +def test_unsupported_word_count_is_rejected(): + from seedpass.core.errors import SeedPassError + from seedpass.core.manager import PasswordManager + + for bad in (0, 11, 18, 25, -12): + with pytest.raises(SeedPassError, match="Seed word count"): + PasswordManager.generate_bip85_seed(object(), words_num=bad) + + def test_all_zero_entropy_is_not_currently_detected(monkeypatch): """Recorded gap, not an endorsement.