Skip to content

fix(security): correction de 8 vulnérabilité - #4

Open
TiotBenjy wants to merge 10 commits into
repod-ce:mainfrom
TiotBenjy:fix/security-issues
Open

TiotBenjy wants to merge 10 commits into
repod-ce:mainfrom
TiotBenjy:fix/security-issues

Conversation

@TiotBenjy

Copy link
Copy Markdown
Contributor

Contexte

Revue de sécurité complète de l'application, backend & frontend + déploiement Docker Compose.

8 sont corrigées ici, une par commit. Pour chacune, les tests ont été écrits avant le correctif et vérifiés et rouges.

Correctifs

# Sévérité Vulnérabilité Correctif
1 Medium, impact technique élevé SSTI menant à une RCE dans les templates email. Les trois environnements Jinja2 rendant du contenu fourni par l'utilisateur étaient des Environment nus. POST /templates/{name}/preview renvoyait la sortie dans la réponse, soit un oracle de RCE direct atteignant la clé GPG de signature du dépôt et JWT_SECRET_KEY SandboxedEnvironment aux trois points de rendu. autoescape n'échappe que la sortie, il ne restreint pas la traversée d'attributs
2 Medium Path traversal par le champ group de l'import APT. Un rôle uploader, typiquement un jeton de CI, écrivait un .deb hors de IMPORTS_DIR, y compris dans les bind mounts en écriture /repos/gnupg, /repos/db et /repos/conf safe_path_join au sink, avant le téléchargement, couvrant les trois appelants, plus un contrôle de frontière renvoyant 400 avant l'ouverture du flux SSE
3 Medium Le rôle d'autorisation venait du claim JWT, pas de la base. 118 points de décision concernés, et POST /auth/refresh recopiait le claim périmé dans un jeton neuf toutes les 45 minutes : rétention indéfinie de droits révoqués Une ligne dans _parse_token : le rôle est relu dans la table users à chaque requête. Le refresh est corrigé mécaniquement, get_current_user_full retournant ce même dict
4 Medium /metrics anonyme exposant des données nominatives. Le chemin brut servait de label Prometheus, mettant noms d'utilisateurs, couples paquet/version et identifiants de groupe dans le registre. Le garde était un if sans else Anonymisation du label en gabarit de route, plus une échelle de créance à trois niveaux. C'est l'anonymisation qui retire la vulnérabilité
5 Medium Wildcard CORS avec credentials par défaut. docker compose up depuis un clone propre livrait allow_origins=["*"] avec allow_credentials=True Refus du wildcard au démarrage, levée en production, et défaut de compose ramené à l'origine explicite que .env.example recommandait déjà
6 Medium POST /packages/install/ atteignable par le rôle reader. Il déclenchait une exécution SSH sur la machine gérée, injectant un paquet amont et sa fermeture de dépendances dans le pool servi, sans audit Suppression de l'endpoint, doublon hérité de POST /artifacts/{name}/install qui porte le garde, le contrôle de dépendances et l'audit
7 Medium bas Annuaire du personnel lisible par le rôle le moins privilégié. GET /groups/{id}/members renvoyait nom complet, adresse e-mail et rôle de chaque membre à tout compte authentifié get_admin_user sur la route, alignant la lecture sur l'écriture. Le bouton frontend qui offrait cette lecture aux non-admins est retiré
8 Low .. accepté dans la suppression d'un groupe d'import. La regex ^[\w.\-+]+$ contient un point littéral, rmtree effaçait alors le contenu du dépôt safe_path_join, refus explicite du chemin résolvant vers la base, et is_dir() à la place de exists()

Décisions de conception

Le rôle reader est un "compte de service". auth/users.py le documente comme distribué à chaque machine cliente APT. C'est ce qui fait passer les vulnérabilités 6 et 7 d'un problème théorique de RBAC à une exposition réelle : le jeton du rôle le plus bas est présent sur tout le parc.

Confinement sémantique plutôt que validation syntaxique. Les vulnérabilités 2 et 8 sont toutes deux corrigées par safe_path_join, jamais par une regex. Celle qui existait dans le code, ^[\w.\-+]+$, acceptait . et .., vérifié par exécution. Toute classe de caractères assez large pour les noms Debian réels (g++, libssl1.1, foo.bar-1+deb) laisse passer les segments de points.

Un helper de confinement encode un prédicat, pas une intention. La vulnérabilité 8 a demandé un refus de . en plus de safe_path_join. Où l'opération était une écriture dans le répertoire de base. Une suppression demande un descendant strict. Le contrôle est ajouté au point d'appel, pas au helper partagé, dont les autres appelants acceptent légitimement ce cas.

