POST data is evil. Let's not let it make its way through the bootstrap process unless it is signed by the FAPI. Many module maintainers are using $_POST data without knowing its implication. We could prevent many CSRF by removing data from the $_POST variable when it is not FAPI signed, or rename to something else like $_POST_RAW or something. This would not break any modules, unless it is not using the FAPI properly, in which case they will learn and fix the module. I could not come up with any use case where unsigned POST data is useful, but if there was any, POST data could be made available in $_POST_RAW.

I have run this idea through bjaspan and Khalid while at DrupalCon and they both liked it. chx suggested to completely destroy POST, but core currently still uses it.

CommentFileSizeAuthor
#1 394782_destroy_POST.patch821 bytesscor

Comments

scor’s picture

scor commented
StatusFileSize
new 394782_destroy_POST.patch821 bytes

here is a first patch. it only supports authenticated users.

gerhard killesreiter’s picture

gerhard killesreiter commented

how would this affect modules like services?

morbus iff’s picture

morbus iff
they/them
commented

Interestingly, this wouldn't stop, I think, the use of file_get_contents('php://input'), which is what xmlrpcs.inc does, failing with an error that asserts that'd get you unclean POST too:

$data = file_get_contents('php://input');
if (!$data) {
die('XML-RPC server accepts POST requests only.');
}

"php://input allows you to read raw POST data. It is a less memory intensive alternative to $HTTP_RAW_POST_DATA and does not need any special php.ini directives. php://input is not available with enctype="multipart/form-data". " How does php://input work against this change? How would you prevent modules from, instead of fixing their code (which might not use forms at all, like killes suggests with Services), to just switch to php://input?

pwolanin’s picture

pwolanin commented

Well, a couple issues with this proposal - just because the POST was generated using forms API does not mean that the module is correctly using the form values - they could be directly peeking into POST. I think D6 core still does this in a few places, e.g.:

modules/taxonomy/taxonomy.admin.inc:220: if ((isset($_POST['op']) && $_POST['op'] == t('Delete')) || isset($_POST['confirm'])) {

scor’s picture

scor commented

@Gerhard: maybe these would need to authenticated themselves beforehand (not sure how). Alternatively we could move $_POST data to $_POST_RAW. The modules using this would be easy to track down and they would need to justify why they need to use POST data in such a way.

@morbus: my proposal really aims at preventing the newbie module maintainers from using the infamous $_POST. I assume that if you're using file_get_contents('php://input'); you likely to know what you're doing.

@peter: this thread is about preventing CSRF via POST data, so if you have used the FAPI to generate it and you are peeking at POST, you are safe against CSRF (even if it's a bad practice). The fact that core still does that is outside the scope of this proposal.

pwolanin’s picture

pwolanin commented

@scor - sure, I rather meant that such a change might not be able to be readily applied to D6.

effulgentsia’s picture

effulgentsia commented
Status: Active » Closed (won't fix)

I think this is too large a change to try to get in to D7 at this point. There's issues like FAPI doesn't provide a token for anonymous users and FAPI also allows some forms to set #token to FALSE to not use a token. It would be hard for bootstrap code to determine those conditions in a secure way and deal with them appropriately, and this issue seems to be specifically about doing just that.

I'm in favor of enhancing Drupal security, but I think we need to figure out specific categories of things we're trying to prevent against. For example:

1) #submit handlers running during CSRF: this is already handled by drupal_validate_form().

2) modules doing anything with $_POST data anywhere other than a #submit handler: how do we educate developers new to Drupal that this is a big no-no unless they fully understand all security implications and are writing their own security logic into that code?

3) modules using $_POST data within a #submit handler: I kind of agree with chx that we should destroy $_POST before running #submit handlers, so that #submit handlers learn to use $form_state['values'] exclusively. But let's leave that to a separate issue, so as not to hijack this one. And at any rate, there are issues with doing that too, like the edge case of multiple forms being processed in a single page request.