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.
Comment | File | Size | Author |
---|---|---|---|
#55 | 1956698-55.patch | 2.73 KB | Gábor Hojtsy |
#55 | interdiff.txt | 1.07 KB | Gábor Hojtsy |
#52 | interdiff.txt | 1.32 KB | Gábor Hojtsy |
#52 | 1956698-52.patch | 2.45 KB | Gábor Hojtsy |
#48 | interdiff-1956698-48.txt | 1.53 KB | damiankloip |
Comments
Comment #1
Dave ReidThis 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?
Comment #2
alexpottHmmm... 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.
Comment #3
chx CreditAttribution: chx commentedBut 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?
Comment #4
gddWhy 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.
Comment #5
alexpottWhy do we protect the .info files in previous versions Drupal then?
Comment #6
gddI have no idea, that seems nonsensical to me as well.
Comment #7
gddThe 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.
Comment #8
jibran#2: 1956698.drupal8.protect-yml.2.patch queued for re-testing.
Comment #10
mtiftIt 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?
Comment #11
gddI 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.
Comment #12
mgiffordre-roll. With yml & without info as per #2.
Comment #13
alexpottWell #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
Comment #14
alexpottComment #15
alexpottComment #16
alexpottRaising priority based on #2321487: Ensure to not serve yml files inside Drupal
Comment #17
dawehnerI wonder whether it would be possible to write tests? Just try to access the files via drupalGet?
Comment #18
xjmYep, 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.Comment #19
xjmTypo: "accesind"
Comment #20
xjmNote 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.Comment #21
xjmI'll change the test per #20 if someone else thinks I should. :)
Comment #24
effulgentsia CreditAttribution: effulgentsia commentedIf 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?
Comment #25
Gábor HojtsyThe 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.
Comment #26
xjmSo 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.
Comment #27
xjmIt 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.Comment #28
damiankloip CreditAttribution: damiankloip commentedI 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?
Comment #29
xjm@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
Comment #30
xjmComment #31
damiankloip CreditAttribution: damiankloip commentedworks for me. Agree restriction is more important, would just be nice to have the best of both worlds!
Comment #32
sunTo 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
.Comment #33
Gábor Hojtsy@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?
Comment #34
xjmRight; 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.
Comment #35
xjmBasically, 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.
Comment #36
effulgentsia CreditAttribution: effulgentsia commentedI 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
andsites/*/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 insites/*/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.Comment #37
xjmDiscussed 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:
Comment #38
xjmOr, alternately:
Comment #39
sunI'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
Comment #40
Dave Reid@sun I believe you missed the "doesn't block yml from the files directory" part there.
Comment #41
damiankloip CreditAttribution: damiankloip commentedAs 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.
Comment #42
xjmEither #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!
Comment #43
iantresman CreditAttribution: iantresman commentedShouldn't we prevent the public access to ALL /module or /core files, and allow only those that are required.
Comment #44
Gábor Hojtsy@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.
Comment #45
xjmThanks @iantresman and @Gábor Hojtsy. #43 is also out of scope here.
Comment #46
xjmComment #47
catchThis 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.
Comment #48
damiankloip CreditAttribution: damiankloip commentedTests for what we need. Should get one fail.
Comment #50
mpdonadioComment #51
Gábor HojtsyI 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.
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.
Comment #52
Gábor HojtsyImplemented those changes. I think this looks good to go.
Comment #53
alexpottwe should assert the file exists - just in case we rename or move them.
Comment #54
alexpottIn 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
Comment #55
Gábor HojtsyHere you go!
Comment #56
alexpottNice one! Thanks.
The patch has tests and increases Drupal 8's security.
Comment #57
catchDo we have tests for the other file types? Might be worth a follow-up issue to add test coverage for those.
Comment #58
catchAlex pointed me to #2332029: Add test coverage for .htaccess rules, which is also a child issue.
Committed/pushed to 8.0.x, thanks!
Comment #61
xjm