From 8564029c2b77fa8f301c84f6533b158331b70d68 Mon Sep 17 00:00:00 2001 From: usermaneGermane Date: Thu, 12 Nov 2026 14:13:26 +0200 Subject: [PATCH] Security fix: take a vault key only with a signature this device trusts Affects the SecretSpace 3 test versions alpha.6 and alpha.7. The app took whatever wrapped vault key the server listed, so a dishonest server could wrap a key of its own, call it a new key generation, and read the vault uploaded under it. - Every wrapped key is signed by the account that wrapped it (account_sync.key_message / sign_key), on create, share and key change. - check_key: a vault this device does not have is taken when signed by its own account or by another account (remembered on first sight, shown as not checked). A new key for a vault it has must be signed by its own account, by the signer of the key it holds, or by a contact whose fingerprint was compared. - Never: an older key generation, another key at the same generation, a key without a signature (KeyNotTrusted; nothing changed, nothing sent). - Vaults from alpha.6/7 are signed at the next sync by a device that holds them. A server without "signed-keys" gets only the Personal vault. - Remove / Change key name the members never checked on this PC. - A plain http:// address of another machine asks before it is used. Needs SecretSpace Server 2.0.0-alpha.5. Tests: test_account_key_trust.py. Co-Authored-By: Claude Fable 5.2 Claude-Session: https://claude.ai/code/session_011ojnyVg7ccsR3pZ64s7aRH --- README.md | 17 +- sourceFiles/account_sync.py | 182 +++++++- sourceFiles/account_sync_ui.py | 22 +- sourceFiles/accounts.py | 3 +- sourceFiles/recovery_ui.py | 4 + sourceFiles/sharing_ui.py | 23 +- .../tests/sync_server/passman_server.py | 65 ++- sourceFiles/tests/test_account_key_trust.py | 392 ++++++++++++++++++ sourceFiles/version.py | 2 +- 9 files changed, 679 insertions(+), 31 deletions(-) create mode 100644 sourceFiles/tests/test_account_key_trust.py diff --git a/README.md b/README.md index 8daef6c..e5fd649 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ package replaces it in one step (`Conflicts`/`Replaces: passman`), and your vaults stay in `~/.local/share/passman`, which SecretSpace keeps using — nothing is moved or renamed. The vault files are the same. -**`main` is 1.0.0-alpha.7, a SecretSpace 3 test version — not for real passwords yet.** +**`main` is 1.0.0-alpha.8, a SecretSpace 3 test version — not for real passwords yet.** For real use keep the [0.11.0 release](https://github.com/usermaneGermane/SecretSpace-Client-Linux/releases/tag/v0.11.0), which the rest of this README describes. It installs from a `.deb` on Ubuntu 22.04 or newer, or runs from source. It has so far been tested in CI only, @@ -31,14 +31,14 @@ on a virtual display, not yet on a real desktop. **Same vaults as on Windows.** The vault file, the sync protocol and the features match SecretSpace for Windows (see the version table under *What is different from Windows*): 0.11.0 = Windows 2.13.0, and this test version = -Windows 3.0.0-alpha.7. +Windows 3.0.0-alpha.8. A vault moves between a Windows PC and a Linux PC unchanged, and both sync with either server. ## SecretSpace 3 — the test version -1.0.0-alpha.7 is the same as [SecretSpace for Windows -3.0.0-alpha.7](https://github.com/usermaneGermane/SecretSpace-Client-Windows#secretspace-3--a-test-version-not-for-real-passwords-yet): +1.0.0-alpha.8 is the same as [SecretSpace for Windows +3.0.0-alpha.8](https://github.com/usermaneGermane/SecretSpace-Client-Windows#secretspace-3--a-test-version-not-for-real-passwords-yet): accounts with a Secret Key and a 12-word recovery phrase, Argon2id, and importing 2.x vaults with their history. It opens SecretSpace 3 accounts only, so do not install it over 0.11.0 — your vaults would not show up (they are @@ -56,7 +56,11 @@ copied a password. Sharing needs SecretSpace Server 2.0.0-alpha.3. Since alpha.7 a forgotten master password is no longer the end: *Forgot your master password?* on the login screen opens the account with its **12-word recovery phrase**, sets a new password and makes a new phrase (needs -SecretSpace Server 2.0.0-alpha.4 for an account on a server). On Linux: +SecretSpace Server 2.0.0-alpha.4 for an account on a server). **alpha.8 is +a security fix** for alpha.6 and alpha.7: a vault key is taken only with +a signature of your own account or of someone you have reason to trust, +so a dishonest server cannot slip in a key of its own (needs SecretSpace +Server 2.0.0-alpha.5 for anything but the Personal vault). On Linux: - **The Secret Key is kept under your master password** (Argon2id, then AES-GCM), not by the system as Windows does with DPAPI. So a copy of the @@ -68,7 +72,7 @@ SecretSpace Server 2.0.0-alpha.4 for an account on a server). On Linux: - The account folders are `700` and their files `600`, like the vault folder; a saved Emergency Kit is `600` too. - *Settings → Backup → Open folder* opens the folder with `xdg-open`. -- The package spells the version `1.0.0~alpha.7`, which apt sorts after +- The package spells the version `1.0.0~alpha.8`, which apt sorts after 0.9.0 and before 1.0.0. | File | What it is | @@ -204,6 +208,7 @@ Where that README names a Windows version, read the Linux one beside it: | 3.0.0-alpha.5 | 1.0.0-alpha.5 | several vaults per account; Sync covers them all (Server 2.0.0-alpha.2) | | 3.0.0-alpha.6 | 1.0.0-alpha.6 | sharing a vault (owner / editor / reader), who changed what (Server 2.0.0-alpha.3) | | 3.0.0-alpha.7 | 1.0.0-alpha.7 | recovering an account with the 12-word phrase, a new phrase (Server 2.0.0-alpha.4) | +| 3.0.0-alpha.8 | 1.0.0-alpha.8 | security fix: signed vault keys (Server 2.0.0-alpha.5) | (0.2.0 added the `.deb` package, which has no Windows counterpart.) diff --git a/sourceFiles/account_sync.py b/sourceFiles/account_sync.py index fc88b8f..d056a0f 100644 --- a/sourceFiles/account_sync.py +++ b/sourceFiles/account_sync.py @@ -25,6 +25,8 @@ """ import base64 +import hashlib +import json import os import urllib.parse @@ -52,11 +54,22 @@ #: ... and of one that recovers an account with its phrase (2.0.0-alpha.4). FEATURE_RECOVERY = "recovery" +#: ... and of one that keeps a signature with every wrapped vault key +#: (2.0.0-alpha.5). Without it this client syncs only the personal vault: +#: a key that comes with no signature is never taken (see check_key). +FEATURE_SIGNED_KEYS = "signed-keys" + class LoginFailed(SyncError): """The server did not take this username + password + Secret Key.""" +class KeyNotTrusted(SyncError): + """The server offers a vault key this device has no reason to trust: it + is not signed by this account or by someone known here. Nothing is + taken, nothing is uploaded under it.""" + + class KeyChanged(SyncError): """The server gives other keys for an account than this device has seen for it: a new account under an old name, or a server that lies.""" @@ -110,6 +123,14 @@ def has_recovery(self) -> bool: self.has_vaults() return FEATURE_RECOVERY in self._features + def has_signed_keys(self) -> bool: + self.has_vaults() + return FEATURE_SIGNED_KEYS in self._features + + def put_key_sig(self, vault_id: str, key_sig: str) -> None: + self._request("PUT", f"/v2/vaults/{vault_id}/key-sig", + body={"key_sig": key_sig}) + # -- recovery (phase 5) ---------------------------------------------------- def recovery_copy(self, username: str) -> dict: return self._request("GET", "/v2/recovery/" + urllib.parse.quote( @@ -139,10 +160,12 @@ def members(self, vault_id: str) -> dict: return self._request("GET", f"/v2/vaults/{vault_id}/members") def put_member(self, vault_id: str, username: str, role: str, - wrapped_key: str = None) -> dict: + wrapped_key: str = None, key_sig: str = None) -> dict: body = {"role": role} if wrapped_key: body["wrapped_key"] = wrapped_key + if key_sig: + body["key_sig"] = key_sig return self._request("PUT", f"/v2/vaults/{vault_id}/members/" + urllib.parse.quote(username, safe=""), body=body) @@ -157,9 +180,11 @@ def vault_log(self, vault_id: str) -> list: return list(self._request("GET", f"/v2/vaults/{vault_id}/log") .get("lines") or []) - def create_vault(self, vault_id: str, sealed_name: str, wrapped_key: str) -> dict: + def create_vault(self, vault_id: str, sealed_name: str, wrapped_key: str, + key_sig: str = "") -> dict: return self._request("POST", "/v2/vaults", body={ - "id": vault_id, "name": sealed_name, "wrapped_key": wrapped_key}) + "id": vault_id, "name": sealed_name, "wrapped_key": wrapped_key, + "key_sig": key_sig}) def rename_vault(self, vault_id: str, sealed_name: str) -> dict: return self._request("PUT", f"/v2/vaults/{vault_id}/name", @@ -215,6 +240,99 @@ def put_vault(self, vault_id: str, blob: str, revision: int) -> int: return int(reply.get("revision", revision + 1)) +def is_plain_remote(url: str) -> bool: + """Is this an unencrypted (http://) address of another machine? On such + a connection someone on the network can take the login and act as the + account on the server (they still cannot read a vault).""" + parts = urllib.parse.urlsplit((url or "").strip()) + if parts.scheme.lower() != "http": + return False + host = (parts.hostname or "").lower() + return host not in ("localhost", "::1") and not host.startswith("127.") + + +# --------------------------------------------------------------------------- +# Whose key is this? (security review, 2026-11-12) +# --------------------------------------------------------------------------- +# The server stores each member's copy of a vault key but must never be able +# to choose one: if it could hand a device a key of its own making, the +# device would encrypt the vault under it at the next sync and the server +# could read everything. So every wrapped key carries a signature by the +# account that wrapped it, and a device takes a key only when it is signed +# by itself or by someone it has reason to trust. +def key_message(vault_hex: str, generation: int, member: str, wrapped_key: str, + signer: str) -> bytes: + """What the signature on a wrapped key covers: which vault, which key + generation, for whom, the wrapped key itself, and who vouches.""" + return json.dumps(["PassMan3 vault key", vault_hex, int(generation), member, + hashlib.sha256(wrapped_key.encode("ascii")).hexdigest(), + signer], separators=(",", ":"), + ensure_ascii=True).encode("ascii") + + +def sign_key(account: accounts.Account, vault_hex: str, generation: int, + member: str, wrapped_key: str) -> str: + return account_crypto.sign(account.keyset, key_message( + vault_hex, generation, member, wrapped_key, account.username)) + + +def check_key(account: accounts.Account, server: "AccountServer", there: dict, + name: str, held_from: str = None) -> str: + """ + Decide whether to take the vault key the server lists (``there``, one + vault of /v2/me) or raise :class:`KeyNotTrusted`. Returns who signed it. + + A key signed by this account is always its own. Otherwise: + + - for a vault this device does not have yet (``held_from`` None), the + signer may be anyone; their keys are remembered on first sight, and + the vault shows as shared by someone "not checked" until the user + compares fingerprints; + - for a vault this device already has (a new key generation), the + signer must be whoever signed the key held now (``held_from``) or + someone whose fingerprint the user has compared. A name the server + could have introduced on its own is not enough to replace a key. + """ + signer = str(there.get("key_signer") or "") + sig = str(there.get("key_sig") or "") + wrapped = str(there.get("wrapped_key") or "") + if not sig or not signer: + raise KeyNotTrusted( + f"The server's key for the vault “{name}” has no signature (it " + "was put there by an older SecretSpace). Sync once from a device " + "that already has this vault, or ask its owner to change the " + "vault's key (Members → Change key). Nothing was changed.") + if signer == account.username: + key = account_crypto.signing_key_of(account.keyset) + else: + seen = account.contact(signer) + new_vault = held_from is None + if seen is not None and (new_vault or seen["verified"] + or signer == held_from): + key = seen["signing_key"] + elif seen is None and new_vault: + keys = lookup_contact(account, server, signer) + trust_contact(account, keys, False) + key = keys["signing_key"] + else: + raise KeyNotTrusted( + f"The server offers a new key for the vault “{name}”, signed " + f"by “{signer}”, whose fingerprint was never checked on this " + f"PC. Nothing was changed. If {signer} is an owner of this " + "vault, compare fingerprints with them (Vaults → Members → " + "Check), then sync again.") + message = key_message(there.get("id", ""), + int(there.get("key_generation", 0) or 0), + account.username, wrapped, signer) + if not account_crypto.verify(key, message, sig): + raise KeyNotTrusted( + f"The key the server offers for the vault “{name}” is not signed " + f"by “{signer}” as it claims. Nothing was changed, and nothing " + "of this vault was sent. If this keeps happening, do not trust " + "this server with that vault.") + return signer + + def server_for(account: accounts.Account) -> AccountServer: found = account.server if not found: @@ -424,7 +542,7 @@ def reconcile_vaults(account: accounts.Account, server: AccountServer) -> VaultC me = server.me() listed = {v["id"]: v for v in me.get("vaults", []) if v.get("kind") != "personal" and v.get("id") != account.personal_id} - if not server.has_vaults(): + if not server.has_vaults() or not server.has_signed_keys(): changes.server_too_old = bool(extra_here) return changes if server.has_sharing() and not me.get("signing_key"): @@ -451,20 +569,34 @@ def reconcile_vaults(account: accounts.Account, server: AccountServer) -> VaultC changes.gone.append(file.name) continue try: - server.create_vault(vault_id, file.sealed_name(), - record["wrapped_key"]) + server.create_vault( + vault_id, file.sealed_name(), record["wrapped_key"], + sign_key(account, vault_id, file.generation, + account.username, record["wrapped_key"])) except Conflict409: raise SyncError(f"The server already has another vault with the " f"id of “{file.name}”. Nothing was changed.") from None - account.mark_vault(vault_id, on_server=True, gone_from_server=False) + account.mark_vault(vault_id, on_server=True, gone_from_server=False, + key_signer=account.username) changes.uploaded.append(file.name) continue if not record.get("on_server") or record.get("gone_from_server"): account.mark_vault(vault_id, on_server=True, gone_from_server=False) generation = int(there.get("key_generation", 0) or 0) - if generation != int(record.get("key_generation", 0) or 0): + held = int(record.get("key_generation", 0) or 0) + if generation < held: + # An older key can only come back from a server that lies (or a + # restored backup): the people removed since could read again. + raise KeyNotTrusted( + f"The server offers an older key for the vault “{file.name}” " + "than this PC holds. Nothing was changed, and nothing of " + "this vault was sent.") + if generation > held: # The key was changed (someone was removed). Before anything - # else: the name and the entries are under the new key now. + # else: is it a key an owner made? Then the name and the entries + # are under it now. + signer = check_key(account, server, there, file.name, + held_from=record.get("key_signer") or "") try: account.rekey_vault(vault_id, there.get("wrapped_key", ""), generation) @@ -472,8 +604,26 @@ def reconcile_vaults(account: accounts.Account, server: AccountServer) -> VaultC raise SyncError(f"The new key of “{file.name}” cannot be " f"opened by this account ({e}). Nothing was " "changed.") from e + account.mark_vault(vault_id, key_signer=signer) changes.rekeyed.append(file.name) file = account.vault_file(vault_id) + elif there.get("wrapped_key", "") != record.get("wrapped_key", ""): + # The same generation with another key: nothing honest does that. + raise KeyNotTrusted( + f"The server's key for the vault “{file.name}” is not the " + "one this PC holds, though the vault's key was not changed. " + "Nothing was changed, and nothing of this vault was sent.") + elif not there.get("key_sig"): + # A vault from before keys were signed: this device vouches for + # the key it already holds, so the account's other devices take it. + try: + server.put_key_sig(vault_id, sign_key( + account, vault_id, held, account.username, + record["wrapped_key"])) + except SyncError: + pass # signed meanwhile: nothing to add + if not record.get("key_signer"): + account.mark_vault(vault_id, key_signer=account.username) role = there.get("role") if there.get("role") in accounts.ROLES \ else accounts.ROLE_OWNER if role != account.vault_role(vault_id): @@ -503,6 +653,7 @@ def reconcile_vaults(account: accounts.Account, server: AccountServer) -> VaultC for vault_id, there in listed.items(): if vault_id in extra_here: continue + signer = check_key(account, server, there, "(new on the server)") try: file = account.adopt_vault(vault_id, there.get("wrapped_key", ""), there.get("name", ""), @@ -512,6 +663,7 @@ def reconcile_vaults(account: accounts.Account, server: AccountServer) -> VaultC except (CryptoError, ValueError) as e: raise SyncError(f"The server lists a vault this account cannot " f"open ({e}). Nothing was changed.") from e + account.mark_vault(vault_id, key_signer=signer) changes.added.append(file.name) return changes @@ -519,7 +671,7 @@ def reconcile_vaults(account: accounts.Account, server: AccountServer) -> VaultC def syncable_vaults(account: accounts.Account, server: AccountServer) -> list: """Ids of the vaults a sync covers: the personal one, and every other that is on the server.""" - if not server.has_vaults(): + if not server.has_vaults() or not server.has_signed_keys(): return [account.personal_id] return [v for v in account.vault_ids() if v == account.personal_id or ( @@ -554,7 +706,8 @@ def put_vault_back(account: accounts.Account, vault_id: str) -> None: again with the next sync (the user's choice, in the Vaults window).""" # It goes back as this account's own vault, with a first key generation. account.mark_vault(vault_id, on_server=False, gone_from_server=False, - role=accounts.ROLE_OWNER, key_generation=0, members=1) + role=accounts.ROLE_OWNER, key_generation=0, members=1, + key_signer=account.username) try: os.remove(account.vault_file(vault_id).base_path) # agreed with a vault that is gone except OSError: @@ -620,7 +773,9 @@ def share_vault(account: accounts.Account, server: AccountServer, vault_id: str, "vault for that.") wrapped = account_crypto.wrap_key(keys["public_key"], file._key, file.id, keys["username"]) - done = server.put_member(vault_id, keys["username"], role, wrapped) + done = server.put_member(vault_id, keys["username"], role, wrapped, + sign_key(account, vault_id, file.generation, + keys["username"], wrapped)) trust_contact(account, keys, True) account.mark_vault(vault_id, members=len(server.members(vault_id) .get("members") or [1])) @@ -694,6 +849,8 @@ def change_key(account: accounts.Account, server: AccountServer, vault_id: str, view = server.rotate(vault_id, { "generation": generation, "remove": list(remove), "wrapped_keys": wrapped, + "key_sigs": {name: sign_key(account, vault_id, generation, name, key) + for name, key in wrapped.items()}, "name": account_crypto.seal_vault_name(new_key, file.id, file.name), "blob": account_crypto.encrypt_vault(new_key, file.id, generation, vault.validate_vault(dict(merged))), @@ -701,6 +858,7 @@ def change_key(account: accounts.Account, server: AccountServer, vault_id: str, # The server has it; now this device. (If this is interrupted, the next # sync finds the new generation and takes the key from the server.) account.rekey_vault(vault_id, wrapped[account.username], generation) + account.mark_vault(vault_id, key_signer=account.username) account.vault_file(vault_id).save_base(merged, sync_client.base_origin_for( server.url, int(view.get("revision", sync.revision + 1)))) account.mark_vault(vault_id, members=len(staying)) diff --git a/sourceFiles/account_sync_ui.py b/sourceFiles/account_sync_ui.py index c468b79..77a558d 100644 --- a/sourceFiles/account_sync_ui.py +++ b/sourceFiles/account_sync_ui.py @@ -154,12 +154,28 @@ def _all_done(app, run): kind = "warn" if changes.server_too_old: parts.append("this server keeps only the Personal vault — the others " - "need SecretSpace Server 2.0.0-alpha.2 or newer") + "need SecretSpace Server 2.0.0-alpha.5 or newer") kind = "warn" _finish(app, "Synced — " + "; ".join(parts) if parts else "Already up to date", kind) +def plain_http_ok(app, url) -> bool: + """Ask before an unencrypted address of another machine is used. True + to go on (always for https:// and for this PC itself).""" + if not account_sync.is_plain_remote(url): + return True + return app._confirm( + "This connection is not encrypted", + "The address starts with http://, so what this PC and the server " + "say to each other can be read on the network in between.\n\n" + "Your vaults stay safe: they are encrypted on this PC before they " + "leave it. But someone listening could take over the account ON " + "THE SERVER: lock you out there, or delete what it holds.\n\n" + "Fine inside a network you trust (your home, your office). Anywhere " + "else use an https:// address.\n\nContinue?", danger=False) + + def _logout_in_background(server): """Log out without the window waiting: a logout is a network call too, and a slow server must never freeze the window (Lab flow, 2026-11-10: @@ -208,6 +224,8 @@ def connect(): "with http:// or https://", text_color=DANGER) token, ca_file = token_entry.get().strip(), ca_entry.get().strip() + if not plain_http_ok(app, url): + return status.configure(text="Contacting the server…", text_color=MUTED) def done(outcome): @@ -276,6 +294,8 @@ def sign_in(): if not (username and password): return status.configure(text="Enter your username and master password.", text_color=DANGER) + if not plain_http_ok(app, url): + return status.configure(text="Signing in — this takes a moment…", text_color=MUTED) def work(): diff --git a/sourceFiles/accounts.py b/sourceFiles/accounts.py index 00b582e..5c7fd73 100644 --- a/sourceFiles/accounts.py +++ b/sourceFiles/accounts.py @@ -407,7 +407,8 @@ def create_vault(self, name: str) -> VaultFile: made.save({"accounts": []}) extra = dict(self._extra()) extra[made.hex] = {"name": name, "wrapped_key": wrapped, - "created_at": vault.utc_now(), "on_server": False} + "created_at": vault.utc_now(), "on_server": False, + "key_signer": self.username} self._write_vaults(extra) # last: it makes it a vault self._vault_keys[made.hex] = key return made diff --git a/sourceFiles/recovery_ui.py b/sourceFiles/recovery_ui.py index 19a5053..8861548 100644 --- a/sourceFiles/recovery_ui.py +++ b/sourceFiles/recovery_ui.py @@ -61,6 +61,10 @@ def go(): return name = user_entry.get().strip() phrase = " ".join(words.get("1.0", "end").split()) + import account_sync_ui # late: it imports dialogs' users + if url_entry.get().strip() and not accounts.account_exists(name) \ + and not account_sync_ui.plain_http_ok(app, url_entry.get()): + return busy["on"] = True err.configure(text="Opening the account — this takes a moment…", text_color=MUTED) diff --git a/sourceFiles/sharing_ui.py b/sourceFiles/sharing_ui.py index 3305bb1..1816e82 100644 --- a/sourceFiles/sharing_ui.py +++ b/sourceFiles/sharing_ui.py @@ -71,10 +71,10 @@ def _with_server(app, work, done, failed): def run(): server = account_sync.logged_in(account) try: - if not server.has_sharing(): + if not server.has_sharing() or not server.has_signed_keys(): raise account_sync.SyncError( "This server cannot share vaults yet: it needs SecretSpace " - "Server 2.0.0-alpha.3 or newer.") + "Server 2.0.0-alpha.5 or newer.") return work(server) finally: server.logout() @@ -213,6 +213,19 @@ def set_role(who, role): run(lambda server: server.put_member(vault_id, who, role), f"Changing what {who} may do…") + def unchecked(leaving=None): + """A paragraph naming the members who stay and whose keys were never + compared on this PC: the new key goes to whatever key the server + gives for them.""" + names = [m["username"] for m in state["members"] + if not m["me"] and not m["verified"] + and m["username"] != leaving] + if not names: + return "" + return ("\n\nNot checked on this PC: " + ", ".join(names) + ". The " + "new key goes to the keys the server gives for them. Compare " + "fingerprints first (Check) if you can.") + def remove(who): if not app._confirm( f"Remove {who}", @@ -220,7 +233,7 @@ def remove(who): "The vault gets a new key in the same step, so they can open " "nothing that is written from now on. What they have already " "seen they may have kept: change those passwords if it " - "matters.\n\nThe vault is synced first."): + "matters.\n\nThe vault is synced first." + unchecked(who)): return run(lambda server: account_sync.change_key( account, server, vault_id, app.vaults[vault_id], remove=[who]), @@ -231,8 +244,8 @@ def change_key(): "Change the vault's key", f"Give “{name}” a new key?\n\nEvery member gets it with their " "next sync. Worth doing after someone left the vault, or if a " - "device that had it was lost. The vault is synced first.", - danger=False): + "device that had it was lost. The vault is synced first." + + unchecked(), danger=False): return run(lambda server: account_sync.change_key( account, server, vault_id, app.vaults[vault_id]), diff --git a/sourceFiles/tests/sync_server/passman_server.py b/sourceFiles/tests/sync_server/passman_server.py index 8c8480a..8632510 100644 --- a/sourceFiles/tests/sync_server/passman_server.py +++ b/sourceFiles/tests/sync_server/passman_server.py @@ -260,6 +260,21 @@ def _archive(self, user: str, revision: int) -> None: #: version, a one-off X25519 public key, a nonce, the key and its tag. WRAPPED_KEY_LEN = 1 + 32 + 12 + 32 + 16 +#: With each wrapped key comes a signature by the account that wrapped it +#: (Ed25519, 64 bytes). A device accepts a vault key only with a signature +#: from itself or from someone it knows -- so a dishonest server cannot hand +#: out a key of its own making and read what is then uploaded under it. The +#: server only keeps and returns the signature; the devices check it. +KEY_SIG_LEN = 64 + + +def _key_sig(value): + """An optional key signature as stored ("" when none came).""" + if value is None or value == "": + return "" + return _b64(_unb64(value, "The key's signature", KEY_SIG_LEN)) + + #: A vault's name arrives sealed under the vault's key, so the server cannot #: read it: a nonce, the name (up to 200 bytes) and a tag. MIN_SEALED_NAME = 12 + 1 + 16 @@ -612,6 +627,8 @@ def _vault_view(self, username: str, vault_id: str): if view["kind"] != "personal": view["name"] = members.get("name", "") view["wrapped_key"] = member.get("wrapped_key", "") + view["key_sig"] = member.get("key_sig", "") + view["key_signer"] = member.get("key_signer", "") return view def me(self, username: str) -> dict: @@ -805,6 +822,7 @@ def create_vault(self, username: str, body: dict) -> dict: name = self._sealed_name(body) wrapped = _b64(_unb64(body.get("wrapped_key"), "The vault key", WRAPPED_KEY_LEN)) + key_sig = _key_sig(body.get("key_sig")) with _lock: record = self.account(username) mine = list(record.get("vaults", [])) @@ -819,7 +837,9 @@ def create_vault(self, username: str, body: dict) -> dict: self._write_json(self._members_path(vault_id), { "kind": "shared", "created_at": utc_now(), "created_by": username, "name": name, "key_generation": 0, - "members": {username: {"role": "owner", "wrapped_key": wrapped}}}) + "members": {username: {"role": "owner", "wrapped_key": wrapped, + "key_sig": key_sig, + "key_signer": username}}}) record["vaults"] = mine + [vault_id] self._write_json(self._account_path(username), record) return self._vault_view(username, vault_id) @@ -911,6 +931,7 @@ def put_member(self, actor: str, vault_id: str, name: str, body: dict) -> dict: return {"username": who, "role": role, "added": False} wrapped = _b64(_unb64(body.get("wrapped_key"), "The vault key", WRAPPED_KEY_LEN)) + key_sig = _key_sig(body.get("key_sig")) if len(people) >= MAX_MEMBERS_PER_VAULT: raise V2Error(403, f"A vault can have {MAX_MEMBERS_PER_VAULT} " "members.") @@ -918,6 +939,7 @@ def put_member(self, actor: str, vault_id: str, name: str, body: dict) -> dict: if len(listed) >= MAX_VAULTS_LISTED: raise V2Error(409, f"“{who}” is in too many vaults already.") people[who] = {"role": role, "wrapped_key": wrapped, + "key_sig": key_sig, "key_signer": actor, "added_by": actor, "added_at": utc_now()} self._write_json(self._members_path(vault_id), members) target["vaults"] = listed + [vault_id] @@ -979,6 +1001,12 @@ def rotate(self, actor: str, vault_id: str, body: dict) -> dict: "meantime. Sync, then try again.") clean = {name: _b64(_unb64(wrapped[name], "A vault key", WRAPPED_KEY_LEN)) for name in staying} + sigs = body.get("key_sigs") or {} + if not isinstance(sigs, dict) or \ + (sigs and sorted(sigs) != sorted(staying)): + raise V2Error(400, "The keys' signatures do not match the " + "members who stay.") + sigs = {name: _key_sig(sigs.get(name)) for name in staying} name = self._sealed_name(body) current = int(self.vaults.meta(vault_id)["revision"]) @@ -1003,6 +1031,8 @@ def rotate(self, actor: str, vault_id: str, body: dict) -> dict: del people[gone] for each in staying: people[each]["wrapped_key"] = clean[each] + people[each]["key_sig"] = sigs[each] + people[each]["key_signer"] = actor members["key_generation"] = generation members["name"] = name self._write_json(self._members_path(vault_id), members) @@ -1010,6 +1040,24 @@ def rotate(self, actor: str, vault_id: str, body: dict) -> dict: self._unlist(gone, vault_id) return self._vault_view(actor, vault_id) + def set_key_sig(self, username: str, vault_id: str, body: dict) -> None: + """ + A member vouches for the wrapped key it already holds, for a vault + made before keys were signed -- so its other devices will accept + that key. Only while that key has no signature: one made by an + owner is never replaced. + """ + sig = _key_sig(body.get("key_sig")) + if not sig: + raise V2Error(400, "The key's signature is missing.") + with _lock: + members = self._shared(vault_id) + entry = members.get("members", {}).get(username) + if entry.get("key_sig"): + raise V2Error(409, "This key is signed already.") + entry["key_sig"], entry["key_signer"] = sig, username + self._write_json(self._members_path(vault_id), members) + def key_generation(self, vault_id: str) -> int: members = self._read_json(self._members_path(vault_id)) or {} return int(members.get("key_generation", 0)) @@ -1101,11 +1149,12 @@ def do_GET(self): # per account (2.0.0-alpha.2). # "sharing" = members, roles, key change and the audit log # (2.0.0-alpha.3). "recovery" = an account recovered with its - # 12-word phrase (2.0.0-alpha.4). + # 12-word phrase (2.0.0-alpha.4). "signed-keys" = a signature is + # kept with every wrapped vault key (2.0.0-alpha.5). return self._send(200, {"ok": True, "service": "passman-sync", "api": [1, 2], "features": ["vaults", "sharing", - "recovery"]}) + "recovery", "signed-keys"]}) if self._is_v2(): return self._v2("GET") @@ -1385,7 +1434,7 @@ def _v2_route(self, method, parts): # Not a member and no such vault answer alike, so a guess at an # id learns nothing. if role is None or tail not in ("", "meta", "name", "members", - "rotate", "log"): + "rotate", "log", "key-sig"): return self._error(404, "No such vault.") if len(parts) == 4 and tail != "members": return self._error(404, "No such endpoint.") @@ -1479,9 +1528,15 @@ def log(accounts, vault_id, actor, address): return {"run": leave, "anyone": True} if (method, tail, count) == ("POST", "rotate", 3): return {"run": body_then(rotate), "anyone": False} + def key_sig(accounts, vault_id, actor, address, body): + accounts.set_key_sig(actor, vault_id, body) + return self._send(200, {}) + if (method, tail, count) == ("GET", "log", 3): return {"run": log, "anyone": False} - if tail in ("name", "rotate", "log") or method == "DELETE" \ + if (method, tail, count) == ("PUT", "key-sig", 3): + return {"run": body_then(key_sig), "anyone": True} + if tail in ("name", "rotate", "log", "key-sig") or method == "DELETE" \ or (tail == "members" and (count == 4 or method != "GET")): return {"run": lambda *a: self._error(404, "No such endpoint."), "anyone": True} diff --git a/sourceFiles/tests/test_account_key_trust.py b/sourceFiles/tests/test_account_key_trust.py new file mode 100644 index 0000000..510f311 --- /dev/null +++ b/sourceFiles/tests/test_account_key_trust.py @@ -0,0 +1,392 @@ +""" +Security review (2026-11-12): a server must never be able to choose a +vault's key. + +The server stores each member's wrapped copy of a vault key. If a device +took whatever key the server listed, a dishonest server could wrap a key of +its own to the member's public key (which it knows), announce it as a new +key generation, and read the vault the device then uploads under it. So +every wrapped key carries a signature by the account that wrapped it, and a +device takes a key only when it is signed by itself or by someone it has +reason to trust. + +Here the server's files are edited directly, as a dishonest server (or +whoever broke into it) would. +""" + +import base64 +import json +import os +import shutil + +import pytest + +import account_crypto +import account_sync +import accounts +import vault +from account_sync import KeyNotTrusted +from sync_client import SyncError + +from test_account_sync import (PW, TOKEN, alice, cheap_argon2, device, # noqa: F401 + new_account, passman_server, server) +from test_account_vaults import sync_all, sites +from test_account_sharing import add_entry, share, rotate, with_server + + +# --------------------------------------------------------------------------- +# The dishonest server's tools +# --------------------------------------------------------------------------- +def vault_folder(server, vault_id): + return os.path.join(server.store.root, "_v2", "vaults", vault_id) + + +def read_members(server, vault_id): + with open(os.path.join(vault_folder(server, vault_id), "members.json"), + encoding="utf-8") as f: + return json.load(f) + + +def write_members(server, vault_id, members): + with open(os.path.join(vault_folder(server, vault_id), "members.json"), "w", + encoding="utf-8") as f: + json.dump(members, f) + + +def public_key_on_server(server, name): + path = os.path.join(server.store.root, "_v2", "accounts", name + ".json") + with open(path, encoding="utf-8") as f: + return base64.b64decode(json.load(f)["public_key"]) + + +def swap_key(server, vault_id, victim, generation, signer=None, signer_keyset=None, + drop_blob=True): + """Give ``victim`` a key the server made itself, as key generation + ``generation``; optionally with a signature by ``signer_keyset`` under + the name ``signer``. Returns the server's key.""" + raw_id = bytes.fromhex(vault_id) + evil = os.urandom(32) + members = read_members(server, vault_id) + wrapped = account_crypto.wrap_key(public_key_on_server(server, victim), evil, + raw_id, victim) + entry = members["members"][victim] + entry["wrapped_key"] = wrapped + entry["key_sig"], entry["key_signer"] = "", "" + if signer: + entry["key_signer"] = signer + entry["key_sig"] = account_crypto.sign( + signer_keyset, account_sync.key_message(vault_id, generation, victim, + wrapped, signer)) + members["key_generation"] = generation + members["name"] = account_crypto.seal_vault_name(evil, raw_id, "Team") + write_members(server, vault_id, members) + if drop_blob: # "nothing uploaded yet": push it all + for name in ("vault.enc", "meta.json"): + try: + os.remove(os.path.join(vault_folder(server, vault_id), name)) + except FileNotFoundError: + pass + return evil + + +def server_can_read(server, vault_id, evil_key): + path = os.path.join(vault_folder(server, vault_id), "vault.enc") + if not os.path.exists(path): + return False + with open(path, "rb") as f: + blob = f.read().decode("ascii") + try: + account_crypto.decrypt_vault(evil_key, blob) + return True + except account_crypto.CryptoError: + return False + + +def make_account(server, device, name, where): # noqa: F811 + device(where) + account = new_account(name).save() + assert account_sync.connect(account, server.url, "", TOKEN) == "created" + sync_all(account) + return account + + +@pytest.fixture +def team(server, device, alice): # noqa: F811 + """alice's own vault Team with a secret in it, on the server.""" + vault_id = alice.create_vault("Team").hex + add_entry(alice, vault_id, "Bank", "TOP-SECRET") + sync_all(alice) + return alice, vault_id + + +def untouched(account, vault_id, key_before): + file = account.vault_file(vault_id) + assert file._key == key_before + assert file.generation == 0 + assert sites(account, vault_id) == ["Bank"] + + +# --------------------------------------------------------------------------- +# A. The signature itself +# --------------------------------------------------------------------------- +def test_a_key_signature_covers_vault_generation_member_key_and_signer(alice): + wrapped = alice.vault_record(alice.create_vault("V").hex)["wrapped_key"] + vault_id = "ab" * 16 + sig = account_sync.sign_key(alice, vault_id, 3, "bob", wrapped) + key = account_crypto.signing_key_of(alice.keyset) + + def ok(*args): + return account_crypto.verify(key, account_sync.key_message(*args), sig) + + assert ok(vault_id, 3, "bob", wrapped, "alice") + assert not ok("cd" * 16, 3, "bob", wrapped, "alice") + assert not ok(vault_id, 4, "bob", wrapped, "alice") + assert not ok(vault_id, 3, "carol", wrapped, "alice") + assert not ok(vault_id, 3, "bob", wrapped[:-4] + "AAAA", "alice") + assert not ok(vault_id, 3, "bob", wrapped, "mallory") + + +def test_a_vault_goes_to_the_server_with_its_owners_signature(server, team): + alice, vault_id = team + entry = read_members(server, vault_id)["members"]["alice"] + assert entry["key_signer"] == "alice" + assert account_crypto.verify( + account_crypto.signing_key_of(alice.keyset), + account_sync.key_message(vault_id, 0, "alice", entry["wrapped_key"], + "alice"), entry["key_sig"]) + assert alice.vault_record(vault_id)["key_signer"] == "alice" + + +# --------------------------------------------------------------------------- +# B. A server that makes up a key for a vault this device already has +# --------------------------------------------------------------------------- +def test_a_key_with_no_signature_is_refused_and_nothing_is_uploaded(server, team): + alice, vault_id = team + key = alice.vault_file(vault_id)._key + evil = swap_key(server, vault_id, "alice", 1) + with pytest.raises(KeyNotTrusted, match="no signature"): + sync_all(alice) + assert not server_can_read(server, vault_id, evil) + untouched(alice, vault_id, key) + + +def test_a_key_signed_by_the_wrong_key_under_the_owners_name_is_refused(server, team): + alice, vault_id = team + key = alice.vault_file(vault_id)._key + evil = swap_key(server, vault_id, "alice", 1, "alice", + account_crypto.new_keyset()) + with pytest.raises(KeyNotTrusted, match="not signed by"): + sync_all(alice) + assert not server_can_read(server, vault_id, evil) + untouched(alice, vault_id, key) + + +def test_a_key_signed_by_an_account_never_seen_here_is_refused(server, team, device): + alice, vault_id = team + key = alice.vault_file(vault_id)._key + mallory = make_account(server, device, "mallory", "m") + device("a") + evil = swap_key(server, vault_id, "alice", 1, "mallory", mallory.keyset) + with pytest.raises(KeyNotTrusted, match="never checked"): + sync_all(alice) + assert not server_can_read(server, vault_id, evil) + untouched(alice, vault_id, key) + assert alice.contact("mallory") is None # and not remembered for it + + +def test_being_seen_once_is_not_enough_to_replace_a_key(server, team, device): + """Two steps: first a harmless vault "shared by mallory" (her key is + remembered, unchecked), then a made-up key for Team signed by her.""" + alice, vault_id = team + key = alice.vault_file(vault_id)._key + mallory = make_account(server, device, "mallory", "m") + bait = mallory.create_vault("Bait").hex + sync_all(mallory) + session = account_sync.logged_in(mallory) + keys = account_sync.lookup_contact(mallory, session, "alice") + account_sync.share_vault(mallory, session, bait, keys, "editor") + session.logout() + device("a") + assert sync_all(alice).added == ["Bait"] + assert alice.contact("mallory")["verified"] is False + evil = swap_key(server, vault_id, "alice", 1, "mallory", mallory.keyset) + with pytest.raises(KeyNotTrusted, match="never checked"): + sync_all(alice) + assert not server_can_read(server, vault_id, evil) + untouched(alice, vault_id, key) + + +def test_an_older_key_generation_is_refused(server, team, device): + alice, vault_id = team + bob = make_account(server, device, "bob", "b") + device("a") + share(alice, vault_id, "bob") + before = os.path.join(server.store.root, "_before") + shutil.copytree(vault_folder(server, vault_id), before) + rotate(alice, vault_id, remove=["bob"]) + add_entry(alice, vault_id, "AfterBob") + sync_all(alice) + # the server (with bob) puts the vault back as it was before the removal + shutil.rmtree(vault_folder(server, vault_id)) + shutil.copytree(before, vault_folder(server, vault_id)) + with pytest.raises(KeyNotTrusted, match="older key"): + sync_all(alice) + assert alice.vault_file(vault_id).generation == 1 + assert sites(alice, vault_id) == ["AfterBob", "Bank"] + + +def test_another_key_at_the_same_generation_is_refused(server, team): + alice, vault_id = team + key = alice.vault_file(vault_id)._key + evil = swap_key(server, vault_id, "alice", 0, "alice", alice.keyset) + # (even "signed by alice": the server cannot do that, this is the worst case) + with pytest.raises(KeyNotTrusted, match="not the\\s+one this PC holds"): + sync_all(alice) + assert not server_can_read(server, vault_id, evil) + untouched(alice, vault_id, key) + + +# --------------------------------------------------------------------------- +# C. A server that invents a vault +# --------------------------------------------------------------------------- +def invent_vault(server, victim, signer="", signer_keyset=None): + vault_id = os.urandom(16).hex() + raw_id = bytes.fromhex(vault_id) + evil = os.urandom(32) + wrapped = account_crypto.wrap_key(public_key_on_server(server, victim), evil, + raw_id, victim) + entry = {"role": "owner", "wrapped_key": wrapped, "key_sig": "", + "key_signer": signer} + if signer_keyset is not None: + entry["key_sig"] = account_crypto.sign(signer_keyset, account_sync.key_message( + vault_id, 0, victim, wrapped, signer)) + os.makedirs(vault_folder(server, vault_id)) + write_members(server, vault_id, { + "kind": "shared", "created_by": victim, "key_generation": 0, + "name": account_crypto.seal_vault_name(evil, raw_id, "Work"), + "members": {victim: entry}}) + path = os.path.join(server.store.root, "_v2", "accounts", victim + ".json") + with open(path, encoding="utf-8") as f: + record = json.load(f) + record["vaults"] = record.get("vaults", []) + [vault_id] + with open(path, "w", encoding="utf-8") as f: + json.dump(record, f) + return vault_id + + +@pytest.mark.parametrize("forged_as_own", [False, True]) +def test_an_invented_vault_is_not_taken_as_ones_own(server, alice, forged_as_own): + made_up = invent_vault(server, "alice", + "alice" if forged_as_own else "", + account_crypto.new_keyset() if forged_as_own else None) + with pytest.raises(KeyNotTrusted): + sync_all(alice) + assert made_up not in alice.vault_ids() + + +# --------------------------------------------------------------------------- +# D. What must keep working +# --------------------------------------------------------------------------- +def test_a_key_change_by_the_owner_who_shared_the_vault_is_taken(server, team, device): + alice, vault_id = team + bob = make_account(server, device, "bob", "b") + device("a") + share(alice, vault_id, "bob") + device("b") + assert sync_all(bob).added == ["Team"] + assert bob.vault_record(vault_id)["key_signer"] == "alice" + device("a") + rotate(alice, vault_id) + add_entry(alice, vault_id, "New") + sync_all(alice) + device("b") + assert sync_all(bob).rekeyed == ["Team"] + assert sites(bob, vault_id) == ["Bank", "New"] + + +def test_a_key_change_by_another_owner_needs_their_fingerprint_checked( + server, team, device): + alice, vault_id = team + bob = make_account(server, device, "bob", "b") + carol = make_account(server, device, "carol", "c") + device("a") + share(alice, vault_id, "bob", accounts.ROLE_OWNER) + share(alice, vault_id, "carol") + device("b") + sync_all(bob) + device("c") + sync_all(carol) # carol got the vault from alice + device("b") + rotate(bob, vault_id) # bob, an owner too, changes the key + # alice compared bob's fingerprint when she shared: she takes his key + device("a") + assert sync_all(alice).rekeyed == ["Team"] + # carol never did: she refuses until she has + device("c") + with pytest.raises(KeyNotTrusted, match="never checked"): + sync_all(carol) + assert carol.vault_file(vault_id).generation == 0 + keys = with_server(carol, lambda s: account_sync.lookup_contact(carol, s, "bob")) + account_sync.trust_contact(carol, keys, True) + assert sync_all(carol).rekeyed == ["Team"] + assert sites(carol, vault_id) == ["Bank"] + + +def test_a_second_device_takes_its_own_accounts_vaults(server, team, device): + alice, vault_id = team + device("b") + other, _ = account_sync.sign_in(server.url, "", "alice", PW, alice.kit) + assert sites(other, vault_id) == ["Bank"] + assert other.vault_record(vault_id)["key_signer"] == "alice" + + +def test_a_vault_from_before_signatures_is_vouched_for_by_a_device_that_has_it( + server, team, device): + alice, vault_id = team + members = read_members(server, vault_id) + members["members"]["alice"].pop("key_sig") + members["members"]["alice"].pop("key_signer") + write_members(server, vault_id, members) + # a new device cannot take it as it is ... + device("b") + with pytest.raises(KeyNotTrusted, match="no signature"): + account_sync.sign_in(server.url, "", "alice", PW, alice.kit) + # ... until a device that holds the vault has synced once + device("a") + sync_all(alice) + assert read_members(server, vault_id)["members"]["alice"]["key_signer"] == "alice" + device("c") + other, _ = account_sync.sign_in(server.url, "", "alice", PW, alice.kit) + assert sites(other, vault_id) == ["Bank"] + + +def test_a_server_without_signed_keys_gets_only_the_personal_vault( + server, team, monkeypatch): + alice, vault_id = team + monkeypatch.setattr(account_sync.AccountServer, "has_signed_keys", + lambda self: False) + session = account_sync.logged_in(alice) + try: + changes = account_sync.reconcile_vaults(alice, session) + assert changes.server_too_old is True + assert account_sync.syncable_vaults(alice, session) == [alice.personal_id] + finally: + session.logout() + + +# --------------------------------------------------------------------------- +# E. Plain http +# --------------------------------------------------------------------------- +@pytest.mark.parametrize("url,plain", [ + ("http://192.168.1.50:8787", True), + ("http://sync.example.org", True), + ("HTTP://10.0.0.3:59872/", True), + ("https://192.168.1.50:8787", False), + ("http://127.0.0.1:8787", False), + ("http://127.5.5.5", False), + ("http://localhost:8787", False), + ("http://[::1]:8787", False), + ("", False), +]) +def test_only_unencrypted_addresses_of_other_machines_are_flagged(url, plain): + assert account_sync.is_plain_remote(url) is plain diff --git a/sourceFiles/version.py b/sourceFiles/version.py index 20d8416..643226b 100644 --- a/sourceFiles/version.py +++ b/sourceFiles/version.py @@ -12,7 +12,7 @@ import re -__version__ = "1.0.0-alpha.7" +__version__ = "1.0.0-alpha.8" #: The container a 3.0 account's vaults are written in (account_crypto). VAULT_FORMAT = "PMV3"