Drupal 8 .htaccess rule to prevent php file access can be easily bypassed

Does our web.config has the same issues?

moved to https://security.drupal.org/node/155254 as per https://security.drupal.org/node/155252

This rule only exists in D8 so I think this can be public. Re-publishing.

Here's a patch that prevents access to /core/vendor/behat/mink/driver-testsuite/web-fixtures/headers.php/1

Also this has nothing to to with vendor specifically... core/lib/Drupal.php/1 is just as accessible as /core/vendor/behat/mink/driver-testsuite/web-fixtures/headers.php/1.

Trying some new patches. Not sure what I did wrong.

Fixing Simpletest's http.php and https.php

What happens with web.config just curious here ...

@dawehner who as an IIS server kicking about?

No idea. Maybe some of the commerce people, as they have developed some mssql driver.

Would query strings break the protection without this patch?

^^ no

Patch looks good, I tested this manually with as many variations as I could based on the initial issue report and couldn't reproduce after applying #17. +1 for RTBC.

I'm not sure that we should be using the rewrite directive to deny access to php files. This change will prevent access to a path alias like "htttp://drupal.org/what.php/5.6-is-best". mod_rewrite is not matching on files it is matching on the requested url. It does not care if the underlying file exists or not - that's what the FileMatch directive is for.

Here's an approach with FilesMatch, see what the bot thinks.

I think this approach is slightly clearer and easier to grok.

I definitely agree the rewrite approach should be ditched. This basically gives us a list of possible front controller names that is simple to understand and alter.

A slight refinement.

We can go further!

A change for my local env crept in.

Missed update.php

#33 is awesome, much simpler to read and using FilesMatch makes sense. +1 from me.

+++ b/.htaccess
@@ -132,22 +143,6 @@ AddEncoding gzip svgz
-  # Allow access to Statistics module's custom front controller.
-  # Copy and adapt this rule to directly execute PHP files in contributed or
-  # custom modules or to run another PHP application in the same directory.

At least this documentation is certainly kinda lost ...

@dawehner good point.

Cool

Added tests for more files and anode aliased to test.php and test.php/test.

This looks like it might actually work!

@pwolanin++

I can confirm this works for htaccess. Can we build in a status check for people who may not have htaccess setup correctly. Something that says their site is insecure, like the update notices?

> Can we build in a status check for people who may not have htaccess setup correctly. Something that says their site is insecure, like the update notices?

If you do, you need to do it with JS and/or PHP-masquerading-as-image but server-to-server requests have been attempted and failed. There are too many variations to make it work reliably. So please do not try it again :)

Rerolled.

I've looked at trying to do @mlhess's suggestion in #48 in light of #51 and #52. It's going to be quite problematic to add a requirement in system_requirement that only appears when rendering through a browser. hook_requirements() is supposedly for CLI as well. I think the best way forward is to open a followup issue for discussing the best way to do this. Which I've created - #2510194: Use JS or PHP-masquerading-as-image to test .htaccess on admin/reports/status

@pwolanin - you're right going with ($|/) - I was trying to reduce complexity :). I've also added test coverage to ensure that a file named foo.php-info.txt is not blocked.

And I've also completed test coverage for the FilesMatch directive in .htaccess and found a bug :)

<FilesMatch "\.(engine|inc|install|make|module|profile|po|sh|.*sql|theme|twig|tpl(\.php)?|xtmpl|yml)(~|\.sw[op]|\.bak|\.orig|\.save)?$|^(\..*|Entries.*|Repository|Root|Tag|Template)$|^#.*#$|\.php(~|\.sw[op]|\.bak|\.orig\.save)$">

Is the current directive in D8 HEAD (and D7). This blocks access_test.module.save and access_test.module.orig but (mistakenly) permits both access_test.php.save and access_test.php.orig - it does however block access_test.php.orig.save (lol).

Care needs to be taken if this patch needs rerolls since our git is configured to ignore .swp , ~, and .orig files. You have to add them with a -f.

As @pwolanin says:

if it's not tested it doesn't work

FYI: The bug wrt to .php.orig.save was added by #1907704: Restrict temporary files created by text editors

#2508025: .htaccess broken regex for \.orig and \.save

This is, like, adding emacs backups or something to core?

Edit: never mind.

Disregard #62. That is an intended part of the test.

One comment typo, fixed on commit.

+++ b/core/modules/system/src/Tests/System/HtaccessTest.php
@@ -54,21 +93,52 @@ protected function getProtectedFiles() {
+    // Test that is it possible to have path aliases containing in .php.

s/in //

Committed/pushed to 8.0.x, thanks!

Moving to 7.x for backport of the relevant bits.

Major for 7.x - we don't have .php file protection there at all, so it's mainly firming up the .orig etc. blocking which is hardening only.

In that typo comment referenced above shouldn't we also s/is it/it is/? Sorry to bring it up.

@Cottser, yes, yes we should.

Also https://www.youtube.com/watch?v=YIyDXCYe7lM

#2511348: HtaccessTest::testFileAccess() has a documentation typo

I found this bug

Ticking the @DongIT checkbox in the credit list.

Does this affect Nginx as well?

@Jorgee nginx has the same security considerations as apache does wrt to access to .php files. Drupal does not supply nginx configuration scripts. But a good place to start to see if the nginx configuration you are using has the expected security provisions is to run HtaccessTest.

I believe this is the same issue I ran across recently. The issue is that the regex is not escaped properly. The rule needs to be written as follows:

RewriteRule "^(.+/.*|autoload)\.php($|//)" - [F]

The regex isn't escaped properly. If it isn't escaped the regex will be be far too broad and will match almost anything. It will NOT match "autoload" but many different strings. I haven't checked any of the patches yet in this thread but I am hoping it has already been addressed.

This issue still affects the latest version of D8

Regarding backport to Drupal 7 I have looked over the patch that was committed to Drupal 8. The change to the FilesMatch line already exists in Drupal 7 and the file autoload.php is not in drupal 7. All that is left are the tests and I'm not sure it's worth the effort.

If I'm correct the backport is not needed or a new issue to specifically add tests for .htaccess in drupal 7 should be created.

Let's handle the backport in the child issue here: #2779833: Fix Drupal 7 .htaccess to protect .orig and .save files from view. It is not possible to completely backport this fix as D7 does not have PHP files execution protection (this is being discussed here: #1587270: Forbid execution of PHP files in subfolders by default (except those needed by core)), so only fix to .orig and .save extensions can be backported + test.

So I think it will be the best to close this as fixed for D8.