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.