Forms: Add SSRF protection for webhook URLs #46424
Conversation
Add URL validation to prevent Server-Side Request Forgery (SSRF) attacks in the Forms webhooks feature. This ensures webhook URLs cannot be used to make requests to internal networks or cloud metadata endpoints. Security measures implemented: - Require HTTPS scheme for all webhook URLs - Use WordPress's wp_http_validate_url() to block private IP ranges (127.x, 10.x, 172.16-31.x, 192.168.x) - Add explicit check for link-local addresses (169.254.x.x) which includes AWS/cloud metadata endpoints not blocked by WordPress
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
github-actions
bot
added
the
[Status] Needs Author Reply
vianasw
added
[Type] Bug
There was a problem hiding this comment.
Pull request overview
This PR adds Server-Side Request Forgery (SSRF) protection to the Forms webhook feature to prevent malicious users from making webhook requests to internal networks or cloud metadata endpoints. The implementation enforces HTTPS-only URLs, leverages WordPress's built-in wp_http_validate_url() function to block private IP ranges, and adds explicit validation for link-local addresses (169.254.x.x range) which includes AWS metadata endpoints.
Key changes:
- Added
validate_webhook_url()method with HTTPS enforcement and SSRF protection - Integrated URL validation into the webhook processing pipeline before webhooks are sent
- Added comprehensive test coverage for various blocked URL types and valid public URLs
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| projects/packages/forms/src/service/class-form-webhooks.php | Implements the core SSRF protection with URL validation, HTTPS requirement, and IP range blocking |
| projects/packages/forms/tests/php/service/Form_Webhooks_Test.php | Adds comprehensive test coverage for HTTP blocking, localhost, private networks (Class A/B/C), link-local addresses, and valid public HTTPS URLs |
| projects/packages/forms/changelog/add-forms-webhook-url-validation | Documents the security patch for SSRF vulnerability protection |
| $ip = filter_var( $host, FILTER_VALIDATE_IP ) ? $host : gethostbyname( $host ); | ||
| if ( filter_var( $ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4 ) ) { | ||
| $ip_long = ip2long( $ip ); |
There was a problem hiding this comment.
Using gethostbyname() for DNS resolution during validation creates a Time-of-Check Time-of-Use (TOCTOU) vulnerability. An attacker could use DNS rebinding to pass validation with a public IP, then have the domain resolve to a private IP when the actual HTTP request is made moments later.
Consider removing the DNS resolution step entirely and only validating IP addresses when they are already provided as IPs. WordPress's wp_http_validate_url() will handle DNS resolution safely at request time.
| $ip = filter_var( $host, FILTER_VALIDATE_IP ) ? $host : gethostbyname( $host ); | |
| if ( filter_var( $ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4 ) ) { | |
| $ip_long = ip2long( $ip ); | |
| // Only perform link-local checks when the host is already a literal IPv4 address. | |
| if ( filter_var( $host, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4 ) ) { | |
| $ip_long = ip2long( $host ); |
There was a problem hiding this comment.
wouldn't removing the gethostbyname make the 169.254.x.x range unprotected for hostnames (a hostname that resolves to this internal IP range)? Besides, wp_http_validate_url() also uses gethostbyname() internally - so it has the same TOCTOU issue. The only difference is we're adding a second DNS lookup.
| public function test_send_webhooks_blocks_private_class_a_urls() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://10.0.0.1/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $http_request_made = false; | ||
| add_filter( | ||
| 'pre_http_request', | ||
| function () use ( &$http_request_made ) { | ||
| $http_request_made = true; | ||
| return array( | ||
| 'response' => array( 'code' => 200 ), | ||
| 'body' => '{"success":true}', | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| $this->assertFalse( $http_request_made, 'HTTP request should not be made for private Class A URLs' ); | ||
| } | ||
|
|
||
| /** | ||
| * Test webhook is blocked when URL points to private Class B network (172.16-31.x.x). | ||
| */ | ||
| public function test_send_webhooks_blocks_private_class_b_urls() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://172.16.0.1/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $http_request_made = false; | ||
| add_filter( | ||
| 'pre_http_request', | ||
| function () use ( &$http_request_made ) { | ||
| $http_request_made = true; | ||
| return array( | ||
| 'response' => array( 'code' => 200 ), | ||
| 'body' => '{"success":true}', | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| $this->assertFalse( $http_request_made, 'HTTP request should not be made for private Class B URLs' ); | ||
| } | ||
|
|
||
| /** | ||
| * Test webhook is blocked when URL points to private Class C network (192.168.x.x). | ||
| */ | ||
| public function test_send_webhooks_blocks_private_class_c_urls() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://192.168.1.1/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $http_request_made = false; | ||
| add_filter( | ||
| 'pre_http_request', | ||
| function () use ( &$http_request_made ) { | ||
| $http_request_made = true; | ||
| return array( | ||
| 'response' => array( 'code' => 200 ), | ||
| 'body' => '{"success":true}', | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| $this->assertFalse( $http_request_made, 'HTTP request should not be made for private Class C URLs' ); | ||
| } |
There was a problem hiding this comment.
The tests for private Class A, B, and C networks (lines 829-941) do not verify that the expected logging events are triggered, unlike the HTTP and localhost tests (lines 713-824). This inconsistency makes it harder to verify that the webhook is being skipped for the correct reason. Consider adding logging verification to these tests to match the pattern established in the earlier tests.
| public function test_send_webhooks_blocks_link_local_urls() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://169.254.169.254/latest/meta-data/', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $http_request_made = false; | ||
| add_filter( | ||
| 'pre_http_request', | ||
| function () use ( &$http_request_made ) { | ||
| $http_request_made = true; | ||
| return array( | ||
| 'response' => array( 'code' => 200 ), | ||
| 'body' => '{"success":true}', | ||
| ); | ||
| } | ||
| ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| $this->assertFalse( $http_request_made, 'HTTP request should not be made for link-local/AWS metadata URLs' ); | ||
| } |
There was a problem hiding this comment.
The test for link-local addresses doesn't verify that the expected logging event is triggered. Unlike the HTTP and localhost tests which verify 'webhook_skipped' event with appropriate reason codes, this test only checks that no HTTP request was made. Add logging verification similar to lines 743-765 to ensure the webhook is skipped with the correct reason.
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
Implements defense-in-depth against SSRF attacks for webhook requests: - Use wp_safe_remote_request() instead of wp_remote_request() for built-in SSRF protection and redirect validation - Add http_request_host_is_external filter to block link-local IPs (169.254.x.x range including AWS metadata endpoint) at request time - Simplify validate_webhook_url() to only check URL format and HTTPS scheme, deferring IP validation to request time This approach prevents TOCTOU (Time-of-Check Time-of-Use) vulnerabilities from DNS rebinding attacks, where a hostname could resolve to a safe IP during validation but a malicious IP during the actual request. The filter is scoped by adding it immediately before the request and removing it immediately after, avoiding the need for instance state.
Extends the request-time SSRF protection to handle IPv6 addresses: - Block IPv6 loopback (::1) - Block IPv6 link-local addresses (fe80::/10) - Block IPv6 unique local addresses (fc00::/7, includes fd00::/8) - Block IPv6 site-local addresses (fec0::/10) - deprecated but still blocked The fc00::/7 range covers private IPv6 networks and cloud metadata endpoints that may be accessible via IPv6 (e.g., fd00::a9fe:a9fe). Also handles bracket stripping for IPv6 literals in URLs (e.g., [::1]).
projects/packages/forms/changelog/add-forms-webhook-url-validation
Outdated
Show resolved
Hide resolved
| public function test_send_webhooks_blocks_localhost_urls() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://127.0.0.1/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| // wp_safe_remote_request() blocks private IPs and returns WP_Error, which gets logged | ||
| $error_meta = get_post_meta( $post_id, '_jetpack_forms_webhook_error', true ); | ||
| $this->assertNotEmpty( $error_meta, 'Error should be logged for localhost URLs' ); | ||
| } | ||
|
|
||
| /** | ||
| * Test webhook is blocked when URL points to private Class A network (10.x.x.x). | ||
| * SSRF protection is handled at request time by wp_safe_remote_request(). | ||
| */ | ||
| public function test_send_webhooks_blocks_private_class_a_urls() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://10.0.0.1/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| // wp_safe_remote_request() blocks private IPs and returns WP_Error, which gets logged | ||
| $error_meta = get_post_meta( $post_id, '_jetpack_forms_webhook_error', true ); | ||
| $this->assertNotEmpty( $error_meta, 'Error should be logged for private Class A URLs' ); | ||
| } |
There was a problem hiding this comment.
These tests rely on WordPress's wp_safe_remote_request() to actually block the request, but they don't set up a pre_http_request filter to prevent any potential network requests from being attempted. While WordPress should block these IPs, it would be safer and faster to add a pre_http_request filter that fails if called, similar to the HTTP blocking test on line 731-741, to ensure no actual network request is made during testing.
| public function test_send_webhooks_blocks_ipv6_loopback() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://[::1]/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| // IPv6 loopback should be blocked | ||
| $error_meta = get_post_meta( $post_id, '_jetpack_forms_webhook_error', true ); | ||
| $this->assertNotEmpty( $error_meta, 'Error should be logged for IPv6 loopback URLs' ); | ||
| } | ||
|
|
||
| /** | ||
| * Test that webhook blocks IPv6 link-local addresses. | ||
| * SSRF protection should block fe80::/10 range. | ||
| */ | ||
| public function test_send_webhooks_blocks_ipv6_link_local() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://[fe80::1]/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| // IPv6 link-local should be blocked | ||
| $error_meta = get_post_meta( $post_id, '_jetpack_forms_webhook_error', true ); | ||
| $this->assertNotEmpty( $error_meta, 'Error should be logged for IPv6 link-local URLs' ); | ||
| } | ||
|
|
||
| /** | ||
| * Test that webhook blocks IPv6 unique local addresses (fd00::/8). | ||
| * SSRF protection should block fc00::/7 range which includes fd00::/8 used for private networks | ||
| * and cloud metadata endpoints like fd00::a9fe:a9fe. | ||
| */ | ||
| public function test_send_webhooks_blocks_ipv6_unique_local() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://[fd00::1]/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| // IPv6 unique local addresses should be blocked | ||
| $error_meta = get_post_meta( $post_id, '_jetpack_forms_webhook_error', true ); | ||
| $this->assertNotEmpty( $error_meta, 'Error should be logged for IPv6 unique local URLs' ); | ||
| } | ||
|
|
||
| /** | ||
| * Test that webhook blocks IPv6 site-local addresses (fec0::/10). | ||
| * These are deprecated but still blocked for security. | ||
| */ | ||
| public function test_send_webhooks_blocks_ipv6_site_local() { | ||
| $form = $this->create_mock_form( | ||
| array( | ||
| 'webhooks' => array( | ||
| array( | ||
| 'webhook_id' => 'test-webhook', | ||
| 'url' => 'https://[fec0::1]/webhook', | ||
| 'format' => 'json', | ||
| 'method' => 'POST', | ||
| 'enabled' => true, | ||
| ), | ||
| ), | ||
| ) | ||
| ); | ||
| $fields = array( $this->create_mock_field( $form, 'test-field', 'test value' ) ); | ||
|
|
||
| $post_id = $this->create_feedback_post( $form, $fields ); | ||
|
|
||
| $webhooks = Form_Webhooks::init(); | ||
| $webhooks->send_webhooks( $post_id, $fields, false, array() ); | ||
|
|
||
| // IPv6 site-local addresses (deprecated) should be blocked | ||
| $error_meta = get_post_meta( $post_id, '_jetpack_forms_webhook_error', true ); | ||
| $this->assertNotEmpty( $error_meta, 'Error should be logged for IPv6 site-local URLs' ); | ||
| } |
There was a problem hiding this comment.
Similar to the localhost and private IP tests, these IPv6 tests don't set up a pre_http_request filter to ensure no actual network request is made. Consider adding a filter that fails if called to make the tests safer and faster.
Always remove http_request_host_is_external even if the request fail to pottentially affect subsequent requests. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| $parsed = wp_parse_url( $url ); | ||
|
|
||
| if ( ! $parsed || empty( $parsed['host'] ) ) { | ||
| return new WP_Error( 'invalid_url', __( 'Invalid webhook URL format.', 'jetpack-forms' ) ); |
There was a problem hiding this comment.
The URL validation uses wp_parse_url() which can be bypassed with malformed URLs. For example, URLs like "https:///example.com" or URLs with unusual syntax might pass validation but behave unexpectedly. Consider adding additional validation to ensure the parsed URL components are well-formed, or use filter_var() with FILTER_VALIDATE_URL as an additional check before wp_parse_url().
Extends the request-time SSRF protection to handle IPv6 addresses: - Block IPv6 loopback (::1) - Block IPv6 link-local addresses (fe80::/10) - Block IPv6 unique local addresses (fc00::/7, includes fd00::/8) - Block IPv6 site-local addresses (fec0::/10) - deprecated but still blocked The fc00::/7 range covers private IPv6 networks and cloud metadata endpoints that may be accessible via IPv6 (e.g., fd00::a9fe:a9fe). Also: - Handles bracket stripping for IPv6 literals in URLs (e.g., [::1]) - Uses dns_get_record() for AAAA lookups to catch hostnames that resolve only to IPv6 addresses (gethostbyname only resolves IPv4) - Extracts IP validation into is_blocked_ip() helper method
| // Check for IPv6 loopback (::1) | ||
| if ( $ip === '::1' ) { |
There was a problem hiding this comment.
The IPv6 loopback check only matches the compressed form ::1 but not other valid representations like 0:0:0:0:0:0:0:1 or ::0:1. An attacker could potentially bypass this check by using an alternative representation of the IPv6 loopback address.
A more robust approach would be to compare the binary representation after inet_pton. After line 244, you could check if the binary equals the binary of ::1 using: if ($ip_binary === inet_pton('::1')) { return true; }
However, this may already be mitigated if WordPress normalizes IPv6 addresses before they reach this filter, but it's better to be defensive.
| // Check for IPv6 loopback (::1) | |
| if ( $ip === '::1' ) { | |
| // Check for IPv6 loopback (::1 and any equivalent textual representation) | |
| $loopback_binary = inet_pton( '::1' ); | |
| if ( $loopback_binary !== false && $ip_binary === $loopback_binary ) { |
| // If host is already an IP, check it directly | ||
| if ( filter_var( $host, FILTER_VALIDATE_IP ) ) { | ||
| return $this->is_blocked_ip( $host ) ? false : $is_external; | ||
| } | ||
|
|
||
| // For hostnames, check IPv4 via gethostbyname | ||
| $ipv4 = gethostbyname( $host ); | ||
| if ( $ipv4 !== $host && $this->is_blocked_ip( $ipv4 ) ) { | ||
| return false; | ||
| } | ||
|
|
||
| // Check IPv6 via DNS AAAA records (gethostbyname only resolves IPv4) | ||
| // This catches hostnames that resolve to blocked IPv6 addresses | ||
| if ( function_exists( 'dns_get_record' ) ) { | ||
| // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- dns_get_record may fail on some systems | ||
| $aaaa_records = @dns_get_record( $host, DNS_AAAA ); | ||
| if ( $aaaa_records ) { | ||
| foreach ( $aaaa_records as $record ) { | ||
| if ( isset( $record['ipv6'] ) && $this->is_blocked_ip( $record['ipv6'] ) ) { | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The DNS resolution operations (gethostbyname and dns_get_record) in this filter callback introduce significant performance overhead. This filter is called by WordPress during HTTP request processing, potentially multiple times per request due to redirects or internal WordPress HTTP API behavior. Performing blocking DNS lookups on every invocation can cause noticeable delays, especially if the DNS server is slow or unresponsive.
Since wp_safe_remote_request already blocks private IPs after DNS resolution, and the http_request_host_is_external filter is specifically designed to run after DNS resolution with the resolved IP, consider simplifying this implementation to only check the resolved IP address passed in the host parameter, rather than performing additional DNS lookups. The filter will receive the already-resolved IP from WordPress's HTTP API, making these additional lookups redundant and inefficient.
| // If host is already an IP, check it directly | |
| if ( filter_var( $host, FILTER_VALIDATE_IP ) ) { | |
| return $this->is_blocked_ip( $host ) ? false : $is_external; | |
| } | |
| // For hostnames, check IPv4 via gethostbyname | |
| $ipv4 = gethostbyname( $host ); | |
| if ( $ipv4 !== $host && $this->is_blocked_ip( $ipv4 ) ) { | |
| return false; | |
| } | |
| // Check IPv6 via DNS AAAA records (gethostbyname only resolves IPv4) | |
| // This catches hostnames that resolve to blocked IPv6 addresses | |
| if ( function_exists( 'dns_get_record' ) ) { | |
| // phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged -- dns_get_record may fail on some systems | |
| $aaaa_records = @dns_get_record( $host, DNS_AAAA ); | |
| if ( $aaaa_records ) { | |
| foreach ( $aaaa_records as $record ) { | |
| if ( isset( $record['ipv6'] ) && $this->is_blocked_ip( $record['ipv6'] ) ) { | |
| return false; | |
| } | |
| } | |
| } | |
| } | |
| /* | |
| * At this point, WordPress's HTTP API should already have resolved the host | |
| * and passed the resolved IP address into this filter. We therefore avoid | |
| * performing additional DNS lookups here for performance reasons and simply | |
| * evaluate the provided IP. | |
| */ | |
| // If host is an IP, check it directly against blocked ranges. | |
| if ( filter_var( $host, FILTER_VALIDATE_IP ) ) { | |
| return $this->is_blocked_ip( $host ) ? false : $is_external; | |
| } | |
| // If host is not an IP (unexpected), fall back to the original decision. |
| $first_byte = ord( $ip_binary[0] ); | ||
| $second_byte = ord( $ip_binary[1] ); |
There was a problem hiding this comment.
The array access on line 255 (second_byte = ord($ip_binary[1])) lacks a bounds check. If inet_pton returns a malformed or short binary string (which could theoretically happen with certain edge cases or unusual input), accessing index 1 without checking the length could trigger a PHP notice or undefined behavior.
Consider adding a length check before accessing the second byte to ensure the binary representation is at least 2 bytes long.
| * This avoids TOCTOU vulnerabilities from DNS resolution during validation. | ||
| * |
There was a problem hiding this comment.
The comment claims this approach "avoids TOCTOU vulnerabilities from DNS resolution during validation," but the block_link_local_requests filter still performs DNS resolution (lines 201-219) at request time. This creates a TOCTOU (Time-of-Check Time-of-Use) vulnerability: an attacker could use DNS rebinding to make the hostname resolve to a safe IP when the filter checks it (lines 202, 211), but resolve to a blocked IP (like 169.254.169.254) when WordPress actually makes the HTTP connection.
Consider either: (1) relying entirely on wp_safe_remote_request's built-in protections without additional DNS lookups, or (2) if additional protection is needed for link-local addresses, document that DNS rebinding attacks remain a possibility and cannot be fully prevented at the application layer.
| * This avoids TOCTOU vulnerabilities from DNS resolution during validation. | |
| * | |
| * This minimizes additional DNS lookups during validation, but DNS rebinding | |
| * and related TOCTOU issues cannot be fully prevented at the application layer. |
Simplify SSRF protection by moving link-local and private IPv6 checks from request-time (http_request_host_is_external filter) to validation time in validate_webhook_url(). This removes the filter-based approach in favor of checking IPs directly when the webhook URL is validated. URLs pointing to blocked IP ranges are now rejected before any HTTP request is made.
vianasw
added
the
Coverage tests to be added later
| if ( $aaaa_records ) { | ||
| foreach ( $aaaa_records as $record ) { | ||
| if ( isset( $record['ipv6'] ) && $this->is_blocked_ip( $record['ipv6'] ) ) { | ||
| return new WP_Error( 'blocked_ip', __( 'Webhook URL cannot point to private or internal networks.', 'jetpack-forms' ) ); |
There was a problem hiding this comment.
This error message and the ones above: I'd be as obscure as we can, no need to tell anyone why we're blocking an URL or IP.
Proposed changes:
wp_http_validate_url()to block private IP ranges (127.x, 10.x, 172.16-31.x, 192.168.x)Other information:
Jetpack product discussion
N/A - Security fix
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Automated tests
jetpack test -v php packages/forms -- --filter="Form_Webhooks"to verify all tests passManual testing
add_filter( ''jetpack_forms_webhooks_enabled', '__return_true' );http://example.com/webhook(HTTP - should be blocked, HTTPS required)https://127.0.0.1/webhook(localhost - should be blocked)https://10.0.0.1/webhook(private Class A - should be blocked)https://172.16.0.1/webhook(private Class B - should be blocked)https://192.168.1.1/webhook(private Class C - should be blocked)https://169.254.169.254/latest/meta-data/(AWS metadata - should be blocked)https://hooks.zapier.com/webhook/123(should work)https://webhook.site/abc123(should work)