diff --git a/AUDIT.md b/AUDIT.md new file mode 100644 index 0000000..0fcd583 --- /dev/null +++ b/AUDIT.md @@ -0,0 +1,238 @@ +# Audit de reprise — Perun for DCS World + +> Revue menée pour décider d'une reprise du projet (upstream `szporwolik/perun`, +> **archivé le 2026-03-29**, licence MIT). Fork de travail : `DaKerboul/perun`, +> miroir Gitea : `git.kerboul.me/kerboul/perun`. +> +> **Méthode.** Revue à 5 casquettes (Architecte, Sécurité, Backend .NET, DCS/Lua, +> DevOps/DBA). **Périmètre audité** : le code réellement écrit par le projet — +> hook Lua (`01_DCS`), app C# WinForms (`02_Windows_App`), wrapper C++ (`03_*`, +> hors arbre Lua 5.1.5 vendoré), schéma MySQL (`04_MySQL`), exemple PHP +> (`05_Misc`). Les ~35 `.c`/`.h` de `lua-5.1.5/` sont du Lua amont vendoré et **ne +> sont pas audités**. +> Date : 2026-06-11. + +--- + +## Synthèse — verdict + +Projet **fonctionnellement riche et bien pensé sur le fond** (modèle de données +propre, couverture événementielle DCS complète, multi-instances, intégrations +SRS/LotATC). Mais **dette de sécurité critique** et code applicatif daté +(monolithes, threading artisanal, zéro test, zéro CI, Windows/.NET Framework +uniquement). C'est une **bonne base à reprendre**, à condition de traiter les +points P0 **avant** toute remise en production. + +| # | Domaine | Gravité | Sévérité | +|---|---------|---------|----------| +| S1 | Injection SQL via données joueur (app C#) | **Critique** | 🔴 P0 | +| S2 | Port TCP sans authentification, bind `0.0.0.0` | **Critique** | 🔴 P0 | +| S3 | XSS stocké dans l'exemple PHP | Élevé | 🟠 P1 | +| S4 | Mot de passe MySQL stocké en clair | Élevé | 🟠 P1 | +| S5 | PII (IP, UCID) sans rétention ni consentement (RGPD) | Élevé | 🟠 P1 | +| B1 | Threading non synchronisé (buffer partagé) | Élevé | 🟠 P1 | +| B2 | `ExecuteReader` pour des INSERT/UPDATE, 1 connexion/frame | Moyen | 🟡 P2 | +| A1 | Monolithes, `dynamic`, aucun test, aucune CI | Moyen | 🟡 P2 | +| L1 | Fuite de variables globales dans le hook DCS | Moyen | 🟡 P2 | +| D1 | Lua 5.1.5 vendoré dans le repo, pas de build reproductible | Moyen | 🟡 P2 | +| D2 | Pas de FK, dépendance à `STRICT_TRANS_TABLES` off, pas de migrations | Faible | 🟢 P3 | + +--- + +## 1. Architecte / Lead — structure & dette + +**Points forts** +- Découpage fonctionnel clair en 5 dossiers numérotés, lisible d'emblée. +- Séparation nette des responsabilités : collecte (Lua) → transport (TCP/DLL) → + persistance (C#) → restitution (PHP). Le protocole de trames (IDs 1/2/3/50…101) + est documenté dans le README. +- Multi-instances supporté de bout en bout (champ `instance` partout). + +**Points faibles** +- **Monolithes.** `DatabaseController.SendToMySql` fait ~290 lignes avec un `switch` + géant mêlant construction SQL, mapping métier et logging + (`01_Classes/DatabaseController.cs:12-304`). Idem `TCPController.StartListen` + (boucles imbriquées sur ~170 lignes). +- **`dynamic` partout** pour le JSON entrant (`DatabaseController.cs:25`, + `TCPController.cs:121`) : aucune validation de schéma, accès `TCPFrame.payload.x` + qui lèvent au moindre champ manquant → exceptions au lieu d'un rejet propre. +- **Plateforme verrouillée** : VS2017, .NET Framework 4.8, WinForms → Windows + uniquement, fin de vie. Or le serveur DCS est Windows, mais l'app de + persistance n'a aucune raison d'y être clouée (elle ne parle que TCP + MySQL). +- **TODO laissé en dur** (commentaire polonais) : + `DatabaseController.cs:92` « TUTAJ DODAC CATCH TBD » → gestion d'erreur inachevée. +- **Aucun test, aucune CI**, dérive de version (cf. §4 L-version). + +**À faire** +- [ ] Extraire la construction SQL dans une couche dédiée (un handler par type de + trame) + DTO typés à la place de `dynamic`. +- [ ] Cibler **.NET 8** + un worker headless multiplateforme ; garder l'UI WinForms + en option (ou la remplacer par un petit panneau web/CLI). +- [ ] Introduire des tests unitaires (parsing de trames, génération SQL) et un + pipeline CI. + +## 2. Sécurité (AppSec) — **bloquant pour la prod** + +**S1 — Injection SQL via données contrôlées par le joueur. 🔴** +L'app paramètre *certaines* valeurs (`@PAR_*`) mais **en concatène des dizaines +d'autres** directement dans le SQL, dont des champs que n'importe quel joueur +maîtrise (UCID, nom, hash de mission, IP, datetime, tous les compteurs `ps_*`) : +- `DatabaseController.cs:128-132` (chat) : `ucid`, `missionhash`, `all`, `datetime`. +- `DatabaseController.cs:159-165` (stats) : `stat_ucid`, `stat_missionhash` et + ~30 valeurs `stat_data_perun.ps_*` injectées telles quelles. +- `DatabaseController.cs:175-177` (login) : `login_ucid`, `login_ipaddr`, `login_datetime`. +- `DatabaseController.cs:50-51, 77-89, 205-206` : `instance`/`type` concaténés. + +Un UCID/nom forgé (ou un module client modifié) permet l'exfiltration ou la +destruction de la base. C'est le **défaut n°1 à corriger**. +→ **Tout** passer en requêtes paramétrées (`MySqlParameter`), sans exception. + +**S2 — Listener TCP non authentifié, exposé. 🔴** +`new TcpListener(IPAddress.Any, intListenPort)` (`TCPController.cs:51`) écoute sur +**toutes** les interfaces, **sans authentification ni allowlist**. Couplé à S1, +n'importe quel hôte joignant le port (48621 par défaut) injecte des trames +arbitraires → compromission complète de la base. +→ Par défaut **bind `127.0.0.1`** (hook et app sont quasi toujours sur la même +machine), + secret partagé/HMAC sur les trames, + allowlist d'IP. + +**S3 — XSS stocké (PHP). 🟠** +`05_Misc/05_PHP_Example/index.php` réinjecte en HTML des données joueur **sans +échappement** : message de chat (`:110`), nom (`:92, :109, :143`), +contenu d'événement (`:127`). Un joueur dont le nom vaut `` +exécute du JS dans le navigateur de l'admin. Les requêtes elles-mêmes sont +statiques (pas de SQLi côté PHP), le risque est l'**XSS**. +→ `htmlspecialchars()` systématique sur toute sortie ; corriger aussi le HTML +invalide (`

