Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WAF: Add support for handling IP ranges in allow/block lists #29131

Merged
merged 15 commits into from
Feb 27, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: added

Added a utility function to extract an array of IP addresses from a given string.
34 changes: 34 additions & 0 deletions projects/packages/ip/src/class-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,4 +167,38 @@ public static function ip_address_is_in_range( $ip, $range_low, $range_high ) {
return false;
}

/**
* Extracts IP addresses from a given string.
*
* We allow for both, one IP per line or comma-; semicolon; or whitespace-separated lists. This also validates the IP addresses
* and only returns the ones that look valid. IP ranges using the "-" character are also supported.
*
* @param string $ips List of ips - example: "8.8.8.8\n4.4.4.4,2.2.2.2;1.1.1.1 9.9.9.9,5555.5555.5555.5555,1.1.1.10-1.1.1.20".
* @return array List of valid IP addresses. - example based on input example: array('8.8.8.8', '4.4.4.4', '2.2.2.2', '1.1.1.1', '9.9.9.9', '1.1.1.10-1.1.1.20')
*/
public static function get_ip_addresses_from_string( $ips ) {
$ips = (string) $ips;
$ips = preg_split( '/[\s,;]/', $ips );

$result = array();

foreach ( $ips as $ip ) {
// Validate both IP values from the range
$range = explode( '-', $ip );
if ( count( $range ) === 2 ) {
if ( filter_var( $range[0], FILTER_VALIDATE_IP ) !== false || filter_var( $range[1], FILTER_VALIDATE_IP ) !== false ) {
Copy link
Contributor

@dkmyta dkmyta Feb 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we'll want && over || to determine that both the starting and ending addresses are valid. Otherwise, I believe things like 12.12.12.1-not.a.valid.ip and not.a.valid.ip-12.12.12.5 will pass through the filter.

I also wonder, during this filtering process, if is there anything else we need to check for as far as inaccuracies go before passing the range as valid. For example, is $range[0] in fact lower than, and not equal to $range[1]?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thanks Dean - I've added some additional validation which should cover everything you've mentioned: f4c2215 👍

$result[] = $ip;
}
continue;
}

// validate the single IP value
if ( filter_var( $ip, FILTER_VALIDATE_IP ) !== false ) {
$result[] = $ip;
}
}

return $result;
}

}
41 changes: 41 additions & 0 deletions projects/packages/ip/tests/php/test-utils.php
Original file line number Diff line number Diff line change
Expand Up @@ -305,4 +305,45 @@ public function test_ip_address_is_in_range() {
$this->assertFalse( Utils::ip_address_is_in_range( $out_range_ip, $range_low, $range_high ) );
}

