Plugin is open to IP address spoofing

[Sidsector9]

Describe the bug

The plugin uses the following function to get client's IP address:

https://github.com/10up/restricted-site-access/blob/df9226bffd67ec96e85428ea1fd5e10c3561ee5d/restricted_site_access.php#L1531-L1562

The problem is that headers such as HTTP_X_FORWARDED_FOR and HTTP_X_FORWARDED can be very easily spoofed.

After a lot of Googling and reviewing some of the popular plugins on WordPress which rely on client IP detection, I found out that they dropped support for the HTTP_X_FORWARDED_* headers and completely relied on the REMOTE_ADDR header.

The REMOTE_ADDR header contains the client's IP address, and if the web server is behind a proxy, the header then contains the IP address of the proxy server.

Proposed solution

1. Add a filter so that users can add a list of trusted proxies
2. Add a filter so that users can add an array of headers that the trusted proxy uses to forward client's real IP address
3. Rely only on the REMOTE_ADDR header as a default

Expected behavior

The plugin shouldn't be open to IP spoofing.

[Sidsector9]

@dkotter I've made the following modification to the function, let me know your thoughts on it:

/**
 * Retrieve the visitor ip address, even it is behind a proxy.
 *
 * @return string
 */
public static function get_client_ip_address() {
    $ip                    = '';
    $remote_addr_header_ip = isset( $_SERVER['REMOTE_ADDR'] ) ? sanitize_text_field( wp_unslash( $_SERVER['REMOTE_ADDR'] ) ) : false;

    /**
     * Assume empty REMOTE_ADDR as unreliable.
     */
    if ( false === $remote_addr_header_ip ) {
        return '';
    }

    /**
     * Accepts the string 'REMOTE_ADDR' or array of trusted proxies.
     * It is advisable to pass 'REMOTE_ADDR' if your proxy server doesn't have
     * a static IP.
     *
     * @param string|array
     *
     * @since 7.3.1
     */
    $trusted_proxies = apply_filters( 'rsa_trusted_proxies', 'REMOTE_ADDR' );

    /**
     * Add headers that your reverse proxy uses to send client IP information.
     *
     * Example of possible values are:
     *
     * HTTP_CF_CONNECTING_IP
     * HTTP_CLIENT_IP
     * HTTP_X_FORWARDED_FOR
     * HTTP_X_FORWARDED
     * HTTP_X_CLUSTER_CLIENT_IP
     * HTTP_FORWARDED_FOR
     * HTTP_FORWARDED
     *
     * @param array
     *
     * @since 7.3.1
     */
    $proxy_trusted_headers = apply_filters( 'rsa_proxy_trusted_headers', array( 'HTTP_X_FORWARDED_FOR' ) );

    if ( is_string( $trusted_proxies ) && 'REMOTE_ADDR' === $trusted_proxies ) {
        if ( ! empty( $proxy_trusted_headers ) ) {
            return self::get_ip_from_headers( $proxy_trusted_headers );
        } else {
            return $remote_addr_header_ip;
        }
    } else if ( is_array( $trusted_proxies ) && ! empty( $trusted_proxies ) ) {
        if ( in_array( $remote_addr_header_ip, $trusted_proxies ) ) {
            return self::get_ip_from_headers( $proxy_trusted_headers );
        }
    }

    return '';
}

/**
 * Returns the first matched IP from the list of array of headers.
 *
 * @return string
 */
public static function get_ip_from_headers( $headers = array() ) {
    foreach ( $headers as $header ) {
        if ( ! isset( $_SERVER[ $header ] ) ) {
            continue;
        }

        foreach ( explode(
            ',',
            sanitize_text_field( wp_unslash( $_SERVER[ $header ] ) )
        ) as $ip ) {
            $ip = trim( $ip ); // just to be safe.

            if ( filter_var( $ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE ) !== false ) {
                return $ip;
            }
        }
    }

    return '';
}

[dkotter]

I looked through the file history to better understand the reasoning behind the current code. Looks like in #11 (Mar 2017) a proposal was put forth to change how we get IP addresses to use something similar to what we have now. The only reasoning there was to better support sites using proxy servers/CDNs.

This PR was closed in favor of #28 (merged Feb 2018). It slightly refactored the code from #11 but had the same basic idea, adding support for more headers. Prior to this change being merged, we only looked at the REMOTE_ADDR header.

Later the HTTP_CF_CONNECTING_IP header was added after a report in #109 about a compatibility issue with Cloudflare (and the fix was in #110, merged Feb 2020).

Looking at the proposed change here, I'm not opposed to considering this approach, I'd suggest opening this as a PR where we can have a better conversation (there's a few things I'd recommend changing). From following the code, I think the suggestion is for anyone using a proxy, utilize the new rsa_trusted_proxies filter to pass in the IP address(es) of their proxy so we only use headers other than REMOTE_ADDR if the request comes directly from the proxy itself, correct? That's probably the only real way to address the potential problem here but it will break backwards compatibility for anyone using a proxy right now, until they add these filters themselves.

I think there's potential for discussion here on if this is a big enough issue to break that compatibility. My understanding of the problem is someone can spoof those headers but they would first need to know what IP address(es) are allowed, correct? It seems like that isn't something that could be easily discovered but I may be overlooking something

