Posted by ultimateboy on February 4, 2013 at 7:47pm
16 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | other |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | patch (to be ported) |
| Issue tags: | needs backport to D6, needs backport to D7, Security improvements |
Issue Summary
A recent security audit of top sites revealed that roughly 1% of CMS powered sites expose their database credentials due to temporary files created by text editors. The article mentions that a fairly good solution to this issue is to block access to these types of files via .htaccess.
This does seem quite reasonable. Patch forthcoming.
Comments
#1
#2
Perhaps we should add a comment for why that part of the filter is there?
#3
Crell, the only reason I didn't is because the rest of the filter isn't commented - though I guess I will agree that this is a slightly more obscure addition to the filter.
#4
The last submitted patch, restrict-text-editor-files-1907704-1.patch, failed testing.
#5
I agree a comment is unnecessary for this hunk.
A followup issue could be created to comment the whole thing if someone feels like it.
#6
re-roll attached with a somewhat less greedy regex. It's up for discussion IMO if the regex should also be limited specifically to filenames that are likely to contain php (*.php,module,inc) or even just variations on settings.php. (However, this would not protect someone who includes sites/all/db.inc or the like from a settings.php file)
#7
I'm surprised. Doesn't it make more sense for us to use a whitelist for this? Aren't we always going to be playing "catch-up" with the future, which may bring us new IDEs with new backup file extensions, new version control systems, et cetera? At some point, we could even allow modules to modify the whitelist under certain conditions.
See #581706: Protect .git, .hg and .bzr directories in .htaccess for a related discussion.
#8
I agree in principle with #7. However, instituting a whitelist seems at least on its face to be a much more major change, with potential for a correspondingly longer development/testing/acceptance period. My personal feeling is that it should be a separate issue, and we shouldn't let perfect be the enemy of good in this case. (and probably #581706: Protect .git, .hg and .bzr directories in .htaccess as well)
#9
@acrollet, yes, a separate issue is probably best. I added #1907934: Whitelist, instead of blacklist, sensitive files and directories.
#10
@acrollet I lean towards:
1) Blacklisting backups of *.php files
2) (and) Blacklisting backups of otherwise blacklisted extensions (*.inc, *.module, etc -- a list which doesn't actually include *.php)
Rationale: Under D7+MAMP, I observed that the blacklist applies to both the file-system and to Drupal content (e.g. nodes published with path.module). I think it's legitimate for a node to have the path "example.bak" -- but the file "example.php.bak" should not be accessible. The attached tightens the blacklist per #1 & #2. It also adds other common extensions (e.g. *.php.bak, *.php.orig)
Re: .git and .svn -- on MAMP and Debian Squeeze, all dot-files appear to be prohibited by default (with or without Drupal's .htaccess). This policy seems to provide pretty broad protection for current/future SCMs.
#11
@totten, of course, this is a good thing, if we can rely on protection in particular Linux distributions. However, doesn't Drupal have a much wider user base?
#12
#1: restrict-text-editor-files-1907704-1.patch queued for re-testing.
#13
Sorry if I introduce noise here but wouldn't it be better to use dedicated .htaccess files in sites/default and sites/default/files to deny all accesses in default directory and allow all accesses in files directory (principle close to the one used in sites/default/files/private)?
If such approach is kept, the procedure given at http://drupal.org/documentation/install/multi-site would probably be impacted.
#14
I saw the same article co-incidentally yesterday and started looking at the Security Review module as a place to make checks.
The article also mentions nano '.save' files as well as vim #settings.php# which the patch seems to miss.
#15
I think it catches the #settings.php# but not .save.
Needs work for the .save files.
re #13 - it's hard to say whether this should impact the whole site or just specific directories. I'd rather it affect everything for now. I've definitely seen sites where important IP addresses or the keys for various services were hard-coded into .module files or .php files inside a module folder and it seems great if we can protect those while we're at it.
#16
ghezlewood: The regex '^#.*#$' should catch '#settings.php#'.
#17
re #15 - Understood. I am just afraid you miss some cases using the current approach.
Anyways, I will probably add some Directory tags in my Apache configuration to make sure that nothing can be downloaded from sites/default/.
#18
re-rolled to additionally restrict .save files.
#19
@acrollet you beat me to it :) Was unsure as to why there is repetition in there though, why are the parts repeated?
#20
ghazlewood: the first section restricts back-up versions of the files that are already restricted (module,inc,tpl, etc) - The second section restricts back-up versions of .php files, which can not be restricted whole-sale, or index.php would not load :)
#21
Thanks, I had a feeling it was for good reason.
#22
Alright, I feel like we've hit the nail on the head with this one. Thanks so much to acrollet and totten for the re-rolls.
After talking with acrollet and greggles in IRC, we agreed that it doesn't really make sense to provide tests for this as it would cause failures on nginx and IIS.
I have manually tested placing temporary files within sites/default as well as other directories and they are correctly forbidden by apache.
#23
This seems like a better tag.
#24
This sounds good. That regex is starting to get ugly as hell though. :)
Committed to 8.x. I'll push once testbot has caught up a bit.
Seems this might be worth a backport to D7?
#25
And... here's the backport.
#26
D7 patch is identical to the D8 version, RTBC'ing
#27
Wow, that's complicated-looking but seems to make sense and work as expected. Nice work.
Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/a16de0d (I removed the trailing spaces on commit, by the way; those weren't in the Drupal 8 patch but somehow wound up in the Drupal 7 one.)
Not much is getting backported to Drupal 6 these days, but as a security improvement maybe this one would be a good idea still?
#28
Apparently this patch broke things on Apache 1.x. Reviews welcome at #1962780: 500 Internal server error on Apache 1.x servers after updating to Drupal 7.22 if anyone has time.
#29
Oops, would need that issue fixed first, but I didn't mean to remove the tag.