Simplify module form structure and fix bugs when Suhosin is used

It would be better to figure out why the brackets cause problems in the first place, and fix the problem at source, rather than just working around it.

Adding some related issues.

At a guess the common factor here is that when the "name" attribute of the checkboxes contains parentheses, e.g. "modules[Core (Experimental)][inline_form_errors][enable]", for some reason this particular provider is doing something non-standard with these, and so the submit handler always thinks the checkboxes are deselected when the form is posted back.

I made a small module (see attached file "repair_packagename.zip") to alter the package names by temporarily removing the round brackets in the info-files:

function MYMODULE_system_info_alter(&$info, $file, $type) {
  if($type == 'module') {
    if ( strpos( $file->info['package'], '(' ) !== false ){
      $file->info['package'] = str_replace('(','',$file->info['package']);
      $file->info['package'] = str_replace(')','',$file->info['package']);
    }
  }    
}

This is not a solution, just a workaround.

Could you ask them what security issue they think they are mitigating by adding this setting? It's not immediately obvious how brackets could be a problem in array keys.

Andrej, did you have them remove the brackets from your black list? Just tried it with my host, and Drupal started doing some very strange things (content being deleted on node save, for example). Perhaps my host didn't adjust the rule correctly, but nevertheless it caused a very stressful few minutes.

Sorry, I´m wrong.

Hello,

I can confirm that removing the brackets "( )" from suhosin.request.array_index_blacklist allowed me to install modules with brackets in their group names.

Tested with Drupal 7.x and 8.x .

Thanks,

•• matt

We ran into this issue with Ubercart (as have many others). It's apparently been an issue for weeks and we had no idea since you get no notice that your modules have been shut off because of it. We found out because it was discovered that people weren't paying sales tax on their items and people were not able to download their e-pubs.

As more and more servers get their software upgraded, this is going to become an even bigger problem. And not everyone is going to have a host that will be willing to make the change for them.

I was able to get my host to fix it. I pointed them to this page and explained to them what was going on.

And this is what they said:

I have made the modification and the suhosin.request.array_index_blacklist has been changed from this:

suhosin.request.array_index_blacklist => '"+<>;()

To this:

suhosin.request.array_index_blacklist => '"+<>;

I too have experienced the problem described herein.
It took me nearly a week to track down this bug report!
Thank goodness this serious problem has at least been brought to the attention of Drupal Core's maintainers.

For additional information, read the following Drupal bug reports:

Drupal Bug Report - Drupal Core - Commerce Contrib Module group names with brackets (parentheses) cause problems
https://www.drupal.org/node/2665152

Drupal Bug Report - Drupal Commerce - Commerce modules get disabled randomly
https://www.drupal.org/node/2448461

The hack workaround for this serious Drupal code bug, a bug that would surely stump the average Drupal user, is that every single .info module configuration file associated with every module wherein the "package" name uses parentheses, square brackets, or braces, to name the module group (called a "package") must be edited to remove the parentheses, brackets, braces, or whatever, from its "package name".

For example, if the package line in a module's *.info file contains the text"

package = Commerce (contrib)

that line must be changed to read:

package = Commerce contrib

This *.info file modification hack must be performed on every module where the problem occurs. There are dozens of them.

This problem in Drupal Core has apparently been known for years, but nobody involved in core has bothered to fix it.

I also posted a workaround that you can do via your web host that allows the modules to all continue to work.

Thanks jsimonis

So, are you suggesting that people try to convince their server providers to make changes to their PHP configurations?

Trying to persuade many server providers to do that would be like trying to persuade the President to push the big red button right now.

What potential intrusion scenario are server hosts trying to prevent by blocking the parentheses "( and )" characters anyway?

If one were to change a local XAMPP PHP environment, would you please provide a more detailed step by step PHP installation modification recipe for our local PHP installations, to mitigate this problem?

@sprite
Guessing this is the backstory https://www.sektioneins.de/blog/14-11-06-drupageddon-vs-suhosin.html

@everyone
Renaming the issue to clarify the incompatibility with Suhosin.

We have two ways forward here:
1) A hook_requirements() telling people to reconfigure "suhosin.request.array_index_blacklist"
Telling hosts that they need to change their configuration could be potentially problematic, but I'm guessing no host wants to be labeled as "incompatible with Drupal".

2) Forbidding parenthesis in module package names.
This wouldn't be easy, but it's doable.

The problem with #2 is that we have no guarantee that the modules page is the only one affected by this Suhosin behavior.
So we'd probably need a much wider policy regarding parenthesis, which isn't doable. Therefore, we fallback to #1.

