From 6f8a04fcadd124055c3366061d0fb92465a13948 Mon Sep 17 00:00:00 2001 From: CyberMind-FR Date: Fri, 17 Jul 2026 12:58:38 +0200 Subject: [PATCH] fix(users): stop leaking password hashes and TOTP secrets to any client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/v1/users/users, /user/{username} and /export returned raw records from users.json, which holds the argon2id password hash, the TOTP *secret*, and the argon2id hashes of the TOTP backup codes. All three routes are gated by `require_jwt` — ANY authenticated account, not just an admin — so the lowest-privilege user could harvest every other account's credentials. The TOTP secret alone lets an attacker mint valid 6-digit codes forever: a complete MFA bypass. This is a privilege-escalation vector on a product whose own charter forbids secrets in clear. /export was the worst: its comment said "Remove sensitive data" while dropping only `provision_results`. A comment asserting a guarantee the code does not provide is worse than no comment — it stops reviewers from looking. Add redact_user() as the single place that decides what a client may see, and route all three through it. `totp` is kept but reduced to its non-secret metadata, and its PRESENCE is preserved: the webui shows the MFA badge on a truthiness test (`u.totp_enabled || u.totp || u.mfa_enabled`), so dropping the key would blank the badge and always emitting one would light it up for users who never enrolled. It lives in its own module rather than main.py because main.py reads the config at import, so importing it requires read access to /etc/secubox/secubox.conf — a security rule must stay testable without privileges. Verified live on gk2 across all three routes: no argon2 hash, no TOTP secret, no backup codes; totp_enabled still correct per account (admin true, gk2 and operator false). Mutation-checked: restoring the passthrough fails 5 of the 9 tests. Co-Authored-By: Gerald KERMA --- packages/secubox-users/api/main.py | 37 ++++--- packages/secubox-users/api/redact.py | 51 +++++++++ .../secubox-users/tests/test_redaction.py | 104 ++++++++++++++++++ 3 files changed, 177 insertions(+), 15 deletions(-) create mode 100644 packages/secubox-users/api/redact.py create mode 100644 packages/secubox-users/tests/test_redaction.py diff --git a/packages/secubox-users/api/main.py b/packages/secubox-users/api/main.py index 392e43fc..141b428a 100644 --- a/packages/secubox-users/api/main.py +++ b/packages/secubox-users/api/main.py @@ -39,6 +39,8 @@ config = get_config("users") if callable(get_config) else {} USERSCTL = "/usr/sbin/usersctl" USERS_FILE = os.environ.get("USERS_FILE", "/etc/secubox/users.json") ROLES_FILE = os.environ.get("ROLES_FILE", "/etc/secubox/roles.json") +from .redact import redact_user # expurgation des secrets (module dédié, testable seul) + SERVICES = ["nextcloud", "gitea", "email", "matrix", "jellyfin", "peertube", "jabber"] # YaCy has a single admin account (no per-user accounts), so its password is # synced from exactly one SecuBox user: the master "admin". Changing that user's @@ -502,12 +504,10 @@ async def get_access(): @app.get("/users", dependencies=[Depends(require_jwt)]) async def list_users(): - """List all users.""" + """List all users (redacted — never ships hashes or TOTP secrets).""" data = load_users() - return { - "users": data.get("users", []), - "total": len(data.get("users", [])) - } + users = [redact_user(u) for u in data.get("users", [])] + return {"users": users, "total": len(users)} @app.get("/user/{username}", dependencies=[Depends(require_jwt)]) async def get_user(username: str): @@ -515,11 +515,12 @@ async def get_user(username: str): data = load_users() for user in data.get("users", []): if user.get("username") == username: + out = redact_user(user) # Add service status - user["service_status"] = {} + out["service_status"] = {} for svc in user.get("services", []): - user["service_status"][svc] = check_service(svc) - return user + out["service_status"][svc] = check_service(svc) + return out raise HTTPException(status_code=404, detail="User not found") @app.post("/user", dependencies=[Depends(require_jwt)]) @@ -1064,14 +1065,20 @@ async def validate_acl(entries: List[ACLEntry]): @app.get("/export", dependencies=[Depends(require_jwt)]) async def export_users(): - """Export all users.""" + """Export all users (redacted). + + This route previously claimed to "remove sensitive data" while dropping only + `provision_results` — it shipped every password hash and TOTP secret into a + downloadable file. A comment asserting a guarantee the code does not provide + is worse than no comment: it stops reviewers from looking. The guarantee now + lives in redact_user(), which is the single place that decides what a client + may see. + """ data = load_users() - # Remove sensitive data - export_data = {"users": [], "groups": data.get("groups", [])} - for user in data.get("users", []): - export_user = {k: v for k, v in user.items() if k != "provision_results"} - export_data["users"].append(export_user) - return export_data + return { + "users": [redact_user(u) for u in data.get("users", [])], + "groups": data.get("groups", []), + } @app.post("/import", dependencies=[Depends(require_permission("system.import"))]) async def import_users(file: UploadFile = File(...)): diff --git a/packages/secubox-users/api/redact.py b/packages/secubox-users/api/redact.py new file mode 100644 index 00000000..1392c67b --- /dev/null +++ b/packages/secubox-users/api/redact.py @@ -0,0 +1,51 @@ +# SPDX-License-Identifier: LicenseRef-CMSD-1.0 +# Copyright (c) 2026 CyberMind — Gérald Kerma +# Source-Disclosed License — All rights reserved except as expressly granted. +# See LICENCE-CMSD-1.0.md for terms. + +""" +SecuBox-Deb :: users — expurgation des secrets avant sortie du processus +CyberMind — https://cybermind.fr + +Module séparé, sans dépendance : `api.main` lit la config au niveau module, donc +l'importer exige de pouvoir lire /etc/secubox/secubox.conf. Une règle de sécurité +doit rester testable sans privilèges — d'où ce fichier, importable seul. +""" +from __future__ import annotations + +# Jamais renvoyés à un client. `password_hash` est une primitive d'attaque +# hors-ligne ; `provision_results` peut contenir des retours de service bruts. +SECRET_KEYS = ("password_hash", "provision_results") + +# Dans le sous-dict `totp` : `secret` permet à lui seul de forger des codes à +# 6 chiffres indéfiniment (contournement complet du MFA) ; `backup_codes` porte +# les hashes argon2id des codes de secours ; `last_step` est un détail interne. +TOTP_SECRET_KEYS = ("secret", "backup_codes", "last_step") + + +def redact_user(user: dict) -> dict: + """Copie d'un enregistrement utilisateur sans aucun secret. + + users.json contient le hash argon2id du mot de passe, le SECRET TOTP et les + hashes des codes de secours. Aucun n'a d'usage côté client. + + Toute route renvoyant un utilisateur DOIT passer par ici. Ces routes sont + gardées par `require_jwt` — n'importe quel compte authentifié, pas seulement + un admin — donc un enregistrement non expurgé transforme le compte le moins + privilégié en moissonneuse d'identifiants de tous les autres. + + `totp` est conservé mais réduit à ses métadonnées non secrètes, et sa + PRÉSENCE est préservée : la webui décide d'afficher le badge MFA par simple + test de véracité (`u.totp_enabled || u.totp || u.mfa_enabled`), donc + supprimer la clé éteindrait le badge en silence, et en émettre une toujours + l'allumerait pour des utilisateurs jamais enrôlés. + + Ne mute jamais l'entrée : elle vient de users.json et serait réécrite telle + quelle au prochain enregistrement. + """ + out = {k: v for k, v in user.items() if k not in SECRET_KEYS} + totp = user.get("totp") + if isinstance(totp, dict): + out["totp"] = {k: v for k, v in totp.items() if k not in TOTP_SECRET_KEYS} + out["totp_enabled"] = bool(isinstance(totp, dict) and totp.get("enabled")) + return out diff --git a/packages/secubox-users/tests/test_redaction.py b/packages/secubox-users/tests/test_redaction.py new file mode 100644 index 00000000..e26d6e89 --- /dev/null +++ b/packages/secubox-users/tests/test_redaction.py @@ -0,0 +1,104 @@ +# SPDX-License-Identifier: LicenseRef-CMSD-1.0 +# Copyright (c) 2026 CyberMind — Gérald Kerma +# Source-Disclosed License — All rights reserved except as expressly granted. +# See LICENCE-CMSD-1.0.md for terms. + +""" +SecuBox-Deb :: users — les secrets ne doivent jamais quitter le processus. + +users.json contient le hash argon2id du mot de passe, le SECRET TOTP et les +hashes des codes de secours. Les routes qui renvoient un utilisateur sont +gardées par `require_jwt` — donc n'importe quel compte authentifié, pas +seulement un admin. Un enregistrement non expurgé transforme le compte le moins +privilégié en moissonneuse d'identifiants. +""" +from api.redact import redact_user + +# La forme réelle observée dans /etc/secubox/users.json sur la board. +REAL_SHAPE = { + "username": "admin", + "email": "gandalf@gk2.net", + "role": "admin", + "enabled": True, + "created": "2026-05-09T11:14:11.719355", + "last_login": "2026-07-17T09:01:36.952678+00:00", + "must_change_password": False, + "google": None, + "services": [], + "password_hash": "$argon2id$v=19$m=1024,t=1,p=1$c2FsdA$hash", + "totp": { + "enabled": True, + "enrolled_at": "2026-05-09T11:20:00", + "secret": "JBSWY3DPEHPK3PXP", + "last_step": 59475852, + "backup_codes": [{"hash": "$argon2id$v=19$m=65536,t=3,p=4$abc$def"}], + }, +} + + +def _flat(obj) -> str: + """Sérialisation grossière : un secret imbriqué reste détectable.""" + import json + return json.dumps(obj) + + +def test_password_hash_never_leaves(): + assert "password_hash" not in redact_user(REAL_SHAPE) + + +def test_totp_secret_never_leaves(): + # Le pire des trois : le secret TOTP seul permet de forger des codes à 6 + # chiffres indéfiniment — un contournement complet du MFA. + out = redact_user(REAL_SHAPE) + assert "secret" not in out["totp"] + assert "JBSWY3DPEHPK3PXP" not in _flat(out) + + +def test_backup_code_hashes_never_leave(): + out = redact_user(REAL_SHAPE) + assert "backup_codes" not in out["totp"] + assert "argon2id" not in _flat(out) + + +def test_no_argon2_hash_survives_anywhere(): + # Filet global : attrape tout futur champ portant un hash. + assert "$argon2" not in _flat(redact_user(REAL_SHAPE)) + + +def test_fields_the_panel_needs_are_preserved(): + out = redact_user(REAL_SHAPE) + for k in ("username", "email", "role", "enabled", "created", + "last_login", "must_change_password", "services"): + assert k in out, f"le panel a besoin de {k}" + + +def test_totp_presence_is_preserved_for_the_mfa_badge(): + # Le panel décide du badge MFA par simple véracité + # (`u.totp_enabled || u.totp || u.mfa_enabled`) : supprimer la clé + # éteindrait le badge en silence. + out = redact_user(REAL_SHAPE) + assert out["totp"] # truthy + assert out["totp"]["enabled"] is True + assert out["totp_enabled"] is True + + +def test_user_without_totp_does_not_gain_a_badge(): + # Symétrique : ne jamais allumer le badge d'un utilisateur non enrôlé. + out = redact_user({"username": "gk2", "password_hash": "$argon2id$x"}) + assert out.get("totp") is None + assert out["totp_enabled"] is False + + +def test_totp_present_but_disabled_reports_not_enabled(): + out = redact_user({"username": "u", "totp": {"enabled": False, "secret": "S"}}) + assert out["totp_enabled"] is False + assert "secret" not in out["totp"] + + +def test_redaction_does_not_mutate_the_stored_record(): + # redact_user reçoit l'objet chargé depuis users.json ; le muter + # corromprait la base au prochain save. + import copy + original = copy.deepcopy(REAL_SHAPE) + redact_user(REAL_SHAPE) + assert REAL_SHAPE == original