Validate that private files aren't the same directory as public files

I disagree. There are a ton of horribly stupid things you can do and yet we do not verify all of them. One might even move this to http://drupal.org/project/security_review issue queue, honestly.

Why on earth is adding validation here so that your privacy on private files isn't completely (and silently) destroyed a "feature request"?

And I guess the sec. team should make the call on whether this is actually critical or not, but it sure seems like it to me.

Steps to reproduce:

1. Enter a private file directory at admin/config/media/file-system or whatever it is to, for example, sites/default/private-files.
2. Change article's image field to "private" file handling.
3. Create a new article with an image.
4. Note that you're denied access if you replace system/files with 'sites/default/private-files'. (Everything works as it should thanks to .htaccess)
5. Now, switch the path of the private files to sites/default/files (same as public). Drupal lets you do this without complaint.
6. Create another article with an image.
7. Note that your image is completely and utterly visible when you repeat step 4. Your security is gone.

Andrew has a hunch that this happens because "pu" comes after "pr" alphabetically, so the .htaccess file that's written is the public files one, not the private files one. One would expect the steps outlined here to break public file handling, not private handling, but in fact it does the opposite.

But in any case, it appears they're mutually exclusive and err on the side of "allow everything," so IMO we need to add validation to prevent users from doing this.

And, there's a patch.

Why are you validating in after_build instead of validate?? what happens to those who upgrade? Why do we check against this stupid configuration and not when someone enables say <embed> for anonymous in filter configuration? What's the difference...? What makes this one so special?

If we shouldn't check this, why do we check for a valid path when adding menu items, or for duplicate path aliases? These are also cases where its not a "bug" in that drupal will run fine with the invalid data, yet its considered important to prevent inconsistent behavior with the site.

While <embed> is a security issue, its still a valid state; Drupal will function according to its own design and documentation, even if the end result is not what the user wanted. I agree that we should not stop a user from configuring the site to be less secure, when doing so follows the design of the configurable.

The key difference is that Drupal specifically defines the intended behavior and setup of the two paths:
Public: "This directory must be ... accessible over the web."
Private: "This directory should not be accessible over the web."
The documentation states a mutually incompatible attribute for each field, and so naturally its an invalid state if both fields share the same value. In addition, when both fields are identical, private files are no longer private, breaking its design.
Validating against this case just makes sense. Every other forum validates itself when necessary to prevent issues like this, so it's natural that these should two.

The case of a user setting the private file path to a public directory would not be a bug. However, a good feature would be a good idea to warn them of this.

I don't understand why the file system settings form uses #after_build at all. This method is called whenever the form is build, so whenever the form is build the system checks whether the directory is protected by a .htaccess file or not. I think that is a pretty random check and thus does not add any security. Moreover the directory and the .htaccess file are build when they don't exist on loading of a form, rather unexpected behavior in my point of view.

The patch above does not work as intended, since the validation comes after the .htaccess files have been created or at least independently of it, so if a private .htaccess will be overwritten it happens if the form validates or not. So since the call to system_check_directory in #after_build is not adding to security, we can remove it and move all the logic of creating and securing the directory to a submit function and keep the validate function from the patch above to take care of validation. I'll write a patch tomorrow if no one beats me to it :)

Here is the patch. I also changed the signature of system_check_directory by adding the optional parameter $private, that is passed into file_create_htaccess to make the function more generic and remove the hard-coded check for the name of the form element file_public_path.

FWIW, this seems like a critical issue to me. I run a site where we upload both private and public documents. Accidentally allowing the private docs to be publicly seen would be a disaster. I do "horribly stupid things" all the time. It's part of being human. If there's a reasonable way software can prevent me from doing some of them, it should.

I think we need to do this without the function signature change, if possible. That sounds like good polish, but not something we should introduce just before RC1.

Patch works, one question: can private == tmp? doesn't really makes sense but maybe check for it as well?