Retirer la donnée sensible plutôt que restreindre l'accès. Sur la vulnérabilité 4, fermer /metrics aurait cassé les scrapes existants, rendu fausses deux lignes de documentation et cassé trois tests, tout en laissant les identifiants dans les labels pour qui détient le jeton. Anonymiser les labels supprime la fuite sans rien casser, et le mode strict garde son enforcement.

Impact opérationnel

Six changements sont visibles en exploitation.

  1. Un déploiement ayant explicitement défini CORS_ORIGINS=* refusera de démarrer en production. Le message indique quoi définir. C'est la posture des quatre blocs de validation déjà présents dans main.py.
  2. POST /api/v1/packages/install/ renvoie désormais 404. Aucun appelant dans le frontend ni dans la documentation. Le chemin légitime est POST /api/v1/artifacts/{name}/install.
  3. GET /api/v1/groups/{id}/members renvoie 403 hors admin.
  4. Les changements de rôle prennent effet immédiatement, rétrogradation comme promotion, sans attendre l'expiration du jeton. L'interface d'un utilisateur rétrogradé reste optimiste au plus 45 minutes, mais chaque appel derrière les boutons affichés renvoie 403.
  5. Le label path de /metrics change de forme, du chemin concret vers le gabarit de route. Un serveur Prometheus ayant déjà scrapé les séries à chemin brut les conserve dans son TSDB pour sa durée de rétention : les purger relève de son API admin, pas de ce correctif.
  6. Quand METRICS_TOKEN est défini, un JWT de rôle auditeur devient une créance alternative acceptée sur /metrics. Le rôle est relu en base, un claim falsifié est refusé.

Les trois environnements Jinja2 qui rendent du contenu fourni par l'utilisateur étaient des `Environment` standard, jamais des `SandboxedEnvironment` :

- _get_env()              relit les corps écrits sur le volume
- render_email_template() rend le sujet stocké (autoescape=False)
- preview_template()      rend le corps brut de la requête

PUT /api/v1/templates/{name} écrit une chaîne arbitraire sur disque sans validation, et POST /api/v1/templates/{name}/preview la rend puis renvoie le résultat dans la réponse ; soit un oracle d'exécution de code en une seule requête. autoescape=True n'y change rien : il échappe la sortie, pas l'accès aux attributs.