/**
* Test `get_ip_addresses_from_string`.
* Covers IPv4 and IPv6 addresses, including ranges, concatenated with various delimiters.
*
* @covers ::get_ip_addresses_from_string
*/
public function test_get_ip_addresses_from_string() {
$string = '';

$delimiters = array( "\n", ',', ';', ' ' );
$delimiters_index = 0;

$ips = array(
// IPv4
'1.1.1.1',
'2.2.2.2',
'3.3.3.3',
'4.4.4.4',
'5.5.5.5-6.6.6.6',
// IPv6
'2001:db8::1',
'2001:db8::2',
'2001:db8::3',
'2001:db8::4',
'2001:db8::5-2001:db8::6',
);

// Generate a string that includes all IPs and delimiters.
foreach ( $ips as $ips_index => $ip ) {
$string .= $ip;

$is_last_loop = $ips_index === count( $ips ) - 1;
if ( ! $is_last_loop ) {
$string .= $delimiters[ $delimiters_index ];
$delimiters_index = count( $delimiters ) === $delimiters_index + 1 ? 0 : $delimiters_index + 1;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd certainly prefer if our expectation was hardcoded here, because this makes it pretty hard to understand what exactly we are expecting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking this out Kolja! Good point - it is much more understandable by hardcoding it: 677b08d


$this->assertEquals( $ips, Utils::get_ip_addresses_from_string( $string ) );
}

}
4 changes: 4 additions & 0 deletions projects/packages/waf/changelog/add-ip-range-support
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: added

Added support for IP ranges in allow and block lists.
3 changes: 2 additions & 1 deletion projects/packages/waf/composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
"require": {
"automattic/jetpack-connection": "@dev",
"automattic/jetpack-constants": "@dev",
"automattic/jetpack-ip": "@dev",
"automattic/jetpack-status": "@dev",
"wikimedia/aho-corasick": "^1.0"
},
Expand Down Expand Up @@ -56,7 +57,7 @@
"link-template": "https://github.com/Automattic/jetpack-waf/compare/v${old}...v${new}"
},
"branch-alias": {
"dev-trunk": "0.9.x-dev"
"dev-trunk": "0.10.x-dev"
}
},
"config": {
Expand Down
27 changes: 3 additions & 24 deletions projects/packages/waf/src/class-waf-rules-manager.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
namespace Automattic\Jetpack\Waf;

use Automattic\Jetpack\Connection\Client;
use Automattic\Jetpack\IP\Utils as IP_Utils;
use Jetpack_Options;
use WP_Error;

Expand Down Expand Up @@ -259,28 +260,6 @@ public static function generate_automatic_rules() {
update_option( self::AUTOMATIC_RULES_LAST_UPDATED_OPTION_NAME, time() );
}

/**
* We allow for both, one IP per line or comma-; semicolon; or whitespace-separated lists. This also validates the IP addresses
* and only returns the ones that look valid.
*
* @param string $ips List of ips - example: "8.8.8.8\n4.4.4.4,2.2.2.2;1.1.1.1 9.9.9.9,5555.5555.5555.5555".
* @return array List of valid IP addresses. - example based on input example: array('8.8.8.8', '4.4.4.4', '2.2.2.2', '1.1.1.1', '9.9.9.9')
*/
public static function ip_option_to_array( $ips ) {
$ips = (string) $ips;
$ips = preg_split( '/[\s,;]/', $ips );

$result = array();

foreach ( $ips as $ip ) {
if ( filter_var( $ip, FILTER_VALIDATE_IP ) !== false ) {
$result[] = $ip;
}
}

return $result;
}

/**
* Generates the rules.php script
*
Expand Down Expand Up @@ -309,8 +288,8 @@ public static function generate_ip_rules() {
$wp_filesystem->mkdir( dirname( $block_ip_file_path ) );
}

$allow_list = self::ip_option_to_array( get_option( self::IP_ALLOW_LIST_OPTION_NAME ) );
$block_list = self::ip_option_to_array( get_option( self::IP_BLOCK_LIST_OPTION_NAME ) );
$allow_list = IP_Utils::get_ip_addresses_from_string( get_option( self::IP_ALLOW_LIST_OPTION_NAME ) );
$block_list = IP_Utils::get_ip_addresses_from_string( get_option( self::IP_BLOCK_LIST_OPTION_NAME ) );

$allow_rules_content = '';
// phpcs:disable WordPress.PHP.DevelopmentFunctions
Expand Down
26 changes: 22 additions & 4 deletions projects/packages/waf/src/class-waf-runtime.php
Original file line number Diff line number Diff line change
Expand Up @@ -663,14 +663,32 @@ function ( $item ) {
}

/**
* Verifies is ip from request is in an array.
* Verifies if the IP from the current request is in an array.
*
* @param array $array Array to verify ip against.
* @param array $array Array of IP addresses to verify the request IP against.
* @return bool
*/
public function is_ip_in_array( $array ) {
$real_ip = $this->request->get_real_user_ip_address();
$real_ip = $this->request->get_real_user_ip_address();
$array_length = count( $array );

for ( $i = 0; $i < $array_length; $i++ ) {
// Check if the IP matches a provided range.
$range = explode( $array[ $i ], '-' );
if ( count( $range ) === 2 ) {
if ( IP_Utils::ip_address_is_in_range( $real_ip, $range[0], $range[1] ) ) {
return true;
}
continue;
}

return in_array( $real_ip, $array, true );
// Check if the IP is an exact match.
if ( $real_ip === $array[ $i ] ) {
return true;
}
}

return false;
}

/**
Expand Down
6 changes: 4 additions & 2 deletions projects/packages/waf/src/class-waf-stats.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

namespace Automattic\Jetpack\Waf;

use Automattic\Jetpack\IP\Utils as IP_Utils;

/**
* Retrieves WAF stats.
*/
Expand All @@ -24,7 +26,7 @@ public static function get_ip_allow_list_count() {
return 0;
}

$results = Waf_Rules_Manager::ip_option_to_array( $ip_allow_list );
$results = IP_Utils::get_ip_addresses_from_string( $ip_allow_list );

return count( $results );
}
Expand All @@ -41,7 +43,7 @@ public static function get_ip_block_list_count() {
return 0;
}

$results = Waf_Rules_Manager::ip_option_to_array( $ip_block_list );
$results = IP_Utils::get_ip_addresses_from_string( $ip_block_list );

return count( $results );
}
Expand Down
5 changes: 5 additions & 0 deletions projects/plugins/jetpack/changelog/add-waf-ip-ranges
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: other
Comment: Updated composer.lock.


54 changes: 52 additions & 2 deletions projects/plugins/jetpack/composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions projects/plugins/protect/changelog/add-waf-ip-ranges
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
Significance: patch
Type: changed
Comment: Updated composer.lock.


54 changes: 52 additions & 2 deletions projects/plugins/protect/composer.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.