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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ultimateboy’s picture

Title: Restrict temporary » Restrict temporary files created by text editors
Status: Active » Needs review
FileSize
533 bytes
Crell’s picture

Perhaps we should add a comment for why that part of the filter is there?

ultimateboy’s picture

Issue tags: +Needs security review

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.

Status: Needs review » Needs work

The last submitted patch, restrict-text-editor-files-1907704-1.patch, failed testing.

greggles’s picture

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.

acrollet’s picture

Status: Needs work » Needs review
FileSize
536 bytes

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)

forestmonster’s picture

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.

acrollet’s picture

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)

forestmonster’s picture

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.

@acrollet, yes, a separate issue is probably best. I added #1907934: Allowlist, instead of denylist, sensitive files and directories.

totten’s picture

@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.

forestmonster’s picture

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

@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?

greggles’s picture

sdelbosc’s picture

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.

ghazlewood’s picture

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.

greggles’s picture

Status: Needs review » Needs work

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.

acrollet’s picture

Status: Needs work » Needs review

ghezlewood: The regex '^#.*#$' should catch '#settings.php#'.

sdelbosc’s picture

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/.

acrollet’s picture

re-rolled to additionally restrict .save files.

ghazlewood’s picture

@acrollet you beat me to it :) Was unsure as to why there is repetition in there though, why are the parts repeated?

acrollet’s picture

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 :)

ghazlewood’s picture

Thanks, I had a feeling it was for good reason.

ultimateboy’s picture

Status: Needs review » Reviewed & tested by the community

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.

greggles’s picture

This seems like a better tag.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D7

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?

ultimateboy’s picture

Status: Patch (to be ported) » Needs review
FileSize
595 bytes

And... here's the backport.

acrollet’s picture

Status: Needs review » Reviewed & tested by the community

D7 patch is identical to the D8 version, RTBC'ing

David_Rothstein’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)
Issue tags: +Needs backport to D6

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?

David_Rothstein’s picture

Issue tags: -Needs backport to D6

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.

David_Rothstein’s picture

Issue tags: +Needs backport to D6

Oops, would need that issue fixed first, but I didn't mean to remove the tag.

Status: Patch (to be ported) » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.