From 20e02164a51ad59eb2174d96fb6a88d92a724659 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Mon, 19 Oct 2026 23:55:09 +0200 Subject: [PATCH] =?UTF-8?q?fix(audit=20L8,=20DNS-Rebinding):=20RemoteFileF?= =?UTF-8?q?etcher=20nur=20mit=20cURL=20und=20mit=20CURLOPT=5FRESOLVE=20auf?= =?UTF-8?q?=20die=20gepr=C3=BCfte=20Adresse=20(Host=20und=20Weiterleitungs?= =?UTF-8?q?ziele)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Sonnet 5.5 --- src/Integrations/Media/RemoteFileFetcher.php | 58 +++++++++-- .../Media/RemoteFileFetcherTest.php | 98 +++++++++++++++++++ 2 files changed, 149 insertions(+), 7 deletions(-) diff --git a/src/Integrations/Media/RemoteFileFetcher.php b/src/Integrations/Media/RemoteFileFetcher.php index 6a0f10b..b4c7810 100644 --- a/src/Integrations/Media/RemoteFileFetcher.php +++ b/src/Integrations/Media/RemoteFileFetcher.php @@ -16,8 +16,11 @@ defined('ABSPATH') || exit; * the font upload. 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() over IPv4 only (core re-validates redirects too), streamed into the temp file and cut off - * one byte above $maxBytes. A failure reaches the caller only as null (no refused/timeout/status oracle, security - * audit M3); the reason goes to the log. Not final so abilities can be tested with a fake. + * one byte above $maxBytes. cURL only: without it the request is refused (the IPv4 rule and the address pinning + * are cURL options), and every host of the chain is pinned to the address that was checked (CURLOPT_RESOLVE, + * so a second DNS answer cannot redirect the connection). A failure reaches the caller only as null (no + * refused/timeout/status oracle, security audit M3); the reason goes to the log. Not final so abilities can be + * tested with a fake. */ class RemoteFileFetcher { @@ -54,22 +57,50 @@ class RemoteFileFetcher return $this->failed($context, $url, 'URL refused'); } + // Audit L8: the IPv4-only rule and the pinning below are cURL options; another transport (fsockopen) would + // ignore them and could reach an AAAA address nobody checked. + if (! $this->curlAvailable()) { + return $this->failed($context, $url, 'no cURL transport'); + } + + // Audit DNS rebinding: allows() resolved the host once and cURL would resolve it again. Every host of this + // request chain (the URL, each redirect target) is resolved here once, checked and handed to cURL as + // CURLOPT_RESOLVE, so the connection goes to the address that was checked. + $pins = []; + $pin = function (string $target) use (&$pins): void { + $host = (string) wp_parse_url($target, PHP_URL_HOST); + $port = (int) wp_parse_url($target, PHP_URL_PORT) + ?: (strtolower((string) wp_parse_url($target, PHP_URL_SCHEME)) === 'https' ? 443 : 80); + $key = $host . ':' . $port; + if (isset($pins[$key]) || filter_var(trim($host, '[]'), FILTER_VALIDATE_IP) !== false) { + return; + } + $addresses = ($this->resolve)($host); + if ($addresses === [] || array_filter($addresses, [PublicAddress::class, 'isPublicIp']) !== $addresses) { + throw new RequestsException('Host resolved to a non-public address', 'lit_mcp_rebinding'); + } + $pins[$key] = $key . ':' . $addresses[0]; + }; // IPv4 only, for this request only: wp_http_validate_url() checks the IPv4 address // (gethostbyname), so an AAAA record must not lead cURL to an unchecked IPv6 target. - $ipv4 = static function ($handle): void { + $ipv4 = function ($handle, $args = null, $target = null) use (&$pins, $pin, $url): void { if (defined('CURLOPT_IPRESOLVE') && defined('CURL_IPRESOLVE_V4')) { - // phpcs:ignore WordPress.WP.AlternativeFunctions.curl_curl_setopt -- http_api_curl hands us the handle. - curl_setopt($handle, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4); + $this->curlSetopt($handle, CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4); + } + $pin(is_string($target) ? $target : $url); + if ($pins !== [] && defined('CURLOPT_RESOLVE')) { + $this->curlSetopt($handle, CURLOPT_RESOLVE, array_values($pins)); } }; // 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 { + $redirect = function ($location) use ($pin): void { if (! $this->isPublicUrl((string) $location)) { throw new RequestsException('Redirect to a non-public address refused', 'lit_mcp_redirect'); } + $pin((string) $location); }; - add_action('http_api_curl', $ipv4); + add_action('http_api_curl', $ipv4, 10, 3); add_action(self::REDIRECT_HOOK, $redirect); try { $response = wp_safe_remote_get($url, [ @@ -95,6 +126,19 @@ class RemoteFileFetcher return $response; } + /** Whether the cURL extension is there; a seam for tests (function_exists() cannot be undone). */ + protected function curlAvailable(): bool + { + return function_exists('curl_init'); + } + + /** @param \CurlHandle $handle */ + protected function curlSetopt($handle, int $option, mixed $value): void + { + // phpcs:ignore WordPress.WP.AlternativeFunctions.curl_curl_setopt -- http_api_curl hands us the handle. + curl_setopt($handle, $option, $value); + } + /** Host (name or IP literal) of $url resolves to public addresses only (PublicAddress). */ private function isPublicUrl(string $url): bool { diff --git a/tests/Unit/Integrations/Media/RemoteFileFetcherTest.php b/tests/Unit/Integrations/Media/RemoteFileFetcherTest.php index 74becf6..df1ea1e 100644 --- a/tests/Unit/Integrations/Media/RemoteFileFetcherTest.php +++ b/tests/Unit/Integrations/Media/RemoteFileFetcherTest.php @@ -139,4 +139,102 @@ final class RemoteFileFetcherTest extends TestCase $this->expectException(\WpOrg\Requests\Exception::class); $listener('http://127.0.0.1/inter.woff2'); } + + /** + * A fetcher whose cURL options are recorded; $dns is called per lookup (host => addresses). + * + * @param \Closure(string): list $dns + * @return object{options: list}&RemoteFileFetcher + */ + private function recordingFetcher(\Closure $dns, bool $curl = true): RemoteFileFetcher + { + return new class ($dns, $curl) extends RemoteFileFetcher { + /** @var list */ + public array $options = []; + + public function __construct(\Closure $dns, private bool $curl) + { + parent::__construct($dns); + } + + protected function curlAvailable(): bool + { + return $this->curl; + } + + protected function curlSetopt($handle, int $option, mixed $value): void + { + $this->options[] = [$option, $value]; + } + }; + } + + /** Audit L8: no cURL, no request – the IPv4 rule and the pinning are cURL options. */ + public function testWithoutCurlNothingIsRequested(): void + { + Functions\expect('wp_safe_remote_get')->never(); + $fetcher = $this->recordingFetcher(static fn (): array => ['93.184.216.34'], false); + + self::assertNull($fetcher->fetch('https://fonts.example.com/inter.woff2', $this->tmp, 2048, 'font upload')); + self::assertStringContainsString('no cURL transport', (string) file_get_contents($this->errorLog)); + } + + /** @return array{0: callable, 1: callable} The http_api_curl and the redirect listener of one fetch(). */ + private function listeners(RemoteFileFetcher $fetcher, string $url): array + { + $curl = $redirect = null; + Actions\expectAdded('http_api_curl')->once()->whenHappen(static function (callable $cb) use (&$curl): void { + $curl = $cb; + }); + Actions\expectAdded('requests-requests.before_redirect')->once() + ->whenHappen(static function (callable $cb) use (&$redirect): void { + $redirect = $cb; + }); + Functions\when('wp_safe_remote_get')->justReturn(['response' => ['code' => 200], 'headers' => []]); + $fetcher->fetch($url, $this->tmp, 2048, 'x'); + + return [$curl, $redirect]; + } + + /** Audit (DNS rebinding): cURL connects to the address that was checked, IPv4 only. */ + public function testTheCheckedAddressIsPinnedForTheHostAndEveryRedirectTarget(): void + { + $fetcher = $this->recordingFetcher(static fn (string $host): array => [ + 'fonts.example.com' => ['93.184.216.34', '93.184.216.35'], 'cdn.example.com' => ['198.51.101.7'], + ][$host]); + [$curl, $redirect] = $this->listeners($fetcher, 'https://fonts.example.com/inter.woff2'); + + $curl(curl_init(), [], 'https://fonts.example.com/inter.woff2'); + self::assertSame( + [[CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4], [CURLOPT_RESOLVE, ['fonts.example.com:443:93.184.216.34']]], + $fetcher->options + ); + + $fetcher->options = []; + $redirect('http://cdn.example.com:8080/inter.woff2'); + $curl(curl_init(), [], 'https://fonts.example.com/inter.woff2'); // the redirect request: same original URL + self::assertSame( + [ + [CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4], + [CURLOPT_RESOLVE, ['fonts.example.com:443:93.184.216.34', 'cdn.example.com:8080:198.51.101.7']], + ], + $fetcher->options + ); + } + + public function testAHostThatTurnsPrivateAfterTheCheckIsNotConnected(): void + { + $lookups = 0; + $fetcher = $this->recordingFetcher(static function () use (&$lookups): array { + return ++$lookups === 1 ? ['93.184.216.34'] : ['127.0.0.1']; + }); + [$curl] = $this->listeners($fetcher, 'https://rebind.example.com/a.woff2'); + + $this->expectException(\WpOrg\Requests\Exception::class); + try { + $curl(curl_init(), [], 'https://rebind.example.com/a.woff2'); + } finally { + self::assertSame([[CURLOPT_IPRESOLVE, CURL_IPRESOLVE_V4]], $fetcher->options, 'no pin to the new address'); + } + } }