diff --git a/Changes.md b/Changes.md index 0fc1276..5bd7a48 100644 --- a/Changes.md +++ b/Changes.md @@ -2,6 +2,65 @@ --- +## 2026-07-30 – Fix: SMTP-Header-/Kommando-Injection über mail_from_email (Issue #148) + +- `SmtpMailer::sendMessage()`/`buildMessage()` verwendeten `fromEmail` roh sowohl im `MAIL FROM:<...>`-SMTP-Kommando als auch im `From:`-Header, während alle anderen dynamischen Werte (Empfänger, `fromName`, `subject`, Anhangsname) bereits validiert bzw. CRLF-bereinigt wurden. `mail_from_email` (Firmeneinstellung, per POST setzbar) durchlief keine serverseitige Prüfung – ein eingeloggter Nutzer konnte per CRLF-Payload zusätzliche SMTP-Kommandos (z.B. eine verdeckte `RCPT TO`-Zeile) einschleusen oder den `From:`-Header manipulieren (CWE-93). +- **`src/Models/Setting.php`:** `Setting::update()` validiert `mail_from_email` jetzt über `filter_var(..., FILTER_VALIDATE_EMAIL)` (neue private Methode `sanitizeFromEmail()`); ungültige Werte (inkl. CRLF-Payloads) werden verworfen statt gespeichert. +- **`src/Services/SmtpMailer.php`:** Defense-in-Depth – neue Methode `sanitizeAddress()` entfernt CR/LF aus `fromEmail`, bevor dieser im `MAIL FROM:`-Kommando und im `From:`-Header verwendet wird. +- **`tests/69_MailFromEmailInjectionTest.php`:** Neuer Test prüft, dass ungültige/CRLF-behaftete Absenderadressen beim Speichern verworfen werden und dass `SmtpMailer::sanitizeAddress()` CR/LF zuverlässig entfernt. + +## 2026-07-27 – Fix: Produktions-Härtung der PHP-Fehlerausgabe (Issue #142) + +- Das Docker-Image (`php:8.3-apache`) übernahm die kompilierten PHP-Defaults, bei denen `display_errors` aktiviert ist – im offiziellen Image wird `php.ini-production` nicht automatisch aktiviert. In Kombination mit `PDO::ATTR_ERRMODE_EXCEPTION` (`src/Database.php`) und fehlendem globalem Error-Handler hätte jeder unerwartete Fehler (SQL-Fehler, DB-Verbindungsproblem, Typfehler) eine vollständige PHP-Fehlermeldung inkl. Stacktrace, absoluten Serverpfaden und ggf. SQL-Details direkt an den Client ausgegeben (CWE-209, Information Exposure Through an Error Message) – auch für nicht angemeldete Nutzer (`/login` ist vor `Auth::require()` erreichbar). +- **`php/error-handling.ini`** (neu): `display_errors = Off`, `display_startup_errors = Off`, `log_errors = On`, `error_reporting = E_ALL & ~E_DEPRECATED`. +- **`Dockerfile`:** Kopiert `php/error-handling.ini` zusätzlich zu `php/uploads.ini` nach `/usr/local/etc/php/conf.d/`. +- **`public/index.php`:** Registriert ganz zu Beginn (vor Autoload/Routing) `set_error_handler` (wandelt Warnings/Notices in `ErrorException` um), `set_exception_handler` und `register_shutdown_function` (fängt Fatal Errors, die nicht als Exception auftreten). Unerwartete Fehler werden per `error_log` serverseitig geloggt; der Client erhält nur eine generische HTTP-500-Seite ohne Details. +- **`tests/68_ProductionErrorHardeningTest.php`:** Neuer Test prüft die Ini-Direktiven, die Dockerfile-Einbindung, die Registrierung der Handler vor dem Autoload sowie dass die an den Client gesendete Fehlerseite keine Exception-Details (Message/Trace/File) enthält. + +## 2026-07-27 – Fix: Stored XSS über XSLT in hochgeladenen XML-Belegen (Issue #135) + +- `BelegController::serveFile()` liefert Belege mit dem in der DB hinterlegten `mime_type` und `Content-Disposition: inline` unverändert an den Browser aus. Da für Belege `application/xml`/`text/xml` als MIME-Typ erlaubt ist, konnte eine hochgeladene XML-Datei mit eingebettetem ``/`xsl:stylesheet` beim Öffnen über den „Datei öffnen“-Link (`target="_blank"`, kein `download`-Attribut) direkt im Browser (v.a. Firefox) per XSLT-Transformation zu ausführbarem `'; + $getBackup = $_GET; + $_GET['_error'] = ['x' => $payload]; + + try { + $controller = new \Steuerkram\Controllers\InvoiceController(); + ob_start(); + $controller->book(['id' => (string)$invoiceId]); + $html = ob_get_clean(); + } finally { + $_GET = $getBackup; + } + + $this->assert( + !str_contains($html, $payload), + 'Roher + + + + XML; + file_put_contents($absPath, $xmlPayload); + + $belegId = \Steuerkram\Models\Beleg::create([ + 'beleg_nr' => $belegNr, + 'filename' => 'rechnung.xml', + 'storage_path' => $storagePath, + 'mime_type' => 'text/xml', + 'file_hash' => hash('sha256', $xmlPayload), + 'beleg_datum' => date('Y-m-d'), + 'rechnungs_nr' => null, + 'betrag_brutto' => 100.00, + 'belegtyp' => 'Eingangsrechnung', + 'notes' => 'XSLT-XSS-Test', + ]); + + $controller = new \Steuerkram\Controllers\BelegController(); + + ob_start(); + $controller->serveFile(['id' => (string)$belegId]); + ob_get_clean(); + + $headers = headers_list(); + $disposition = ''; + foreach ($headers as $header) { + if (stripos($header, 'Content-Disposition:') === 0) { + $disposition = $header; + } + } + + $this->assertNotEmpty( + $disposition, + 'Es wurde ein Content-Disposition-Header gesetzt' + ); + $this->assert( + stripos($disposition, 'attachment') !== false, + "XML-Beleg wird als attachment statt inline ausgeliefert (erhalten: {$disposition})" + ); + + @unlink($absPath); + } +} diff --git a/tests/68_ProductionErrorHardeningTest.php b/tests/68_ProductionErrorHardeningTest.php new file mode 100644 index 0000000..b9b220b --- /dev/null +++ b/tests/68_ProductionErrorHardeningTest.php @@ -0,0 +1,78 @@ +assert(is_file($iniPath), 'php/error-handling.ini existiert'); + $ini = file_get_contents($iniPath) ?: ''; + $this->assert( + (bool) preg_match('/^\s*display_errors\s*=\s*Off\s*$/mi', $ini), + 'display_errors ist in error-handling.ini auf Off gesetzt' + ); + $this->assert( + (bool) preg_match('/^\s*log_errors\s*=\s*On\s*$/mi', $ini), + 'log_errors ist in error-handling.ini auf On gesetzt' + ); + + $dockerfile = file_get_contents($baseDir . '/Dockerfile') ?: ''; + $this->assert( + str_contains($dockerfile, 'php/error-handling.ini') && str_contains($dockerfile, '/usr/local/etc/php/conf.d/'), + 'Dockerfile kopiert error-handling.ini in den PHP conf.d-Ordner' + ); + + $index = file_get_contents($baseDir . '/public/index.php') ?: ''; + $this->assert( + str_contains($index, 'set_exception_handler'), + 'index.php registriert einen globalen Exception-Handler' + ); + $this->assert( + str_contains($index, 'set_error_handler'), + 'index.php registriert einen globalen Error-Handler' + ); + $this->assert( + str_contains($index, 'register_shutdown_function'), + 'index.php registriert einen Shutdown-Handler für Fatal Errors' + ); + + // Der Handler muss vor dem Autoload/Routing stehen, damit auch frühe Fehler abgefangen werden. + $handlerPos = strpos($index, 'set_exception_handler'); + $autoloadPos = strpos($index, "require BASE_DIR . '/vendor/autoload.php'"); + $this->assert( + $handlerPos !== false && $autoloadPos !== false && $handlerPos < $autoloadPos, + 'Exception-Handler wird vor dem Autoload registriert' + ); + + // Sicherstellen, dass im an den Client gesendeten HTML keine Exception-Details landen. + $exceptionHandlerBlock = substr($index, $handlerPos, 400); + foreach (['getMessage()', 'getTraceAsString()', 'getFile()', '$e->'] as $forbidden) { + $this->assert( + !str_contains($this->extractEchoBlock($index), $forbidden), + "Client-Ausgabe enthält keinen Verweis auf {$forbidden}" + ); + } + unset($exceptionHandlerBlock); + } + + private function extractEchoBlock(string $index): string + { + $start = strpos($index, "echo '"); + $end = strpos($index, "};", $start !== false ? $start : 0); + if ($start === false || $end === false) { + return ''; + } + return substr($index, $start, $end - $start); + } +} diff --git a/tests/69_MailFromEmailInjectionTest.php b/tests/69_MailFromEmailInjectionTest.php new file mode 100644 index 0000000..8fbc904 --- /dev/null +++ b/tests/69_MailFromEmailInjectionTest.php @@ -0,0 +1,72 @@ + $before['firma'] ?: 'Testfirma', + 'strasse' => $before['strasse'] ?: 'Teststr. 1', + 'plz' => $before['plz'] ?: '12345', + 'ort' => $before['ort'] ?: 'Testort', + ]); + + // Gültige Adresse wird unverändert übernommen + Setting::update(array_merge($base, [ + 'mail_from_email' => 'rechnung@firma.example', + ])); + $this->assertEquals('rechnung@firma.example', Setting::get()['mail_from_email'], 'Gültige Absenderadresse wird gespeichert'); + + // CRLF-Payload (SMTP-Kommando-Injection) wird beim Speichern verworfen, nicht nur bereinigt + Setting::update(array_merge($base, [ + 'mail_from_email' => "admin@firma.example>\r\nRCPT TO:\r\nDATA", + ])); + $stored = Setting::get()['mail_from_email']; + $this->assert( + !str_contains($stored, "\r") && !str_contains($stored, "\n"), + 'Gespeicherte Absenderadresse enthält kein CR/LF' + ); + $this->assertEquals('', $stored, 'Ungültige (CRLF-behaftete) Absenderadresse wird verworfen statt gespeichert'); + + // Andere ungültige Werte (kein E-Mail-Format) werden ebenfalls verworfen + Setting::update(array_merge($base, [ + 'mail_from_email' => 'keine-email-adresse', + ])); + $this->assertEquals('', Setting::get()['mail_from_email'], 'Nicht-E-Mail-Wert wird verworfen'); + + // Ursprünglichen Zustand wiederherstellen + Setting::update(array_merge($base, [ + 'mail_from_email' => $before['mail_from_email'] ?? '', + ])); + + // Defense-in-depth: SmtpMailer::sanitizeAddress() entfernt CRLF selbst, + // falls eine mit CRLF behaftete Adresse trotzdem in den Mailer gelangt. + $mailer = new SmtpMailer( + host: '127.0.0.1', + port: 1, + encryption: 'none', + user: '', + pass: '', + fromEmail: 'absender@example.com', + fromName: 'Absender', + ); + $ref = new \ReflectionMethod($mailer, 'sanitizeAddress'); + $ref->setAccessible(true); + $result = $ref->invoke($mailer, "admin@firma.example>\r\nRCPT TO:"); + $this->assert( + !str_contains($result, "\r") && !str_contains($result, "\n"), + 'sanitizeAddress() entfernt CR/LF aus der Absenderadresse' + ); + } +}