Forbid execution of PHP files in subfolders by default (except those needed by core)

In all honesty, I don't see the security risk for this issue.

Most .php files that should not be directly called are:

• Template files with some HTML and variable mashing/printing
• Class files defining a single class
• Module files defining functions
• D8 config JSON files, starting with die()

None of the above files do something when directly executed except for perhaps a template file calling a function it doesn't know. In fact, none of the above files should call any Drupal related function outside of a class or function scope (except maybe for template files).

It would be bad practice to have any .php file in Core that could actually expose data if run on its own.

I understand your reasoning and agree with it.

This would, however, require more documentation than just the comment in the .htaccess file?
And correct me if I'm wrong, but the rewrite condition looks buggy as well.

RewriteCond %{REQUEST_URI} !^.*/core/[^/]*\.php$

• Allows /core/install.php (good)
• Forbids /core/modules/statistics/statistics.php (bad?)
• Allows /sites/all/modules/mymodule/core/evil.php (bad?)

Looks good to me.

Sorry this doesn't make sense to me. Contrib modules also ship with custom .php files sometimes doing similar things to statistics.php etc., and uncommenting this would unexpectedly break them.

Again, looks good to me.
Especially with those extra comments.

Will not mark as RTBC myself until at least one more person has reviewed this.

I'd be fine with enabling this by default with a whitelist for core but the commented out approach is an improvement.

Just a follow up on this issue. The patch still applies and it's RTBC.

I don't really see this as a feature... it is just helpful! Retitling to say what we are actually doing.

Committed bc44cbd and pushed to 8.x. Thanks!

Reopening because I think we should actually uncomment this by default. It provides a great security improvement and if we leave it commented out then the people who need it most (those who are likely to make a server security configuration mistake) are least likely to get the protection. This kind of protection would have helped to protect drupal.org against the security breach that was found in May 2013. How can we leave something like that uncommented?

Contrib modules also ship with custom .php files sometimes doing similar things to statistics.php etc., and uncommenting this would unexpectedly break them.

Those modules should have instructions for how to modify .htaccess. People who need the performance/scalability benefits of calling a .php file directly are exactly the kind of people who are most comfortable editing .htaccess (or more likely: nginx or the httpd.conf itself).

My strong opinion is that the .htaccess in core should have active (i.e. not commented-out) rules that support core modules and has comments about how to set it up for others.

Happy to provide a patch if folks agree.

Making title match current proposal.

I agree that this would be much more beneficial if it was not commented out by default.

In my experience modules with directly callable php files are mostly ones that need to avoid the Drupal bootstrap for performance reasons, such as chat backends or ad servers. If this is the case, then I think it is reasonable to expect users to edit .htaccess to use this, particularly as they can nearly always provide a Drupal callback as a fallback.

If we feel this is too onerous, one alternative would be to provide a whitelist based on a naming pattern - e.g. files named with a specific prefix, or inside a certain canonically named directory (e.g. drupal-php-chatserver.php or drupal-php/chatserver.php) - these could live inside the module directory and avoid needing a specific .htaccess whitelist for each.

This would be weaker from a strict security point of view, as it would not protect against attackers who are able to write files into these directories. On the other hand it would protect against "example" php scripts that ship with libraries (source of many widely exploited holes in the past), as well as accidentally live testing code. The latter is probably a more practical risk than the former, given that (in most situations) if there is an attacker who can write into these directories it is already game over.

Thanks for the feedback, Owen.

What do folks think about this proposal?

Thanks for the review/feedback.

Updated the comment and rule.

This looks good to me - it looks like statistics still has some test fails - I wonder if the REQUEST_URI condition needs to allow querystring parameters (dropping the "$")?

The only nitpick I can see is that the comment about copying/adapting the statistics line and the actual RewriteRule line that denies access are potentially a little unclear (people might copy the RewriteRule also). It might be clearer if we updated the comment to specify that it is just the RewriteCond and/or added a comment above the RewriteRule to indicate that this is the line actually denying access.