Why does the package name have to be in the checkbox form item key at all? Can we just remove it? Or if we can't remove it, can we sanitize the package name slightly before it goes into the key?

That's a good point.

Thus far I haven't had any issues with web hosts not being willing to make this small change. As was posted above, it all stems from Drupalgeddon. Had I asked them to remove it all from the settings, they likely would have said no, but just the parenthesis wasn't an issue since it's so commonly used.

I do wish that another solution would come about, but at least by doing this you're covering everything that has the () and not just the modules page.

FYI, for those in need of a solution quickly, tfranz's module from #8 above worked for us until we were able to get the Suhosin settings changed on our server.

https://www.drupal.org/node/2665152#comment-11016175

Thanks,

•• matt

tfranz's module from comment #8 saved the day for me.
Thanks !

I replaced info files in any modules that had brackets in their module grouping (project) name with ones where the brackets had been removed and things seem to be working fine. The same modules are enabled as before the info files were changed.

This is a very easy mod, so unless there's some disaster involved in doing otherwise, I strongly support suggestion above, that next release of any such modules has similarly changed info files, and comes with notice that this has been changed.

Considering that this is a SuHosin addition to prevent injection attacks, I suggest the parenthesis be nixed in package names instead of removing the protection from a server. Also that our standards be updated to state that package names can not include parenthesis.

While we wait on a decision about how best to handle the issue long term:

1. the module posted by tfranz can be added to a site and keep things working without changing code in any modules
2. the script posted by Andrej Galuf can be used to change the code of site modules quickly but would need to be re-run anytime an update is done
3. the script posted by Andrej Galuf can also be used to change the code of modules in a dev environment and committed to the repository for a next release (nixing the parenthesis will not be too painful)

We experienced this issue on a new server we manage, PHP 5.5.36 with latest SuHosin and about 40 Drupal sites. We prefer the improved server security.

Hi guys,

I worked around it by removing brackets on modules I really needed, but then I'm worried about other websites on that server which may encounter this issue sooner or later. The funny thing is I upgraded a commerce distro on the same host and nothing odd happened. With ubercart, however, everything went down.

My host told me to change the PHP version from 5.5 to 5.6 in cpanel and disable SuHosin in the process. I run multiple sites, and don't see that as an optimal solution.

I encountered the issue upgrading from 7.44 to 7.52 (the first security upgrade on core since 7.44) so I think this thread will be visited a bit more often now as others updating on security releases will encounter the same. My host didn't alter anything, so one way or another the new core must have something to do with it with it I think?

But just removing any brackets in future releases is a great fix really...

Sincerely,

Simply "forbidding" parentheses in package names is not a solution, it's just a bandage which avoids one particular common usage and fails to fix the underlying problem. Consider the suhosin rule in question:

suhosin.request.array_index_blacklist => '"+<>;()

This means that any package name containing ANY of the characters '"+<>;() is going to experience this exact same problem. So just forbidding () is not sufficient. And even if you forbid ALL those characters in package names, the problem will crop up again the next time this suhosin rule is changed and new characters are blacklisted.

Note that the suhosin rule blacklists those characters when they appear in array indexes, so the reason this is a problem is because Drupal core surrounds the package name with square brackets in the name attribute of the input tag, which make the package name look like an array index.

A proper fix can't involve just restricting package names because it's not just on the module install page that this is a problem. In reality, any form element that uses one of these suhosin-restricted characters in a form array index can potentially fail upon submission. The problem is evident on the module install page primarily because of the consequence of the error - not being able to install a module - and because everyone uses the module install page therefore an error on this page affects more people. Likewise a fix (changing package names) that ONLY applies to the module install page may help a lot of people but it does not fix the underlying problem.

Much hassle could be eliminated if drupal modules avoided aggravating the problem meanwhile, no?

This has bitten us on Drupal.org's events site. We enabled a module via the modules page and it disabled all of our payment processors on the day we launched Drupalcon Baltimore.

Well in Drupal 8 core ships with package: Core (Experimental) for all experimental modules - eg. migrate.

I think we need to address this in the form API since that is the only way we can have a generic fix. #28 is correct that this can happen with any form element since it is about array keys found in the POST data.

So actually there is no reason to have the package in the form element name. This is going to break a lot of tests. As this is a problem in Drupal 8 we need to fix this there first. Also as a form structure change we can only fix this in 8.3.x. This is going to break a lot tests...

Fixing some tests and fixing the logic in \Drupal\system\Form\ModulesListForm::buildModuleList()

@alexpott this looks like hiding a problem as #28 states, what if tomorrow someone will decide to change this blacklist?
Maybe just use a sort of coding/decoding for form elements' keys taking into account this setting, BTW we already use to replace "-" with underscore in submit buttons

