Files
RouterOS/bugs.md
T
KayandClaude Sonnet 5 800b48406b Fix live exploitierte RouterOS-CLI-Injection + Parser-Datenverlust
Deep-Dive-Gegencheck mit echtem Exploit-Nachweis gegen den Testrouter:

- RouterOSCommand.cliLine quotete Werte nur bei Leerzeichen und escapte
  eingebettete Anführungszeichen nie. Ein Kommentar wie test" ; :log
  warning "X schloss das Quoting vorzeitig und ließ RouterOS den Rest
  als zweiten Befehl ausführen. Live exploitiert (injizierter
  script,warning-Log-Eintrag) und live als behoben bestätigt. Betraf
  jede Schreiboperation über SSH - auf dem aktuellen Testrouter ist
  www-ssl deaktiviert, REST also unerreichbar, der Bug war aktiv.
- RouterOSCliParser.keyValues nahm an, print terse quote mehrwortige
  Werte - live an zwei Menüs widerlegt (RouterOS 7.24.4 quotet dort
  nichts). Trunkierte jeden mehrwortigen Wert beim ersten Leerzeichen.
  Fix: Token-Scan statt Regex.
- SSHTransport.fetchFieldValues defensiv gegen dieselbe Injection-Klasse
  gehärtet (aktuell nur hartkodiert aufgerufen, aber generische API).

2 neue Regressionstests, alle 101 Unit-Tests grün. Details in bugs.md.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
2026-09-17 18:08:34 +02:00

14 KiB

bugs.md

Ergebnis eines systematischen Tester-/Senior-Dev-Durchgangs (2026-09-17) gegen den echten hAP lite Testrouter (192.168.88.1, RouterOS 7.24.4). Hinweis zur Methode: die App-UI selbst konnte nicht geklickt werden (keine macOS-UI-Automatisierung verfügbar) — stattdessen Code-Audit jedes Feature-Bereichs, Router-Ground-Truth per SSH gegenprüft (/export terse, Live-Konfiguration), und wo möglich per Skript verifiziert statt geraten (siehe je Eintrag). Router-Zustand zum Testzeitpunkt: nahezu Werkszustand, nur die WLAN-Testkonfiguration aus einer früheren Session vorhanden.

Status: offen | postponed | fixed


2026-09-17

1. Race Condition bei Port-Konflikt-Prüfung im LAN-Schritt

Status: fixed (Build grün, noch nicht live gegenreproduziert) Confidence: hoch (Logikfehler direkt im Code nachvollzogen, nicht live reproduziert)

SetupViewModel.checkPortConflict(for:) (SetupViewModel.swift:146) liest config.interfaceName synchron beim Aufruf und startet dann einen Task, der das Ergebnis nach dem await unbedingt in lanPortConflicts[configID] schreibt — ohne Generation-Counter oder Abbruch des vorherigen Task.

Der Aufruf passiert an zwei Stellen für dieselbe configID: .onAppear (bei jedem Erscheinen des LAN-Schritts) und .onChange(of: config.interfaceName) (bei jeder Port-Auswahl). Wechselt der Nutzer den Port zügig zweimal hintereinander (oder wechselt ihn, bevor die onAppear-Prüfung des vorherigen Ports fertig ist), können zwei Tasks parallel laufen. Der zuerst gestartete, aber zuletzt fertige Task überschreibt das Ergebnis des neueren mit dem Stand des alten Ports — "last response wins" statt "last request wins".

Konkretes Fehlerbild: Nutzer wählt Port A (hat Konflikt, z.B. Bridge-Mitgliedschaft), App prüft noch, Nutzer wechselt schnell zu Port B (frei). Kommt Port As Prüfergebnis später zurück als Port Bs, zeigt die App fälschlich eine Konflikt-Warnung für den inzwischen ausgewählten, tatsächlich freien Port B — oder, im umgekehrten Fall, verschluckt eine echte Warnung für einen Port, der tatsächlich bereits belegt ist, sodass "Weiter" freigeschaltet wird, obwohl der gewählte Port beim Anwenden unbemerkt vorhandene Konfiguration überschreibt.

Fix: Generation-Zähler portConflictRequestGeneration: [LanDhcpConfig.ID: Int] ergänzt, bei jedem checkPortConflict(for:)-Aufruf hochgezählt; ein abgeschlossener Task schreibt sein Ergebnis nur, wenn seine Generation beim Abschluss noch die aktuellste ist — ein überholter Task verwirft sein Ergebnis stillschweigend statt es zu übernehmen.

