Make WordPress Core

Opened 12 years ago

Closed 9 years ago

#23416 closed defect (bug) (fixed)

Form handlers assume $_POST elements will be strings

Reported by: tabacco's profile tabacco Owned by: kloon's profile Kloon
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.5.1
Component: Comments Keywords: needs-patch
Focuses: Cc:

Description

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;

Attachments (1)

23416.diff (1.2 KB) - added by Kloon 10 years ago.
validate comment $_POST vars

Download all attachments as: .zip

Change History (16)

#1 @scribu
12 years ago

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?

#2 @tabacco
12 years ago

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. And if nothing else, I think you can't be too careful with input.

Last edited 12 years ago by tabacco (previous) (diff)

#3 @scribu
12 years ago

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

#4 @tabacco
12 years ago

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.

Last edited 12 years ago by tabacco (previous) (diff)

#5 @tabacco
12 years ago

  • Component changed from Comments to Warnings/Notices

#6 @nacin
11 years ago

  • Component changed from Warnings/Notices to Comments
  • Keywords 2nd-opinion added

#7 @boonebgorges
10 years ago

  • Keywords needs-patch good-first-bug added; 2nd-opinion removed
  • Milestone changed from Awaiting Review to Future Release

#8 @Kloon
10 years ago

  • Keywords has-patch added; needs-patch removed

Added some validation around the comment $_POST vars.

@Kloon
10 years ago

validate comment $_POST vars

#9 in reply to: ↑ description @cisco87
10 years ago

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;
Last edited 10 years ago by cisco87 (previous) (diff)

#10 @DrewAPicture
10 years ago

  • Owner set to Kloon
  • Status changed from new to assigned

#11 @rachelbaker
9 years ago

  • Keywords needs-refresh added

#12 @wonderboymusic
9 years ago

  • Keywords needs-refresh removed
  • Milestone changed from Future Release to 4.4

#13 @wonderboymusic
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

#14 @johnbillion
9 years ago

  • Keywords needs-patch added; good-first-bug has-patch removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

#15 @johnbillion
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

Note: See TracTickets for help on using tickets.