Sure, how about these comments?

I tried dropping the $ - let's see if that works.

If it does, we might have to do it in a few other cases, right? e.g. for the token on cron.php?

The current rules do not appear to work when Drupal is installed in a subdirectory. Testbots intentionally check-out/install Drupal in a subdirectory, so that the case is covered by all test runs and not an afterthought.

Once this patch passes tests, it should be tested against a mod_alias configuration. (example)

Attached patch should resolve the problem of Drupal potentially being located in a subdirectory (which is the case on testbots).

Added a prominent note/comment for that.

A few simplifications.

Change notice: https://www.drupal.org/node/2302063

FWIW: The custom statistics.php script is pretty much pointless today. It only made sense in the past, because it skipped/omitted larger parts of Drupal's bootstrap. Like any other front controller, it has to boot a kernel/container now. The only part that is still short-circuited is the routing system, but routing should be fast, so I doubt that it's worth to keep it. (Separate issue; don't know whether there's one already.)

Yay!

Committed 168c314 and pushed to 8.x. Thanks!

For #31, created #2302385: Remove custom statistics.php front controller

Is there a reason to not backport this to Drupal 7?

I think we should do that, yes.

Wouldn't it break modules such as statistics on D7 ?

@pounard we could just specifically allow access to them the same way we do now in D8.

Here are the files I see:

find . -name '*.php' | grep -v "tpl.php" | grep -v simpletest | grep -v api.php | grep -v template.php
./authorize.php
./cron.php
./index.php
./install.php
./modules/statistics/statistics.php
./sites/default/default.settings.php
./sites/example.sites.php
./themes/garland/theme-settings.php
./update.php
./xmlrpc.php

Not sure about the D7 backport. This broke one important (and IMO valid) use case, which is on shared hosts with only a single web root, it was common that if Drupal was your main website, to install it in there, but also have other PHP projects, such as WordPress or whatever, installed in sub-directories of your Drupal site.

I discovered this accidentally today trying to get XHProf up and running. In theory I should've just been able to toss the xhprof_html folder in my Drupal root (this works in Drupal 7), but it breaks in Drupal 8 because of this patch.

You'll probably argue that people doing things as I was doing are 'doing it wrong' and that's fine, but wanted to raise it as an issue. I'm not sure what the workaround is for people with only a single web root who want to run Drupal in parallel with other PHP projects.

You're doing it wrong :-P

Much like Drupal 7 -> Drupal "getting off the island", this is basically another case of our typical apache configuration becoming out of date with the rest of the world. It's true that you have to do something different, but...that different thing is better. Yes, there is a learning curve, but there are learning curves everywhere and we accept them and try to flatten them. The .htaccess has comments about how to enable additional php files - did you find them overly confusing? We can improve them for sure.

Well, the problem is I didn't know it was .htaccess, at first. I was getting 403 errors when trying to access the XHProf directory and since this same workflow had always worked in D7 I figured XHProf was doing something weird. Spent way too long chasing my tail with that before I thought of mv .htaccess htaccess-blah and problem solved. git blame landed me here.

What might help is if there was a commented-out example to enable access to an external script. But it sounds like that's not desired, and what people in my position should do is _______ in order to install Drupal + WordPress side-by-side, defaulting to Drupal. (Have both drupal and wordpress as separate folders in webroot and symlink to Drupal's index.php? No idea, honestly.) So maybe figure out what _____ is and document it in the handbook and link there in the .htaccess file.

In any event though, my point was IMO this is too disruptive to do in D7 anymore.

Maybe we could block this for sites/*/libraries only? (in combination with #667058: Add a libraries folder with a README.txt in it to DRUPAL_ROOT)

I think that would have a similar security benefit (in practice), but it would be less disruptive. Although there still might be a few cases where it would cause some disruption.

I'd suggest sites/*/libraries and sites/*/files - there is some defense in depth benefit to the files directory even though that should already be blocked by the .htaccess file that core creates there.