Opened 12 years ago
Closed 9 years ago
#23416 closed defect (bug) (fixed)
Form handlers assume $_POST elements will be strings
Reported by: | tabacco | Owned by: | 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)
Change History (16)
#2
@
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.
#3
@
12 years ago
So, what would be the practical benefit if people running fuzzers wouldn't get a warning?
#4
@
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.
#7
@
10 years ago
- Keywords needs-patch good-first-bug added; 2nd-opinion removed
- Milestone changed from Awaiting Review to Future Release
#8
@
10 years ago
- Keywords has-patch added; needs-patch removed
Added some validation around the comment $_POST vars.
#9
in reply to:
↑ description
@
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[]=baror
comment[]=fooThen 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;
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 generatesauthor[]=foo&author[]=bar
.This is an old debate, though: how far should we go in preventing notices?