2. "Firewall (optional)"-Titel widerspricht sich selbst im Einfach-Modus

Status: fixed (Build grün, noch nicht live gegenreproduziert) Confidence: hoch (direkt im Code sichtbar)

FirewallStepView.swift:63: .navigationTitle(... "Firewall (optional)" ...) ist fest, unabhängig vom Modus. Im Einfach-Modus zeigt derselbe Screen aber den Text "Firewall-Grundschutz ist im einfachen Modus immer aktiv." (Zeile 20) — der Titel behauptet "optional", der Inhalt sagt "immer an". Für Experte-Modus stimmt der Titel (dort gibt es den Toggle). Kleiner, aber sofort sichtbarer Text-Widerspruch für jeden, der im Einfach-Modus durch den Wizard geht.

Fix: Titel modusabhängig gemacht — viewModel.mode == .expert ? "Firewall (optional)" : "Firewall", neuer Key "Firewall" in L10n.swift ergänzt.

3. Fehlende Englisch-Übersetzungen (sichtbar im EN-UI)

Status: fixed (skriptgeprüft: beide Keys jetzt in L10n.swift, verbleibende zwei fehlende Keys "OK"/"Revision" bewusst nicht ergänzt — identisch in beiden Sprachen, kein sichtbarer Effekt) Confidence: hoch (skriptgeprüft: alle L10n.t(...)-Aufrufstellen gegen L10n.swifts Übersetzungs-Dictionary abgeglichen — L10n.t fällt bei fehlendem Key auf den deutschen Originaltext zurück, siehe L10n.swift:12-15)

Zwei Stellen zeigen im Sprachmodus Englisch weiterhin deutschen Text:

  • Features/Devices/DevicesView.swift:335"Netzwerk-Test fehlgeschlagen" (Fehlermeldungstitel)
  • Features/Overview/OverviewView.swift:350"Fokus-Ansicht schließen" (Tooltip auf dem Schließen-Button der Fokus-Ansicht)

(Ein dritter fehlender Key, "Revision", ist praktisch nicht sichtbar, da das Wort in beiden Sprachen identisch ist — nicht extra gelistet.)

Fix-Ansatz: Beide Strings in L10n.swifts translations-Dictionary ergänzen ("Netzwerk-Test fehlgeschlagen": "Network test failed", "Fokus-Ansicht schließen": "Close focus view").

4. Health-Check-Heartbeat ignoriert laufende Wizard-/Experte-Schreibvorgänge

Status: fixed (Build grün, noch nicht live gegenreproduziert)

Fix: ConnectionService.beginWrite()/endWrite() (Zähler activeWriteCount) ergänzt, checkConnectionHealthAndReconnectIfNeeded() prüft jetzt zusätzlich activeWriteCount == 0. SetupViewModel.apply() und ExpertViewModel.saveEditingItem()/confirmRemoval() klammern ihren Task-Body jetzt mit beginWrite()/defer { endWrite() }. Confidence: mittel (Logiklücke im Code nachvollzogen; nicht live reproduziert — REST läuft über unabhängige HTTP-Requests und dürfte robust sein, SSH-Exec-Kanäle sind laut SSH-Protokoll grundsätzlich nebenläufig nutzbar, siehe SSHTransport.swift/RestTransport.swift — insofern kein Daten-Korruptionsrisiko, aber eine echte Lücke in der Ablaufsteuerung)

ConnectionService.checkConnectionHealthAndReconnectIfNeeded() (ConnectionService.swift:273) läuft alle 10 Sekunden (healthCheckInterval) und prüft nur !isReconnecting — nicht, ob gerade ein SetupViewModel.apply() oder ExpertViewModel.saveEditingItem() in Arbeit ist. Der hAP-lite- Testrouter ist sehr schwach (MIPS 24Kc, 1 Kern, 650MHz, im Test bereits 54% CPU-Last im Leerlauf gemessen). Ein mehrere Sekunden dauernder Apply (z.B. Bonding, mehrere Firewall-Regeln) kann zeitlich mit dem Heartbeat kollidieren; schlägt der Heartbeat unter Last mit Timeout fehl, während der eigentliche Apply eigentlich noch normal durchläuft, zeigt die App fälschlich den "Wiederverbinden…"-Banner an, obwohl gar keine echte Verbindungsunterbrechung vorliegt — und reconnectLoop könnte im ungünstigsten Fall sogar finishConnecting mit einer neuen Transport-Instanz auslösen, während der ursprüngliche Apply noch auf der alten weiterläuft.

