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.
Comment | File | Size | Author |
---|---|---|---|
#25 | restrict-text-editor-files-1907704-25.patch | 595 bytes | ultimateboy |
#18 | restrict-text-editor-files-1907704-18.patch | 593 bytes | acrollet |
#10 | restrict-text-editor-files.patch | 580 bytes | totten |
#6 | restrict-text-editor-files-1907704-6.patch | 536 bytes | acrollet |
#1 | restrict-text-editor-files-1907704-1.patch | 533 bytes | ultimateboy |
Comments
Comment #1
ultimateboy CreditAttribution: ultimateboy commentedComment #2
Crell CreditAttribution: Crell commentedPerhaps we should add a comment for why that part of the filter is there?
Comment #3
ultimateboy CreditAttribution: ultimateboy commentedCrell, 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.
Comment #5
gregglesI agree a comment is unnecessary for this hunk.
A followup issue could be created to comment the whole thing if someone feels like it.
Comment #6
acrollet CreditAttribution: acrollet commentedre-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)
Comment #7
forestmonster CreditAttribution: forestmonster commentedI'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.
Comment #8
acrollet CreditAttribution: acrollet commentedI 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)
Comment #9
forestmonster CreditAttribution: forestmonster commented@acrollet, yes, a separate issue is probably best. I added #1907934: Allowlist, instead of denylist, sensitive files and directories.
Comment #10
totten CreditAttribution: totten commented@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.
Comment #11
forestmonster CreditAttribution: forestmonster commented@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?
Comment #12
greggles#1: restrict-text-editor-files-1907704-1.patch queued for re-testing.
Comment #13
sdelbosc CreditAttribution: sdelbosc commentedSorry 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.
Comment #14
ghazlewood CreditAttribution: ghazlewood commentedI 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.
Comment #15
gregglesI 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.
Comment #16
acrollet CreditAttribution: acrollet commentedghezlewood: The regex '^#.*#$' should catch '#settings.php#'.
Comment #17
sdelbosc CreditAttribution: sdelbosc commentedre #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/.
Comment #18
acrollet CreditAttribution: acrollet commentedre-rolled to additionally restrict .save files.
Comment #19
ghazlewood CreditAttribution: ghazlewood commented@acrollet you beat me to it :) Was unsure as to why there is repetition in there though, why are the parts repeated?
Comment #20
acrollet CreditAttribution: acrollet commentedghazlewood: 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 :)
Comment #21
ghazlewood CreditAttribution: ghazlewood commentedThanks, I had a feeling it was for good reason.
Comment #22
ultimateboy CreditAttribution: ultimateboy commentedAlright, 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.
Comment #23
gregglesThis seems like a better tag.
Comment #24
webchickThis 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?
Comment #25
ultimateboy CreditAttribution: ultimateboy commentedAnd... here's the backport.
Comment #26
acrollet CreditAttribution: acrollet commentedD7 patch is identical to the D8 version, RTBC'ing
Comment #27
David_Rothstein CreditAttribution: David_Rothstein commentedWow, 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?
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedApparently 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.
Comment #29
David_Rothstein CreditAttribution: David_Rothstein commentedOops, would need that issue fixed first, but I didn't mean to remove the tag.