diff --git a/docs/guides/media.md b/docs/guides/media.md index 7374bb4..3964c32 100644 --- a/docs/guides/media.md +++ b/docs/guides/media.md @@ -26,7 +26,9 @@ nie auf „indirekt“ – ihr Bild- bzw. Dateiergebnis wird nur bei direktem Au ## Hochladen - `lit-media-upload-from-url` (direkt): lädt eine öffentlich erreichbare `http(s)`-Datei; interne oder - lokale Adressen werden abgelehnt. + lokale Adressen werden abgelehnt (auch als Ziel einer Weiterleitung). Scheitert der Abruf selbst, lautet die + Meldung immer „Die Datei konnte nicht geladen werden.“ – dann eine andere öffentliche URL versuchen oder die + Datei per `lit/media-upload` hochladen. - `lit/media-upload` (indirekt): lädt eine Datei als Base64, höchstens 10 MB – größere Dateien per URL. - Beide: optional `title`, `alt`, `post_id` (Eintrag, dem das Medium zugeordnet wird – braucht Bearbeitungsrecht); erlaubte Dateitypen jpg, png, gif, webp, avif, pdf, mp4, mp3; Größenlimit siehe die diff --git a/src/Integrations/Media/MediaImporter.php b/src/Integrations/Media/MediaImporter.php index 3cab21e..bff2f34 100644 --- a/src/Integrations/Media/MediaImporter.php +++ b/src/Integrations/Media/MediaImporter.php @@ -4,19 +4,23 @@ declare(strict_types=1); namespace LIT\MCP\Integrations\Media; +use Closure; use LIT\MCP\Abilities\Errors; use LIT\MCP\Abilities\WriteFailed; use LIT\MCP\Limits; use LIT\MCP\Log; use Throwable; use WP_Error; +use WpOrg\Requests\Exception as RequestsException; /** * Imports files into the media library (spec §6.3, §10). * - * - URL: only http(s), checked with wp_http_validate_url() before any request and fetched - * with wp_safe_remote_get() (rejects internal addresses, also after redirects), streamed - * into a temp file and cut off one byte above the upload limit. + * - URL: only http(s), checked with wp_http_validate_url() and the plugin's own range check + * (PublicAddress, independent of the core version) before any request and on every redirect, + * fetched with wp_safe_remote_get() (core re-validates redirects too), streamed into a temp file + * and cut off one byte above the upload limit. Network failures reach the client only as one + * generic message (no refused/timeout/status oracle, security audit M3); details go to the log. * - Base64: decoded strictly; the size is checked against Limits::inlineBytes() (upload limit, * at most 10 MB – the whole file sits in memory) before anything is decoded or written. * - Type: whitelist (filter lit_mcp_upload_mimes) intersected with what WordPress allows the @@ -44,16 +48,26 @@ class MediaImporter private const TIMEOUT = 30; private const ALT_KEY = '_wp_attachment_image_alt'; + private const REDIRECT_HOOK = 'requests-requests.before_redirect'; - public function __construct(private Limits $limits) + /** @var Closure(string): list Host name => IPv4 addresses. */ + private Closure $resolve; + + /** @param (Closure(string): list)|null $resolve DNS lookup; null = the real one. */ + public function __construct(private Limits $limits, ?Closure $resolve = null) { + $this->resolve = $resolve ?? PublicAddress::dnsResolver(); } /** @param array{title?: string, alt?: string, post_id?: int} $meta */ public function fromUrl(string $url, ?string $filename, array $meta): int|WP_Error { $scheme = strtolower((string) wp_parse_url($url, PHP_URL_SCHEME)); - if (! in_array($scheme, ['http', 'https'], true) || ! wp_http_validate_url($url)) { + if ( + ! in_array($scheme, ['http', 'https'], true) + || ! wp_http_validate_url($url) + || ! $this->isPublicUrl($url) + ) { return Errors::invalidInput( 'Die URL ist nicht erlaubt.', ['url' => 'nur öffentlich erreichbare http(s)-Adressen'], @@ -126,7 +140,15 @@ class MediaImporter curl_setopt($handle, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4); } }; + // Every redirect target gets the plugin's own range check too; Requests turns the + // exception into a failed request (WP_Http returns a WP_Error). + $redirect = function ($location): void { + if (! $this->isPublicUrl((string) $location)) { + throw new RequestsException('Redirect to a non-public address refused', 'lit_mcp_redirect'); + } + }; add_action('http_api_curl', $ipv4); + add_action(self::REDIRECT_HOOK, $redirect); try { $response = wp_safe_remote_get($url, [ 'timeout' => self::TIMEOUT, @@ -136,15 +158,16 @@ class MediaImporter ]); } finally { remove_action('http_api_curl', $ipv4); + remove_action(self::REDIRECT_HOOK, $redirect); } if ($response instanceof WP_Error) { - return $this->downloadFailed($response->get_error_message()); + return $this->downloadFailed($url, $response->get_error_message()); } $status = (int) wp_remote_retrieve_response_code($response); if ($status !== 200) { - return $this->downloadFailed(sprintf('HTTP-Status %d', $status)); + return $this->downloadFailed($url, sprintf('HTTP status %d', $status)); } $length = (string) wp_remote_retrieve_header($response, 'content-length'); @@ -336,10 +359,19 @@ class MediaImporter } } - private function downloadFailed(string $reason): WP_Error + /** Host (name or IP literal) of $url resolves to public addresses only (PublicAddress). */ + private function isPublicUrl(string $url): bool { + return PublicAddress::hostIsPublic((string) wp_parse_url($url, PHP_URL_HOST), $this->resolve); + } + + /** One message for every network failure (security audit M3); the reason is logged only. */ + private function downloadFailed(string $url, string $reason): WP_Error + { + Log::failure('media import from ' . $url, $reason); + return Errors::invalidInput( - 'Download fehlgeschlagen: ' . $reason, + 'Die Datei konnte nicht geladen werden.', ['url' => 'nicht abrufbar'], 'URL prüfen; nur öffentlich erreichbare Dateien können importiert werden.' ); diff --git a/src/Integrations/Media/PublicAddress.php b/src/Integrations/Media/PublicAddress.php new file mode 100644 index 0000000..9f119aa --- /dev/null +++ b/src/Integrations/Media/PublicAddress.php @@ -0,0 +1,138 @@ + $resolve Host name => its addresses. + */ + public static function hostIsPublic(string $host, Closure $resolve): bool + { + $host = trim($host, '[]'); + if ($host === '') { + return false; + } + + // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- see isPublicIp(). + if (@inet_pton($host) !== false) { + return self::isPublicIp($host); + } + + $addresses = $resolve($host); + if ($addresses === []) { + return false; + } + + foreach ($addresses as $address) { + if (! self::isPublicIp($address)) { + return false; + } + } + + return true; + } + + /** @return Closure(string): list IPv4 lookup like the import request makes (cURL is IPv4-only there). */ + public static function dnsResolver(): Closure + { + return static function (string $host): array { + $addresses = gethostbynamel($host); + + return is_array($addresses) ? array_values($addresses) : []; + }; + } + + private static function isPublicV4(string $packed): bool + { + foreach (self::V4_BLOCKED as [$network, $bits]) { + if (self::inRange($packed, (string) inet_pton($network), $bits)) { + return false; + } + } + + return true; + } + + private static function isPublicV6(string $packed): bool + { + foreach (self::V6_EMBEDDING as [$prefix, $bits, $offset]) { + if (self::inRange($packed, (string) inet_pton($prefix), $bits)) { + return self::isPublicV4(substr($packed, $offset, 4)); + } + } + + foreach (self::V6_BLOCKED as [$network, $bits]) { + if (self::inRange($packed, (string) inet_pton($network), $bits)) { + return false; + } + } + + return true; + } + + /** Whether the first $bits bits of $address and $network (packed, same length) are equal. */ + private static function inRange(string $address, string $network, int $bits): bool + { + if (strlen($address) !== strlen($network)) { + return false; + } + + $bytes = intdiv($bits, 8); + if (substr($address, 0, $bytes) !== substr($network, 0, $bytes)) { + return false; + } + + $rest = $bits % 8; + if ($rest === 0) { + return true; + } + + $mask = (0xFF << (8 - $rest)) & 0xFF; + + return (ord($address[$bytes]) & $mask) === (ord($network[$bytes]) & $mask); + } +} diff --git a/tests/Integration/media-admin-smoke.sh b/tests/Integration/media-admin-smoke.sh index e39c710..e33863b 100755 --- a/tests/Integration/media-admin-smoke.sh +++ b/tests/Integration/media-admin-smoke.sh @@ -73,6 +73,10 @@ pass "upload, search, preview, update (single ok, mixed batch refused), download # 3. Import protection SSRF=$(tool lit-media-upload-from-url '{"url":"http://127.0.0.1/wp-login.php"}') [ "$(error_code "$SSRF")" = "validation_failed" ] || fail "internal URL accepted: $SSRF" +for internal in "http://169.254.169.254/latest/meta-data/" "http://100.64.0.1/x.png" "http://[::ffff:127.0.0.1]/x.png"; do + BLOCKED=$(tool lit-media-upload-from-url "$(jq -cn --arg u "$internal" '{url: $u}')") + [ "$(error_code "$BLOCKED")" = "validation_failed" ] || fail "internal URL $internal accepted: $BLOCKED" +done FAKE=$(ability lit/media-upload "{\"filename\":\"evil.png\",\"data\":\"$(printf 'type; + } + } +} diff --git a/tests/Unit/Integrations/Media/MediaImporterTest.php b/tests/Unit/Integrations/Media/MediaImporterTest.php index af8d17f..6ba1cf4 100644 --- a/tests/Unit/Integrations/Media/MediaImporterTest.php +++ b/tests/Unit/Integrations/Media/MediaImporterTest.php @@ -81,9 +81,10 @@ final class MediaImporterTest extends TestCase parent::tearDown(); } - private function importer(): MediaImporter + /** @param list $addresses What every host name resolves to (no DNS in unit tests). */ + private function importer(array $addresses = ['93.184.216.34']): MediaImporter { - return new MediaImporter(new Limits()); + return new MediaImporter(new Limits(), static fn (string $host): array => $addresses); } private function assertTempFilesRemoved(): void @@ -332,8 +333,11 @@ final class MediaImporterTest extends TestCase $result = $this->importer()->fromUrl('https://example.com/fehlt.jpg', null, []); + // Security M3: no refused/timeout/404 oracle for the client; the detail goes to the log only. self::assertToolError($result, 'validation_failed'); - self::assertStringContainsString('Not Found', $result->get_error_message()); + self::assertStringContainsString('Die Datei konnte nicht geladen werden.', $result->get_error_message()); + self::assertStringNotContainsString('Not Found', $result->get_error_message()); + self::assertStringContainsString('Not Found', (string) file_get_contents($this->errorLog)); $this->assertTempFilesRemoved(); } @@ -345,7 +349,9 @@ final class MediaImporterTest extends TestCase $result = $this->importer()->fromUrl('https://example.com/fehlt.jpg', null, []); self::assertToolError($result, 'validation_failed'); - self::assertStringContainsString('404', $result->get_error_message()); + self::assertStringContainsString('Die Datei konnte nicht geladen werden.', $result->get_error_message()); + self::assertStringNotContainsString('404', $result->get_error_message()); + self::assertStringContainsString('404', (string) file_get_contents($this->errorLog)); $this->assertTempFilesRemoved(); } @@ -464,4 +470,40 @@ final class MediaImporterTest extends TestCase self::assertToolError($result, 'write_failed'); } + + /** Security M3: the plugin's own range check, whatever wp_http_validate_url() of the core version says. */ + public function testPrivateTargetsAreRejectedEvenWhenCoreLetsThemPass(): void + { + Functions\when('wp_http_validate_url')->returnArg(); + Functions\expect('wp_safe_remote_get')->never(); + + foreach (['http://169.254.169.254/latest/meta-data/', 'http://100.64.0.1/x.png', 'http://[fd00::1]/x.png'] as $url) { + self::assertToolError($this->importer()->fromUrl($url, null, []), 'validation_failed', $url); + } + + $internal = $this->importer(['10.0.0.5'])->fromUrl('https://intern.example.com/x.png', null, []); + self::assertToolError($internal, 'validation_failed'); + $unresolvable = $this->importer([])->fromUrl('https://nirgends.example.com/x.png', null, []); + self::assertToolError($unresolvable, 'validation_failed'); + } + + /** Redirect targets get the same check (Requests' before_redirect hook, only during the import). */ + public function testRedirectsToPrivateTargetsAreStopped(): void + { + $this->respond('PNGDATA', 200); + $listener = null; + Actions\expectAdded('requests-requests.before_redirect')->once() + ->whenHappen(static function (callable $callback) use (&$listener): void { + $listener = $callback; + }); + Functions\when('wp_check_filetype_and_ext')->justReturn(['ext' => false, 'type' => false, 'proper_filename' => false]); + + $this->importer()->fromUrl('https://example.com/logo.png', null, []); + + self::assertIsCallable($listener); + self::assertFalse(has_action('requests-requests.before_redirect')); + $listener('https://cdn.example.com/logo.png'); // public: no exception + $this->expectException(\WpOrg\Requests\Exception::class); + $listener('http://169.254.169.254/latest/meta-data/'); + } } diff --git a/tests/Unit/Integrations/Media/PublicAddressTest.php b/tests/Unit/Integrations/Media/PublicAddressTest.php new file mode 100644 index 0000000..8cfefc5 --- /dev/null +++ b/tests/Unit/Integrations/Media/PublicAddressTest.php @@ -0,0 +1,63 @@ + */ + public static function blocked(): array + { + $ips = [ + '0.0.0.0', '10.1.2.3', '100.64.0.1', '100.127.255.254', '127.0.0.1', '169.254.169.254', + '172.16.0.1', '172.31.255.255', '192.0.0.8', '192.0.2.1', '192.168.1.1', '198.18.0.1', + '198.51.100.7', '203.0.113.9', '224.0.0.1', '240.0.0.1', '255.255.255.255', + '::', '::1', 'fc00::1', 'fd12:3456::1', 'fe80::1', 'febf::1', 'ff02::1', '2001:db8::1', + '::ffff:127.0.0.1', '::ffff:10.0.0.1', '::ffff:169.254.169.254', '::ffff:a9fe:a9fe', + '64:ff9b::a9fe:a9fe', '::127.0.0.1', + ]; + + return array_combine($ips, array_map(static fn (string $ip): array => [$ip], $ips)); + } + + /** @dataProvider blocked */ + public function testPrivateAndReservedAddressesAreNotPublic(string $ip): void + { + self::assertFalse(PublicAddress::isPublicIp($ip)); + } + + public function testPublicAddressesPass(): void + { + foreach (['93.184.216.34', '100.63.255.255', '100.128.0.1', '172.32.0.1', '8.8.8.8', '2606:4700::1111', '::ffff:8.8.8.8'] as $ip) { + self::assertTrue(PublicAddress::isPublicIp($ip), $ip); + } + + self::assertFalse(PublicAddress::isPublicIp('not-an-ip')); + } + + public function testHostsAreResolvedAndEveryAddressMustBePublic(): void + { + $resolve = static fn (string $host): array => [ + 'cdn.example.com' => ['93.184.216.34'], + 'mixed.example.com' => ['93.184.216.34', '169.254.169.254'], + 'metadata.example.com' => ['169.254.169.254'], + ][$host] ?? []; + + self::assertTrue(PublicAddress::hostIsPublic('cdn.example.com', $resolve)); + self::assertFalse(PublicAddress::hostIsPublic('mixed.example.com', $resolve)); + self::assertFalse(PublicAddress::hostIsPublic('metadata.example.com', $resolve)); + self::assertFalse(PublicAddress::hostIsPublic('unresolvable.example.com', $resolve)); + self::assertFalse(PublicAddress::hostIsPublic('169.254.169.254', $resolve), 'IP literal, no lookup'); + self::assertFalse(PublicAddress::hostIsPublic('[fe80::1]', $resolve), 'IPv6 literal in brackets'); + self::assertTrue(PublicAddress::hostIsPublic('8.8.8.8', $resolve)); + self::assertFalse(PublicAddress::hostIsPublic('', $resolve)); + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index db243e4..6179425 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -15,3 +15,4 @@ require_once __DIR__ . '/Stubs/WpPost.php'; require_once __DIR__ . '/Stubs/WpTerm.php'; require_once __DIR__ . '/Stubs/WpApplicationPasswords.php'; require_once __DIR__ . '/Stubs/WpListTable.php'; +require_once __DIR__ . '/Stubs/RequestsException.php';