Problem/Motivation

In Drupal 8 \Drupal\Component\PhpStorage\FileStorage::htaccessLines() contains

# If we know how to do it safely, disable the PHP engine entirely.
<IfModule mod_php5.c>
php_flag engine off
</IfModule>

But that doesn't cover PHP7.

History

This was originally submitted via https://security.drupal.org/node/add/project-issue/drupal on http://security.drupal.org by mkalkbrenner and Sweetchuck. After investigation and discussion, it was decided that this could be a public issue because there are other protections in place, so this is a hardening and not a vulnerability.

if you are allowing overrides the FILES directive is going to be obeyed - https://httpd.apache.org/docs/2.4/mod/core.html#allowoverride

This is actually the third line of defense. The SetHandler at the top of the .htaccess in the files folder also blocks php execution. The .htaccess file in the docroot will also cause any php file not in its whitelist to throw a 403.

It is not a problem to put stanzas for all 3 possible module identifiers in: mod_php, php5_module, php7_module.

There is not a generic identifier like mod_php that would cover all cases.

https://github.com/php/php-src/blob/PHP-5.5.38/sapi/apache2handler/mod_p...
has php5_module

https://github.com/php/php-src/blob/master/sapi/apache2handler/mod_php7.c
has php7_module

The php_flag engine off protection was originally added because it was easy to add, and because it protects against some unusual theoretical configurations that the other code in .htaccess doesn't protect against. But it's only there as a backup... and even on PHP 5 it only runs for sites using mod_php, not all sites.

Proposed resolution

Add another section for php(no version) and php7 so it will be like

<IfModule mod_php5.c>
  php_flag engine off
</IfModule>
<IfModule mod_php7.c>
  php_flag engine off
</IfModule>

Remaining tasks

  1. Add credit (from the s.d.o issue) for: mkalkbrenner, Sweetchuck, pwolanin, David_Rothstein, sidharrell

User interface changes

No

API changes

No

Data model changes

No

CommentFileSizeAuthor
#9 2820611.D8_mod_php7_engine_off.9.patch629 bytesYesCT
#9 interdiff.2820611.7.9.txt540 bytesYesCT
#7 D8_mod_php7_engine_off.patch491 bytesmkalkbrenner
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

YesCT created an issue. See original summary.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

(I have some more details: a patch, and people to credit, that I'll add tomorrow.)

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes

added more history from comments on the s.d.o and people to be added for credit.

YesCT’s picture

YesCT CreditAttribution: YesCT commented
YesCT’s picture

YesCT CreditAttribution: YesCT commented
David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein as a volunteer commented
Issue tags: +Needs backport to D7
mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
FileSize
D8_mod_php7_engine_off.patch491 bytes

Here's my original patch for the issue.

mkalkbrenner’s picture

mkalkbrenner
Primary language German
Location 🇩🇪
CreditAttribution: mkalkbrenner at bio.logis Genetic Information Management GmbH commented
Status: Active » Needs review
YesCT’s picture

YesCT CreditAttribution: YesCT commented
FileSize
interdiff.2820611.7.9.txt540 bytes
2820611.D8_mod_php7_engine_off.9.patch629 bytes

Here is a patch with the other suggestion from the s.d.o issue, another php mod but without the version number.

Next I'm going to look into what situations that would benefit.

Note, I looked at other .htaccess and sections to see about the white space between these hunks and it seems like white space is before a comment (so we could put specific comments before each, or when similar hunks are under one comment heading, there are no new lines between them.

For example:

From the same FileStorage.php file, these have their own comments

# Deny all requests from Apache 2.4+.
<IfModule mod_authz_core.c>
  Require all denied
</IfModule>

# Deny all requests from Apache 2.0-2.2.
<IfModule !mod_authz_core.c>
  Deny from all
</IfModule>

From the main .htaccess, these have no newline

# Protect files and directories from prying eyes.
<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\.(?!well-known).*|Entries.*|Repository|Root|Tag|Template|composer\.(json|lock))$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig|\.save)$">
  <IfModule mod_authz_core.c>
    Require all denied
  </IfModule>
  <IfModule !mod_authz_core.c>
    Order allow,deny
  </IfModule>
</FilesMatch>

So, if each fits under the comment, and doesn't need its own comment, I think no newline is necessary.

YesCT’s picture

YesCT CreditAttribution: YesCT commented

I pinged Sweetchuck via their contact form asking them to post why they suggested adding mod_php.c

I searched for a link to the source for that, hoping to find some docs, and my searches didn't find it.
I did find https://github.com/orieg/ganglia-svn.r2340-mod_php/blob/master/gmond/mod... but that repo doesn't look like the best source.

I have a suspicion that if we are only supporting php5 and php7, that we dont need the mod_php.c...
Anyone have info on that?

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh at Cognizant Softvision commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

mod_php.c is not a valid module.

Patch #7 passed my local testing. As noted in the description, the main .htaccess file and the SetHandler directive already prevent PHP files in the files directory from being executed. This patch just provides PHP 7 with the additional level of protection that PHP 5 already enjoys. There is no need for additional automated tests or for existing .htaccess files to be updated.

Darren Oh’s picture

Darren Oh
Primary language English
Location Lakeland, Florida
CreditAttribution: Darren Oh at Cognizant Softvision commented
Issue summary: View changes

alexpott credited Sweetchuck.

alexpott credited pwolanin.

alexpott credited sidharrell.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed fd194bb and pushed to 8.8.x. Thanks!

I've committed #7 since #10 doesn't point to anything that says 

+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
@@ -84,9 +84,15 @@ public static function htaccessLines($private = TRUE) {
+<IfModule mod_php.c>
+  php_flag engine off
+</IfModule>

is correct - and if it was then I guess we'd have it already.

Backport issues are now handled via a separate issue.

  • alexpott committed fd194bb on 8.8.x 
    Issue #2820611 by YesCT, mkalkbrenner, Darren Oh, pwolanin,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mcdruid’s picture

mcdruid
Location 🇬🇧🇪🇺
CreditAttribution: mcdruid commented

#3055805: [D7] file.inc generated .htaccess doesn't cover PHP 7 for the backport.