Security enhancement, better handling of 403, better documentation of .htaccess

Bevan - February 5, 2008 - 03:38
Project:Drupal
Version:7.x-dev
Component:base system
Category:feature request
Priority:normal
Assigned:Unassigned
Status:patch (code needs work)
Description

This patch blocks http access to files named CHANGELOG.txt COPYRIGHT.txt INSTALL.txt LICENSE.txt MAINTAINERS.txt UPGRADE.txt and README.txt. This is a security improvement because it makes it considerably more difficult for an ill-intending site visitor to find what version of drupal and/or modules the site is using and what security vulnerabilities the website is likely to be open to. All the above files have CVS version numbers and other subtle differences that help identify the drupal version in use.

This patch also implements handling of file access denied errors in drupal. For example, if a user tries to access example.com/modules or example.com/CHANGELOG.txt the user is presented drupal's 403 access denied page, instead of the web server's 403 access denied page. It does this by implementing ErrorDocument 403 /index.php in .htaccess and by checking for $_SERVER['REDIRECT_STATUS'] == 403 in system_init(). This is a usability and security improvement. Usability, because the user still feels like they are on the site if they got there accidentally. Security, because it helps obfuscate the reason why access is denied by revealing less information about the file system the site is running on. This change may have side effects and if there are concerns I'll split it into a separate patch.

There are also minor changes to .htaccess RewriteRule flags that should have no change on behaviour, but better document apache's handling of the .htaccess file.

AttachmentSize
deny_access_files.patch3.13 KB

#1

BioALIEN - February 5, 2008 - 04:07

Subscribing, I see merit in the problem this issue is resolving. Displaying the server's 403 could stand better against DDoS attacks on a site with cache = off as it bypasses Drupal and the heavy load that comes with it.

#2

Bevan - February 5, 2008 - 04:14

It also moves the ID CVS keyword to the top, for consistency

#3

Bevan - February 5, 2008 - 04:24

This is a drupal 5 version with only the changes that modify behaviour.

AttachmentSize
217921.patch1.6 KB

#4

meba - March 27, 2008 - 22:47
Status:patch (code needs review)» patch (code needs work)

Did not apply to HEAD. Otherwise, it works as expected. I like it, but is there a way to detect a 403 without an additional if() in init?

#5

pwolanin - March 29, 2008 - 12:33

The concern about .txt files was addressed in this issue: http://drupal.org/node/79018 So to the extent that this is only about making .txt files non-readable, this is a duplicate issue.

However, the 403 redirect part might be a useful addition.

#6

Bevan - April 1, 2008 - 21:09
Status:patch (code needs work)» patch (code needs review)

Rerolled for head. No changes.

#79018 seems to have a lot of conflict, bias, opinion and subjectiveness. I didn't read it to the end. It also seemed important. Is there any conclusion on the whether this is a good approach or not?

AttachmentSize
217921.patch3.14 KB

#7

pwolanin - April 1, 2008 - 21:47
Status:patch (code needs review)» patch (code needs work)

The conclusion from the other issue was that trying to block the .txt files in .htaccess is unacceptable (for a variety of reasons), and that the effect on security is really theatrical, not substantive. So, however you feel about that, I'd suggest limiting this patch to the 403 part.

#8

Bevan - April 1, 2008 - 23:04

Thanks. I'll do that.

#9

lilou - September 1, 2008 - 19:35

bump.

 
 

Drupal is a registered trademark of Dries Buytaert.