[peterwilsoncc]

I think this is challenging because it would break the plugin on major CDNs. CloudFlare, CloudFront and WP VIP each forward the IP address using one or more of these headers.

It would be good if the plugin could detect whether the request comes from some of the more popular CDNs but I am not sure whether any of them provide source information via a HTTP header that can't be spoofed.

[Sidsector9]

From following the code, I think the suggestion is for anyone using a proxy, utilize the new rsa_trusted_proxies filter to pass in the IP address(es) of their proxy so we only use headers other than REMOTE_ADDR if the request comes directly from the proxy itself, correct?

That's correct.

That's probably the only real way to address the potential problem here but it will break backwards compatibility for anyone using a proxy right now, until they add these filters themselves.

I missed to mention this would be a breaking change. Something that customers need to take into consideration before updating if we ever go ahead with this.

My understanding of the problem is someone can spoof those headers but they would first need to know what IP address(es) are allowed, correct? It seems like that isn't something that could be easily discovered but I may be overlooking something

I am not an expert in security so I don't know the technical term for it (maybe @peterwilsoncc can help) but there's this situation where an insider leaks/sells sensitive data (in this case the unrestricted IP addresses) outside the organization.

[dkotter]

Re-opening this issue as we got another security report around the same thing.

We previously addressed this in #198 but the approach we took there was to add new filters that could be used to make things more secure but the default experience wasn't changed. It seems that to appease the new round of security reports, we need to make the default the more secure experience and provide a way for individual sites to trust whatever headers make sense for them.

As such, in discussing internally, we landed on the following approach:

Introduce two new settings: one to set the list of trusted headers and one to set the list of trusted proxy IP addresses. Something like the following:

For new installs, the only header we trust would be REMOTE_ADDR. For sites that need to trust additional headers, they would use the new Trusted headers setting to add those in (like HTTP_CF_CONNECTING_IP as an example).

In addition, for an added layer of security for sites that are behind a proxy and need to trust additional headers, you can utilize the new Trusted proxy IP addresses setting to enter in one or more IP addresses that their proxy uses.

These settings would in essence be used where we currently have the rsa_trusted_headers and rsa_trusted_proxies filters (though those filters can remain).

In order to maintain backwards compatibility, the thought is to distinguish between a new install and an existing install and automatically set our list of trusted headers to match what we have in the codebase right now for existing installs. Those installs could then remove any of those headers they don't want to trust and additionally add in any trusted proxy IP addresses.

I think the easiest way to make this distinction between new and existing installs is to look for the presence of existing RSA data and if that data doesn't exist, we can assume it's a new install (or an install that hasn't been configured yet). If the data exists but no data exists for these new settings, we can assume it's an existing install. In that scenario, we can automatically set the trusted headers to match our current approved list. This should help keep backwards compat.

[peterwilsoncc]

As discussed via Slack, I think this is a good approach but am not sure the UI is required. It's quite a technical problem so the defaults of the existing filter could be modified depending on the version in which the plugin was activated.

A naive approach (that doesn't yet consider network activation) may be to record the version in which the plugin was activated and set the filter defaults accordingly:

diff --git a/restricted_site_access.php b/restricted_site_access.php
index ae550fb..267a67d 100644
--- a/restricted_site_access.php
+++ b/restricted_site_access.php
@@ -1575,6 +1575,11 @@ class Restricted_Site_Access {
 	 * @param boolean $network_active Whether the plugin network active.
 	 */
 	public static function activation( $network_active ) {
+		// @todo handle network activation.
+		if ( ! get_option( 'rsa_activation_version', false ) && ! get_option( 'rsa_options', false ) ) {
+			update_option( 'rsa_activation_version', RSA_VERSION );
+		}
+
 		if ( ! $network_active ) {
 			update_option( 'blog_public', 2 );
 		}
@@ -1736,15 +1741,21 @@ class Restricted_Site_Access {
 	 */
 	public static function get_ip_from_headers() {
 		$ip              = '';
-		$trusted_headers = array(
-			'HTTP_CF_CONNECTING_IP',
-			'HTTP_CLIENT_IP',
-			'HTTP_X_FORWARDED_FOR',
-			'HTTP_X_FORWARDED',
-			'HTTP_X_CLUSTER_CLIENT_IP',
-			'HTTP_FORWARDED_FOR',
-			'HTTP_FORWARDED',
-		);
+
+		// @todo handle network activation.
+		if ( version_compare( get_option( 'rsa_activation_version', '0.0.0' ), '7.4.2', '<' ) ) {
+			$trusted_headers = array(
+				'HTTP_CF_CONNECTING_IP',
+				'HTTP_CLIENT_IP',
+				'HTTP_X_FORWARDED_FOR',
+				'HTTP_X_FORWARDED',
+				'HTTP_X_CLUSTER_CLIENT_IP',
+				'HTTP_FORWARDED_FOR',
+				'HTTP_FORWARDED',
+			);
+		} else {
+			$trusted_headers = array();
+		}
 
 		/**
 		 * Filter hook to set array of trusted IP address headers.

That would reduce the risk of a site administrator modifying the settings without being 100% clear on what they were doing.