` fermé par `

` ligne 44). + +**S4 — Identifiants MySQL en clair. 🟠** +Le mot de passe est stocké en `String` dans les user settings .NET +(`02_Forms/form_Main.cs:119`, clé `MYSQL_Password` de `app.config`) → écrit en +clair dans `user.config`. +→ Chiffrer via **DPAPI** (`ProtectedData`) ou déléguer à un gestionnaire de +secrets ; a minima ne jamais journaliser la chaîne de connexion. + +**S5 — Données personnelles (RGPD). 🟠** +Le hook collecte et stocke **adresses IP** et **UCID** des joueurs +(`Perun-hook.lua:364-366` → `pe_DataPlayers_lastip`, `pe_LogLogins_ip`), sans +politique de rétention ni information des joueurs. +→ Définir une durée de rétention + purge, anonymiser/hacher l'IP si non +nécessaire, documenter (mention serveur + Discord). + +## 3. Backend / .NET — qualité & robustesse + +**B1 — Threading artisanal non synchronisé. 🟠** +Le thread TCP écrit dans `Globals.arrMySQLSendBuffer` (tableau fixe) pendant que +le thread d'envoi le lit, **sans verrou** (`TCPController.cs:130-142`). Buffer +plein = paquets **silencieusement perdus** (`:139-141`), scan linéaire O(n) par +paquet. Accès concurrents → corruption/race. +→ Remplacer par une `BlockingCollection`/`Channel` thread-safe et bornée. + +**B2 — Accès base inefficace. 🟡** +- `ExecuteReader()` utilisé pour exécuter des lots d'INSERT/UPDATE + (`DatabaseController.cs:217`) : devrait être `ExecuteNonQuery()`. +- **Une nouvelle `MySqlConnection` ouverte/fermée par trame** (`:34, :292`) : + pas de réutilisation du pool, surcoût réseau par paquet. +- Tout est **synchrone bloquant** (pas d'`async`/`await`). +→ Connexion/pool réutilisé, requêtes asynchrones, `ExecuteNonQueryAsync`. + +**Autres** +- Dépendance `Newtonsoft.Json` → migrable vers `System.Text.Json`. +- Gestion d'erreurs par numéros MySQL en dur (`:246-279`) : utile mais fragile, + à compléter (le catch manquant signalé en `:92`). + +## 4. DCS / Lua / Intégration + +**Points forts** +- Couverture événementielle DCS **très complète** : kill (avec catégorisation + PvP/AI), friendly fire, crash, eject, takeoff/landing (airfield/ship/FARP), + multicrew, change_slot, connect/disconnect, chat, MOTD. C'est le vrai actif du + projet (`Perun-hook.lua:725-879`). +- Comptage de stats maison car les stats natives DCS sont peu fiables (choix + assumé et pertinent). + +**Points faibles** +- **L1 — Fuite de variables globales 🟡** : plusieurs variables sont assignées + sans `local` dans l'environnement *hook* (privilégié et partagé) — ex. + `_temp_killers`/`_temp_event_type` (`:754-756`), `_master_type`/`_master_slot`/ + `_sub_slot` (`:813`), `_temp_airfield` (`:849`). Risque de collision avec + d'autres hooks installés sur le serveur. +- **L-version — dérive de version** : la version du hook est codée en dur + `"v0.12.1"` (`:30`) et vit séparément de la version de l'app (`Globals.VersionPerun`). + → source unique de vérité (tag git → injecté au build). +- **Coût par frame** : `onSimulationFrame` fait du `table.concat`/JSON à chaque + frame ; sur serveur chargé, surveiller le budget temps (déjà mesuré en µs dans + les logs — bon réflexe à conserver/exposer). +- **Dépendance à une DLL compilée** (`perun.dll`, issue de `03_Perun_Lua_Wrapper`) + livrée hors repo : reproductibilité du build à fiabiliser (cf. D1). +- TCP **en clair**, pas de TLS (acceptable en loopback, à revoir si distant). + +**À faire** +- [ ] `local` sur toutes les temporaires ; passe `luacheck`. +- [ ] Versionner hook + app depuis le tag git. +- [ ] Documenter/reproduire le build de `perun.dll` (CMake déjà présent). + +## 5. DevOps / DBA / Release + +**Base de données — plutôt saine.** +- InnoDB + `utf8mb4`/`unicode_ci`, PK/`AUTO_INCREMENT`, **clés UNIQUE** + pertinentes (UCID, hash, type, stats par mission+ucid+type) et index sur les + colonnes de tri (`datetime`, `instance`, `type`) — `04_MySQL/m1081_perun.sql`. +- **D2 🟢** : pas de **`FOREIGN KEY`** (intégrité référentielle non garantie), + dépendance documentée à **`STRICT_TRANS_TABLES` désactivé** (README:42,84) — + c'est-à-dire qu'on s'appuie sur la coercition/troncature silencieuse de MySQL, + ce qui masque des bugs. Nom de fichier cryptique (`m1081_perun.sql`). +- Pas d'outil de **migration** (un seul dump). → introduire des migrations + versionnées (Flyway/dbmate/sqitch) et, à terme, des FK + un mode strict assumé. + +**Build / release / repo** +- **D1 🟡** : arbre **Lua 5.1.5 complet vendoré** dans `03_Perun_Lua_Wrapper/lua-5.1.5/` + → gonfle le repo et l'audit. Le passer en sous-module / téléchargement au build. +- Build **manuel VS2017**, pas de CI, pas d'artefacts reproductibles. +- Contributions historiquement attendues sur la branche `dev` (README:135). +- Pas de `.gitignore` racine (un seul dans le wrapper). + +**À faire** +- [ ] CI (build C# + `luacheck` + lint PHP + validation du schéma SQL). +- [ ] Migrations DB versionnées ; activer les FK progressivement. +- [ ] Sortir Lua amont du repo ; pipeline de build de la DLL. +- [ ] Releases taguées avec binaires (`perun.dll` + app) attachés. + +--- + +## Feuille de route priorisée + +**P0 — Sécurité bloquante (avant toute prod)** +1. Paramétrer **100 %** des requêtes SQL (S1). +2. Bind loopback par défaut + auth/allowlist sur le listener TCP (S2). + +**P1 — Durcissement & conformité** +3. Échappement HTML de l'exemple PHP (S3). +4. Chiffrer le mot de passe MySQL — DPAPI (S4). +5. Rétention/anonymisation IP & UCID, doc RGPD (S5). +6. File thread-safe bornée à la place du buffer tableau (B1). + +**P2 — Modernisation** +7. Découper les monolithes, DTO typés au lieu de `dynamic` (A1). +8. Connexion poolée + accès DB async + `ExecuteNonQuery` (B2). +9. `local` + `luacheck` sur le hook, version unifiée depuis git (L1). +10. Cibler .NET 8 / worker multiplateforme. +11. Tests + CI. + +**P3 — Hygiène long terme** +12. Migrations DB, FK, mode SQL strict assumé (D2). +13. Désvendoriser Lua, build reproductible de la DLL (D1). +14. Releases taguées + artefacts. + +## Plan de reprise suggéré + +1. **Geler le comportement** : quelques tests de caractérisation sur le parsing de + trames et la génération SQL, pour refactorer sans régresser. +2. **Sprint sécu (P0)** sur une branche `security/sql-and-tcp`, puis tag + `v0.13.0-rc` testé en loopback. +3. **P1** en incréments livrables. +4. Décider de la cible app (garder WinForms vs worker .NET 8) **avant** d'attaquer + P2 — ça oriente tout le refactor. + +> Adapté à ton contexte : tu fais déjà tourner Gitea + CI sur le cluster ; un +> pipeline build/lint Perun s'y intègre directement, et la communauté Commus DCS +> est un terrain de test naturel pour les stats.