diff --git a/Changes.md b/Changes.md index f85a534..d5b10da 100644 --- a/Changes.md +++ b/Changes.md @@ -2,6 +2,14 @@ --- +## 2026-08-21 – SMTP-SSRF-Schutz: IP-Pinning gegen DNS-Rebinding, Admin-Pflicht für SMTP-Einstellungen (#193) + +- **`src/Services/SsrfGuard.php`:** neue `resolvePinnedHost(string $host): string[]` – wie `assertPublicHost()`, gibt aber zusätzlich die geprüfte(n) IP-Adresse(n) zurück, analog zu `resolvePinned()` für https-URLs. `assertPublicHost()` ruft sie jetzt intern auf. +- **`src/Services/SmtpMailer.php`:** `connect()` löste den SMTP-Host bei `SsrfGuard::assertPublicHost()` und beim späteren `stream_socket_client('tcp://'.$host …')` zweimal unabhängig auf – ein Angreifer mit kurzer DNS-TTL konnte zwischen Prüfung und Verbindungsaufbau auf eine private/interne Adresse umschwenken (DNS-Rebinding/TOCTOU). Neue private Methode `buildConnectTarget()` nutzt die von `resolvePinnedHost()` gepinnte IP für `stream_socket_client()`; der ursprüngliche Hostname bleibt für SNI/Zertifikatsprüfung über `stream_context`-Option `ssl.peer_name` erhalten. +- **`src/Controllers/SettingsController.php`:** `update()` und `mailTest()` verlangten bislang keine Admin-Rolle – jeder eingeloggte Benutzer konnte `mail_smtp_host`/`mail_smtp_port` setzen und per „SMTP testen" sofort einen Verbindungsversuch des Servers auslösen. Beide Methoden rufen jetzt `Auth::requireAdmin()`. `GET /settings` (Firmendaten anzeigen) bleibt für alle eingeloggten Benutzer erreichbar. +- **`tests/98_SmtpSsrfPinningTest.php`** (neu): `SsrfGuard::resolvePinnedHost()` sowie `SmtpMailer::buildConnectTarget()` (per Reflection) – Verbindungsziel enthält die geprüfte IP statt des Hostnamens, `peer_name` bleibt gepinnt, private Hosts werden weiterhin abgelehnt. +- **`tests/e2e/specs/settings-smtp-authorization.spec.js`** (neu): Nicht-Admin erhält 403 auf `POST /settings` und `POST /settings/mail-test`, Admin weiterhin nicht. + ## 2026-08-18 – Inline-Styles und Inline-Handler reduziert (#160) Erster Umsetzungsschritt der in Issue #160 beschriebenen schrittweisen Reduktion (975 → 858 `style="`-Vorkommen, 51 → 0 Inline-Handler in `src/Views`). Kein Big-Bang über alle Views, sondern die im Ticket priorisierten Schritte 1–4: diff --git a/src/Controllers/SettingsController.php b/src/Controllers/SettingsController.php index 03c137b..6906c51 100644 --- a/src/Controllers/SettingsController.php +++ b/src/Controllers/SettingsController.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace Steuerkram\Controllers; +use Steuerkram\Auth; use Steuerkram\Config; use Steuerkram\Models\BankSource; use Steuerkram\Models\Setting; @@ -33,6 +34,12 @@ class SettingsController extends BaseController public function update(array $params): void { + // Firmendaten und SMTP-Zugangsdaten (inkl. mail_smtp_host) landen im + // selben Formular – ohne Admin-Pflicht könnte jeder eingeloggte + // Benutzer den SMTP-Host setzen und per mailTest() sofort einen + // Verbindungsversuch dorthin auslösen (Issue #193). + Auth::requireAdmin(); + $data = $_POST; $errors = []; @@ -163,6 +170,8 @@ class SettingsController extends BaseController public function mailTest(array $params): void { + Auth::requireAdmin(); + header('Content-Type: application/json'); $settings = Setting::get(); $to = $settings['email'] ?? ''; diff --git a/src/Services/SmtpMailer.php b/src/Services/SmtpMailer.php index 43a9f44..4fa4671 100644 --- a/src/Services/SmtpMailer.php +++ b/src/Services/SmtpMailer.php @@ -56,19 +56,7 @@ class SmtpMailer private function connect(): void { - SsrfGuard::assertPublicHost($this->host); - - $address = $this->encryption === 'ssl' - ? 'ssl://' . $this->host - : 'tcp://' . $this->host; - - $ctx = stream_context_create([ - 'ssl' => [ - 'verify_peer' => true, - 'verify_peer_name' => true, - 'allow_self_signed' => false, - ], - ]); + [$address, $ctx] = $this->buildConnectTarget(); $sock = stream_socket_client( $address . ':' . $this->port, @@ -97,6 +85,38 @@ class SmtpMailer } } + /** + * Löst den SMTP-Host per SsrfGuard::resolvePinnedHost() auf und pinnt die + * dabei geprüfte(n) IP-Adresse(n) für den eigentlichen Verbindungsaufbau: + * stream_socket_client() verbindet direkt zur IP statt den Hostnamen + * erneut über den System-Resolver aufzulösen. Ohne dieses Pinning könnte + * ein Angreifer mit kurzer DNS-TTL zwischen Prüfung und Connect auf eine + * private/interne Adresse umschwenken (DNS-Rebinding/TOCTOU, Issue #193). + * Für SNI und Zertifikatsprüfung bleibt der ursprüngliche Hostname über + * den Stream-Context ('ssl' => 'peer_name') erhalten. + * + * @return array{0:string,1:resource} [Verbindungsziel "scheme://ip", Stream-Context] + */ + private function buildConnectTarget(): array + { + $ips = SsrfGuard::resolvePinnedHost($this->host); + $ip = str_contains($ips[0], ':') ? '[' . $ips[0] . ']' : $ips[0]; + + $scheme = $this->encryption === 'ssl' ? 'ssl' : 'tcp'; + $address = $scheme . '://' . $ip; + + $ctx = stream_context_create([ + 'ssl' => [ + 'verify_peer' => true, + 'verify_peer_name' => true, + 'allow_self_signed' => false, + 'peer_name' => $this->host, + ], + ]); + + return [$address, $ctx]; + } + private function ehlo(): void { $this->cmd('EHLO ' . (gethostname() ?: 'localhost')); diff --git a/src/Services/SsrfGuard.php b/src/Services/SsrfGuard.php index ce330a3..dec6f0d 100644 --- a/src/Services/SsrfGuard.php +++ b/src/Services/SsrfGuard.php @@ -92,6 +92,22 @@ class SsrfGuard * @throws \RuntimeException wenn der Host kein zulässiges Ziel ist */ public static function assertPublicHost(string $host): void + { + self::resolvePinnedHost($host); + } + + /** + * Wie assertPublicHost(), gibt aber zusätzlich die dabei geprüfte(n) + * IP-Adresse(n) zurück, damit der Aufrufer sie beim tatsächlichen + * Verbindungsaufbau pinnen kann – analog zu resolvePinned(), aber ohne + * URL-Schema (z.B. für den SMTP-Host aus stream_socket_client(), siehe + * Issue #193: ohne Pinning löst der spätere Connect den Hostnamen erneut + * auf, was DNS-Rebinding/TOCTOU zwischen Prüfung und Verbindung erlaubt). + * + * @return string[] geprüfte IP-Adresse(n), mindestens ein Eintrag + * @throws \RuntimeException wenn der Host kein zulässiges Ziel ist + */ + public static function resolvePinnedHost(string $host): array { $host = trim($host, " \t\n\r\0\x0B[]"); if ($host === '') { @@ -100,7 +116,7 @@ class SsrfGuard if (filter_var($host, FILTER_VALIDATE_IP)) { self::assertPublicIp($host); - return; + return [$host]; } if (strtolower($host) === 'localhost') { @@ -114,6 +130,8 @@ class SsrfGuard foreach ($ips as $ip) { self::assertPublicIp($ip); } + + return $ips; } private static function assertPublicIp(string $ip): void diff --git a/tests/98_SmtpSsrfPinningTest.php b/tests/98_SmtpSsrfPinningTest.php new file mode 100644 index 0000000..adfcf54 --- /dev/null +++ b/tests/98_SmtpSsrfPinningTest.php @@ -0,0 +1,93 @@ +assertNotEmpty($ips, 'resolvePinnedHost() liefert IP-Adressen für einen gültigen Hostnamen'); + foreach ($ips as $ip) { + $this->assert( + filter_var($ip, FILTER_VALIDATE_IP) !== false, + 'Jeder Eintrag ist eine gültige IP-Adresse (Eintrag: ' . $ip . ')' + ); + } + + // IP-Literal als Host: kein DNS-Lookup nötig, aber weiterhin gegen + // private/interne Bereiche geprüft. + $this->assertEquals(['8.8.8.8'], SsrfGuard::resolvePinnedHost('8.8.8.8'), 'IP-Literal wird unverändert zurückgegeben'); + $this->assertRejected('127.0.0.1'); + $this->assertRejected('localhost'); + $this->assertRejected('169.254.169.254'); + + // SmtpMailer::buildConnectTarget(): verbindet zur gepinnten IP, nicht + // erneut zum Hostnamen – und hält den Hostnamen für SNI/Zertifikats- + // prüfung über den Stream-Context ('peer_name') fest. + $mailer = new SmtpMailer('example.com', 465, 'ssl', 'user', 'pass', 'from@example.com', 'Absender'); + [$address, $ctx] = $this->buildConnectTarget($mailer); + + $this->assert(str_starts_with($address, 'ssl://'), 'Verbindungsziel nutzt das SSL-Schema für encryption=ssl'); + $host = substr($address, strlen('ssl://')); + $this->assert( + filter_var($host, FILTER_VALIDATE_IP) !== false, + 'Verbindungsziel enthält eine IP-Adresse statt des Hostnamens (Ziel: ' . $address . ')' + ); + $this->assert( + in_array($host, SsrfGuard::resolvePinnedHost('example.com'), true), + 'Ziel-IP stammt aus der SsrfGuard-Auflösung des Hostnamens (Ziel: ' . $host . ')' + ); + + $sslOptions = stream_context_get_options($ctx)['ssl'] ?? []; + $this->assertEquals('example.com', $sslOptions['peer_name'] ?? null, 'Stream-Context pinnt peer_name auf den ursprünglichen Hostnamen (für SNI/Zertifikatsprüfung)'); + $this->assert(($sslOptions['verify_peer'] ?? false) === true, 'Zertifikatsprüfung bleibt trotz IP-Verbindung aktiv (verify_peer)'); + $this->assert(($sslOptions['verify_peer_name'] ?? false) === true, 'Zertifikatsprüfung bleibt trotz IP-Verbindung aktiv (verify_peer_name)'); + + // tcp:// für unverschlüsselt/STARTTLS statt ssl:// + $mailerTls = new SmtpMailer('example.com', 587, 'tls', 'user', 'pass', 'from@example.com', 'Absender'); + [$addressTls] = $this->buildConnectTarget($mailerTls); + $this->assert(str_starts_with($addressTls, 'tcp://'), 'Verbindungsziel nutzt das TCP-Schema für encryption=tls (STARTTLS folgt später)'); + + // Private/interne SMTP-Hosts werden weiterhin abgelehnt, bevor überhaupt + // ein Verbindungsversuch unternommen wird. + $mailerPrivate = new SmtpMailer('127.0.0.1', 25, 'none', 'user', 'pass', 'from@example.com', 'Absender'); + try { + $this->buildConnectTarget($mailerPrivate); + $this->assert(false, 'buildConnectTarget() lehnt private SMTP-Hosts ab'); + } catch (\RuntimeException $e) { + $this->assert(true, 'buildConnectTarget() lehnt private SMTP-Hosts ab'); + } + } + + private function assertRejected(string $host): void + { + try { + SsrfGuard::resolvePinnedHost($host); + $this->assert(false, 'Privater/interner Host (' . $host . ') wird abgelehnt'); + } catch (\RuntimeException $e) { + $this->assert(true, 'Privater/interner Host (' . $host . ') wird abgelehnt'); + } + } + + private function buildConnectTarget(SmtpMailer $mailer): array + { + $method = new \ReflectionMethod($mailer, 'buildConnectTarget'); + $method->setAccessible(true); + return $method->invoke($mailer); + } +} diff --git a/tests/e2e/specs/settings-smtp-authorization.spec.js b/tests/e2e/specs/settings-smtp-authorization.spec.js new file mode 100644 index 0000000..00ad06e --- /dev/null +++ b/tests/e2e/specs/settings-smtp-authorization.spec.js @@ -0,0 +1,80 @@ +// @ts-check +// Fix #193: SettingsController::update()/mailTest() verlangten kein +// Auth::requireAdmin() – jeder eingeloggte Benutzer konnte mail_smtp_host +// setzen und per "SMTP testen" sofort einen Verbindungsversuch des Servers +// zu einem beliebigen Ziel auslösen (Ausgangspunkt für DNS-Rebinding/SSRF). +// Dieser Test prüft die Durchsetzung auf HTTP-Ebene, die die PHP-Suite nicht +// erreicht (Router-Guard, Auth::requireAdmin()). + +const { test, expect } = require('@playwright/test') +const { uniqueTag } = require('../helpers/app') + +test.describe('Autorisierung: SMTP-Einstellungen nur für Admins (Fix #193)', () => { + test('Nicht-Admin wird von POST /settings und /settings/mail-test ausgesperrt, Admin nicht', async ({ page, browser }) => { + const username = uniqueTag('noadmin').toLowerCase().replace(/[^a-z0-9-]/g, '').slice(0, 64) + const password = 'TempPass123!' + + await page.goto('/useradmin') + await page.fill('#field-username', username) + await page.fill('#field-password', password) + await page.click('#submit-btn') + await page.waitForURL('/useradmin') + + const row = page.locator('tr', { hasText: username }) + await expect(row).toBeVisible() + + try { + const context = await browser.newContext({ storageState: { cookies: [], origins: [] } }) + const userPage = await context.newPage() + await userPage.goto('/login') + await userPage.fill('#username', username) + await userPage.fill('#password', password) + await userPage.click('form[action="/login"] button[type="submit"]') + await userPage.waitForURL(url => !url.pathname.startsWith('/login')) + + // GET /settings bleibt für alle eingeloggten Benutzer erreichbar + // (Firmendaten sind kein Admin-Feature) – nur die SMTP-mutierenden + // Endpunkte werden geschützt. + await userPage.goto('/settings') + const csrfToken = await userPage.evaluate(() => document.querySelector('meta[name="csrf-token"]')?.content || '') + expect(csrfToken).not.toBe('') + + const mailTestResponse = await userPage.request.post('/settings/mail-test', { + headers: { 'X-CSRF-Token': csrfToken }, + form: {}, + }) + expect(mailTestResponse.status()).toBe(403) + await expect(mailTestResponse.text()).resolves.toContain('Kein Zugriff') + + const updateResponse = await userPage.request.post('/settings', { + headers: { 'X-CSRF-Token': csrfToken }, + form: { firma: 'Sollte nicht gespeichert werden', strasse: 'X', plz: '12345', ort: 'X', mail_smtp_host: '127.0.0.1' }, + }) + expect(updateResponse.status()).toBe(403) + await expect(updateResponse.text()).resolves.toContain('Kein Zugriff') + + await context.close() + } finally { + const delForm = row.locator('form[action$="/delete"]') + if (await delForm.count() > 0) { + await delForm.locator('button[type="submit"]').click() + await page.waitForLoadState('networkidle') + } + } + }) + + test('Admin darf /settings/mail-test weiterhin aufrufen (kein 403)', async ({ page }) => { + await page.goto('/settings') + const csrfToken = await page.evaluate(() => document.querySelector('meta[name="csrf-token"]')?.content || '') + expect(csrfToken).not.toBe('') + + const response = await page.request.post('/settings/mail-test', { + headers: { 'X-CSRF-Token': csrfToken }, + form: {}, + }) + // Egal ob die SMTP-Verbindung selbst gelingt oder scheitert (abhängig von + // den hinterlegten Test-Einstellungen) – der Admin-Zugriff selbst darf + // nicht mit 403 verweigert werden. + expect(response.status()).not.toBe(403) + }) +})