From 5ada0e50387bd55a622aa519ee7265f2a8febfd5 Mon Sep 17 00:00:00 2001 From: Bernd Steckmeister Date: Sat, 4 Jul 2026 14:46:27 +0200 Subject: [PATCH] =?UTF-8?q?security:=20server.py=20Restbefunde=20aus=20Rev?= =?UTF-8?q?iew=20(r)=20=E2=80=93=20GET-Leser-Lock,=20Content-Length-Haertu?= =?UTF-8?q?ng,=20secrets-Token?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Fable 5 --- CHANGES.md | 8 +++++++ PROGRESS.md | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++ ROADMAP.md | 4 ++++ 3 files changed, 80 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 9bdc75e..f6eb669 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1265,3 +1265,11 @@ 2026-07-04 14:19 [Edit] /home/steggi/pyramid/PROGRESS.md 2026-07-04 14:20 [Edit] /home/steggi/pyramid/PROGRESS.md 2026-07-04 14:20 [Edit] /home/steggi/pyramid/ROADMAP.md +2026-07-04 14:43 [Edit] /home/steggi/matrix/server.py +2026-07-04 14:44 [Edit] /home/steggi/matrix/server.py +2026-07-04 14:44 [Edit] /home/steggi/matrix/server.py +2026-07-04 14:44 [Edit] /home/steggi/matrix/server.py +2026-07-04 14:44 [Edit] /home/steggi/matrix/server.py +2026-07-04 14:45 [Edit] /home/steggi/pyramid/PROGRESS.md +2026-07-04 14:46 [Edit] /home/steggi/pyramid/PROGRESS.md +2026-07-04 14:46 [Edit] /home/steggi/pyramid/ROADMAP.md diff --git a/PROGRESS.md b/PROGRESS.md index b03220b..46626d6 100644 --- a/PROGRESS.md +++ b/PROGRESS.md @@ -13,6 +13,59 @@ Schritt (und beim Abbruch mitten im Schritt den Zwischenstand). Format: --- +## 2026-07-04 – Fable-5-Review (r): ADMIN_LOCK-Umbau (q) geprüft – korrekt, aber 3 kleinere Restbefunde gefunden und gefixt + +**Erledigt:** Review-Pass über die jüngste erledigte Arbeit – den ADMIN_LOCK-Umbau +aus (q) (Commit c8ff850) kritisch gegengelesen und als Punkt (r) im +Fable-5-Review-Abschnitt abgehakt. Der Umbau selbst ist korrekt (Locking-Struktur, +Routen-Reihenfolge, Handler-Logik unverändert). Drei kleinere Restbefunde in +`server.py` gefunden und gefixt: + +1. **GET-Leser-Race (gleiche Familie wie (q), dort übersehen):** (q) hatte nur + POST-gegen-POST betrachtet – `GET /api/registration-status` liest + `conduit.toml` aber OHNE Lock, während `toggle-registration`/`create-invite` + die Datei per Truncate+Write in-place neu schreiben → seit multithreaded (p) + kann der Leser eine halb geschriebene/leere Datei sehen (Dashboard zeigt + transient falschen Registrierungs-Status). Fix: Lesen unter `ADMIN_LOCK`. + **Wichtige Erkenntnis dabei:** Der naheliegendere Fix (atomares `os.replace` + statt In-Place-Write) wäre FALSCH – `conduit.toml` ist als **einzelne Datei** + in den Container gemountet (`./conduit.toml:/etc/conduit.toml:ro`), ein + Replace tauscht den Inode und der Container sähe für immer die alte Datei. + Im Code als Kommentar festgehalten, damit das nie jemand „verbessert". +2. **Negative `Content-Length`:** `livekit-token`, `e2ee-diagnostics` und + `ban`/`unban` prüften nur `length > MAX` – ein negativer Wert wäre + durchgerutscht und `rfile.read(-n)` liest bis EOF (Thread hängt bis zum + 30-s-Timeout). Fix: `length < 0` wird wie „zu groß" abgelehnt (413). +3. **`ban`/`unban` ohne Body-Limit + `random` für Einladungs-Token:** + ban/unban lasen beliebig große Bodies (und halten dabei seit (q) den + ADMIN_LOCK) → 4-KB-Deckel wie bei livekit-token. Einladungs-Token + (`create-invite`) kamen aus dem vorhersagbaren `random`-PRNG, obwohl sie + die Registrierung am Homeserver freischalten → `secrets.choice`. + +**Verifiziert (headless, nach Dienst-Neustart per kill/`Restart=always`):** +`py_compile` sauber, Diff gegen Backup minimal wie geplant +(`server.py.bak-20260704-review-r`). 8 Routen-Checks: stats.html 200, +registration-status 200 (liest jetzt unter Lock), ban ohne Nutzer localhost → +400 „Kein Nutzer angegeben", **ban mit 8-KB-Body → 413 (neu)**, **livekit-token +mit `Content-Length: -5` → sofort 413 statt Hänger (neu)**, livekit-token +ungültiger Matrix-Token → 401, e2ee-diagnostics falscher Token → 403, +Admin-Route öffentlich über die Dashboard-Domain → 403. Test-Deny-Zeile aus +`security_alerts.log` wieder entfernt. + +**Offen/Nächster Schritt:** Unverändert – alle offenen ROADMAP-Punkte brauchen +PC/Gerät (SQLCipher, LiveKit-Client-Umstellung, Härtetest) oder Bernd +(Secret-Rotation, Dashboard-Fernzugriff A/B, Call-Pilot-Go). + +**Stolperfallen:** (1) Bei Nebenläufigkeits-Reviews nicht nur Schreiber gegen +Schreiber prüfen, sondern auch die LESER derselben Ressource – `do_GET` war in +(q) komplett außen vor. (2) „Atomares Schreiben per `os.replace`" ist bei +Docker-**Datei**-Bind-Mounts eine Falle: der Mount hängt am Inode, nach einem +Replace sieht der Container die alte Datei weiter. In-Place-Write ist dort +absichtlich richtig. (3) `Content-Length` kann negativ sein – wer nur nach oben +deckelt, lässt `read()` bis EOF blockieren. + +--- + ## 2026-07-04 – Fable-5-Review (q): Threading-Härtung (p) tiefer geprüft – echte Nebenläufigkeits-Race in den Admin-Routen gefunden und mit ADMIN_LOCK behoben **Erledigt:** Review-Pass über die jüngste erledigte Arbeit – die Threading-Umstellung @@ -745,6 +798,21 @@ schließt den Review ab. vorher; Race-Beweis (2 parallele `run-stats` 2633 ms ≈ 2× einzeln 1388 ms → serialisiert); Non-Blocking-Beweis (öffentliches `livekit-token` während gehaltenem Lock in 11 ms). Details im PROGRESS-Eintrag „(q)" oben. +- [x] **(r) ADMIN_LOCK-Umbau aus (q) (c8ff850) geprüft – Umbau korrekt, 3 kleinere + Restbefunde gefixt (2026-07-04).** Locking-Struktur und Routen-Reihenfolge von (q) + bestätigt (öffentliche Routen vor dem Gate/Lock, Handler-Logik unverändert). + Befunde: (1) `GET /api/registration-status` las `conduit.toml` ohne Lock, während + die Admin-Routen sie per Truncate+Write neu schreiben – Leser konnte seit + multithreaded (p) eine halb geschriebene Datei sehen; Fix: Lesen unter `ADMIN_LOCK` + (bewusst KEIN `os.replace`: die Datei ist als Einzeldatei in den Container + gemountet, Inode-Falle – im Code dokumentiert). (2) Negative `Content-Length` + ließ `read()` bis EOF blockieren (Thread hing bis zum 30-s-Timeout) – bei + `livekit-token`/`e2ee-diagnostics`/`ban`/`unban` wird `< 0` jetzt mit 413 + abgelehnt. (3) `ban`/`unban` ohne Body-Limit (hielten dabei den ADMIN_LOCK) → + 4-KB-Deckel; Einladungs-Token von `random` auf `secrets` umgestellt (Token + schaltet Registrierung frei). Headless verifiziert (8 Checks inkl. 8-KB-Body → + 413, `Content-Length: -5` → sofort 413 statt Hänger). Details im + PROGRESS-Eintrag „(r)" oben. **Fazit:** Die Sonnet-5-Arbeit war handwerklich sauber; die zwei echten Lücken (verlorene Kommentare, fehlender Soft-Logout-Guard) hatte die abgebrochene diff --git a/ROADMAP.md b/ROADMAP.md index 1640719..892334f 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -79,6 +79,10 @@ Punkte abhaken (`[x]`), wenn erledigt UND in `PROGRESS.md` protokolliert. `send_admin`-Readback) ist mit `ADMIN_LOCK` geschlossen, das NUR die Admin-Mutationen serialisiert – die öffentlichen Routen bleiben nebenläufig; headless verifiziert (2× parallel = 2× Laufzeit, öffentlicher Pfad 11 ms). + Zusatz (Fable-5-Review (r), 2026-07-04): drei Restbefunde gefixt – + `registration-status`-Leser jetzt unter ADMIN_LOCK (Truncate-Write-Race), + negative/übergroße `Content-Length` überall abgelehnt (kein Thread-Hänger, + ban/unban 4-KB-Deckel), Einladungs-Token per `secrets` statt `random`. - [x] **LiveKit-Token-Route server-seitig gebaut + verifiziert** (Fable-5-Review (n), PROGRESS.md 2026-07-04). `POST /api/livekit-token` läuft live in `server.py` (whoami-Auth, stdlib-HS256-Minting aus `livekit.yaml`), headless geprüft