Un admin (ou tout porteur d'un token admin volé) pouvait donc exécuter du code dans le conteneur backend, qui détient la clé GPG privée de signature du dépôt, JWT_SECRET_KEY, le DSN PostgreSQL et les secrets SMTP/LDAP déchiffrés. C'est le seul primitif d'évaluation de code de l'application : les ~40 appels subprocess utilisent tous des listes argv avec shell=False, donc aucune fonctionnalité légitime n'offrait déjà ce niveau d'accès.

Correctif :
    - SandboxedEnvironment sur les trois sinks
    - save_template() restreint aux noms connus de DEFAULTS (même contrat que reset_template()), sinon n'importe quel nom crée un .html/.json arbitraire dans TEMPLATES_DIR ; le routeur renvoie 404

Vérifié avec jinja2==3.1.6 : 7 payloads (cycler, lipsum, __mro__, namespace, joiner, self) s'exécutaient avant, tous bloqués par SecurityError après ; les 8 templates par défaut et les 7 sujets rendent à l'identique.

tests/test_email_templates.py : 7 passed.
Le champ `group` de ImportRequest/BatchImportRequest n'etait valide nulle part et finissait en composant de chemin dans services/importer_apt.py:_import_one_locked() :
      group_dir = IMPORTS_DIR / (group or pkg_name)
      group_dir.mkdir(parents=True, exist_ok=True)
      shutil.copy2(str(path), str(group_dir / path.name))

Un compte de rôle `uploader`, le plus bas rôle disposant d'un droit d'écriture et typiquement porte par un token de CI, pouvait poster {"package":"nginx","distribution":"jammy","group":"../../../tmp/pwn"} sur POST /import/fetch et faire écrire le .deb hors de IMPORTS_DIR, partout ou le process backend peut écrire, y compris les bind mounts en ecriture /repos/dists, /repos/conf, /repos/db et /repos/gnupg. Une valeur absolue etait pire : pathlib ecarte entierement la base, Path("/repos/imports") / "/tmp/x" vaut "/tmp/x".

Deux gardes :

- services/importer_apt.py, garde faisant autorite, place en tête de _import_one_locked donc avant le telechargement. Il couvre d'un seul endroit les trois appelants du sink : import_router, mirror_manager (group géneré serveur) et upload.py:679, ou group vient des metadonnées de contrôle d'un .deb uploade et reste influençable. Le placer en tête évite aussi de laisser un .deb orphelin dans POOL_DIR, dont la copie a lieu juste avant.
- routers/import_router.py, garde de frontiere renvoyant 400 avant la construction de la StreamingResponse : une HTTPException levee depuis le generateur SSE n'atteindrait plus le client.

Pas de regex : DELETE /import/groups/{group_name} (^[\w.\-+]+$, import_router.py:969) accepte "." et "..", le point étant litteral dans la classe de caracteres. Toute regex assez large pour accepter les vrais noms Debian (g++, libssl1.1, foo.bar-1+deb) risque d'accepter "..". safe_path_join() resout et compare a la base.

Non-regression : safe_path_join n'impose aucune restriction de charset, il ne rejette que la sortie de la base. Les valeurs reelles des trois appelants passent. group=None et group="" restent falsy, le repli sur le nom du paquet est inchange, et la colonne packages.import_group n'est pas touchée.

Tests: tests/test_import_group_path_traversal.py, 20 cas (traversee relative, absolue, "..", plus non-regression sur les valeurs réelles).
Ecrits avant le correctif : 12 échecs, tous des assertions de securite.
Suite complete : 1500 passed, 1 skipped.
… JWT

_parse_token() (auth/dependencies.py) chargeait la ligne utilisateur puis la jetait : elle ne servait que de test d'existence et d'activité. _require_role() décidait ensuite sur data["role"], donc sur le claim du token, un instantané figé à l'émission. Cela alimentait get_admin_user, get_maintainer_user, get_uploader_user et get_auditor_user, soit 118 points de décision.

Aggravation : POST /auth/refresh (auth/router.py:227) réémettait un token de 60 minutes en recopiant ce claim, et le frontend appelle ce refresh toutes les 45 minutes (AuthContext.js). Un rôle périmé était donc reconduit indéfiniment, sans aucune interaction.

Le défaut était silencieux : PATCH /auth/users/{username} renvoyait 200, modifiait la base et écrivait l'entrée d'audit, alors que la rétrogradation n'avait aucun effet. Seule la désactivation coupait réellement l'accès, parce que get_user() filtre active = true.

Correctif : _parse_token() écrase désormais le claim par le rôle courant lu en base, avant toute décision d'accès. Comme get_current_user_full() retourne directement ce dict et que /auth/refresh le recopie, cette seule ligne ferme aussi la boucle de reconduction, sans modifier la logique du routeur. Seule la clé « role » est remplacée : « username », « full_name » et surtout « jti » (absent de la ligne users, indispensable à /auth/logout) restent ceux du token.

Coût nul : get_user() était déjà appelé à chaque requête, on cesse seulement de jeter son résultat. .get("role", "reader") plutôt que ["role"] pour rester fail-closed, une clé absente donnant le rôle le moins privilégié au lieu d'un KeyError sur le chemin d'authentification de toutes les routes.

Tests: tests/test_role_from_db.py, 7 cas. Écrits avant le correctif : 4 échecs (rétrogradation, refresh, promotion, claim falsifié). Un test verrouille en plus la forme du dict de claims, pour interdire à un futur refactor de perdre le jti dont /auth/logout a besoin. Suite complète : 1507 passed, 1 skipped.
…-open « else » : quand METRICS_TOKEN n'est pas défini.

Le défaut livré, la fonction ne vérifiait rien. Trois docstrings promettaient un repli sur get_auditor_user qui n'a jamais été écrit, et un import était mort. Un commentaire affirmait que le jeton était généré aléatoirement, ce qui était faux.

En parallèle, metrics_middleware.py posait request.url.path brut en label Prometheus. Le registre pouvait contenir des valeurs comme /api/v1/auth/users/alice, /api/v1/artifacts/nginx/versions/1.24.0-1/download et /api/v1/security/packages/openssl/3.0.14/cve : la liste des comptes touchés par une opération d'administration et l'inventaire des paquets hébergés, lisibles anonymement puisque /metrics est proxyé publiquement par défaut (frontend/nginx.conf, nginx/tls-proxy.conf) et que le port backend écoute sur 0.0.0.0.

C'est l'anonymisation qui retire la vulnérabilité : le label porte désormais le gabarit de route, jamais le chemin concret, donc aucune donnée nominative n'existe plus dans le registre. La lecture se fait après call_next, scope ["route"] étant posé pendant le dispatch par fastapi.routing.APIRoute.matches et absent avant. Le repli quand aucune route ne correspond est une constante fixe, __unmatched__, et surtout pas le chemin demandé : les 404 sont entièrement pilotées par le client, ce repli aurait réintroduit la fuite et la cardinalité non bornée.

L'accès anonyme reste le repli documenté quand aucun METRICS_TOKEN n'est configuré, conformément à README.md et backend.env.example, dont aucune ligne ne devient fausse. Quand un jeton est configuré, l'enforcement reste strict et un JWT de rôle admin, maintainer ou auditor devient une créance alternative, ce qui évite de distribuer le jeton de scrape à un humain. Le rôle est relu en base via get_user_role(), jamais pris dans le claim, donc un JWT falsifié est refusé. Refus uniforme en 403 pour que /metrics ne serve pas d'oracle de validation de tokens volés.

Note d'exploitation : un serveur Prometheus ayant déjà scrapé les séries à chemin brut les conserve dans son TSDB pour sa durée de rétention. Les purger relève de son API admin.
…tials

docker-compose.yaml posait « CORS_ORIGINS: ${CORS_ORIGINS:-*} ». Les deux env_file sont « required: false » et aucun .env n'existe dans le dépôt : un « docker compose up -d » depuis un clone propre livrait donc allow_origins=["*"] avec allow_credentials=True.

La règle habituelle « wildcard plus credentials, le navigateur neutralise » ne s'applique pas. Vérifié par exécution, starlette 1.6.0 : la combinaison fait basculer cors.py sur allow_explicit_origin(), qui renvoie l'Origin de l'appelant au lieu de « * », accompagnée de Access-Control-Allow-Credentials: true. Le navigateur voit une origine explicite et autorise la lecture.

Conséquence : une page web visitée par un utilisateur interne pouvait appeler le backend, y compris lié sur un réseau interne ou 127.0.0.1:8000, et lire les réponses. Notamment GET /api/v1/setup/preflight, qui n'exige pas d'authentification et divulgue noms d'hôtes internes, capacité disque et versions des outils. Portée bornée : les jetons sont des Bearer en localStorage, pas des cookies, aucune session existante n'était rejouable.

main.py refuse désormais le wildcard au démarrage : levée en production, avertissement en développement, comme les quatre blocs de validation déjà présents dans le fichier. Le test porte sur l'appartenance à la liste, comme Starlette lui-même, donc un wildcard noyé dans une liste est attrapé aussi.
…reader

L'endpoint n'était gardé que par Depends(get_current_user), donc par n'importe quel compte authentifié. Il atteignait services/download.py, qui ouvre une session SSH paramiko et lance « bash ~/repodata/download-package-dep.sh <nom> » sur la machine gérée.

Suppression plutôt que restriction du rôle. C'est un doublon hérité de POST /artifacts/{name}/install, qui atteint le même sink mais exige get_uploader_user, refuse en 409 quand des dépendances manquent et écrit une entrée d'audit. Le doublon n'avait aucun des trois, ce qui est précisément pourquoi il n'a jamais reçu le garde de son remplaçant. Le passer à get_uploader_user aurait fermé le trou d'autorisation en laissant subsister un second chemin vers le pool servi contournant les deux autres garde-fous.

Aucun appelant : installPackage était exporté dans frontend/src/api.js sans être utilisé par aucune page, et aucune documentation ne mentionne la route. L'export est retiré, il aurait pointé vers un 404.
L'endpoint renvoie {username, added_at, added_by, full_name, email, role} sans filtrage ni response_model. N'importe quel compte authentifié obtenait donc nom complet, adresse e-mail et rôle de chaque membre, dont la liste des comptes admin : une cible toute faite pour du hameçonnage ou de la pulvérisation de mots de passe.

Le rôle reader est concerné, et auth/users.py le documente comme compte de service pour les machines clientes APT, donc distribué à chaque machine. GET /groups fournissant les identifiants de groupe, leur caractère non devinable ne protégeait rien.
…'import

DELETE /import/groups/{group_name} validait par re.match(r'^[\w.\-+]+$', group_name), dont la classe de caractères contient un point littéral : « .. » satisfait donc entièrement ce motif ancré.

IMPORTS_DIR / ".." résout vers la racine du dépôt, et le seul contrôle restant était exists(), vrai pour /repos/imports/.. ; pas de is_dir(), pas de confinement. shutil.rmtree effaçait le contenu du dépôt dans l'ordre de  readdir, potentiellement /repos/gnupg (clé de signature privée), /repos/pool, /repos/db ou /repos/audit, jusqu'à buter sur une frontière de bind mount.
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.

1 participant