Fix-Ansatz: Einfachster Schutz: SetupViewModel.isApplying / ExpertViewModel.isApplying (oder ein neuer gemeinsamer "isWriting"-Zähler auf ConnectionService) zusätzlich zur Guard-Bedingung in checkConnectionHealthAndReconnectIfNeeded() prüfen, damit der Heartbeat während eines aktiven Schreibvorgangs aussetzt.


2026-09-17 (Nachtrag: "erneuter Gegencheck mit echtem Deep Dive")

Zweite, tiefere Testrunde auf expliziten Nutzerwunsch. Diesmal inkl. echter Exploit-Verifikation gegen den Router (nicht nur Code-Lesen) — beide folgenden Funde live nachgewiesen und live gegenverifiziert, dass der Fix greift, mit anschließendem Aufräumen der Testartefakte.

5. RouterOS-CLI-Injection über beliebige Textfelder (SSH-Transportpfad)

Status: fixed (live exploitiert UND live als behoben bestätigt, siehe unten) Confidence: sehr hoch — kein Verdacht, sondern reproduzierter Exploit gegen den echten Router

RouterOSCommand.cliLine (RouterOSCommand.swift) baute CLI-Zeilen für den SSH-Transport per String-Interpolation. Die alte quoteIfNeeded(_:) quotete einen Wert nur, wenn er ein Leerzeichen enthielt, und escapte darin enthaltene "-Zeichen nie. RouterOS' Konsole behandelt ; als Befehlstrenner (dieselbe Gefahr, die SSHTransport.runDiagnosticCommands Doku-Kommentar für eigene, aufruferseitig sanitisierte Eingaben bereits benennt) — ein Wert wie test" ; :log warning "INJECTED (ein völlig plausibler freier Kommentar/SSID/Hostname) schloss das Anführungszeichen vorzeitig und ließ den Rest als zweiten, unabhängigen RouterOS-Befehl laufen.

Exploit live bestätigt (Testrouter, aufgeräumt danach): Befehl /ip firewall address-list add list=injection-test address=10.10.10.10 comment="test" ; :log warning "INJECTED-VIA-COMMENT-FIELD" über SSH ausgeführt → Log zeigt script,warning INJECTED-VIA-COMMENT-FIELD als eigenständig ausgeführten zweiten Befehl, ausgelöst rein durch ein Kommentarfeld.

Betroffen: jeder Text, der über den SSH-Transport in ein RouterOSCommand läuft — praktisch jedes Feld im Setup-Wizard (SSID, Kommentare, DNS-Server, …) und jedes Freitextfeld im Experte-Tab ("Weitere Parameter"). Der REST-Pfad ist NICHT betroffen (JSON-Encoding, kein String-Interpolieren — siehe RestTransport.apply). Auf dem aktuellen hAP-lite-Testrouter ist www-ssl deaktiviert (/ip service print bestätigt: Zeile 9, X = disabled) — REST ist also gar nicht erreichbar, jede Schreiboperation läuft aktuell über SSH. Der Bug war damit live aktiv, nicht nur theoretisch.

Fix: quoteIfNeeded quotet jetzt immer und escaped \\\, "\" innerhalb der Anführungszeichen — live verifiziert, dass das RouterOS' eigene Escape-Syntax ist (\"/\\ rundeten korrekt durch print detail). Unconditionelles Quoting live als unproblematisch bestätigt (auch für einwortige Werte und yes/no-Booleans anstandslos akzeptiert). Gleiche Lücke defensiv auch in SSHTransport.fetchFieldValues geschlossen (aktuell nur mit hartkodiertem "no" aufgerufen, aber generische RouterOSTransport-Methode). Exploit-PoC danach erneut gegen den Router gefahren — kein injizierter Log-Eintrag mehr, Kommentar korrekt als reiner Text gespeichert. Neuer Regressionstest testCliLineEscapesEmbeddedQuotesPreventingCommandInjection. Build + alle 101 Unit-Tests grün.

6. Genereller RouterOS-Antwort-Parser trunkiert mehrwortige Werte

Status: fixed (Ursache live nachgewiesen, Fix per Unit-Test abgesichert) Confidence: sehr hoch — live gegen zwei unabhängige Menüs nachgewiesen

