WordPress.org

Make WordPress Core

Opened 14 months ago

Last modified 3 months ago

#23416 new defect (bug)

Form handlers assume $_POST elements will be strings

Reported by: tabacco Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.5.1
Component: Comments Keywords: 2nd-opinion
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;

Change History (6)

comment:1 scribu14 months 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 tabacco14 months 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 14 months ago by tabacco (previous) (diff)

comment:3 scribu14 months ago

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

comment:4 tabacco14 months ago

The whole reason attackers use fuzzers is to find unhandled warnings they can exploit.

Version 0, edited 14 months ago by tabacco (next)

comment:5 tabacco14 months ago

  • Component changed from Comments to Warnings/Notices

comment:6 nacin3 months ago

  • Component changed from Warnings/Notices to Comments
  • Keywords 2nd-opinion added
Note: See TracTickets for help on using tickets.