Problem/Motivation

Currently, Apache, IIS, and any other web server, will deliver YAML (.yml) files, assuming they have not been denied at the global level.

In Drupal 8, YAML can contain sensitive information that may allow intruders to gain insight into a system, or outright information that may be private. This is particularly of issue since default values for variables are set in YAML now.

In Drupal 7 and earlier, this information was typically in the .info, .module, or .inc files, which were protected by the primary .htaccess from the site.

Proposed resolution

The default .htaccess and web.config files should have rules in them to deny access to YAML, by default, that are residing in the normal locations.

The files directory (or directories) should not be subject to these restrictions.

Remaining tasks

None?

User interface changes

None.

API changes

None.

Original report by @alexpott

We should be preventing apache from serving YAML files as it'll be possible to get all sorts of information from them. The config directories are protected by their own .htaccess files but I don't think we should be exposing default module configuration eg. core/modules/system/config/system.site.yml or service config eg. core/lib/Drupal/Core/CoreBundle.yml (see #1939660: Use YAML as the primary means for service registration) either.

I'm not 100% certain this is the right approach - creating this issue to track the discussion.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

This sounds dubious. We should protect from yml in certain directories, but we shouldn't deny access completely to a serialization format? This seems comparable to if we blocked .json file extensions?

alexpott’s picture

Hmmm... after having slept on it I'm not sure that we can actually avoid blacklisting all .yml files since #1793074: Convert .info files to YAML and in fact we no longer need to protect .info files.

It'd then be up to the user to whitelist any directories they want to serve .yml files from.

chx’s picture

But total top level yaml ban, I do not know that would cover public files, that does seem a little excessive. Can't we coax Apache (RewriteRule *module*/.yml - [F]) to deny only in module dirs?

gdd’s picture

Why would we even want to block module dirs? This is all open source, anyone who wants to can see what is in the module dirs just by downloading the source code? I didn't read the linked issue but if it is requiring that we store some dynamic stuff that needs to be protected in module dirs, then I would argue it is a broken approach.

alexpott’s picture

Why do we protect the .info files in previous versions Drupal then?

gdd’s picture

I have no idea, that seems nonsensical to me as well.

gdd’s picture

The issue in question was #81845: Protect .info files from prying eyes and didn't go through much community discussion. I think this is a stupid thing to block, while it does allow sniffing modules, there are plenty of other ways to do that. There was another more recent issue about blocking CHANGELOG.txt in which we decided not to block it despite that it could be used to fingerprint sites #79018: protect Drupal core .txt files.

So I don't think we have any need to block .yml in the module directories, it doesn't actually make a site more secure and you shouldn't be providing secret information in them to begin with.

jibran’s picture

#2: 1956698.drupal8.protect-yml.2.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Security, +Configuration system

The last submitted patch, 1956698.drupal8.protect-yml.2.patch, failed testing.

mtift’s picture

It looks like most of the comments in this issue are against blocking access to .yml files. The OP mentions core/lib/Drupal/Core/CoreBundle.yml, which no longer exists (in that form, at least) and system.site.yml, which does not seem to contain much in the way of sensitive data.

I looked around quite a bit and could not fine other .yml files that would pose an obvious security risk. Can anyone else think of others?

gdd’s picture

I think the main concern is with contrib, where you will be storing things like API keys and such. There are also some contrib modules that store passwords to external sites in plaintext which I think is completely stupid but the security team has decided not to issue SAs to such modules.

mgifford’s picture

Issue summary: View changes
FileSize
669 bytes

re-roll. With yml & without info as per #2.

alexpott’s picture

Well #2253109: Followup: Bring .htaccess and web.config up to date has gone and prevented access to yml files :( - added a followup there to change to be only .info.yml

alexpott’s picture

alexpott’s picture

alexpott’s picture

Priority: Normal » Critical
dawehner’s picture

I wonder whether it would be possible to write tests? Just try to access the files via drupalGet?

xjm’s picture

Status: Needs work » Needs review
FileSize
830 bytes
1.5 KB

Yep, it's totally testable with a web test. I guess my only question is that modifying a .htaccess is actually a standard part of site config. If someone wants for whatever reason to allow access to yml files and so modifies their .htaccess, presumably this test would fail unless they also hacked core. Do we care?

We can also expand this test for all the other various extensions, but out of scope here. There's separate tests of .htaccess functionality specifically for the files directory in \Drupal\system\Tests\Files, but nothing that I found for other .htaccess functionality.

Currently in HEAD we do restrict access to .info.yml files -- this broadens it to all .yml files.

xjm’s picture

+++ b/core/modules/system/src/Tests/System/HtaccessTest.php
@@ -0,0 +1,29 @@
+   * Tests accessind disallowed file extensions.

Typo: "accesind"

xjm’s picture

Status: Needs review » Needs work

Note that #18 couples the test to the existence of core.services.yml. This doesn't include risk of false positives because it tests for a 403 and if the file is moved it'd be a 404, but it'd more best practice to add a test.myfile.yml file to a test module somewhere. Not sure if that's overkill or not.

xjm’s picture

Status: Needs work » Needs review

I'll change the test per #20 if someone else thinks I should. :)

The last submitted patch, 12: 1956698.drupal8.protect-yml.12.patch, failed testing.

The last submitted patch, 18: htaccess-1956698-FAIL.patch, failed testing.

effulgentsia’s picture

Title: Prevent access to YAML files using .htaccess » Prevent access to YAML files using .htaccess and web.config
Status: Needs review » Needs work
Issue tags: +Media Initiative

If we're gonna do this, we need to include web.config as well.

I think #1 is still a good point though. Denying an extension globally means uploaded files in sites/default/files get banned too, and there can certainly be use cases for wanting a file field to allow .yml uploads. Same could be argued for the other things we currently ban in .htaccess too, but I think .yml might be the one with the most common use cases. Therefore, would it make sense to require this patch to also include a solution for that, or should that be a followup?

Gábor Hojtsy’s picture

The denial of .po had similar problems before for people running local copies of l10n_server, which outputs .po files in a file directory which are then denied for download :D So I agree this may be a problem to globally deny. Also agreed web.config support is in order then.

xjm’s picture

I think #1 is still a good point though. Denying an extension globally means uploaded files in sites/default/files get banned too, and there can certainly be use cases for wanting a file field to allow .yml uploads. Same could be argued for the other things we currently ban in .htaccess too, but I think .yml might be the one with the most common use cases. Therefore, would it make sense to require this patch to also include a solution for that, or should that be a followup?

So maybe it makes sense to deny access generally, but override that with an allow for the files directory? I think that'd make sense to do in this issue then.

xjm’s picture

Status: Needs work » Needs review
FileSize
1.31 KB
2.3 KB

It looks like the .htaccess files for the files directories are generated from FileStorage::htaccessLines() (aside: which has a useless procedural wrapper we should kill since it's hardly a public function anyway). There's currently nothing in there along the lines of overriding general rules, so maybe a followup would actually be better (do the critical security hardening first, and then discuss the best solution for allowing downloads of the restricted extensions elsewhere since it also includes those other out-of-scope extensions). So I've just fixed the web.config and the minor typo here and I'll file a followup issue.

damiankloip’s picture

I agree we should not be able to access yml files like core/core.services.yml - that's terrible. However, this patch means I can not serve up any yml files at all. Can't we just restrict this to certain dirs instead?

xjm’s picture

Component: configuration system » batch system

@damiankloip, I thought about that, but we'd essentially need to restrict everything except the files directory -- core, the base directory, all module directories, who knows what subdirectories of them, etc. Those seem like a lot of specific rules, when a general rule + followup exception for the files directory seems much cleaner. I've filed it as a followup because it seemed to me that the need to block access to yml files the same as we would other metadata files seems more critical than allowing the smaller usecase of serving them by default. (Willing to be convinced it needs to be done in this issue though if others disagree.)

Filed:
#2332027: Make it possible to download .htaccess-restricted extensions from the files directory
#2332029: Add test coverage for .htaccess rules
#2332047: Deprecate unneeded file_htaccess_lines() wrapper

xjm’s picture

Component: batch system » base system
damiankloip’s picture

works for me. Agree restriction is more important, would just be nice to have the best of both worlds!

sun’s picture

To repeat the first dozen comments of this issue, I don't understand why we need to block access to .yml files either.

This issue was only bumped to critical due to #2321487: Ensure to not serve yml files inside Drupal

But that talks about services*.yml files only. That makes sense.

Only blocking those (everywhere) is a simple adjustment to the existing FilesMatch in /.htaccess.

Gábor Hojtsy’s picture

@sun: Drupal 8 is increasingly "programmed" with yml files, including default configuration (views, entities, etc). These used to be in install and PHP export files in Drupal 7 and well protected from 3rd party exploration. Not in Drupal 8. I take it you don't agree that those would need to be made non-accessible?

xjm’s picture

Right; one part of it is about disclosure making it easier to find exploits. E.g., a ton of information about access restrictions is saved to yaml files. If it makes sense to restrict all those other extensions, it makes sense to restrict yaml, I think.

xjm’s picture

Basically, there's a ton of data in yml files that would be disclosed anonymously. We have absolutely no way of knowing when this data might actually be not for anonymous consumption.

effulgentsia’s picture

I agree with #33. Core invents a bunch of YML file types: e.g., routing, and contrib can define more types like that. And sites can have custom, (i.e., not publicly available on d.o.), modules in the /modules and sites/*/modules directories. At the very least in those directories, I think we need a complete deny on *.yml. I think a global deny in the root directory with an override of allow in sites/*/files is fine though, unless someone here knows of a use case where that's insufficient. So, +1 to #27 with the followups in #29, but not RTBCing it yet, since it doesn't seem we're all in agreement on it yet.

xjm’s picture

Discussed with @Dave Reid, @neclimdul, and @tim.plunkett. Dave gave the example of the xmlsitemap module, which needs to serve sitemap.xml from the root of the site directory, and pointed out that if we were using XML as our metadata format, its usecase wouldn't be met. A compromise would be restricting:

/core/*/*.yml
/sites/*/modules/*/*.yml
/sites/*/themes/*/*.yml
/sites/*/profiles/*/*.yml
/modules/*/*.yml
/themes/*/*.yml
/profiles/*/*.yml
xjm’s picture

Or, alternately:

davereid: if we can do it so that it doesn't block yml from the files directory, and only blocks files that exist, I'd be ok with that

sun’s picture

only blocks files that exist

I'm a developer. I use Drupal for my own blog. I write a nice blog post to explain Drupal. Being a good teacher, I attach the corresponding .yml files of my example code to my blog post. They upload just fine. I feel satisfied and publish my post.

24h later, there are a dozen of comments complaining that people cannot access the example code files. Drupal, WTF? #FAIL

Dave Reid’s picture

@sun I believe you missed the "doesn't block yml from the files directory" part there.

damiankloip’s picture

As I mentioned in #28, I would not like to see this restricted across the board. The list in #37 seems reasonable. Then the e.g. use case outlined by sun in #39 is fine. But we get the files directory being able to serve yml files.

I feel like I rolled over too easily in #31. Basically, yes please for #37.

xjm’s picture

Assigned: Unassigned » xjm

Either #38 or #37 address the usecase in #39. #37, however, is far more fragile, and is the only coupling to specific directory paths that would be in core's .htaccess.

I'll update the patch and add test coverage to demonstrate that a yaml file can be served from the files directory to the patch so it's clear. Thanks @Dave Reid @damiankloip and @sun!

iantresman’s picture

Shouldn't we prevent the public access to ALL /module or /core files, and allow only those that are required.

Gábor Hojtsy’s picture

@iantresman: how do you tell which files will be useful? I mean JS, CSS, images, etc. but there are new file formats and extensions appearing still. It would suck if a contrib would need core blessing to include some new format.

xjm’s picture

Thanks @iantresman and @Gábor Hojtsy. #43 is also out of scope here.

xjm’s picture

catch’s picture

Issue tags: +D8 upgrade path

This will require manual editing of .htaccess/web.config on existing installs, so tagging D8 upgrade path so we make the change before we officially support existing installs in any way.

damiankloip’s picture

Tests for what we need. Should get one fail.

Status: Needs review » Needs work

The last submitted patch, 48: 1956698-48.patch, failed testing.

mpdonadio’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
Gábor Hojtsy’s picture

Status: Needs review » Needs work

I think the test looks good and if we remove the files check for the scope of this issue, we should get it in. Then the files scope has an issue at #2332027: Make it possible to download .htaccess-restricted extensions from the files directory to resolve, which would get the portion of the test dealing with that. #2332029: Add test coverage for .htaccess rules can expand the tests to other things but YML files.

+++ b/core/modules/system/src/Tests/System/HtaccessTest.php
@@ -0,0 +1,53 @@
+   * Tests accessing YAML file extensions.

We are not accessing the extensions, but the files :) My suggestion would be "Tests accessing files with .yml extensions at various locations." if it fits 80 chars.

Marking needs work for (a) fixing the wording on the test method (b) removing the file upload testing and moving it to #2332027: Make it possible to download .htaccess-restricted extensions from the files directory. Looks good otherwise.

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
2.45 KB
1.32 KB

Implemented those changes. I think this looks good to go.

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/src/Tests/System/HtaccessTest.php
@@ -0,0 +1,31 @@
+    // Try accessing the core services YAML file.
+    $this->drupalGet('core/core.services.yml');
+    $this->assertResponse(403);
+    // Try accessing a core module YAML file.
+    $this->drupalGet('core/modules/system/system.services.yml');
+    $this->assertResponse(403);

we should assert the file exists - just in case we rename or move them.

alexpott’s picture

In fact in might be nice to add an assertion method here that you just pass a file name and it asserts that the file exists and the access is denied so it can be reused by #2332029: Add test coverage for .htaccess rules

Gábor Hojtsy’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
2.73 KB

Here you go!

alexpott’s picture

Status: Needs review » Reviewed & tested by the community

Nice one! Thanks.

The patch has tests and increases Drupal 8's security.

catch’s picture

Do we have tests for the other file types? Might be worth a follow-up issue to add test coverage for those.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Alex pointed me to #2332029: Add test coverage for .htaccess rules, which is also a child issue.

Committed/pushed to 8.0.x, thanks!

  • catch committed 7725fcf on
    Issue #1956698 by Gábor Hojtsy, xjm, alexpott, damiankloip, mgifford:...

Status: Fixed » Closed (fixed)

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

xjm’s picture

Assigned: xjm » Unassigned