Two days ago the web security company Sucuri disclosed a vulnerability in the very popular WordPress plugin, WP Statistics, which has 500,000+ active installations, and claimed it had been fixed. The post is fairly hard to follow and seems to mostly make a case that firewalls can introduce additional security risk, which is odd argument for a provider of a firewall to make.

Considering Sucuri’s recent track record of getting basic details wrong when it comes to WordPress plugin vulnerabilities, including claiming that vulnerability existed that didn’t and trashing a developer falsely, you can’t take their claims at face value. There post makes it hard to follow what exactly the issue is, but more importantly it neither provides a proof of concept or provides an explanation of how the vulnerability was supposed to have been fixed, so without doing additional work it isn’t possible to confirm if what they claimed is correct.

Other sources on data on WordPress plugin vulnerabilities didn’t do that work. Here is one that is reused by many other sources, the WPScan Vulnerability Database claiming the vulnerability was fixed:

A weekly post of vulnerabilities by a website name WPCampus also claimed it was fixed.

There also is a post on the Threatpost titled “WordPress Plugin WP Statistics Patches XSS Flaw”, which as you can guess from the title, makes the same claim.

sanitize_text_field() Isn’t Always The Right Sanitization Function

Looking at the changes made in the version that was supposed to fix this and Sucuri’s post it looked to us like the vulnerability wasn’t properly resolved.

The function get_ip() in the file /includes/classes/class-wp-statistics.php gets the IP address of visitors for the plugin and is where the vulnerability is. It looks like this in the version that was supposed to fix the vulnerability:

797
798
799
800
801
802
803
804
805
806
807
808
809
810
811
812
813
814
815
816
817
818
819
820
821
822
823
824
825
826
827
828
829
830
831
832
833
834
835
836
837
838
839
840

public function get_IP() {
 
	//Check If Rest Api Request
	if ( $this->restapi->is_rest() ) {
		$this->ip = $this->restapi->params( 'ip' );
 
		return $this->ip;
	}
 
	// Check to see if we've already retrieved the IP address and if so return the last result.
	if ( $this->ip !== false ) {
		return $this->ip;
	}
 
	// Get User Set $_SERVER HEADER
	$ip_method = self::getIPMethod();
 
	// Get User IP
	if ( isset( $_SERVER[ $ip_method ] ) ) {
		$this->ip = sanitize_text_field( $_SERVER[ $ip_method ] );
	}
 
	/**
	 * This Filter Used For Custom $_SERVER String
	 */
	$user_ip = apply_filters( 'wp_statistics_sanitize_user_ip', $this->ip );
 
	// Check If X_FORWARDED_FOR
	foreach ( explode( ',', $user_ip ) as $ip ) {
		$ip = trim( $ip );
		if ( filter_var( $ip, FILTER_VALIDATE_IP, FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE ) !== false ) {
			$user_ip = $ip;
		}
	}
 
	// If no valid ip address has been found, use 127.0.0.1 (aka localhost).
	if ( false === $user_ip ) {
		$this->ip = '127.0.0.1';
	} else {
		$this->ip = $user_ip;
	}
 
	return $this->ip;
}

The change made in the new version was to change this line in the previous version:

816

$this->ip = $_SERVER[ $ip_method ];

To this:

816

$this->ip = sanitize_text_field( $_SERVER[ $ip_method ] );

So it sanitizes the value of the IP address of the visitor using sanitize_text_field(), where it previously wasn’t (unless there are multiple IPs). There is a limitation to using sanitize_text_field() to sanitize the value, which Sucuri should be aware of, as a post in May attributed to the same author as the post about this vulnerability explained:

The name of our user was sanitized using the sanitize_text_field method, which performs the following actions per the WordPress documentation:

• ​​Checks for invalid UTF-8
• ​​Converts single < characters to entities
• ​​Strips all tags
• Removes line breaks, tabs, and extra whitespace
  ​​Strips octets

​​You might notice it removes the tags, but it does not mention being safe as attribute. ​

So if the value of the visitor IP address is output in an attribute there would still be a problem here.

While Sucuri didn’t provide a proof of concept, we had no problem coming up with one and testing things out. When we did that we found that the value in fact is output as an attribute. The location we found right away in our testing was on the plugin’s Visitor page. The underlying line of code that causes that is in the file /includes/log/last-visitor.php:

184

$ip_string = "<a href='" . WP_Statistics_Admin_Pages::admin_url( 'visitors', array( 'type' => 'last-all-visitor', 'ip' => $items->ip ) ) . "'>{$items->ip}</a>";

That outputs the value in the “href” attribute of an “a” tag.

If malicious JavaScript is set to the IP address that would be output on that page, which is persistent cross-site scripting (XSS) since the value is stored in the WordPress database.

What would be better would be to use to sanitizes this would be the existing code in that function for validating an IP address, which currently only is used if there are multiple IP addresses.

We have notified the developer that this hasn’t actually been resolved.

The good news here is that this wouldn’t impact most websites using the plugin because by default the IP address value cannot be set to malicious JavaScript, though for those it does, they have had multiple entities provide further exposure of the vulnerability while leading people to believe it has been fixed.

With our service we actually test out vulnerabilities before adding them to our data set so we avoid our customers getting misled like this.