Form handlers assume $_POST elements will be strings

[tabacco]

I'm running Wordpress 3.5.1 on PHP 5.3.

This example is from lines 50-53 of wp-comments-post.php:

$comment_author       = ( isset($_POST['author']) )  ? trim(strip_tags($_POST['author'])) : null;
$comment_author_email = ( isset($_POST['email']) )   ? trim($_POST['email']) : null;
$comment_author_url   = ( isset($_POST['url']) )     ? trim($_POST['url']) : null;
$comment_content      = ( isset($_POST['comment']) ) ? trim($_POST['comment']) : null;

The issue is that If your post data contains something like:

author[]=foo&author[]=bar

or

comment[]=foo

Then the corresponding values in $_POST will be arrays, not strings, causing an E_WARNING.

There seem to be a number of other places where $_POST data is passed directly to PHP string functions as well, causing potential warnings. These should be handled gracefully by checking the type of the element being grabbed from $_POST first:

$comment_author       = ( isset($_POST['author']) && is_string($_POST['author']) )   ? trim(strip_tags($_POST['author'])) : null;
$comment_author_email = ( isset($_POST['email']) && is_string($_POST['email']) )     ? trim($_POST['email']) : null;
$comment_author_url   = ( isset($_POST['url']) && is_string($_POST['url']) )         ? trim($_POST['url']) : null;
$comment_content      = ( isset($_POST['comment']) && is_string($_POST['comment']) ) ? trim($_POST['comment']) : null;

[scribu]

The fact that WP doesn't expect an array in $_POST['author'] isn't a bug in itself, since comments can only have a single author. It seems to me that the bug lies in whatever code generates author[]=foo&author[]=bar.

This is an old debate, though: how far should we go in preventing notices?

[tabacco]

No code generates it, but that doesn't mean it can't happen with fuzzers or pen testers. And in this case it's not a notice, it's a warning. You can make the case for not worrying about notices, but I do think warnings should be protected against.

[scribu]

So, what would be the practical benefit if people running fuzzers wouldn't get a warning?

[tabacco]

The whole reason attackers use fuzzers is to find unhandled warnings and errors they can exploit. Not properly checking your inputs is a vulnerability waiting to happen. Just because it's not exploitable in this case doesn't mean it isn't somewhere else in the WP code base, or that it won't be in a future update.

[Kloon]

Added some validation around the comment $_POST vars.

[Kloon]

validate comment $_POST vars

[cisco87]

What about a function like this which allows you to sanitise an array recursively and to apply filters to each of the keys to strip_tags or whatever?
Whether its an array or a value it just iterate and recurse allowing you to create filter for all the values in an array like for authors[] or the key author just hooking "filter_array_authors" or "filter_array_author"
and you can call it with
$_POST = recur_filter($_POST);

function recur_filter($vars, $pkey = null) {
	$filter = "filter_array";
	
	if(is_array($vars)) {
		foreach($vars as $key => $var) {
			$m_filter = $filter;
			if(!empty($pkey)) {
				$m_filter .= "_{$pkey}";
			}
			else if(!is_numeric($key) && !empty($key)) {
				$m_filter .= "_{$key}";
			}
			
			if(is_array($var)) {
				$arg = "";
				if(!is_numeric($key) && !empty($key)) {
					$arg = $key;
				}
				else if(!empty($pkey)) {
					$arg = $pkey;
				}
				$vars[$key] = recur_filter($vars[$key], $arg);
			}
			else {
				$vars[$key] = apply_filters($m_filter, $var);
			}
		}
	}
	else {
		if(!empty($filter)) {
			$filter .= "_{$pkey}";
		}
		$vars = apply_filters($filter, $vars);
	}
	return $vars;
}

Replying to tabacco:

I'm running Wordpress 3.5.1 on PHP 5.3.

This example is from lines 50-53 of wp-comments-post.php:

$comment_author       = ( isset($_POST['author']) )  ? trim(strip_tags($_POST['author'])) : null;
$comment_author_email = ( isset($_POST['email']) )   ? trim($_POST['email']) : null;
$comment_author_url   = ( isset($_POST['url']) )     ? trim($_POST['url']) : null;
$comment_content      = ( isset($_POST['comment']) ) ? trim($_POST['comment']) : null;

The issue is that If your post data contains something like:

author[]=foo&author[]=bar

or

comment[]=foo

Then the corresponding values in $_POST will be arrays, not strings, causing an E_WARNING.

There seem to be a number of other places where $_POST data is passed directly to PHP string functions as well, causing potential warnings. These should be handled gracefully by checking the type of the element being grabbed from $_POST first:

$comment_author       = ( isset($_POST['author']) && is_string($_POST['author']) )   ? trim(strip_tags($_POST['author'])) : null;
$comment_author_email = ( isset($_POST['email']) && is_string($_POST['email']) )     ? trim($_POST['email']) : null;
$comment_author_url   = ( isset($_POST['url']) && is_string($_POST['url']) )         ? trim($_POST['url']) : null;
$comment_content      = ( isset($_POST['comment']) && is_string($_POST['comment']) ) ? trim($_POST['comment']) : null;

[wonderboymusic]

In 34274:

In wp-comments-post.php, sanity check a few of the comment inputs that are expected to be a string beford calling string-only functions on them.

Props Kloon.
Fixes #23416.

[johnbillion]

r34274 broke the error message that's shown when the user enters an invalid email address, because the email address is now stripped if it's not invalid, causing the please fill the required fields (name, email) message to show, instead of please enter a valid email address.

Related: #34059

[johnbillion]

In 34693:

Revert the introduction of validation of the comment author's email address when sanity checking input in wp-comments-post.php (introduced in r34274). This allows a problematic email address to fall through to the correct validation routine and show the "Please enter a valid email address" as expected.

Fixes #23416