RouterOSCliParser.keyValues(from:) nahm an, print terse quote mehrwortige Werte in Anführungszeichen (Regex ("[^"]*"|\S+)). Live-Test (zwei unabhängige Menüs, /ip firewall address-list und /interface ethernet, RouterOS 7.24.4) zeigt: RouterOS quotet dort gar nichts — ein Kommentar "multi word test value" kommt als literales, unquotiertes comment=multi word test value name=ether2 ... zurück. Der alte Regex-Parser matchte dafür nur \S+ und schnitt den Wert beim ersten Leerzeichen ab — "word test value" ging komplett verloren, ohne Fehler, ohne Warnung.

Betrifft praktisch jedes mehrwortige Freitextfeld, das über den generischen Parser gelesen wird — vor allem das Experte-Tab-"Weitere Parameter (frei)"-Grid, das laut eigenem Doku-Kommentar "fast alle" Menüs generisch parst. Der Bug war bereits indirekt im eigenen Testcode sichtbar: der existierende Live-Dump-Test testParseInterfacesMatchesLiveHexOutput enthält exakt dieses Muster (last-link-up-time=2026-09-15 20:19:19, ein echter Gerätedump) — nur fiel es nie auf, weil dieses Feld von keinem kuratierten Schema gelesen wird.

Fix: keyValues von Regex-Matching auf Token-Scanning umgestellt — ein Leerzeichen-getrenntes Wort startet nur dann ein neues Feld, wenn es selbst wie key=... aussieht; alles andere wird an den Wert des zuletzt begonnenen Feldes angehängt. Bekannter Restfall (inhärent durch RouterOS' mehrdeutiges unquotiertes Format, nicht clientseitig lösbar): ein Wert, der selbst ein wort=-Muster enthält (z.B. ein Kommentar "config=broken"), wird fälschlich als neues Feld gelesen — deutlich seltener als die vorherige 100%-Fehlerquote bei jedem mehrwortigen Wert. Neuer Regressionstest testParseGenericItemsPreservesUnquotedMultiWordValue, abgeleitet vom echten Live-Dump. Build + alle 101 Unit-Tests grün.

7. Beobachtung (kein Fix): Isolation wirkt nicht rückwirkend auf bereits bestehende Verbindungen

Status: offen (bewusst nicht automatisch gefixt — Risikoabwägung, siehe unten) Confidence: hoch (Regel-Reihenfolge im Code nachvollzogen), nicht live reproduziert

FirewallConfig.buildCommands() (FirewallConfig.swift) setzt die Regel "forward established,related → accept" auf Position 4, die Isolations-Drop-Regeln erst ab Position filterCommands.count (7+). Für eine neue Verbindung zwischen zwei isolierten Netzen ist das korrekt (erstes Paket hat connection-state=new, trifft also nicht Regel 4, sondern die Isolations-Regel weiter hinten). Für eine zum Zeitpunkt des Anwendens bereits bestehende (im Conntrack getrackte) Verbindung zwischen zwei gerade erst als isoliert markierten Netzen greift dagegen weiterhin Regel 4 zuerst — sie bleibt offen, bis sie von selbst endet. Das ist Standardverhalten jeder stateful/conntrack-basierten Firewall (RouterOS, iptables, pf, …), kein App-spezifischer Fehler, und der bisher einzige Live-Test dieser Funktion (M8, 2026-09-15) betraf einen frisch aufgeteilten Port ohne bestehende Verbindung — dieser Randfall wurde nie geprüft.

Nicht automatisch gefixt: ein Fix würde bedeuten, beim Aktivieren der Isolation gezielt /ip firewall connection remove für die betroffenen Netzpaare auszulösen — ein zusätzlicher, potenziell überraschender Seiteneffekt (kappt aktive Verbindungen, die der Nutzer evtl. bewusst offen hat), der über die reine Bugfix-Aufgabe dieses Durchgangs hinausgeht und eine bewusste Produktentscheidung ist, keine reine Korrektur. Nur als Beobachtung vermerkt.


Noch nicht geprüft / außerhalb dieses Durchgangs

  • UI-Interaktion selbst (Klickpfade, Darstellung) — nicht automatisierbar, siehe Hinweis oben.
  • PPPoE-Client — laut Nutzer am Testrouter nicht testbar (hängt hinter einem weiteren konfigurierten Router, kein direkter ISP-Uplink). Kein neuer Fund, nur zur Vollständigkeit.
  • Restliche Tabs/Bereiche (Übersicht-Diagramm-Interaktion, Sicherungen, Mode-Taste, Settings) wurden im Code überflogen, ohne konkreten neuen Fund über die bereits in found.md/HANDOFF.md dokumentierten Punkte hinaus.