Patch is looking good but...

Looks good to me.

Does it make sense to add the $private parameter present in #14 since this is going into D8 first? I liked that inclusion and think it got striped out because of the D7 API freeze.

+++ b/modules/system/system.admin.inc
@@ -1734,13 +1734,17 @@ function system_clear_page_cache_submit($form, &$form_state) {
+    '#submit' => array('system_file_system_settings_submit'),
...
-    '#after_build' => array('system_check_directory'),

@@ -1749,7 +1753,6 @@ function system_file_system_settings() {
-    '#after_build' => array('system_check_directory'),

@@ -1758,7 +1761,6 @@ function system_file_system_settings() {
-    '#after_build' => array('system_check_directory'),

@@ -1780,6 +1782,34 @@ function system_file_system_settings() {
+/**
+ * Check to see if the public, private, and temporary paths exist or can be
+ * created if needed.
+ */
+function system_file_system_settings_submit($form, &$form_state) {
+  system_check_directory($form['file_public_path']);
+  system_check_directory($form['file_private_path']);
+  system_check_directory($form['file_temporary_path']);
+}

Moving this logic into a submit handler is utterly wrong, since http://api.drupal.org/api/drupal/modules--system--system.module/function... throws form validation errors. system_check_directory() has to be invoked before (like #after_build) or within form validation.

Powered by Dreditor.

Last patch failed due to missing group index in getInfo().

#26: validate-secure-directories.patch queued for re-testing.

Tagging for reroll.

Re-rolled. Updated #26, and removed the extra if($form_element['#name'] == 'file_public_path' check that was still hanging around, but as far as I can tell, was no longer necessary with the introduction of the $private parameter.

I tend to agree with sun's argument in #23, in that it feels wrong to be calling form_set_error() within system_check_directory() from a submit function ... but in the end, I felt more strongly about the argument in #24, that we shouldn't be modifying the file structure within the #validate function.

So unless we were to somehow solve the 'drupal_mkdir() as a validation check' issue, the approach in this patch feels like the lesser of two evils ...

Thanks @jthorson!

This has min docs gate trouble. http://drupal.org/core-gates#documentation-block-requirements

+++ b/core/modules/system/system.admin.incundefined
@@ -1769,6 +1770,34 @@ function system_file_system_settings() {
+ * Validates that the private and temporary directories do not resolve to the
+ * same directory location as the public directory.
+ */
+function system_file_system_settings_validate($form, &$form_state) {
...
+/**
+ * Checks that the public, private, and temporary paths exist, or can be created
+ * if necessary.
+ */
+function system_file_system_settings_submit($form, &$form_state) {

These need one line < 80 char summaries and typed params like:

* @param array $form
* An associative array containing the structure of the form.
* @param array $form_state
* A reference to a keyed array containing the current state of the form.

+++ b/core/modules/system/system.moduleundefined
@@ -2682,8 +2682,11 @@ function system_admin_menu_block($item) {
+ * @param $private
+ *   If $private is TRUE, create an .htaccess file configured for secure access
+ *   in the directory specified by $form_element.
  */
...
+function system_check_directory($form_element, $private = TRUE) {

I think this need a type: boolean and (optional)

major, meh!

Yeah, I guess someone from security team can change it back to a higher priority. The earlier discussion about priority was from 2010.

#31: validate-secure-directories-31-test-only.patch queued for re-testing.

#31: validate-secure-directories-31.patch queued for re-testing.

Updated docblocks as per #1354: [Obsolete] API documentation and comment standards.

More docblock tweaks and formatting consistency.

+++ b/core/modules/system/system.admin.incundefined
@@ -1722,15 +1722,20 @@ function system_clear_page_cache_submit($form, &$form_state) {
  * @ingroup forms
  * @see system_settings_form()
+ * @see system_file_system_settings_validate()
+ * @see system_file_system_settings_submit()
  */
 function system_file_system_settings() {

So.. this is not a _form() form constructor. Is that because this is a special .. something?

consider putting @ingroup forms after the @see's

+++ b/core/modules/system/system.admin.incundefined
@@ -1770,6 +1773,36 @@ function system_file_system_settings() {
+ * Validate system_file_system_settings forms.
+ *
+ * @see system_file_system_settings_submit().
+ */
+function system_file_system_settings_validate($form, &$form_state) {
...
+/**
+ * Verify public, private, and temporary paths exist, or can be created.
+ *
+ * @see system_file_system_settings_validate()
+ */
+function system_file_system_settings_submit($form, &$form_state) {

Maybe more like

/**
* Form validation handler for system_file_system_settings forms.
*
* @see system_file_system_settings_form_submit()
*/
function system_file_system_settings_form_validate($form, &$form_state) {
...
}

/**
* Form submission handler for system_file_system_settings forms.
*
* Verify public, private, and temporary paths exist, or can be created. (or not here, and inline in the function)
*
* @see system_file_system_settings_form_validate()
*/
function system_file_system_settings_form_submit($form, &$form_state) {

No dot in the @see, and keywords in comment like Form validation handler, etc

Should the function names be ..._form_validate() and _form_submit()

from http://drupal.org/node/1354#forms

[check the names of functions, and names in the comments... my cut and paste is not exact.]

#40 is a review of #38

for #39 ... just make your new stuff or the lines you changed match the standard.
Don't fix the whole file. Instead, make a follow-up to do that later.

Ooops ... lost the test file on the last few posts!

Docs updated as per #40, and added the test file back in.

+++ b/core/modules/system/system.admin.incundefined
@@ -1771,8 +1773,9 @@ function system_file_system_settings() {
- * Validates that the private and temporary directories do not resolve to the
- * same directory location as the public directory.
+ * Form validation handler for the system_file_system_settings() form.
+ *
+ * @see system_file_system_settings_submit()

@@ -1789,8 +1792,11 @@ function system_file_system_settings_validate($form, &$form_state) {
- * Checks that the public, private, and temporary paths exist, or can be created
- * if necessary.
+ * Form submission handler for the system_file_system_settings form.
+ *
+ * Verify public, private, and temporary paths exist, or can be created.
+ *
+ * @see system_file_system_settings_validate()
  */
 function system_file_system_settings_submit($form, &$form_state) {

Making the one line that matches the pattern in 1354 is good.

But let's keep the comment that adds information about what it does specifically.

Add the extra lines like you did for the _submit()

(Or move the comments inline into the validate().)

The rest of the changes look really good!
The bot said green. :)

Is it worth doing a tests only version of this to show the bot fails?

Added back the description to the system_file_system_settings() form.

The test-only patch is posted in #31, and the interdiff in #42 shows that only docs have changed since then.

Attached are some nit-picky type of comments from reading through this patch.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemFilePathsTest.phpundefined
@@ -0,0 +1,58 @@
+ * @file
+ * Definition of Drupal\system\Tests\System\SystemFilePathsTest.

This should be 'Contains' instead of 'Definition of'.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemFilePathsTest.phpundefined
@@ -0,0 +1,58 @@
+  public static function getInfo() {

My understanding is that there should be a blank line after a class declartion and before the closing brace.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemFilePathsTest.phpundefined
@@ -0,0 +1,58 @@
+    $this->admin_user = $this->drupalCreateUser(array('access administration pages', 'administer site configuration'));
+    $this->drupalLogin($this->admin_user);

A class member should be 'adminUser' instead of 'admin_user' (camelCase).

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemFilePathsTest.phpundefined
@@ -0,0 +1,58 @@
+  function testUniquePrivateDirectoryPath() {
+    $this->drupalGet('admin/config/media/file-system');

Missing docblock for what this is testing.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemFilePathsTest.phpundefined
@@ -0,0 +1,58 @@
+    $this->assertText(t('The private file system path must not be the same as the public file system path.'), t('Relative private path is not public path.'));
+    $this->assertText(t('The temporary directory must not be the same as the public file system path.'), t('Relative temporary path is not public path.'));

Throughout patch... Assertion messages should not be translated with t(). These recently have been removed from most tests in D8.

+++ b/core/modules/system/system.admin.incundefined
@@ -1770,6 +1773,41 @@ function system_file_system_settings() {
+ * Form submission handler for the system_file_system_settings form.

system_file_system_settings() [Needs ()].

+++ b/core/modules/system/system.moduleundefined
@@ -2664,8 +2664,11 @@ function system_admin_menu_block($item) {
+ * @param boolean $private
+ *   (optional) If $private is TRUE, create an .htaccess file configured for
+ *   secure access in the directory specified by $form_element.

The type hint should be 'bool' instead of 'boolean'. Also please add type hint to all other @param/@return directives in any docblock changed.

Finally, this @param directive needs to explicitly state what the default value is (ie 'Defaults to TRUE.').

Updated with changes from #45.

Thanks @jthorson. The changes look good.

Wow ... those look odd and unrelated!

Re-uploading to re-test (and still save the previous results incase we've got a random failure somewhere).

triggering the testbot

I'm not sure why this got demoted to "minor" ("meh" is not very descriptive :P) but publicly accessible private files does not in any way sound minor to me.

Updated issue summary.

Code looks good, bot is happy, only some minor comment issues.

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemFilePathsTest.phpundefined
@@ -0,0 +1,63 @@
+ * Test setting paths for the file system.

Tests the setting ...

+++ b/core/modules/system/lib/Drupal/system/Tests/System/SystemFilePathsTest.phpundefined
@@ -0,0 +1,63 @@
+   * Test that the private and temporary file paths must differ from the public.

Tests ...

+++ b/core/modules/system/system.admin.incundefined
@@ -1720,17 +1720,22 @@ function system_clear_page_cache_submit($form, &$form_state) {
+ * @ingroup forms

I think there should be a blank line above this.

+++ b/core/modules/system/system.admin.incundefined
@@ -1770,6 +1773,41 @@ function system_file_system_settings() {
+    form_set_error('file_temporary_path', t('The temporary directory must not be the same as the public file system path.'));

why directory, why not "temporary file system path", so it's similar to public and private one?

Updated "Test" -> "Tests", and added the blank line before the @ingroup statement.

why directory, why not "temporary file system path", so it's similar to public and private one?

The "temporary directory" wording matches the actual label that is used on the field within the UI ... so that it is obvious to the user which field the error message is referencing.

Seems like people are not heeding my warnings.

What happens to those who upgrade? Why do we check against this stupid configuration and not when someone enables say <embed> for anonymous in filter configuration? What's the difference...? What makes this one so special?

None of this was addressed. It is hardly an answer "we can check for this but we can't for filter so let's do this!". I could list N other stupid ways to configure your site. Say, public files go under files/public and private go under files/private just to stay close to this issue. There is no point, absolutely none in staking out one single security fail and "fixing" it. This would lead to people think that "oh I tried to do it, Drupal warned me it was unsafe, I changed it a little, now it doesn't, I am surely good!" and also "Drupal warned me there it doesn't warn me here, I am surely good!". Do not create unreasonable expectations.

And the question for those who upgrade went completely unaddressed. I have no idea what so ever how to fix that.

Re #51 "publicly accessible private files does not in any way sound minor to me."

Not to me either but this patch does not even attempt to address that.

For example, if the patch would write a random size image into the private directory, try to <img src> it (styled margin-left: -5000px to be invisible) and checking dimensions via JS, that would be an attempt worthy of discussion to validate whether private files are accessible via public. We would still need to address of how not to create a false sense of security and how to deal with upgrade.

Updated issue summary.