WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 6 months ago

#23416 assigned defect (bug)

Form handlers assume $_POST elements will be strings

Reported by: tabacco Owned by: Kloon
Milestone: Future Release Priority: normal
Severity: normal Version: 3.5.1
Component: Comments Keywords: good-first-bug has-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 9 months ago.
validate comment $_POST vars

Download all attachments as: .zip

Change History (11)

comment:1 @scribu2 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?

comment:2 @tabacco2 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 2 years ago by tabacco (previous) (diff)

comment:3 @scribu2 years ago

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

comment:4 @tabacco2 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 2 years ago by tabacco (previous) (diff)

comment:5 @tabacco2 years ago

  • Component changed from Comments to Warnings/Notices

comment:6 @nacin19 months ago

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

comment:7 @boonebgorges9 months ago

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

comment:8 @Kloon9 months ago

  • Keywords has-patch added; needs-patch removed

Added some validation around the comment $_POST vars.

@Kloon9 months ago

validate comment $_POST vars

comment:9 in reply to: ↑ description @cisco878 months 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 8 months ago by cisco87 (previous) (diff)

comment:10 @DrewAPicture6 months ago

  • Owner set to Kloon
  • Status changed from new to assigned
Note: See TracTickets for help on using tickets.