Is it possible to understand cons of that blacklist? There's html5 attributes & value restrictions https://html.spec.whatwg.org/multipage/syntax.html#attributes-2 so why not just hook_requirements() as #19 suggests?

@andypost well in this instance there actually is no point putting this information in the form element names as #34 proves so it's a simple fix. Doing a generic fix for this will probably be impossible till the next major version of Drupal because it is a massive and complex change to FAPI. Doing the find replace on form element names will break #states for example.

I've tested checkboxes in the field API and we use the machine names for form element names so fortunately we're good to go there. I thought about just doing a replace on the package when it is used as a form element name to result in a "lighter" fix but I think changing the form is the better solution so I think we should do that. Whatever we do there is a certain amount of disruption and many of the places changed by this patch would still have to change. We've already documented that form element names are not API.

Also just confirmed that admin/modules/uninstall does not have the same problem because the package name is not used - phew :)

Changing form structure may need change record but maybe file separate issue for form structure and use this one for documentation updates?
I guess some contrib can be affected like added core issues, I remember DS module and features faced with that

@andypost well this issue has always been about not being able to install modules due to Suhosin and this is what is fixed here. I agree that we should have a generic plan issue to try to make Drupal as Suhosin compatible as possible. But I don't see why we should re-scope this issue.

@alexpott asked me if I know any tests in contrib that install modules in the UI because this would break those tests.

I found a few modules (in the list of modules that I have checked out locally) with such a test: redis, search_api, search_api_solr and maybe another simplenews.

Will be an easy fix for all of them, so I think that shouldn't prevent us from doing this.

In regards to committing this to 8.2.x or not, it would actually make it easier for those modules. d.o defaults to running tests with 8.3, so we would have to fix it *now* anyway. And then it would actually be a (small) problem if those tests would no longer pass against 8.2.x, but we already have such cases as well.

Committing to 8.2.x is a bigger problem for modules that actually alter that page, like module_filter, for those this is likely a problem and could actually break their functionality, not just their tests.

The issue summary is out of date.

Created the CR: https://www.drupal.org/node/2842642

patch does exactly what CR about
PS: maybe just better naming - Simplify modules form structure to support Suhosin

I agree with #38 and I think the patch there is a reasonable approach to solving the immediate problem. RTBC.

This patch is a big change for a stable release. For Drupal 7 maybe we can just replace the offending characters in the form key, but leave the form structure the same? That way there's no effect on most modules. (Or maybe even examine the Suhosin setting, if present, and only do the replacement based on what we find there?)

I like the idea in #9 of just having Drupal override the Suhosin setting, but I'm not sure it's possible in all setups - see https://suhosin.org/stories/configuration.html#suhosin-perdir.

Also this issue isn't really a security improvement - it is just working around an overzealous setting in some server configurations.

Committed/pushed to 8.4.x and cherry-picked to 8.3.x. Thanks!

Someone please open the 7.x backport issue. I've tagged for release notes.

I think this broke HEAD: https://www.drupal.org/pift-ci-job/594707

Somehow this change has cause the admin/modules form to only work when the Text module is installed ??!?!?!

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -145,11 +145,11 @@ public function buildForm(array $form, FormStateInterface $form_state) {
     // Iterate over each of the modules.
-    $form['modules']['#tree'] = TRUE;
     foreach ($modules as $filename => $module) {
       if (empty($module->info['hidden'])) {
         $package = $module->info['package'];
         $form['modules'][$package][$filename] = $this->buildRow($modules, $module, $distribution);
+        $form['modules'][$package][$filename]['#tree'] = TRUE;
       }
     }

This is the meat of the change... the problem is cause because of the filter field which is called 'text' - the same name as the module. Nice.

So we can just re-parent the module field to not have the package as part of the form element name.

Using #parents is good here, if anyone alters this form we don't want any risk of clashing with 000s of modules. Moving to RTBC since it's a small change really, most of it's updating the tests again.

It would be good to get a live test with Suhosin.

@cilefen it is not too tricky to do. Just install the extension following the instructions here. https://suhosin.org/stories/install.html

With the extension enabled you can not install an experimental core module such as workflows on HEAD. With the patch you can. Here's a screen capture of me trying to install an experimental module with and without the patch and suhosin extension installed http://recordit.co/TMM5uaTXu0

I've manually tested this to my satisfaction. +1

Just added a change record. https://www.drupal.org/node/2851653

Thanks Alex for the new approach. Committed 91736ca and pushed to 8.4.x. Backported as db95ca8 and pushed to 8.3.x.

change record updated (8.4 changed to 8.3) and published

Reopening for backport to 7.x.