Security: A $_POST from the past
Heine —Mon, 2007/12/03 - 10:16
A code construct that surprisingly survived for a long time since the initial 4.7 release is still haunting us. I'm talking about the use of confirmation forms, limping behind the times, ever since the rise of the glorious Form API (ahem). Let's see to it that this coelocanthian code becomes extinct in the next few months.
Though it's not so much the confirmation forms, as it is the way confirm_form is used:
function foo($cid) {
if ($_POST['confirm']) {
// Delete the comment.
commment_delete($cid);
}
else {
return drupal_get_form('comment_confirm_delete', $cid);
}
}
function comment_confirm_delete($cid) {
// ...
return confirm_form(array(), t('Are you sure you want to delete X'), 'some-path');
}
If you know something about FAPI, you realize that the comment delete action is taken on submission of the confirmation form without ever passing validation. And whenever a form submission doesn't pass validation before an action is being taken, it becomes vulnerable to a Cross Site Request Forgery (CSRF).
A cross site request forgery can happen when you login to your site, then visit another (malicious) site. Suppose you use a search form on the other site; the moment you submit the search, your browser will post the contents of the form to the malicious site. So far, nothing malicious happened. Now, what if the malicious site used your site's URL as parameter to the form action attribute? In that case, the browser posts the contents of the form to your own site. Before doing so, your browser would "notice" that it has a cookie for your site and happily sends it along. Which of course means that the form is posted with your credentials even though it is without intent. While a random search form won't do much on your site, a carefully crafted form and well-chosen URL will trigger actions on your site which may range from posting PHP code, deleting users or changing your password. That is why the FAPI adds a secret to every form, and verifies this so called token during the form validation step. No validation step means no token verification and thus no CSRF protection.
It is very easy to get CSRF protection by converting your forms to a proper FAPI "three stage" model where actions only happen in the submit stage. That would make a proper confirmation:
return drupal_get_form('comment_confirm_delete', $cid);
}
function comment_confirm_delete($cid) {
$form = array(
'cid' => array(
'#type' => 'value',
'#value' => $cid,
),
);
return confirm_form($form, t('Are you sure you want to delete X'), 'some-path');
}
function comment_confirm_delete_submit($form_id, $form_values) {
// Delete the comment.
comment_delete($form_values['cid']);
}
We've tried to excorcise this bug several times, and wondered why it was so persistent, until Barry Jaspan noticed that the API docs for confirm_form still recommended this practice. No need to say we immediately changed the documentation. Sorry for being asleep all this time.
So if you find yourself still inspecting $_POST on confirmation, you now know what to do. Should you maintain a module on Drupal.org that still does this, contact security@drupal.org.