Skip to content

573 - Check URLs are valid before attempting to perform HTTP request - #578

Open
markfullmer wants to merge 11 commits into
php-embed:masterfrom
markfullmer:573-valid-url
Open

markfullmer wants to merge 11 commits into
php-embed:masterfrom
markfullmer:573-valid-url

Conversation

@markfullmer

Copy link
Copy Markdown

Purpose

Resolves #573

Implementation

Adds a new isValidUrl() helper function that:

  • checks the URL using the standard PHP FILTER_VALIDATE_URL
  • checks IPs the host resolves to and uses dns_get_record() to check if a record exists
  • if dns_get_record() fails, try gethostbyname()
  • Filter any found IPs against PHP's reserved and privileged ranges (FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE)

This helper function is added to Extractor::resolveUri(), adjacent to the check for HTTP URLs.

Finally, the existing isHttp() function is updated to use a stricter "allow-list" methodology, rejecting anything that does not match the http or https schema, as suggested in #573.

Test coverage

Test coverage is added in FunctionsTest. All of its scenarios pass. There are other failing tests, but these are unchanged compared to the failing tests prior to this code change.

@Vitorinox

Copy link
Copy Markdown
Collaborator

Hi @markfullmer, thank you for the quick follow-up and for being upfront about how the cURL changes were put together. Several of your ideas are exactly the right direction: pinning the validated IPs with CURLOPT_RESOLVE, restricting protocols to HTTP(S), and following redirects manually.

Rewriting your branch felt wrong. I'm preparing a separate PR that builds on your ideas

I re-tested 0810bef against master (c45a900) and found a few things that I think need a different approach rather than more patches on top:

  1. Relative URLs are still broken. Reverting isHttp() helped, but Extractor::resolveUri() still calls isValidUrl() on the raw string before resolving it, so /img.png, ../x and //cdn... throw. The suite still goes from 1 failure on master to 18 (favicon/icon/image/feeds in PagesTest).
  2. PHP 8.0/8.1 compatibility. FILTER_FLAG_GLOBAL_RANGE only exists since PHP 8.2; on 8.0/8.1 the undefined constant is a fatal error, and on 7.4 it produces a wrong mask. composer.json allows ^7.4|^8 and CI tests 7.4 to 8.4. CI didn't catch it because the fork workflow is still waiting for approval.
  3. Other request paths. Page-declared oEmbed endpoints (<link type="application/json+oembed" href="http://127.0.0.1/...">), adapter API calls and meta-refresh go through Crawler::sendRequest(), so validation needs to live at request time, not at URL-resolution time. Separately, LinkedData lets ml/json-ld fetch any remote @context via file_get_contents, bypassing the Crawler entirely.
  4. Smaller gaps. The NAT64 prefix in hex ([64:ff9b::a9fe:a9fe]) still passes, and FILTER_VALIDATE_URL rejects valid hosts with underscores and non-punycode IDNs.

Because the fix point moves (validation in the HTTP dispatcher on the resolved absolute URI, explicit IPv4/IPv6 range tables that behave the same on PHP 7.4 to 8.5, manual redirects inside the dispatcher so oEmbed and adapter calls are covered too), rewriting your branch felt wrong. I'm preparing a separate PR that builds on your ideas, and I'll credit you in it. The JSON-LD part will go in its own smaller PR. I'd really appreciate your review when it's up, and if you'd rather take any part of it forward yourself, I'm happy to coordinate.

Thanks again for pushing this forward!

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible SSRF Exploitation via Scheme Validation Bypass

3 participants